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

[Michael Walle]

Am 2022-07-06 22:46, schrieb Aidan MacDonald:
> > Am 2022-07-04 18:01, schrieb Aidan MacDonald:
> > > > 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..
>
> I found three drivers (not exhaustive) that have fields for setting the
> output level: gpio-amd8111, gpio-creg-snps, and gpio-lp3943. Admittedly
> that's more than I expected, so maybe we shouldn't dismiss the idea of
> multi-bit output fields.

I'm not dismissing it, but I want to wait for an actual user
for it :)

> If you still think the API you're suggesting is better then I can go
> with it, but IMHO it's more code and more special cases, even if only
> a little bit.

What bothers me with your approach is that you are returning
an integer and in one case you are interpreting it as gpio
direction and in the other case you are interpreting it as
a gpio state, while mine is more explicit, i.e. a
xlate_direction() for the direction and if there will be
support for multi bit input/output then there would be a
xlate_state() or similar. Granted, it is more code, but
easier to understand IMHO.

-michael