mbox series

[0/3] gpio-regmap support for register fields and other hooks

Message ID 20220703111057.23246-1-aidanmacdonald.0x0@gmail.com
Headers show
Series gpio-regmap support for register fields and other hooks | expand

Message

Aidan MacDonald July 3, 2022, 11:10 a.m. UTC
This is a small series to expand the usefulness of gpio-regmap.

Patch 1 allows GPIO direction and level to be mapped to values in a register
field for cases where a one-bit-per-GPIO mapping is insufficient.

Patch 2 allows gpio-regmap to be used for the GPIO portion of a combined
pin control + GPIO driver by deferring some ops to the pin control subsystem.

Patch 3 allows drivers to provide a custom ->to_irq() hook for the GPIO chip
as an alternative to using an IRQ domain.

Aidan MacDonald (3):
  gpio: regmap: Support registers with more than one bit per GPIO
  gpio: regmap: Support combined GPIO and pin control drivers
  gpio: regmap: Support a custom ->to_irq() hook

 drivers/gpio/gpio-regmap.c  | 110 ++++++++++++++++++++++++++----------
 include/linux/gpio/regmap.h |  24 ++++++++
 2 files changed, 103 insertions(+), 31 deletions(-)

Comments

Aidan MacDonald July 4, 2022, 4:03 p.m. UTC | #1
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> 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.

The documentation states that ->reg_field_xlate returns values[0] and
values[1] for low/high level or input/output direction. IOW, 0 is low
level / input direction and 1 is high level / output direction.

