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

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

[Aidan MacDonald]

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

-- 
2.35.1

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

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

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/gpio/gpio-regmap.c  | 73 +++++++++++++++++++++----------------
 include/linux/gpio/regmap.h | 14 +++++++
 2 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 6383136cbe59..9256b922c654 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -27,6 +27,10 @@ struct gpio_regmap {
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
+	void (*reg_field_xlate)(struct gpio_regmap *gpio,
+				unsigned int base, unsigned int offset,
+				unsigned int *reg, unsigned int *mask,
+				unsigned int *values);
 
 	void *driver_data;
 };
@@ -52,10 +56,22 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
 	return 0;
 }
 
+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;
+}
+
 static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
 	unsigned int base, val, reg, mask;
+	unsigned int values[2];
 	int ret;
 
 	/* we might not have an output register if we are input only */
@@ -64,15 +80,13 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 	else
 		base = gpio_regmap_addr(gpio->reg_set_base);
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	if (ret)
-		return ret;
+	gpio->reg_field_xlate(gpio, base, offset, &reg, &mask, values);
 
 	ret = regmap_read(gpio->regmap, reg, &val);
 	if (ret)
 		return ret;
 
-	return !!(val & mask);
+	return (val & mask) == values[1];
 }
 
 static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
@@ -81,12 +95,10 @@ static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
 	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
 	unsigned int reg, mask;
+	unsigned int values[2];
 
-	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	if (val)
-		regmap_update_bits(gpio->regmap, reg, mask, mask);
-	else
-		regmap_update_bits(gpio->regmap, reg, mask, 0);
+	gpio->reg_field_xlate(gpio, base, offset, &reg, &mask, values);
+	regmap_update_bits(gpio->regmap, reg, mask, values[!!val]);
 }
 
 static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
@@ -94,14 +106,15 @@ static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
 	unsigned int base, reg, mask;
+	unsigned int values[2];
 
 	if (val)
 		base = gpio_regmap_addr(gpio->reg_set_base);
 	else
 		base = gpio_regmap_addr(gpio->reg_clr_base);
 
