Re: [PATCH v2 1/2] pinctrl: ocelot: Fix pincfg for lan966x

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 07/08/2022 23:58, Andy Shevchenko wrote:

Hi Andy,

> 
> On Fri, Jul 8, 2022 at 10:10 PM Horatiu Vultur
> <horatiu.vultur@microchip.com> wrote:
> >
> > The blamed commit introduce support for lan966x which use the same
> > pinconf_ops as sparx5. The problem is that pinconf_ops is specific to
> > sparx5. More precisely the offset of the bits in the pincfg register are
> > different and also lan966x doesn't have support for
> > PIN_CONFIG_INPUT_SCHMITT_ENABLE.
> >
> > Fix this by making pinconf_ops more generic such that it can be also
> > used by lan966x. This is done by introducing 'ocelot_pincfg_data' which
> > contains the offset and what is supported for each SOC.
> 
> ...
> 
> > +struct ocelot_pincfg_data {
> > +       bool has_schmitt;
> > +       u8 schmitt_bit;
> > +       u8 pd_bit;
> > +       u8 pu_bit;
> > +       u8 drive_bits;
> 
> I would go with mandatory fields first and leave optional (that is
> with boolean flag) at last.
> 
> > +};
> 
> ...
> 
> >  struct ocelot_pinctrl {
> >         struct device *dev;
> >         struct pinctrl_dev *pctl;
> > @@ -330,6 +331,12 @@ struct ocelot_pinctrl {
> >         struct pinctrl_desc *desc;
> >         struct ocelot_pmx_func func[FUNC_MAX];
> >         u8 stride;
> > +       struct ocelot_pincfg_data *pincfg_data;
> 
> It might waste too many bytes in some cases. I would recommend moving
> it somewhere above, definitely before the u8 member.
> 
> > +};
> 
> Yes, I understand that for a certain architecture it might be the same
> result in sizeof(), the rationale is to make code better in case
> somebody copies'n'pastes pieces or ideas from it.
> 
> ...
> 
> >                 if (param == PIN_CONFIG_BIAS_DISABLE)>                         val = (val == 0);
> >                 else if (param == PIN_CONFIG_BIAS_PULL_DOWN)
> > -                       val = (val & BIAS_PD_BIT ? true : false);
> > +                       val = (val & info->pincfg_data->pd_bit ? true : false);
> >                 else    /* PIN_CONFIG_BIAS_PULL_UP */
> > -                       val = (val & BIAS_PU_BIT ? true : false);
> > +                       val = (val & info->pincfg_data->pu_bit ? true : false);
> >                 break;
> 
> > +               val = (val & info->pincfg_data->schmitt_bit ? true : false);
> 
> 
> !!(val & ...) will be a much shorter equivalent to ternary.
> 
> >                 break;
> 
> ...
> 
> > +static struct ocelot_match_data ocelot_desc = {
> > +       .desc = {
> > +               .name = "ocelot-pinctrl",
> > +               .pins = ocelot_pins,
> > +               .npins = ARRAY_SIZE(ocelot_pins),
> > +               .pctlops = &ocelot_pctl_ops,
> > +               .pmxops = &ocelot_pmx_ops,
> > +               .owner = THIS_MODULE,
> > +       }
> 
> Please, keep a comma here. It's definitely not a terminating entry, so
> it might help in the future.
> 
> Ditto for all cases like this.
> 
> >  };
> 
> ...
> 
> > +       struct ocelot_match_data *data;
> 
> Any specific reason why this is not const?
> 
> ...
> 
> > +       data = (struct ocelot_match_data *)device_get_match_data(dev);
> 
> And here you drop the qualifier...
> 
> I would recommend making it const and dropping the cast completely.

If I make this const, but then few lines after I will get the following
warnings:

drivers/pinctrl/pinctrl-ocelot.c:1983:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1983 |  info->desc = &data->desc;
     |             ^
drivers/pinctrl/pinctrl-ocelot.c:1984:20: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1984 |  info->pincfg_data = &data->pincfg_data;
     |                    ^

Of course I can make also info->desc and info->pincfg_data const but
then I will get the following warning:

drivers/pinctrl/pinctrl-ocelot.c: In function ‘ocelot_pinctrl_register’:
drivers/pinctrl/pinctrl-ocelot.c:1723:53: warning: passing argument 2 of ‘devm_pinctrl_register’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1723 |  info->pctl = devm_pinctrl_register(&pdev->dev, info->desc, info);
     |                                                 ~~~~^~~~~~
In file included from include/linux/gpio/driver.h:10,
                 from drivers/pinctrl/pinctrl-ocelot.c:10:
                      include/linux/pinctrl/pinctrl.h:166:26: note: expected ‘struct pinctrl_desc *’ but argument is of type ‘const struct pinctrl_desc *’
166 |     struct pinctrl_desc *pctldesc,
    |     ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

> 
> > +       if (!data)
> > +               return -EINVAL;
> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
/Horatiu