Embedding the array in a struct seems like a better idea though, thanks.
Aidan MacDonald July 4, 2022, 4:38 p.m. UTC | #2
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> wrote:
>>
>> Some GPIO chips require a custom to_irq() callback for mapping
>> their IRQs, eg. because their interrupts come from a parent IRQ
>> chip where the GPIO offset doesn't map 1-to-1 with hwirq number.
>
> Don't they follow a hierarchical IRQ domain in that case?
>
> And to be honest after the commit ef38237444ce ("gpiolib: add a
> warning on gpiochip->to_irq defined") I have no idea how it works in
> your case and also I feel this patch is a wrong direction of
> development.

My own use case is an MFD device with a shared IRQ chip that is
used by other sub-drivers. This is a very common case that seems
to map onto ->to_irq() cleanly. Do we really need an IRQ domain?
What you're suggesting would be a 1-to-1 mapping from GPIO offset
to hwirq number in a virtual domain, then remapping to the real
hwirq number, which seems unnecessarily complicated when we can
just change the GPIO offset -> hwirq mapping.

The commit you mentioned is warning users of GPIOLIB_IRQCHIP when a
custom ->to_irq() method is overridden. That's not relevant here.
Using an IRQ domain also overrides ->to_irq() so I included a check
in this patch to ensure gpio-regmap chips are well-behaved.
Andy Shevchenko July 6, 2022, 11:45 a.m. UTC | #3
On Tue, Jul 5, 2022 at 1:22 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:

...

> Is that really better than simply using ->to_irq()?

We have Intel PMIC drivers (that are in MFD) and they have respective
GPIO drivers, none of them is using ->to_irq() and all of them provide
IRQ functionality. Can it be taken as an example or is it something
quite different to your hardware?
Linus Walleij July 6, 2022, 12:02 p.m. UTC | #4
On Tue, Jul 5, 2022 at 1:08 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

> I'm not trying to argue that hierarchical IRQ domains are always a bad
> thing -- I'm just pointing out they're not always useful or necessary.
> All your points make sense when the GPIO controller is a large distinct
> block with potentially many GPIOs. When we're dealing with an MFD device
> with just a few GPIOs, maybe even just one, having a separate IRQ domain
> makes less sense; the added structure is generally not useful.

Do you mean your driver does this:

MFD main device
MFD irqchip
 |
 +->  MFD gpiochip
         No irqchip here, so .to_irq() just refers ^ to that one up there

IIUC you mean that if I want to use the irqchip directly then
I have to refer to the MFD irqchip, I just cannot refer to the
gpiochip subnode because that one does not have an irqchip.

// Getting GPIO from gpiochip and irq from MFD device
// for the same GPIO line
gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
irqs = <&mfd 114 IRQ_EDGE_RISING>;

Then for a Linux driver this can be papered over by using the
.to_irq() callback and just defining gpios.

This isn't very good, if you created a separate gpiochip then you
should have a separate (hierarchical) irqchip associated with that
gpiochip as well.

// Getting GPIO and irq from the same gpiochip node
gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
irqs = <&gpio 3 IRQ_EDGE_RISING>;

I made this mistake with the ab8500 driver and
I would not do it like this today. I would use hierarchical gpio
irqchip. And I should go and fix it. (Is on my TODO.)

> Looking at other GPIO drivers using a hierarchical IRQ domain, they
> include their own IRQ chips with specialized ops. In my case I don't
> need any of that (and it'd be the same with other MFD devices) so it
> looks like using an IRQ domain would mean I'd have to create a fake
> IRQ chip and domain just to translate between two number spaces.
>
> Is that really better than simply using ->to_irq()?

To be honest most irqchips are "fake", what they mostly do is figure
out which of a few internal sources that fired the irq, so it models the
different things connected to a single IRQ line.

So yeah, I think the hierarchical irqchip is worth it, especially if that
means the offset of the irqs and gpios become the same.

Maybe we can add more helpers in the core to make it dirt simple
though? It would help others with the same problem.

Yours,
Linus Walleij
Aidan MacDonald July 6, 2022, 1:50 p.m. UTC | #5
Linus Walleij <linus.walleij@linaro.org> writes:

> On Tue, Jul 5, 2022 at 1:08 PM Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> wrote:
>> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> I'm not trying to argue that hierarchical IRQ domains are always a bad
>> thing -- I'm just pointing out they're not always useful or necessary.
>> All your points make sense when the GPIO controller is a large distinct
>> block with potentially many GPIOs. When we're dealing with an MFD device
>> with just a few GPIOs, maybe even just one, having a separate IRQ domain
>> makes less sense; the added structure is generally not useful.
>
> Do you mean your driver does this:
>
> MFD main device
> MFD irqchip
>  |
>  +->  MFD gpiochip
>          No irqchip here, so .to_irq() just refers ^ to that one up there
>
> IIUC you mean that if I want to use the irqchip directly then
> I have to refer to the MFD irqchip, I just cannot refer to the
> gpiochip subnode because that one does not have an irqchip.

Yep, that's right.

> // Getting GPIO from gpiochip and irq from MFD device
> // for the same GPIO line
> gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
> irqs = <&mfd 114 IRQ_EDGE_RISING>;
>
> Then for a Linux driver this can be papered over by using the
> .to_irq() callback and just defining gpios.
>
> This isn't very good, if you created a separate gpiochip then you
> should have a separate (hierarchical) irqchip associated with that
> gpiochip as well.
>
> // Getting GPIO and irq from the same gpiochip node
> gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
> irqs = <&gpio 3 IRQ_EDGE_RISING>;
>
> I made this mistake with the ab8500 driver and
> I would not do it like this today. I would use hierarchical gpio
> irqchip. And I should go and fix it. (Is on my TODO.)
>

If moving to hierarchical IRQ chips is the plan, could we add a note
to say .to_irq() is discouraged and shouldn't be used in new code?
Based on what you're saying (which I agree makes sense) it sounds
like there's really no reason to ever use .to_irq().

>> Looking at other GPIO drivers using a hierarchical IRQ domain, they
>> include their own IRQ chips with specialized ops. In my case I don't
>> need any of that (and it'd be the same with other MFD devices) so it
>> looks like using an IRQ domain would mean I'd have to create a fake
>> IRQ chip and domain just to translate between two number spaces.
>>
>> Is that really better than simply using ->to_irq()?
>
> To be honest most irqchips are "fake", what they mostly do is figure
> out which of a few internal sources that fired the irq, so it models the
> different things connected to a single IRQ line.
>
> So yeah, I think the hierarchical irqchip is worth it, especially if that
> means the offset of the irqs and gpios become the same.
>
> Maybe we can add more helpers in the core to make it dirt simple
> though? It would help others with the same problem.
>
> Yours,
> Linus Walleij

Okay, that sounds like a good plan. I'll look more carefully at the
existing drivers and see if I can use existing gpiolib helpers.

One potential issue (from reading the code) is that hierarchical IRQ
domains seemingly can't have a non-hierarchical domain as the parent:
irq_domain_alloc_irqs_parent() calls irq_domain_alloc_irqs_hierarchy()
and the latter fails with -ENOSYS for a non-hierarchical domain.

In my case I'm using a regmap IRQ chip, which is non-hierarchical,
so perhaps that will need to be expanded? 

Regards,
Aidan
Aidan MacDonald July 6, 2022, 8:46 p.m. UTC | #6
Michael Walle <michael@walle.cc> writes:

> Am 2022-07-04 18:01, schrieb Aidan MacDonald:
>> Michael Walle <michael@walle.cc> 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..
>

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.

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.

>>>  - 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?
Aidan MacDonald July 6, 2022, 8:53 p.m. UTC | #7
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, Jul 5, 2022 at 1:22 PM Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> wrote:
>
> ...
>
>> Is that really better than simply using ->to_irq()?
>
> We have Intel PMIC drivers (that are in MFD) and they have respective
> GPIO drivers, none of them is using ->to_irq() and all of them provide
> IRQ functionality. Can it be taken as an example or is it something
> quite different to your hardware?

In the Intel PMICs the MFD irqchip has a single interrupt for all GPIOs.
The GPIO driver then has its own irqchip and it looks at other registers
to find out which GPIO interrupt fired. It's a typical cascaded setup.

In my case the MFD irqchip has one interrupt per GPIO. The GPIO driver
does not need its own irqchip; everything is handled by the MFD irqchip.
Existing examples include wm831x, wm8994, da9052, and tps6586x.
Michael Walle July 7, 2022, 7:44 a.m. UTC | #8
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
Aidan MacDonald July 7, 2022, 2:58 p.m. UTC | #9
Michael Walle <michael@walle.cc> 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. :)