-	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	regmap_write(gpio->regmap, reg, mask);
+	gpio->reg_field_xlate(gpio, base, offset, &reg, &mask, values);
+	regmap_update_bits(gpio->regmap, reg, mask, values[1]);
 }
 
 static int gpio_regmap_get_direction(struct gpio_chip *chip,
@@ -109,6 +122,7 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
 	unsigned int base, val, reg, mask;
+	unsigned int values[2];
 	int invert, ret;
 
 	if (gpio->reg_dir_out_base) {
@@ -121,47 +135,36 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
 		return -EOPNOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	if (ret)
-		return ret;
+	gpio->reg_field_xlate(gpio, base, offset, &reg, &mask, values);
 
 	ret = regmap_read(gpio->regmap, reg, &val);
 	if (ret)
 		return ret;
 
-	if (!!(val & mask) ^ invert)
-		return GPIO_LINE_DIRECTION_OUT;
-	else
+	if ((val & mask) == values[invert])
 		return GPIO_LINE_DIRECTION_IN;
+	else
+		return GPIO_LINE_DIRECTION_OUT;
 }
 
 static int gpio_regmap_set_direction(struct gpio_chip *chip,
 				     unsigned int offset, bool output)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
-	unsigned int base, val, reg, mask;
-	int invert, ret;
+	unsigned int base, reg, mask;
+	unsigned int values[2];
 
 	if (gpio->reg_dir_out_base) {
 		base = gpio_regmap_addr(gpio->reg_dir_out_base);
-		invert = 0;
 	} else if (gpio->reg_dir_in_base) {
 		base = gpio_regmap_addr(gpio->reg_dir_in_base);
-		invert = 1;
+		output = !output;
 	} else {
 		return -EOPNOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	if (ret)
-		return ret;
-
-	if (invert)
-		val = output ? 0 : mask;
-	else
-		val = output ? mask : 0;
-
-	return regmap_update_bits(gpio->regmap, reg, mask, val);
+	gpio->reg_field_xlate(gpio, base, offset, &reg, &mask, values);
+	return regmap_update_bits(gpio->regmap, reg, mask, values[output]);
 }
 
 static int gpio_regmap_direction_input(struct gpio_chip *chip,
@@ -215,6 +218,10 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (config->reg_dir_out_base && config->reg_dir_in_base)
 		return ERR_PTR(-EINVAL);
 
+	/* only one of these should be provided */
+	if (config->reg_field_xlate && config->reg_mask_xlate)
+		return ERR_PTR(-EINVAL);
+
 	gpio = kzalloc(sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
 		return ERR_PTR(-ENOMEM);
@@ -225,6 +232,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	gpio->ngpio_per_reg = config->ngpio_per_reg;
 	gpio->reg_stride = config->reg_stride;
 	gpio->reg_mask_xlate = config->reg_mask_xlate;
+	gpio->reg_field_xlate = config->reg_field_xlate;
 	gpio->reg_dat_base = config->reg_dat_base;
 	gpio->reg_set_base = config->reg_set_base;
 	gpio->reg_clr_base = config->reg_clr_base;
@@ -242,6 +250,9 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (!gpio->reg_mask_xlate)
 		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
 
+	if (!gpio->reg_field_xlate)
+		gpio->reg_field_xlate = gpio_regmap_simple_field_xlate;
+
 	chip = &gpio->gpio_chip;
 	chip->parent = config->parent;
 	chip->fwnode = config->fwnode;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index a9f7b7faf57b..a673dbfe88a3 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -37,6 +37,10 @@ struct regmap;
  *			offset to a register/bitmask pair. If not
  *			given the default gpio_regmap_simple_xlate()
  *			is used.
+ * @reg_field_xlate:	(Optional) Translates base address and GPIO offset
+ *			to register, mask, and field values. If not given
+ *			the default gpio_regmap_field_xlate() is used, which
+ *			is implemented in terms of ->reg_mask_xlate.
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
@@ -45,6 +49,12 @@ struct regmap;
  * register and mask pair. The base address is one of the given register
  * base addresses in this structure.
  *
+ * ->reg_field_xlate supports chips that have more than one bit per GPIO for
+ * controlling the output level or direction. In addition to the register and
+ * mask it should output field values for low/high level or in/out direction
+ * in ``values[0]`` and ``values[1]``. If ->reg_field_xlate is provided then
+ * it overrides any ->reg_mask_xlate callback.
+ *
  * Although all register base addresses are marked as optional, there are
  * several rules:
  *     1. if you only have @reg_dat_base set, then it is input-only
@@ -81,6 +91,10 @@ struct gpio_regmap_config {
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
+	void (*reg_field_xlate)(struct gpio_regmap *gpio,
+				unsigned int base, unsigned int offset,
+				unsigned int *reg, unsigned int *mask,
+				unsigned int *values);
 
 	void *drvdata;
 };
-- 
2.35.1

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Aidan MacDonald]

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.

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

[Michael Walle]

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

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

And in the {set,get}_direction() you can then check both
values and convert it from or to GPIO_LINE_DIRECTION_{OUT,IN}.

Thoughts?

-michael

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

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

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

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

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

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

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

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

[Aidan MacDonald]

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?

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

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

[Aidan MacDonald]

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

[PATCH 2/3] gpio: regmap: Support combined GPIO and pin control drivers

[Aidan MacDonald]

Allow gpio-regmap to be used for the GPIO portion of a combined
pin control and GPIO driver by setting the has_pinctrl flag. This
flag will cause GPIO direction set ops to be implemented as calls
to pinctrl_gpio_direction_input/output() instead of updating the
direction set registers directly.

Note that reg_dir_out/in_base is still required for implementing
the GPIO chip's ->get_direction() callback.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/gpio/gpio-regmap.c  | 20 ++++++++++++++++++++
 include/linux/gpio/regmap.h |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 9256b922c654..4bc01329fb14 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -24,6 +24,8 @@ struct gpio_regmap {
 	unsigned int reg_dir_in_base;
 	unsigned int reg_dir_out_base;
 
+	unsigned int has_pinctrl:1;
+
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
@@ -170,14 +172,24 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 static int gpio_regmap_direction_input(struct gpio_chip *chip,
 				       unsigned int offset)
 {
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+
+	if (gpio->has_pinctrl)
+		return pinctrl_gpio_direction_input(chip->base + offset);
+
 	return gpio_regmap_set_direction(chip, offset, false);
 }
 
 static int gpio_regmap_direction_output(struct gpio_chip *chip,
 					unsigned int offset, int value)
 {
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+
 	gpio_regmap_set(chip, offset, value);
 
+	if (gpio->has_pinctrl)
+		return pinctrl_gpio_direction_output(chip->base + offset);
+
 	return gpio_regmap_set_direction(chip, offset, true);
 }
 
@@ -218,6 +230,14 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (config->reg_dir_out_base && config->reg_dir_in_base)
 		return ERR_PTR(-EINVAL);
 
+	/*
+	 * we need a direction register for implementing ->get_direction
+	 * even if ->direction_input/output is handled by pin control
+	 */
+	if (config->has_pinctrl && !(config->reg_dir_in_base ||
+				     config->reg_dir_out_base))
+		return ERR_PTR(-EINVAL);
+
 	/* only one of these should be provided */
 	if (config->reg_field_xlate && config->reg_mask_xlate)
 		return ERR_PTR(-EINVAL);
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index a673dbfe88a3..47acea8cca32 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -33,6 +33,10 @@ struct regmap;
  * @ngpio_per_reg:	Number of GPIOs per register
  * @irq_domain:		(Optional) IRQ domain if the controller is
  *			interrupt-capable
+ * @has_pinctrl:	If set, the GPIO chip is part of a combined pin control
+ *			and GPIO driver; use pinctrl_gpio_direction_input() and
+ *			pinctrl_gpio_direction_output() to implement direction
+ *			set operations.
  * @reg_mask_xlate:     (Optional) Translates base address and GPIO
  *			offset to a register/bitmask pair. If not
  *			given the default gpio_regmap_simple_xlate()
@@ -88,6 +92,8 @@ struct gpio_regmap_config {
 	int ngpio_per_reg;
 	struct irq_domain *irq_domain;
 
+	unsigned int has_pinctrl:1;
+
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
-- 
2.35.1

Re: [PATCH 2/3] gpio: regmap: Support combined GPIO and pin control drivers

[Andy Shevchenko]

On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
>
> Allow gpio-regmap to be used for the GPIO portion of a combined
> pin control and GPIO driver by setting the has_pinctrl flag. This
> flag will cause GPIO direction set ops to be implemented as calls
> to pinctrl_gpio_direction_input/output() instead of updating the
> direction set registers directly.
>
> Note that reg_dir_out/in_base is still required for implementing
> the GPIO chip's ->get_direction() callback.

...

> +       /*
> +        * we need a direction register for implementing ->get_direction
> +        * even if ->direction_input/output is handled by pin control
> +        */

/*
 * Multi-line comments go with this format
 * or style. Pay attention to the capitalization
 * and English grammar, e.g. period at the end of sentence(s).
 */

...

> +       if (config->has_pinctrl && !(config->reg_dir_in_base ||
> +                                    config->reg_dir_out_base))

Can you re-indent this either to be one line or put the second part of
the conditional onto the second line?

And why not use && everywhere?

> +               return ERR_PTR(-EINVAL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] gpio: regmap: Support combined GPIO and pin control drivers

[Aidan MacDonald]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Sun, Jul 3, 2022 at 1:11 PM Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> wrote:
>>
>> Allow gpio-regmap to be used for the GPIO portion of a combined
>> pin control and GPIO driver by setting the has_pinctrl flag. This
>> flag will cause GPIO direction set ops to be implemented as calls
>> to pinctrl_gpio_direction_input/output() instead of updating the
>> direction set registers directly.
>>
>> Note that reg_dir_out/in_base is still required for implementing
>> the GPIO chip's ->get_direction() callback.
>
> ...
>
>> +       /*
>> +        * we need a direction register for implementing ->get_direction
>> +        * even if ->direction_input/output is handled by pin control
>> +        */
>
> /*
>  * Multi-line comments go with this format
>  * or style. Pay attention to the capitalization
>  * and English grammar, e.g. period at the end of sentence(s).
>  */
>

I used this "style" to match the surrounding code, but I suppose
I might as well fix the other comments while I'm here.

>> +       if (config->has_pinctrl && !(config->reg_dir_in_base ||
>> +                                    config->reg_dir_out_base))
>
> Can you re-indent this either to be one line or put the second part of
> the conditional onto the second line?

Yep.

>
> And why not use && everywhere?
>

No reason to be honest, but maybe it's easier to understand?

  "has pin control and doesn't set reg_dir_in_base or reg_dir_out_base".

Using && is more like this:

  "has pin control, doesn't set reg_dir_in_base, and doesn't
  set reg_dir_out_base".

[PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald]

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.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/gpio/gpio-regmap.c  | 17 +++++++++++++++++
 include/linux/gpio/regmap.h |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 4bc01329fb14..d11b202e51fd 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -34,6 +34,8 @@ struct gpio_regmap {
 				unsigned int *reg, unsigned int *mask,
 				unsigned int *values);
 
+	int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset);
+
 	void *driver_data;
 };
 
@@ -193,6 +195,13 @@ static int gpio_regmap_direction_output(struct gpio_chip *chip,
 	return gpio_regmap_set_direction(chip, offset, true);
 }
 
+static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+
+	return gpio->to_irq(gpio, offset);
+}
+
 void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
 {
 	return gpio->driver_data;
@@ -242,6 +251,10 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (config->reg_field_xlate && config->reg_mask_xlate)
 		return ERR_PTR(-EINVAL);
 
+	/* an irq_domain will override the to_irq hook, so don't allow both */
+	if (config->irq_domain && config->to_irq)
+		return ERR_PTR(-EINVAL);
+
 	gpio = kzalloc(sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
 		return ERR_PTR(-ENOMEM);
@@ -253,6 +266,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	gpio->reg_stride = config->reg_stride;
 	gpio->reg_mask_xlate = config->reg_mask_xlate;
 	gpio->reg_field_xlate = config->reg_field_xlate;
+	gpio->to_irq = config->to_irq;
 	gpio->reg_dat_base = config->reg_dat_base;
 	gpio->reg_set_base = config->reg_set_base;
 	gpio->reg_clr_base = config->reg_clr_base;
@@ -302,6 +316,9 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 		chip->direction_output = gpio_regmap_direction_output;
 	}
 
+	if (gpio->to_irq)
+		chip->to_irq = gpio_regmap_to_irq;
+
 	ret = gpiochip_add_data(chip, gpio);
 	if (ret < 0)
 		goto err_free_gpio;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 47acea8cca32..9755854d6747 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -45,6 +45,8 @@ struct regmap;
  *			to register, mask, and field values. If not given
  *			the default gpio_regmap_field_xlate() is used, which
  *			is implemented in terms of ->reg_mask_xlate.
+ * @to_irq:		(Optional) hook for supporting custom IRQ mappings,
+ *			behaves the same as the gpio_chip to_irq hook.
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
@@ -102,6 +104,8 @@ struct gpio_regmap_config {
 				unsigned int *reg, unsigned int *mask,
 				unsigned int *values);
 
+	int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset);
+
 	void *drvdata;
 };
 
