Re: [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO

[Andy Shevchenko]

On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
<aidanmacdonald.0x0@xxxxxxxxx> wrote:
>
> Some devices use a multi-bit register field to change the GPIO
> input/output direction. Add the ->reg_field_xlate() callback to
> support such devices in gpio-regmap.
>
> ->reg_field_xlate() builds on ->reg_mask_xlate() by allowing the
> driver to return a mask and values to describe a register field.
> gpio-regmap will use the mask to isolate the field and compare or
> update it using the values to implement GPIO level and direction
> get and set ops.

Thanks for the proposal. My comments below.

...

> +static void
> +gpio_regmap_simple_field_xlate(struct gpio_regmap *gpio,
> +                              unsigned int base, unsigned int offset,
> +                              unsigned int *reg, unsigned int *mask,
> +                              unsigned int *values)
> +{
> +       gpio->reg_mask_xlate(gpio, base, offset, reg, mask);
> +       values[0] = 0;
> +       values[1] = *mask;

This is a fragile and less compile-check prone approach. If you know
the amount of values, make a specific data type for that, or pass the
length of the output buffer..

> +}

...

> +       unsigned int values[2];

> +       return (val & mask) == values[1];

> +       unsigned int values[2];

How will the callee know that it's only 2 available?


> +       regmap_update_bits(gpio->regmap, reg, mask, values[!!val]);

If we have special meaning of the values, perhaps it needs to follow
an enum of some definitions, so everybody will understand how indices
are mapped to the actual data in the array.

> +       unsigned int values[2];

> +       regmap_update_bits(gpio->regmap, reg, mask, values[1]);

> +       unsigned int values[2];

> +       if ((val & mask) == values[invert])

How do you guarantee this won't overflow? (see above comment about
indices mapping)

> +       unsigned int values[2];

As per above comments.

-- 
With Best Regards,
Andy Shevchenko