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@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)
> >
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