-- 
2.35.1

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald]

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.

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Linus Walleij]

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

What is the usecase for this?

Since GPIO chips and IRQ chips are orthogonal there is absolutely
no guarantee that ->to_irq() is called before a driver start to use
an IRQ from the irqchip side:

(quoting Documentation/driver-api/gpio/driver.rst)

 It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
 is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
 irq_chip are orthogonal, and offering their services independent of each
 other.

 gpiod_to_irq() is just a convenience function to figure out the IRQ for a
 certain GPIO line and should not be relied upon to have been called before
 the IRQ is used.

 Always prepare the hardware and make it ready for action in respective
 callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
 been called first.

(end quote)

Using ->to_irq() makes sense in a few cases such as when
a GPIO key that can also poll for state want to get hold of an
IRQ to react to edges.

Now: if a consumer requests IRQ nr 3 from your driver say from ACPI or
from a device tree, and as you say GPIOs and IRQs are not 1-to-1 mapped,
so IRQ nr 3 may be coming from GPIO 314, isn't this going to be really
messy for users? One local numberspace for GPIO and another local
numberspace for IRQs?

To me it seems like the reasoning is something like

- I only use GPIO line numbers like <&gpio 3>;
- Then I call gpiod_to_irq() on that number so I do not need to
  deal with looking up the IRQ some other way
