| Message ID | f622670c0af1bf01bf7c2f16241db0c36233a7d9.1693444193.git.ysato@users.sourceforge.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DeviceTree support for SH7751 based boards. | expand |
On 31/08/2023 03:11, Yoshinori Sato wrote: > Some parameters only platform_device. > Added same parameters in OF property. Subject: everything can be "add of properties". Is some next commit adding properties to this file going to have the same subject? Please write subjects matching changes. > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > --- > .../devicetree/bindings/display/sm501fb.txt | 11 ++ > drivers/mfd/sm501.c | 113 +++++++++++++++++- Nope, bindings are never mixed with drivers. And you clearly did not run checkpatch, so: Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time, thus I will skip this patch entirely till you follow the process allowing the patch to be tested. Please kindly resend and include all necessary To/Cc entries. > 2 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/sm501fb.txt b/Documentation/devicetree/bindings/display/sm501fb.txt > index 1c79c267a57f..4c4920394431 100644 > --- a/Documentation/devicetree/bindings/display/sm501fb.txt > +++ b/Documentation/devicetree/bindings/display/sm501fb.txt > @@ -20,6 +20,17 @@ Optional properties: > set different foreign endian. > - big-endian: available on little endian systems, to > set different foreign endian. > +- sm501,devices: select peripheral functions. > + available usb-host, usb-gadget, ssp0, ssp,1 uart0, uart1, accel, > + ac97, i2s, gpio and all. > +- sm501,mclk: SM501 mclk frequency. > +- sm501,m1xclk: SM501 m1xclk frequency. > +- sm501,misc-timing: SM501 Miscellaneous Timing reg value. > +- sm501,misc-control: SM501 Miscellaneous Control reg value. > +- sm501,gpio-low: SM501 GPIO31-0 Control reg value. > +- sm501,gpio-high: SM501 GPIO63-32 Control reg value. > +- sm501,num-i2c: I2C channel number. > +- sm501,gpio-i2c: I2C assigned GPIO. Sorry, new properties are allowed only in DT schema format. Convert bindings to DT schema first. Best regards, Krzysztof
Hi Sato-san, On Fri, Sep 1, 2023 at 12:23 AM Yoshinori Sato <ysato@users.sourceforge.jp> wrote: > Some parameters only platform_device. > Added same parameters in OF property. > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> Thanks for your patch! > --- a/drivers/mfd/sm501.c > +++ b/drivers/mfd/sm501.c > @@ -80,6 +80,9 @@ struct sm501_devdata { > unsigned int irq; > void __iomem *regs; > unsigned int rev; > +#if defined(CONFIG_OF) > + struct device_node *np; > +#endif "np" is only used during probing, so you can just pass it as a parameter instead. > }; > > > @@ -1370,6 +1373,106 @@ static int sm501_init_dev(struct sm501_devdata *sm) > return 0; > } > > +static unsigned int sm501_parse_devices_str(const char *str) This function is unused when COFNIG_OF=n, so please move inside the #idef below. > +{ > + char *sep; > + unsigned int device = 0; > + int i; unsigned int > + int len; > + static const struct { > + char *devname; > + unsigned int devid; > + } devlist[] = { > + { "usb-host", SM501_USE_USB_HOST }, > + { "usb-gadget", SM501_USE_USB_SLAVE }, > + { "ssp0", SM501_USE_SSP0 }, > + { "ssp1", SM501_USE_SSP1 }, > + { "uart0", SM501_USE_UART0 }, > + { "uart1", SM501_USE_UART1 }, > + { "accel", SM501_USE_FBACCEL }, > + { "ac97", SM501_USE_AC97 }, > + { "i2s", SM501_USE_I2S }, > + { "gpio", SM501_USE_GPIO }, > + { "all", SM501_USE_ALL }, > + }; > + > + do { > + sep = strchr(str, ','); > + len = sep ? sep - str : strlen(str); > + for (i = 0; i < ARRAY_SIZE(devlist); i++) { > + if (strncasecmp(str, devlist[i].devname, len) == 0) { > + device |= devlist[i].devid; > + break; > + } > + } > + str = sep + 1; > + } while (sep); > + return device; > +} > + > +#if defined(CONFIG_OF) > +static void sm501_of_read_reg_init(struct sm501_devdata *sm, > + const char *propname, struct sm501_reg_init *val) > +{ > + u32 u32_val; > + > + if (!of_property_read_u32_index(sm->np, propname, 0, &u32_val)) > + val->set = u32_val; > + if (!of_property_read_u32_index(sm->np, propname, 1, &u32_val)) > + val->mask = u32_val; Error handling? > +} > + > +static int sm501_parse_dt(struct sm501_devdata *sm) > +{ > + struct sm501_platdata *plat; > + const char *devstr; > + u32 u32_val; > + > + if (sm->np == NULL) > + return 0; This cannot happen. > + plat = kzalloc(sizeof(struct sm501_platdata), GFP_KERNEL); devm_kzalloc(), to simplify error handling > + if (plat == NULL) > + return -ENOMEM; > + plat->init = kzalloc(sizeof(struct sm501_initdata), GFP_KERNEL); devm_kzalloc() > + if (plat->init == NULL) > + goto err; > + > + if (!of_property_read_string(sm->np, "sm501,devices", &devstr)) > + plat->init->devices = sm501_parse_devices_str(devstr); > + if (!of_property_read_u32_index(sm->np, "sm501,mclk", 0, &u32_val)) > + plat->init->mclk = u32_val; > + if (!of_property_read_u32_index(sm->np, "sm501,m1xclk", 0, &u32_val)) > + plat->init->m1xclk = u32_val; Do you need any error handling? > static int sm501_plat_probe(struct platform_device *dev) > { > struct sm501_devdata *sm; > @@ -1384,7 +1487,6 @@ static int sm501_plat_probe(struct platform_device *dev) > sm->dev = &dev->dev; > sm->pdev_id = dev->id; > sm->platdata = dev_get_platdata(&dev->dev); > - Please keep this blank line. > ret = platform_get_irq(dev, 0); > if (ret < 0) > goto err_res; Gr{oetje,eeting}s, Geert
diff --git a/Documentation/devicetree/bindings/display/sm501fb.txt b/Documentation/devicetree/bindings/display/sm501fb.txt index 1c79c267a57f..4c4920394431 100644 --- a/Documentation/devicetree/bindings/display/sm501fb.txt +++ b/Documentation/devicetree/bindings/display/sm501fb.txt @@ -20,6 +20,17 @@ Optional properties: set different foreign endian. - big-endian: available on little endian systems, to set different foreign endian. +- sm501,devices: select peripheral functions. + available usb-host, usb-gadget, ssp0, ssp,1 uart0, uart1, accel, + ac97, i2s, gpio and all. +- sm501,mclk: SM501 mclk frequency. +- sm501,m1xclk: SM501 m1xclk frequency. +- sm501,misc-timing: SM501 Miscellaneous Timing reg value. +- sm501,misc-control: SM501 Miscellaneous Control reg value. +- sm501,gpio-low: SM501 GPIO31-0 Control reg value. +- sm501,gpio-high: SM501 GPIO63-32 Control reg value. +- sm501,num-i2c: I2C channel number. +- sm501,gpio-i2c: I2C assigned GPIO. Example for MPC5200: display@1,0 { diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c index 28027982cf69..39871ca1b9f7 100644 --- a/drivers/mfd/sm501.c +++ b/drivers/mfd/sm501.c @@ -80,6 +80,9 @@ struct sm501_devdata { unsigned int irq; void __iomem *regs; unsigned int rev; +#if defined(CONFIG_OF) + struct device_node *np; +#endif }; @@ -1370,6 +1373,106 @@ static int sm501_init_dev(struct sm501_devdata *sm) return 0; } +static unsigned int sm501_parse_devices_str(const char *str) +{ + char *sep; + unsigned int device = 0; + int i; + int len; + static const struct { + char *devname; + unsigned int devid; + } devlist[] = { + { "usb-host", SM501_USE_USB_HOST }, + { "usb-gadget", SM501_USE_USB_SLAVE }, + { "ssp0", SM501_USE_SSP0 }, + { "ssp1", SM501_USE_SSP1 }, + { "uart0", SM501_USE_UART0 }, + { "uart1", SM501_USE_UART1 }, + { "accel", SM501_USE_FBACCEL }, + { "ac97", SM501_USE_AC97 }, + { "i2s", SM501_USE_I2S }, + { "gpio", SM501_USE_GPIO }, + { "all", SM501_USE_ALL }, + }; + + do { + sep = strchr(str, ','); + len = sep ? sep - str : strlen(str); + for (i = 0; i < ARRAY_SIZE(devlist); i++) { + if (strncasecmp(str, devlist[i].devname, len) == 0) { + device |= devlist[i].devid; + break; + } + } + str = sep + 1; + } while (sep); + return device; +} + +#if defined(CONFIG_OF) +static void sm501_of_read_reg_init(struct sm501_devdata *sm, + const char *propname, struct sm501_reg_init *val) +{ + u32 u32_val; + + if (!of_property_read_u32_index(sm->np, propname, 0, &u32_val)) + val->set = u32_val; + if (!of_property_read_u32_index(sm->np, propname, 1, &u32_val)) + val->mask = u32_val; +} + +static int sm501_parse_dt(struct sm501_devdata *sm) +{ + struct sm501_platdata *plat; + const char *devstr; + u32 u32_val; + + if (sm->np == NULL) + return 0; + plat = kzalloc(sizeof(struct sm501_platdata), GFP_KERNEL); + if (plat == NULL) + return -ENOMEM; + plat->init = kzalloc(sizeof(struct sm501_initdata), GFP_KERNEL); + if (plat->init == NULL) + goto err; + + if (!of_property_read_string(sm->np, "sm501,devices", &devstr)) + plat->init->devices = sm501_parse_devices_str(devstr); + if (!of_property_read_u32_index(sm->np, "sm501,mclk", 0, &u32_val)) + plat->init->mclk = u32_val; + if (!of_property_read_u32_index(sm->np, "sm501,m1xclk", 0, &u32_val)) + plat->init->m1xclk = u32_val; + sm501_of_read_reg_init(sm, "sm501,misc-timing", &plat->init->misc_timing); + sm501_of_read_reg_init(sm, "sm501,misc-control", &plat->init->misc_control); + sm501_of_read_reg_init(sm, "sm501,gpio-low", &plat->init->gpio_low); + sm501_of_read_reg_init(sm, "sm501,gpio-high", &plat->init->gpio_high); + +#ifdef CONFIG_MFD_SM501_GPIO + if (plat->init->devices & SM501_USE_GPIO) { + if (!of_property_read_u32_index(sm->np, "sm501,num-i2c", 0, &u32_val)) + plat->gpio_i2c_nr = u32_val; + } + if (plat->gpio_i2c_nr > 0) { + plat->gpio_i2c = kcalloc(plat->gpio_i2c_nr, + sizeof(struct sm501_platdata_gpio_i2c), + GFP_KERNEL); + if (plat->gpio_i2c == NULL) + goto err; + of_property_read_variable_u32(sm->np, "sm501,gpio-i2c", + plat->gpio_i2c, + plat->gpio_i2c_nr * 5); + } +#endif + sm->platdata = plat; + return 0; +err: + kfree(plat->init); + kfree(plat); + return -ENOMEM; +} +#endif + static int sm501_plat_probe(struct platform_device *dev) { struct sm501_devdata *sm; @@ -1384,7 +1487,6 @@ static int sm501_plat_probe(struct platform_device *dev) sm->dev = &dev->dev; sm->pdev_id = dev->id; sm->platdata = dev_get_platdata(&dev->dev); - ret = platform_get_irq(dev, 0); if (ret < 0) goto err_res; @@ -1406,6 +1508,15 @@ static int sm501_plat_probe(struct platform_device *dev) goto err_res; } +#if defined(CONFIG_OF) + if (dev->dev.of_node) { + sm->np = dev->dev.of_node; + ret = sm501_parse_dt(sm); + if (ret) + goto err_res; + } +#endif + platform_set_drvdata(dev, sm); sm->regs = ioremap(sm->io_res->start, resource_size(sm->io_res));
Some parameters only platform_device. Added same parameters in OF property. Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> --- .../devicetree/bindings/display/sm501fb.txt | 11 ++ drivers/mfd/sm501.c | 113 +++++++++++++++++- 2 files changed, 123 insertions(+), 1 deletion(-)