From: Lee Jones <lee@kernel.org>
To: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: linux-sh@vger.kernel.org, "Helge Deller" <deller@gmx.de>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"Sam Ravnborg" <sam@ravnborg.org>,
"Javier Martinez Canillas" <javierm@redhat.com>,
"Rob Herring" <robh@kernel.org>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Subject: Re: [PATCH v4 24/37] mfd: sm501: Convert platform_data to OF property
Date: Thu, 23 Nov 2023 13:59:53 +0000 [thread overview]
Message-ID: <20231123135953.GF1243364@google.com> (raw)
In-Reply-To: <478cf6465ab23eaf00515b8d067101bec514358b.1699856600.git.ysato@users.sourceforge.jp>
On Tue, 14 Nov 2023, Yoshinori Sato wrote:
> Various parameters of SM501 can be set using platform_data,
> so parameters cannot be passed in the DeviceTree target.
> Expands the parameters set in platform_data so that they can be
> specified using DeviceTree properties.
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
> drivers/mfd/sm501.c | 70 +++++++++++++++++++++++++++++++++++
> drivers/video/fbdev/sm501fb.c | 70 +++++++++++++++++++++++++++++++++--
> include/linux/sm501.h | 3 +-
> 3 files changed, 138 insertions(+), 5 deletions(-)
Where are the Device Tree bindings? Do they already exist?
I'd be interested to see how well they fair with the DT maintainers.
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 28027982cf69..4f9c9c5936ff 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1370,6 +1370,69 @@ static int sm501_init_dev(struct sm501_devdata *sm)
> return 0;
> }
>
> +#ifdef CONFIG_OF
I very much dislike #ifery in C files.
> +static void sm501_of_read_reg_init(struct device_node *np,
> + const char *propname, struct sm501_reg_init *val)
> +{
> + u32 u32_val[2];
> +
> + if (!of_property_read_u32_array(np, propname, u32_val, sizeof(u32_val))) {
> + val->set = u32_val[0];
> + val->mask = u32_val[1];
> + }
> +}
> +
> +static int sm501_parse_dt(struct sm501_devdata *sm, struct device_node *np)
> +{
> + struct sm501_platdata *plat;
> + u32 u32_val;
> +
> + plat = devm_kzalloc(sm->dev, sizeof(*plat), GFP_KERNEL);
> + if (!plat)
> + return -ENOMEM;
> +
> + plat->init = devm_kzalloc(sm->dev, sizeof(*plat->init), GFP_KERNEL);
> + if (!plat->init)
> + return -ENOMEM;
> +
> + if (!of_property_read_u32(np, "smi,devices", &u32_val))
> + plat->init->devices = u32_val;
> +
> + if (!of_property_read_u32(np, "smi,mclk", &u32_val))
> + plat->init->mclk = u32_val;
> +
> + if (!of_property_read_u32(np, "smi,m1xclk", &u32_val))
> + plat->init->m1xclk = u32_val;
> +
> + sm501_of_read_reg_init(np, "smi,misc-timing", &plat->init->misc_timing);
> + sm501_of_read_reg_init(np, "smi,misc-control", &plat->init->misc_control);
> + sm501_of_read_reg_init(np, "smi,gpio-low", &plat->init->gpio_low);
> + sm501_of_read_reg_init(np, "smi,gpio-high", &plat->init->gpio_high);
So all of these properties are optional?
> +#ifdef CONFIG_MFD_SM501_GPIO
> + if (plat->init->devices & SM501_USE_GPIO) {
> + if (!of_property_read_u32_index(np, "smi,num-i2c", 0, &u32_val))
> + plat->gpio_i2c_nr = u32_val;
> + else
> + plat->gpio_i2c_nr = 0;
This is already zero, no?
> + }
> + if (plat->gpio_i2c_nr > 0) {
> + int sz_gpio;
> +
> + sz_gpio = sizeof(struct sm501_platdata_gpio_i2c) * plat->gpio_i2c_nr;
sizeof(plat->gpio_i2c) * plat->gpio_i2c_nr ?
And put it inside the devm_kzalloc() call.
> + plat->gpio_i2c = devm_kzalloc(sm->dev, sz_gpio, GFP_KERNEL);
> + if (plat->gpio_i2c == NULL)
if (!plat->gpio_i2c)
> + return -ENOMEM;
> +
> + of_property_read_variable_u32(np, "smi,gpio-i2c",
> + plat->gpio_i2c, sz_gpio / sizeof(int));
> + }
> +#endif /* CONFIG_MFD_SM501_GPIO */
> + sm->platdata = plat;
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> static int sm501_plat_probe(struct platform_device *dev)
> {
> struct sm501_devdata *sm;
> @@ -1406,6 +1469,13 @@ static int sm501_plat_probe(struct platform_device *dev)
> goto err_res;
> }
>
> +#ifdef CONFIG_OF
> + if (dev->dev.of_node) {
if (IS_ENABLED(CONFIG_OF) && dev->dev.of_node)
... and let the compiler do the rest.
> + ret = sm501_parse_dt(sm, dev->dev.of_node);
> + if (ret)
> + goto err_res;
> + }
> +#endif
> platform_set_drvdata(dev, sm);
>
> sm->regs = ioremap(sm->io_res->start, resource_size(sm->io_res));
> diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
> index d6fdc1737cd2..36a080dd35a1 100644
> --- a/drivers/video/fbdev/sm501fb.c
> +++ b/drivers/video/fbdev/sm501fb.c
> @@ -1932,10 +1932,62 @@ static int sm501fb_start_one(struct sm501fb_info *info,
> return 0;
> }
>
> +#if defined(CONFIG_OF)
> +static struct sm501_platdata_fbsub *read_fbsub(struct device_node *np, const char *ch_name)
> +{
> + struct sm501_platdata_fbsub *fbsub = NULL;
> + struct fb_videomode *def_mode;
> + struct device_node *child;
> + const void *prop;
> + u32 flags;
> + u32 bpp;
> + int len;
> +
> + child = of_get_child_by_name(np, ch_name);
> + if (child == NULL)
> + return NULL;
> +
> + prop = of_get_property(child, "edid", &len);
> + if (prop && len == EDID_LENGTH) {
> + struct fb_monspecs *specs;
> + u8 *edid;
> +
> + edid = kmemdup(prop, EDID_LENGTH, GFP_KERNEL);
> + if (edid) {
> + specs = kzalloc(sizeof(*specs), GFP_KERNEL);
> + if (specs) {
> + fb_edid_to_monspecs(edid, specs);
> + def_mode = specs->modedb;
> + }
> + kfree(specs);
> + }
> + kfree(edid);
> + }
> +
> + if (of_property_read_u32(child, "bpp", &bpp))
> + bpp = 0;
> + if (of_property_read_u32(child, "smi,flags", &flags))
> + flags = 0;
> +
> + if (def_mode || bpp || flags) {
> + fbsub = kzalloc(sizeof(*fbsub), GFP_KERNEL);
> + if (fbsub) {
> + fbsub->def_mode = def_mode;
> + fbsub->def_bpp = bpp;
> + fbsub->flags = flags;
> + }
> + }
> + return fbsub;
> +}
> +#endif
All of this needs moving to the display driver. And I have suspicions
that all of the new code above should live there too.
> static int sm501fb_probe(struct platform_device *pdev)
> {
> - struct sm501fb_info *info;
> struct device *dev = &pdev->dev;
> + struct sm501fb_info *info;
> + const void *prop;
> + const char *cp;
> + int len;
> int ret;
>
> /* allocate our framebuffers */
> @@ -1957,9 +2009,7 @@ static int sm501fb_probe(struct platform_device *pdev)
> int found = 0;
> #if defined(CONFIG_OF)
> struct device_node *np = pdev->dev.parent->of_node;
> - const u8 *prop;
> - const char *cp;
> - int len;
> + struct sm501_platdata_fbsub *sub;
>
> info->pdata = &sm501fb_def_pdata;
> if (np) {
> @@ -1974,6 +2024,18 @@ static int sm501fb_probe(struct platform_device *pdev)
> if (info->edid_data)
> found = 1;
> }
> + if (of_property_read_bool(np, "route-crt-panel"))
> + info->pdata->fb_route = SM501_FB_CRT_PANEL;
> + else
> + info->pdata->fb_route = SM501_FB_OWN;
> + if (of_property_read_bool(np, "swap-fb-endian"))
> + info->pdata->flags |= SM501_FBPD_SWAP_FB_ENDIAN;
> + sub = read_fbsub(np, "crt");
> + if (sub)
> + info->pdata->fb_crt = sub;
> + sub = read_fbsub(np, "panel");
> + if (sub)
> + info->pdata->fb_pnl = sub;
> }
> #endif
> if (!found) {
> diff --git a/include/linux/sm501.h b/include/linux/sm501.h
> index 2f3488b2875d..5c9a683b0615 100644
> --- a/include/linux/sm501.h
> +++ b/include/linux/sm501.h
> @@ -6,6 +6,8 @@
> * Vincent Sanders <vince@simtec.co.uk>
> */
>
> +#include <dt-bindings/display/sm501.h>
> +
> extern int sm501_unit_power(struct device *dev,
> unsigned int unit, unsigned int to);
>
> @@ -35,7 +37,6 @@ extern unsigned long sm501_modify_reg(struct device *dev,
> unsigned long clear);
>
>
> -/* Platform data definitions */
>
> #define SM501FB_FLAG_USE_INIT_MODE (1<<0)
> #define SM501FB_FLAG_DISABLE_AT_EXIT (1<<1)
> --
> 2.39.2
>
--
Lee Jones [李琼斯]
next prev parent reply other threads:[~2023-11-23 13:59 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-14 7:59 [PATCH v4 00/37] Device Tree support for SH7751 based board Yoshinori Sato
2023-11-14 7:59 ` [PATCH v4 01/37] sh: passing FDT address to kernel startup Yoshinori Sato
2023-11-14 8:44 ` Sergei Shtylyov
2023-11-14 7:59 ` [PATCH v4 02/37] sh: Kconfig unified OF supported targets Yoshinori Sato
2023-11-14 12:46 ` Arnd Bergmann
2023-11-14 7:59 ` [PATCH v4 03/37] sh: Enable OF support for build and configuration Yoshinori Sato
2023-11-14 7:59 ` [PATCH v4 04/37] dt-bindings: interrupt-controller: Add header for Renesas SH3/4 INTC Yoshinori Sato
2023-11-14 21:25 ` Krzysztof Kozlowski
2023-11-14 7:59 ` [PATCH v4 05/37] sh: GENERIC_IRQ_CHIP support for CONFIG_OF=y Yoshinori Sato
2023-11-14 7:59 ` [PATCH v4 06/37] sh: kernel/setup Update DT support Yoshinori Sato
2023-11-14 7:59 ` [PATCH v4 07/37] sh: Fix COMMON_CLK support in CONFIG_OF=y Yoshinori Sato
2023-11-14 7:59 ` [PATCH v4 08/37] clocksource: sh_tmu: CLOCKSOURCE support Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 09/37] dt-bindings: timer: renesas,tmu: add renesas,tmu-sh7750 Yoshinori Sato
2023-11-14 21:26 ` Krzysztof Kozlowski
2023-11-15 8:17 ` Geert Uytterhoeven
2023-11-15 21:05 ` Krzysztof Kozlowski
2023-11-14 8:00 ` [PATCH v4 10/37] sh: Common PCI framework support Yoshinori Sato
2023-11-14 13:03 ` Arnd Bergmann
2023-11-14 8:00 ` [PATCH v4 11/37] sh: Add old PCI drivers compatible stub Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 12/37] pci: pci-sh7751: Add SH7751 PCI driver Yoshinori Sato
2023-11-20 18:16 ` Bjorn Helgaas
2023-11-14 8:00 ` [PATCH v4 13/37] dt-bindings: pci: pci-sh7751: Add SH7751 PCI Yoshinori Sato
2023-11-14 21:27 ` Krzysztof Kozlowski
2023-11-14 8:00 ` [PATCH v4 14/37] dt-bindings: clock: sh7750-cpg: Add renesas,sh7750-cpg header Yoshinori Sato
2023-11-14 21:29 ` Krzysztof Kozlowski
2023-11-14 8:00 ` [PATCH v4 15/37] clk: Compatible with narrow registers Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 16/37] clk: renesas: Add SH7750/7751 CPG Driver Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 17/37] irqchip: Add SH7751 INTC driver Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 18/37] dt-bindings: interrupt-controller: renesas,sh7751-intc: Add json-schema Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 19/37] irqchip: SH7751 IRL external encoder with enable gate Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 20/37] dt-bindings: interrupt-controller: renesas,sh7751-irl-ext: Add json-schema Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 21/37] serial: sh-sci: fix SH4 OF support Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 22/37] dt-bindings: serial: renesas,scif: Add scif-sh7751 Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 23/37] dt-bindings: display: smi,sm501: SMI SM501 binding json-schema Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 24/37] mfd: sm501: Convert platform_data to OF property Yoshinori Sato
2023-11-23 13:59 ` Lee Jones [this message]
2023-11-14 8:00 ` [PATCH v4 25/37] dt-binding: sh: cpus: Add SH CPUs json-schema Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 26/37] dt-bindings: vendor-prefix: Add new vendor iodata and smi Yoshinori Sato
2023-11-14 16:35 ` Geert Uytterhoeven
2023-11-14 8:00 ` [PATCH v4 27/37] dt-bindings: ata: ata-generic: Add new targets Yoshinori Sato
2023-11-14 18:19 ` Rob Herring
2023-11-14 8:00 ` [PATCH v4 28/37] dt-bindings: soc: renesas: sh: Add SH7751 based target Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 29/37] sh: SH7751R SoC Internal peripheral definition dtsi Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 30/37] sh: add RTS7751R2D Plus DTS Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 31/37] sh: Add IO DATA LANDISK dts Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 32/37] sh: Add IO DATA USL-5P dts Yoshinori Sato
2023-11-14 9:02 ` Sergei Shtylyov
2023-11-14 9:04 ` Sergei Shtylyov
2023-11-14 8:00 ` [PATCH v4 33/37] sh: j2_mimas_v2.dts update Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 34/37] sh: Add dtbs target support Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 35/37] sh: RTS7751R2D Plus OF defconfig Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 36/37] sh: LANDISK " Yoshinori Sato
2023-11-14 8:00 ` [PATCH v4 37/37] sh: j2_defconfig: update Yoshinori Sato
2023-11-14 8:58 ` [PATCH v4 00/37] Device Tree support for SH7751 based board John Paul Adrian Glaubitz
2023-11-14 18:39 ` John Paul Adrian Glaubitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231123135953.GF1243364@google.com \
--to=lee@kernel.org \
--cc=deller@gmx.de \
--cc=javierm@redhat.com \
--cc=linux-sh@vger.kernel.org \
--cc=robh@kernel.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
--cc=u.kleine-koenig@pengutronix.de \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).