Migrate i8255 GPIO drivers to regmap API

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-regmap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/gpio/gpio-regmap.c +6 -1

index 6383136cbe59..f907c9c19fce 100644
@@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
 	unsigned int base, val, reg, mask;
 	int invert, ret;
 
+	if (gpio->reg_dat_base && !gpio->reg_set_base)
+		return GPIO_LINE_DIRECTION_IN;
+	if (gpio->reg_set_base && !gpio->reg_dat_base)
+		return GPIO_LINE_DIRECTION_OUT;
+
 	if (gpio->reg_dir_out_base) {
 		base = gpio_regmap_addr(gpio->reg_dir_out_base);
 		invert = 0;

@@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	else if (gpio->reg_set_base)
 		chip->set = gpio_regmap_set;
 
+	chip->get_direction = gpio_regmap_get_direction;
 	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
-		chip->get_direction = gpio_regmap_get_direction;
 		chip->direction_input = gpio_regmap_direction_input;
 		chip->direction_output = gpio_regmap_direction_output;
 	}

Signature · 1 line

2.38.1

Quoted 5 lines · William Breathitt Gray

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Seems legit to me. Have you checked if we have any gpio-regmap drivers that have something like this in their configuration already? In such cases we need to be sure they behave as expected.

From the code perspective:

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 34 lines

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-regmap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/gpio/gpio-regmap.c +6 -1

index 6383136cbe59..f907c9c19fce 100644
@@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
 	unsigned int base, val, reg, mask;
 	int invert, ret;
 
+	if (gpio->reg_dat_base && !gpio->reg_set_base)
+		return GPIO_LINE_DIRECTION_IN;
+	if (gpio->reg_set_base && !gpio->reg_dat_base)
+		return GPIO_LINE_DIRECTION_OUT;
+
 	if (gpio->reg_dir_out_base) {
 		base = gpio_regmap_addr(gpio->reg_dir_out_base);
 		invert = 0;

@@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	else if (gpio->reg_set_base)
 		chip->set = gpio_regmap_set;
 
+	chip->get_direction = gpio_regmap_get_direction;
 	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
-		chip->get_direction = gpio_regmap_get_direction;
 		chip->direction_input = gpio_regmap_direction_input;
 		chip->direction_output = gpio_regmap_direction_output;
 	}

Signature · 1 line

2.38.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 11 lines · Andy Shevchenko

Quoted 5 lines · William Breathitt Gray

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Seems legit to me. Have you checked if we have any gpio-regmap drivers that have something like this in their configuration already? In such cases we need to be sure they behave as expected.

Quoted 2 lines

From the code perspective:

Reviewed-by Andy Shevchenko <[email protected]>

I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like the change in this patch will cause problems for them, but I'll let Michael Walle and Robert Marko comment if they see issues here.

William Breathitt Gray

Quoted 40 lines

Quoted 32 lines

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-regmap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/gpio/gpio-regmap.c +6 -2

index 6383136cbe59..f907c9c19fce 100644
@@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
    unsigned int base, val, reg, mask;
    int invert, ret;
 
+   if (gpio->reg_dat_base && !gpio->reg_set_base)
+           return GPIO_LINE_DIRECTION_IN;
+   if (gpio->reg_set_base && !gpio->reg_dat_base)
+           return GPIO_LINE_DIRECTION_OUT;
+
    if (gpio->reg_dir_out_base) {
            base = gpio_regmap_addr(gpio->reg_dir_out_base);
            invert = 0;

@@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
    else if (gpio->reg_set_base)
            chip->set = gpio_regmap_set;
 
+   chip->get_direction = gpio_regmap_get_direction;
    if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
-           chip->get_direction = gpio_regmap_get_direction;
            chip->direction_input = gpio_regmap_direction_input;
            chip->direction_output = gpio_regmap_direction_output;
    }
--

Quoted 2 lines

2.38.1

--
With Best Regards,
Andy Shevchenko

Quoted 11 lines · William Breathitt Gray

Quoted 5 lines

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Seems legit to me. Have you checked if we have any gpio-regmap drivers that have something like this in their configuration already? In such cases we need to be sure they behave as expected.

Quoted 2 lines

From the code perspective:

Reviewed-by Andy Shevchenko <[email protected]>

I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like the change in this patch will cause problems for them, but I'll let Michael Walle and Robert Marko comment if they see issues here.

Am 2022-11-13 14:21, schrieb William Breathitt Gray:

Quoted 15 lines · Andy Shevchenko

Quoted 14 lines

Quoted 5 lines · William Breathitt Gray

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Seems legit to me. Have you checked if we have any gpio-regmap drivers that
have something like this in their configuration already? In such cases we need
to be sure they behave as expected.

Quoted 8 lines

Quoted 2 lines

From the code perspective:

Reviewed-by Andy Shevchenko <[email protected]>

I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like the change in this patch will cause problems for them, but I'll let Michael Walle and Robert Marko comment if they see issues here.

For the sl28cpld driver this shouldn't be a problem. So for that

Acked-by Michael Walle <[email protected]>

But back when I wrote gpio-regmap the bgpio served as a blue print. There is the same handling. If you look at gpiolib-sysfs.c there is a comment about the direction property:

* MAY BE OMITTED if kernel won't allow direction changes

So from a gpiolib/sysfs POV I'm not sure about this change. Does get_direction == NULL means setting the direction isn't possible? OTHO there is a fat "MAY" :)

Which brings me to the question of "why this change?". The commit message doesn't mention it. Just out of curiosity.

Am 2022-11-13 14:21, schrieb William Breathitt Gray:

Quoted 15 lines · Andy Shevchenko

Quoted 14 lines

Quoted 5 lines · William Breathitt Gray

If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration.

Seems legit to me. Have you checked if we have any gpio-regmap drivers that
have something like this in their configuration already? In such cases we need
to be sure they behave as expected.

Quoted 8 lines

Quoted 2 lines

From the code perspective:

Reviewed-by Andy Shevchenko <[email protected]>

I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like the change in this patch will cause problems for them, but I'll let Michael Walle and Robert Marko comment if they see issues here.

For the sl28cpld driver this shouldn't be a problem. So for that

Acked-by Michael Walle <[email protected]>

But back when I wrote gpio-regmap the bgpio served as a blue print. There is the same handling. If you look at gpiolib-sysfs.c there is a comment about the direction property:

* MAY BE OMITTED if kernel won't allow direction changes

So from a gpiolib/sysfs POV I'm not sure about this change. Does get_direction == NULL means setting the direction isn't possible? OTHO there is a fat "MAY" :)

Which brings me to the question of "why this change?". The commit message doesn't mention it. Just out of curiosity.

-michael

Quoted 4 lines

Which brings me to the question of "why this change?". The commit message doesn't mention it. Just out of curiosity.

-michael

Currently, the 104-idi-48 module implements a get_direction() callback that is executed in situations such as gpiod_get_direction() which aren't necessarily related to sysfs. In this patch series, the 104-idi-48 module is migrated to the gpio_regmap API, but loses this get_direction() support because it's an input-only configuration. The purpose of this patch is to prevent that regression by supporting get_direction() for input-only/output-only configurations.

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

+ * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
+ * in the range [0, num_regs[

Quoted 4 lines · William Breathitt Gray

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

...

Quoted 2 lines

+ * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
+ * in the range [0, num_regs[

Not sure if it's a typo ([ vs. ]), but if you want to say "not including the last", use mathematical notation, i.e. "[0, num_regs)".

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · William Breathitt Gray

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

Quoted 5 lines

Quoted 2 lines

+ * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
+ * in the range [0, num_regs[

Not sure if it's a typo ([ vs. ]), but if you want to say "not including the last", use mathematical notation, i.e. "[0, num_regs)".

I was following the convention used in the @get_irq_reg description, but I agree that mathematical notation would be much clearer.

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

Quoted 4 lines · William Breathitt Gray

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

Can you provide examples of something that would make sense to open code in a driver rather than factoring out? It looks like this has been added due to one of the devices you're looking at for some reason disabling it's upstream interrupt when all of the downstream interrupts are masked, while weird that doesn't seem especally device specific.

Quoted 7 lines · Mark Brown

Quoted 4 lines · William Breathitt Gray

Provide a public callback handle_mask_sync() that drivers can use when they have more complex IRQ masking logic. The default implementation is regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own callback.

Quoted 6 lines

Can you provide examples of something that would make sense to open code in a driver rather than factoring out? It looks like this has been added due to one of the devices you're looking at for some reason disabling it's upstream interrupt when all of the downstream interrupts are masked, while weird that doesn't seem especally device specific.

Sure, I actually intend to use this callback for the 104-idi-48 module as well in the v3 submission so I'll describe that situations well.

For the 104-dio-48e we have the following:

Base Address +B (Write): Enable Interrupt
Base Address +B (Read): Disable Interrupt
Base Address +F (Read/Write): Clear Interrupt

So for 104-dio-48e, any write to 0xB will enable interrupts, while any read will disable interrupts; interrupts are with either a read or any write to 0xF. There's no status register either so software just has to assume that if an interrupt is raised then it was for the 104-dio-48e device.

For the 104-idi-48, we do get a status register and some basic masking but it's broken down by banks rather than individual GPIO; there are six 8-bit banks (Port 0 Low Byte, Port 0 Mid Byte, Port 0 High Byte, Port 1 Low Byte, Port 1 Mid Byte, Port 1 High Byte):

Base Address + 0 (Read/Write): Port 0 Low Byte Base Address + 1 (Read/Write): Port 0 Mid Byte Base Address + 2 (Read/Write): Port 0 High Byte Base Address + 3: N/A
Base Address + 4 (Read/Write): Port 1 Low Byte Base Address + 5 (Read/Write): Port 1 Mid Byte Base Address + 6 (Read/Write): Port 1 High Byte Base Address + 7 (Read): IRQ Status Register/IRQ Clear
Bit 0-5: Respective Bank IRQ Statuses
Bit 6: IRQ Status (Active Low)
Bit 7: IRQ Enable Status
Base Address + 7 (Write): IRQ Enable/Disable
Bit 0-5: Respective Bank IRQ Enable/Disable

In this case, masking a bank will mask all 8 GPIO within that bank; so ideally I want a way to only mask a bank when all GPIO are masked, and unmasking when at least one is unmasked.

Are there existing ways to support these kinds of configuration in regmap_irq?

William Breathitt Gray

The regmap API supports IO port accessors so we can take advantage of regmap abstractions rather than handling access to the device registers directly in the driver. The 104-dio-48e and gpio-mm modules depend on the i8255 library and are thus updated accordingly.

By leveraging the gpio_regmap API, the i8255 library is reduced to simply a devm_i8255_regmap_register() function, a configuration structure struct i8255_regmap_config, and a helper macro i8255_volatile_regmap_range() provided to simplify volatile PPI register hinting for the regmap.

Cc Andy Shevchenko <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/Kconfig            |   2 +
 drivers/gpio/gpio-104-dio-48e.c | 397 ++++++++++-------------------
 drivers/gpio/gpio-gpio-mm.c     | 151 +++--------
 drivers/gpio/gpio-i8255.c       | 429 +++++++++++---------------------
 drivers/gpio/gpio-i8255.h       |  80 +++---

Quoted 10 lines · William Breathitt Gray

The regmap API supports IO port accessors so we can take advantage of regmap abstractions rather than handling access to the device registers directly in the driver. The 104-dio-48e and gpio-mm modules depend on the i8255 library and are thus updated accordingly.

By leveraging the gpio_regmap API, the i8255 library is reduced to simply a devm_i8255_regmap_register() function, a configuration structure struct i8255_regmap_config, and a helper macro i8255_volatile_regmap_range() provided to simplify volatile PPI register hinting for the regmap.

Quoted 1 line

Cc Andy Shevchenko <[email protected]>

Suggested-by?
(I'm not insisting, just consider if it's appropriate to use here or in the
other patches)

Quoted 7 lines

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/Kconfig            |   2 +
 drivers/gpio/gpio-104-dio-48e.c | 397 ++++++++++-------------------
 drivers/gpio/gpio-gpio-mm.c     | 151 +++--------
 drivers/gpio/gpio-i8255.c       | 429 +++++++++++---------------------
 drivers/gpio/gpio-i8255.h       |  80 +++---

Can we actually split this to a few steps:
- providing gpio-i8255-regmap
- providing gpio-mm-regmap
- converting the driver
- removing not used modules (one by one)
?

In this case if any regression somewhere appears, we can always perform a (semi-)revert for a certain driver.

Quoted 14 lines · Andy Shevchenko

Quoted 5 lines · William Breathitt Gray

 drivers/gpio/Kconfig            |   2 +
 drivers/gpio/gpio-104-dio-48e.c | 397 ++++++++++-------------------
 drivers/gpio/gpio-gpio-mm.c     | 151 +++--------
 drivers/gpio/gpio-i8255.c       | 429 +++++++++++---------------------
 drivers/gpio/gpio-i8255.h       |  80 +++---

Can we actually split this to a few steps:
- providing gpio-i8255-regmap
- providing gpio-mm-regmap
- converting the driver
- removing not used modules (one by one)
?

Quoted 2 lines

In this case if any regression somewhere appears, we can always perform a (semi-)revert for a certain driver.

Sure, I can split the regmap_irq migration for 104-dio-48e into a separate precursor patch to reduce the amount of changes we see here and provide a revert path for these IRQ changes. I can do a similar change for 104-idi-48 as well.

The rest of the changes for 104-dio-48 and gpio-mm are essentially just the regmap configurations, so the patch will be largely identical even if we migrate gpio-i8255 to regmap API first before migrating again to the gpio_regmap in a second patch.

Quoted 2 lines · William Breathitt Gray

Quoted 1 line · Andy Shevchenko

On Thu, Nov 10, 2022 at 08:55:53PM -0500, William Breathitt Gray wrote:

Quoted 20 lines

Quoted 12 lines

Quoted 4 lines

 drivers/gpio/gpio-104-dio-48e.c | 397 ++++++++++-------------------
 drivers/gpio/gpio-gpio-mm.c     | 151 +++--------
 drivers/gpio/gpio-i8255.c       | 429 +++++++++++---------------------
 drivers/gpio/gpio-i8255.h       |  80 +++---

Can we actually split this to a few steps:
- providing gpio-i8255-regmap
- providing gpio-mm-regmap
- converting the driver
- removing not used modules (one by one)
?

Quoted 2 lines

In this case if any regression somewhere appears, we can always perform a (semi-)revert for a certain driver.

Sure, I can split the regmap_irq migration for 104-dio-48e into a separate precursor patch to reduce the amount of changes we see here and provide a revert path for these IRQ changes. I can do a similar change for 104-idi-48 as well.

Quoted 4 lines

The rest of the changes for 104-dio-48 and gpio-mm are essentially just the regmap configurations, so the patch will be largely identical even if we migrate gpio-i8255 to regmap API first before migrating again to the gpio_regmap in a second patch.

Sorry, I realize now that you meant to split the i8255 gpio_regmap additions to their own patch, perform the driver migrations in the own respective patches, and then finally remove the dangling unused i8255 functions and structures. Yes I think that would make for a cleaner patch series so I'll split it up that way.

drivers/gpio/gpio-i8255.c +14 -0

+int devm_i8255_regmap_register(struct device *const dev,
+			       const struct i8255_regmap_config *const config)
+{
+	struct gpio_regmap_config gpio_config = {0};
+	unsigned long i;
+	int err;
+
+	if (!config->parent)
+		return -EINVAL;
+
+	if (!config->map)
+		return -EINVAL;
+
+	if (!config->num_ppi)

Am 2022-11-11 02:55, schrieb William Breathitt Gray:

drivers/gpio/gpio-i8255.c line 126

Quoted 2 lines

drivers/gpio/gpio-i8255.c +2 -0

+	gpio_config.parent = config->parent;
+	gpio_config.regmap = config->map;

I'd propose to add a new config flag to indicate that accesses to the device will be fast:

gpio_config.regmap_has_fast_io = true;

which will then set gpio->can_sleep = false.

Quoted 7 lines

Am 2022-11-11 02:55, schrieb William Breathitt Gray:

drivers/gpio/gpio-i8255.c line 126

Quoted 2 lines

drivers/gpio/gpio-i8255.c +2 -0

+	gpio_config.parent = config->parent;
+	gpio_config.regmap = config->map;

I'd propose to add a new config flag to indicate that accesses to the device will be fast:

Quoted 3 lines

gpio_config.regmap_has_fast_io = true;

which will then set gpio->can_sleep = false.

It's probably useful to provide a query function in the regmap API for generic regmap users like this.

Am 2022-11-17 17:21, schrieb Mark Brown:

Quoted 2 lines

It's probably useful to provide a query function in the regmap API for generic regmap users like this.

Now I'm confused. Last time, I've proposed that, there was push back from you:
https://lore.kernel.org/linux-gpio/[email protected]/

That being said, I'd prefer to have such a query API :)

Quoted 4 lines · Michael Walle

Am 2022-11-17 17:21, schrieb Mark Brown:

Quoted 2 lines

It's probably useful to provide a query function in the regmap API for generic regmap users like this.

Quoted 5 lines

Now I'm confused. Last time, I've proposed that, there was push back from you:
https://lore.kernel.org/linux-gpio/[email protected]/

That being said, I'd prefer to have such a query API :)

Now we have a generic user which cares about the distinction.