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

Fair enough. I'll use your approach then.

I thought the semantic mix-up was a worthwhile compromise, but
perhaps not. Technically the part that 'interprets' is not the
values themselves, but the index into the values array, which
makes things a bit more confusing. You have to keep in mind how
the registers would behave if you had a single bit, because it's
the bit value that indexes the values array. It's not _that_ hard
to keep straight but obviously... not as obvious. :)