Re: [RESEND RFC PATCH 08/12] mfd/sm501: Add OF properties.

Hi Sato-san,

On Fri, Sep 1, 2023 at 12:23 AM Yoshinori Sato
<[email protected]> wrote:
> Some parameters only platform_device.
> Added same parameters in OF property.
>
> Signed-off-by: Yoshinori Sato <[email protected]>

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds