| Message ID | 20220703111057.23246-1-aidanmacdonald.0x0@gmail.com |
|---|---|
| Headers | show |
| Series | gpio-regmap support for register fields and other hooks | expand |
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.
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.
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?
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
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
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?
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.
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
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. :)