Re: [PATCH v6 05/40] pinctrl: add a Cirrus ep93xx SoC pin controller

[Nikita Shubin <nikita.shubin@xxxxxxxxxxx>]

Hello Andy!

On Wed, 2023-12-13 at 19:51 +0200, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 11:20:22AM +0300, Nikita Shubin wrote:
> > Add a pin control (only multiplexing) driver for ep93xx SoC so
> > we can fully convert ep93xx to device tree.
> > 
> > This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
> > variants, this is chosen based on "compatible" in device tree.
> 
> Mostly nit-picks below, with the exception to setting device node.
> See below.
> 
> ...
> 
> > +/*
> > + * There are several system configuration options selectable by
> > the DeviceCfg and SysCfg
> > + * registers. These registers provide the selection of several pin
> > multiplexing options and also
> > + * provide software access to the system reset configuration
> > options. Please refer to the
> > + * descriptions of the registers, “DeviceCfg” on page 5-25 and
> > “SysCfg” on page 5-34, for a
> > + * detailed explanation.
> > + */
> > +#define EP93XX_SYSCON_DEVCFG_D1ONG     BIT(30) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_D0ONG     BIT(29) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_IONU2     BIT(28) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_GONK      BIT(27) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_TONG      BIT(26) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_MONG      BIT(25) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_A2ONG     BIT(22) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_A1ONG     BIT(21) /* not used */
> > +#define EP93XX_SYSCON_DEVCFG_HONIDE    BIT(11) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_GONIDE    BIT(10) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_PONG      BIT(9) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_EONIDE    BIT(8) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_I2SONSSP  BIT(7) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_I2SONAC97 BIT(6) /* done */
> > +#define EP93XX_SYSCON_DEVCFG_RASONP3   BIT(4) /* done */
> 
> What are these comments supposed to mean?
> 
> ...
> 
> > +static const struct pinctrl_ops ep93xx_pctrl_ops = {
> > +       .get_groups_count = ep93xx_get_groups_count,
> > +       .get_group_name = ep93xx_get_group_name,
> > +       .get_group_pins = ep93xx_get_group_pins,
> 
> > +       .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> > +       .dt_free_map = pinconf_generic_dt_free_map,
> 
> Hmm... Don you need to ifdef these fields?

>From now on we can't live without CONFIG_OF, so i don't think it's
necessary.

> 
> > +};
> 
> ...
> 
> > +static const struct pinfunction ep93xx_pmx_functions[] = {
> > +       PINCTRL_PINFUNCTION("spi", spigrps, ARRAY_SIZE(spigrps)),
> 
> Is array_size.h being included?
> 
> > +       PINCTRL_PINFUNCTION("ac97", ac97grps,
> > ARRAY_SIZE(ac97grps)),
> > +       PINCTRL_PINFUNCTION("i2s", i2sgrps, ARRAY_SIZE(i2sgrps)),
> > +       PINCTRL_PINFUNCTION("pwm", pwm1grps, ARRAY_SIZE(pwm1grps)),
> > +       PINCTRL_PINFUNCTION("keypad", keypadgrps,
> > ARRAY_SIZE(keypadgrps)),
> > +       PINCTRL_PINFUNCTION("pata", idegrps, ARRAY_SIZE(idegrps)),
> > +       PINCTRL_PINFUNCTION("lcd", rastergrps,
> > ARRAY_SIZE(rastergrps)),
> > +       PINCTRL_PINFUNCTION("gpio", gpiogrps,
> > ARRAY_SIZE(gpiogrps)),
> > +};
> 
> ...
> 
> > +       switch (pmx->model) {
> > +       case EP93XX_9301_PINCTRL:
> > +               grp = &ep9301_pin_groups[group];
> > +               break;
> > +       case EP93XX_9307_PINCTRL:
> > +               grp = &ep9307_pin_groups[group];
> > +               break;
> > +       case EP93XX_9312_PINCTRL:
> > +               grp = &ep9312_pin_groups[group];
> > +               break;
> 
> default?
> 
> > +       }
> 
> ...
> 
> > +       pmx->model = (int)(uintptr_t)id->driver_data;
> 
> Is the model defined as int (signed)?
> 
> Otherwise can we use proper type?
> 
> ...
> 
> > +       /* using parent of_node to match in
> > get_pinctrl_dev_from_of_node() */
> > +       device_set_of_node_from_dev(dev, adev->dev.parent);
> 
> Hmm... This takes references in comparison to device_set_node(). Is
> it intended?

Nope, switched to "device_set_node(dev, dev_fwnode(adev-dev.parent));".

> 
> ...
> 
> > +       pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc,
> > pmx);
> > +       if (IS_ERR(pmx->pctl))
> > +               return dev_err_probe(dev, PTR_ERR(pmx->pctl),
> > "could not register pinmux driver\n");
> 
> It can be written as
> 
>         pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc,
> pmx);
>         ret = PTR_ERR_OR_ZERO(...);
>         if (ret)
>                 return dev_err_probe(dev, ret, "could not register
> pinmux driver\n");
> 
> (makes line shorter). But it's up to you.
>