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

[Sander Vanheule <sander@svanheule.net>]

On Mon, 2021-05-24 at 14:32 +0300, Andy Shevchenko wrote:
> Oops, I had sent this privately, Cc'ing to ML.

I'll repeat my replies here then.

> 
> On Mon, May 24, 2021 at 12:08 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > 
> > On Mon, May 24, 2021 at 1:34 AM Sander Vanheule <sander@svanheule.net> 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)
> > 

I had to rework rtl8231_pinctrl_init_functions a bit, but outside of that
function this string array is indeed constant.


> > > +};
> > 
> > ...
> > 
> > > +       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.

Ok, changed.


> > 
> > ...
> > 
> > > +       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?
> > 

Checked the documentation again, and this actually doesn't return error values.
Only valid pointers or NULL. Will change accordingly here, and also in the LED
driver.


> > ...
> > 
> > > +       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()
> > 

gpiochip_add_data_with_key can indeed return EPROBE_DEFER (when gpiolib isn't
loaded yet, if I understand correctly). I'll replace dev_err by dev_err_probe.


Best,
Sander