- request_irq();
- Profit.

There is no guarantee that the API will be used like that at all, actually
it is uncommon.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Sun, Jul 3, 2022 at 1:10 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.
>>
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>
> What is the usecase for this?

This is a generic GPIO chip; so I guess any use case for ->to_irq()?

> Since GPIO chips and IRQ chips are orthogonal there is absolutely
> no guarantee that ->to_irq() is called before a driver start to use
> an IRQ from the irqchip side:
>
> (quoting Documentation/driver-api/gpio/driver.rst)
>
>  It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
>  is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
>  irq_chip are orthogonal, and offering their services independent of each
>  other.
>
>  gpiod_to_irq() is just a convenience function to figure out the IRQ for a
>  certain GPIO line and should not be relied upon to have been called before
>  the IRQ is used.
>
>  Always prepare the hardware and make it ready for action in respective
>  callbacks from the GPIO and irq_chip APIs. Do not rely on gpiod_to_irq() having
>  been called first.
>
> (end quote)

That's fine, and I don't require ->to_irq() to be called. The IRQ chip
in my case is provided by an MFD driver (axp20x to be specific) and it
*does* work orthogonally to the GPIO driver -- the GPIO driver neither
knows nor cares about the IRQ chip.

> Using ->to_irq() makes sense in a few cases such as when
> a GPIO key that can also poll for state want to get hold of an
> IRQ to react to edges.

