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

[Michael Walle]

Am 2022-07-04 18:01, schrieb Aidan MacDonald:
> 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.

I'd prefer to keep it to the current use case. I'm not sure if
there are many controllers which have more than one bit for
the input and output state. And if, we are still limited to
one register, what if the bits are distributed among multiple
registers..

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

It is just an internal implementation detail; I'm not talking
about changing the configuration. And actually, there was
once confusion about the reg_dir_in_base and reg_dir_out_base, IIRC.
I'd need to find that thread again. But for now, I'd keep the
configuration anyway.

Think about it. If you already have the value translation (which you
 need), why would you still do the invert inside the
{set,get}_direction? It is just a use case of the translation
function actually. (Also, an invert only makes sense with a one
bit value).

You could do something like:
if (config->reg_dir_out_base) {
   gpio->xlat_direction = gpio_regmap_simple_xlat_direction;
   gpio->reg_dir_base = config->reg_dir_out_base;
}
if (config->reg_dir_in_base) {
   gpio->xlat_direction = gpio_regmap_simple_xlat_direction_inverted;
   gpio->reg_dir_base = config->reg_dir_in_base;
}

But both of these function would be almost the same, thus my
example below.

Mhh. Actually I just noticed while writing this.. we need a new
config->reg_dir_base anyway, otherwise you'd need to either pick
reg_dir_in_base or reg_dir_out_base to work with a custom
.xlat_direction callback.

if (config->xlat_direction) {
   gpio->xlat_direction = config->gpio_xlat_direction;
   gpio->reg_dir_base = config->reg_dir_base;
}

Since there are no users of config->reg_dir_in_base, we can just kill
that one. These were just added because it was based on bgpio. Then
it will just be:

gpio->reg_dir_base = config->reg_dir_base;
gpio->direction_xlat = config->direction_xlat;
if (!gpio->direction_xlat)
  gpio->direction_xlat = gpio_regmap_simple_direction_xlat;

If someone needs an inverted direction, he can either have a custom
direction_xlat or we'll introduce a config->invert_direction option.

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

Yes. I was thinking it isn't an issue because the driver implementing 
this
will need to know the mask anyway. But maybe it is better to also pass
the mask, which was obtained by the .reg_mask_xlat(). Or we could just
repeat the corresponding value within the value and the caller could
also apply the mask to this returned value.

I.e. if you have a two bit value 01 for output and 10 for input and
you have a 32bit register with 16 values, you can use
 *dir_out = 0x55555555;
 *dir_in = 0xaaaaaaaa;

Not that easy to understand. But maybe you find it easier than me
to write documentation ;)

-michael

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