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

[Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>]

Michael Walle <michael@xxxxxxxx> writes:

> Am 2022-07-03 13:10, schrieb Aidan MacDonald:
>> 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 working on this. Here are my thoughts on how to improve
> it:
>  - I'm wary on the value translation of the set and get, you
>    don't need that at the moment, correct? I'd concentrate on
>    the direction for now.
>  - I'd add a xlate_direction(), see below for an example

Yeah, I only need direction, but there's no advantage to creating a
specific mechanism. I'm not opposed to doing that but I don't see
how it can be done cleanly. Being more general is more consistent
for the API and implementation -- even if the extra flexibility
probably won't be needed, it doesn't hurt.

>  - because we can then handle the value too, we don't need the
>    invert handling in the {set,get}_direction. drop it there
>    and handle it in a simple_xlat. In gpio_regmap,
>    store "reg_dir_base" and "invert_direction", derived from
>    config->reg_dir_in_base and config->reg_dir_out_base.
>

I think this is more complicated and less consistent than handling
reg_dir_in/out_base separately.

> static int gpio_regmap_simple_xlat_direction(struct gpio_regmap *gpio
>                                              unsigend int base,
>                                              unsigned int offset,
>                                              unsigned int *dir_out,
>                                              unsigned int *dir_in)
> {
>     unsigned int line = offset % gpio->ngpio_per_reg;
>     unsigned int mask = BIT(line);
>
>     if (!gpio->invert_direction) {
>         *dir_out = mask;
>         *dir_in = 0;
>     } else {
>         *dir_out = 0;
>         *dir_in = mask;
>     }
>
>     return 0;
> }

This isn't really an independent function: what do *dir_out and *dir_in
mean on their own? You need use the matching mask from ->reg_mask_xlate
for those values to be of any use. And those two functions have to match
up because they need to agree on the same mask.

>
> And in the {set,get}_direction() you can then check both
> values and convert it from or to GPIO_LINE_DIRECTION_{OUT,IN}.

Agreed, checking both values and erroring out if the register has an
unexpected value is a good idea.

>
> Thoughts?
>
> -michael