Re: [PATCH v3 5/6] pinctrl: Add RTL8231 pin control and GPIO support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

Oops, I had sent this privately, Cc'ing to ML.

On Mon, May 24, 2021 at 12:08 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Mon, May 24, 2021 at 1:34 AM Sander Vanheule <sander@xxxxxxxxxxxxx> wrote:
> >
> > This driver implements the GPIO and pin muxing features provided by the
> > RTL8231. The device should be instantiated as an MFD child, where the
> > parent device has already configured the regmap used for register
> > access.
> >
> > Although described in the bindings, pin debouncing and drive strength
> > selection are currently not implemented. Debouncing is only available
> > for the six highest GPIOs, and must be emulated when other pins are used
> > for (button) inputs anyway.
>
> ...
>
> > +struct rtl8231_function {
> > +       const char *name;
> > +       unsigned int ngroups;
> > +       const char **groups;
>
> const char * const * groups?
> (Double check this, because I don't know if it's really const in your case)
>
> > +};
>
> ...
>
> > +       const struct rtl8231_pin_desc *desc =
> > +               (struct rtl8231_pin_desc *) &rtl8231_pins[group_selector].drv_data;
>
> Casting from/to void * is redundant in C.
>
> ...
>
> > +       struct rtl8231_pin_desc *desc =
> > +               (struct rtl8231_pin_desc *) &rtl8231_pins[offset].drv_data;
>
> Ditto.
>
> ...
>
> > +       ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names);
> > +       ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl->functions), GFP_KERNEL);
> > +       if (!ctrl->functions) {
>
> > +               dev_err(dev, "failed to allocate pin function descriptors\n");
>
> Dtop this noisy message, user space will print the similar one.
>
> > +               return -ENOMEM;
> > +       }
>
> ...
>
> > +       ctrl->map = dev_get_regmap(dev->parent, NULL);
> > +       if (!ctrl->map)
> > +               return -ENODEV;
> > +
> > +       if (IS_ERR(ctrl->map))
> > +               return PTR_ERR(ctrl->map);
>
> Hmm... Is it really the case that you have to check for different values?
> What does NULL mean? Optional?
>
> ...
>
> > +       gr = devm_gpio_regmap_register(dev, &gpio_cfg);
> > +       if (IS_ERR(gr)) {
>
> > +               dev_err(dev, "failed to register gpio controller\n");
> > +               return PTR_ERR(gr);
>
> Is it possible to get a deferred probe here? If so, use dev_err_probe()
>
> > +       }
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko