Hi Andy,
On Tue, 2021-05-18 at 00:42 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM 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_pin_desc {
> > + unsigned int number;
> > + const char *name;
> > + enum rtl8231_pin_function functions;
> > + u8 reg;
> > + u8 offset;
> > + u8 gpio_function_value;
> > +};
>
> I would see rather
>
> sturct pinctrl_pin_desc desc;
>
> Where drv_data describes the rest of the data for pin
>
I've split up the definitions into two parts:
* pinctrl_pin_desc with the standard info, which has drv_data pointing to...
* a device-specific rtl8231_pin_desc, with the register field info and
alternate function
So the pin descriptions are now entirely static, and only the pin functions are
assembled at runtime.
>
> > +static int rtl8231_get_group_pins(struct pinctrl_dev *pctldev, unsigned int
> > selector,
> > + const unsigned int **pins, unsigned int *num_pins)
> > +{
>
> > + if (selector < ARRAY_SIZE(rtl8231_pins)) {
>
> Can we use traditional pattern, i.e.
>
> if (... >= ARRAY_SIZE(...))
> return -EINVAL;
>
> ...
> return 0;
>
> ?
Sure. Will be implemented in v3.
>
> > + *pins = &rtl8231_pins[selector].number;
> > + *num_pins = 1;
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > +}
>
> ...
>
> > +static int rtl8231_set_mux(struct pinctrl_dev *pctldev, unsigned int
> > func_selector,
> > + unsigned int group_selector)
> > +{
>
> > + int err = 0;
>
> Redundant variable.
>
> > + switch (func_flag) {
> > + case RTL8231_PIN_FUNCTION_LED:
> > + case RTL8231_PIN_FUNCTION_PWM:
> > + err = regmap_update_bits(ctrl->map, desc->reg,
> > function_mask, ~gpio_function);
> > + break;
> > + case RTL8231_PIN_FUNCTION_GPIO:
> > + err = regmap_update_bits(ctrl->map, desc->reg,
> > function_mask, gpio_function);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
I've reworked this whole section a bit. since a pin is either (only) GPIO, or
some alternative function, this could be done with a simpler if/else.
>
> > +static const struct pinmux_ops rtl8231_pinmux_ops = {
> > + .set_mux = rtl8231_set_mux,
> > + .get_functions_count = rtl8231_get_functions_count,
> > + .get_function_name = rtl8231_get_function_name,
> > + .get_function_groups = rtl8231_get_function_groups,
> > + .gpio_request_enable = rtl8231_gpio_request_enable,
>
> > + .strict = true
>
> Leave comma for non-terminator entries.
>
> > +};
> > +
> > +
>
> One blank line is enough.
>
> ...
>
> > +static int rtl8231_pinctrl_init_functions(struct device *dev, struct
> > rtl8231_pin_ctrl *ctrl)
> > +{
> > + struct rtl8231_function *function;
> > + const char **group_name;
> > + unsigned int f_idx;
> > + unsigned int pin;
> > +
> > + ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names);
> > + ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl-
> > >functions), GFP_KERNEL);
> > + if (IS_ERR(ctrl->functions)) {
>
> Wrong.
I was somehow thinking that this would either return an error value or a valid
point. Don't know where I got that, but should be fixed here and for the other
kallocs.
Best,
Sander