This is my use case; specifically, GPIO keys and ASoC jack detection.

> Now: if a consumer requests IRQ nr 3 from your driver say from ACPI or
> from a device tree, and as you say GPIOs and IRQs are not 1-to-1 mapped,
> so IRQ nr 3 may be coming from GPIO 314, isn't this going to be really
> messy for users? One local numberspace for GPIO and another local
> numberspace for IRQs?

Well, this is how MFD drivers with GPIO functionality often work, they
aren't creating a special IRQ sub-domain for GPIOs and it doesn't seem
to be a problem there. Most likely because those MFD devices are being
used for GPIO keys or something similar.

Referring to the interrupt directly would make sense if the GPIO was
wired to another chip's IRQ line, but that is unlikely to be the case
for MFD devices because they're behind a slow bus.

> To me it seems like the reasoning is something like
>
> - I only use GPIO line numbers like <&gpio 3>;
> - Then I call gpiod_to_irq() on that number so I do not need to
>   deal with looking up the IRQ some other way
> - request_irq();
> - Profit.
>

Yeah, that's accurate for my use case.

> There is no guarantee that the API will be used like that at all, actually
> it is uncommon.
>
> Yours,
> Linus Walleij

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.

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

Regards,
Aidan

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald]

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.

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Linus Walleij]

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

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Aidan MacDonald]

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

Re: [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook

[Linus Walleij]

On Wed, Jul 6, 2022 at 4:21 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:

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

There is a reason to use .to_itq() to get the irq associated with
a GPIO, in a Linux kernel context. Especially when doing exactly
what IRQ keys are doing: finding the IRQ for a GPIO line, if any.

But it is not intended to provide a random IRQ from anywhere
else in the system. Maybe I could clarify that.

There are many old boardfile-instances of GPIO where .to_irq()
may be doing something really funky, these are not really up to
date with the current idea of how we model systems in hardware
description languages such as DT or ACPI DSDT.

There may be archs that do not even have DT or ACPI or similar,
there it would be fine to use it in a more liberal way. This may
be the case for m68k or something so I cannot speak about
everything here.

I understand that it might be annoying that this is a bit fuzzy
around the edges.

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

Yes that is a characteristic of hierarchical irq domains, they have to
be hierarchical all the way down. It is very often what we want
as well.

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

Yes I'm sorry if it adds work to your plate :(

However I think the end result would be very useful for everyone.
Maybe Marc Z has something to add here?

Yours,
Linus Walleij