[PATCH 1/1] gpio: exar: access MPIO registers on slave chips

[PATCH 1/1] gpio: exar: access MPIO registers on slave chips

[Qingtao Cao]

When EXAR xr17v35x chips are cascaded in order to access the MPIO registers
(part of the Device Configuration Registers) of the slave chips, an offset
needs to be applied based on the number of master chip's UART channels.

Signed-off-by: Qingtao Cao <qingtao.cao.au@gmail.com>
---
 drivers/gpio/gpio-exar.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index d37de78247a6..1110c8f92c58 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -20,6 +20,7 @@
 #define EXAR_OFFSET_MPIOSEL_LO 0x93
 #define EXAR_OFFSET_MPIOLVL_HI 0x96
 #define EXAR_OFFSET_MPIOSEL_HI 0x99
+#define EXAR_UART_CHANNEL_SIZE 0x400
 
 #define DRIVER_NAME "gpio_exar"
 
@@ -31,26 +32,37 @@ struct exar_gpio_chip {
 	int index;
 	char name[20];
 	unsigned int first_pin;
+	/*
+	 * The offset to the slave device's (if existing)
+	 * Device Configuration Registers
+	 */
+	unsigned int slave_offset;
 };
 
 static unsigned int
 exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 {
-	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
-						   : EXAR_OFFSET_MPIOSEL_LO;
+	int addr;
+
+	addr = (offset % 16 + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
+							: EXAR_OFFSET_MPIOSEL_LO;
+	return offset / 16 ? addr + exar_gpio->slave_offset : addr;
 }
 
 static unsigned int
 exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 {
-	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
-						   : EXAR_OFFSET_MPIOLVL_LO;
+	int addr;
+
+	addr = (offset % 16 + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
+							: EXAR_OFFSET_MPIOLVL_LO;
+	return offset / 16 ? addr + exar_gpio->slave_offset : addr;
 }
 
 static unsigned int
 exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 {
-	return (offset + exar_gpio->first_pin) % 8;
+	return (offset % 16 + exar_gpio->first_pin) % 8;
 }
 
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -153,6 +165,21 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!exar_gpio)
 		return -ENOMEM;
 
+	if (pcidev->device & 0xf000) {
+		/*
+		 * xr17v354 or xr17v358 slaves have the same amount of
+		 * MPIOs as the master
+		 */
+		ngpios += ngpios;
+
+		/*
+		 * The last 4 bits of the master's PCI Device ID is
+		 * the number of its UART channels
+		 */
+		exar_gpio->slave_offset = (pcidev->device & 0xf) *
+				EXAR_UART_CHANNEL_SIZE;
+	}
+
 	/*
 	 * We don't need to check the return values of mmio regmap operations (unless
 	 * the regmap has a clock attached which is not the case here).
-- 
2.17.1

Re: [PATCH 1/1] gpio: exar: access MPIO registers on slave chips

[Andy Shevchenko]

On Thu, Sep 1, 2022 at 12:41 PM Qingtao Cao <qingtao.cao.au@gmail.com> wrote:
>
> When EXAR xr17v35x chips are cascaded in order to access the MPIO registers
> (part of the Device Configuration Registers) of the slave chips, an offset
> needs to be applied based on the number of master chip's UART channels.

...

>  #define EXAR_OFFSET_MPIOSEL_LO 0x93
>  #define EXAR_OFFSET_MPIOLVL_HI 0x96
>  #define EXAR_OFFSET_MPIOSEL_HI 0x99

+ Blank line.

> +#define EXAR_UART_CHANNEL_SIZE 0x400

Add a comment explaining what this does include, etc.

...

> +       /*
> +        * The offset to the slave device's (if existing)
> +        * Device Configuration Registers

Always finish multi-line comments with a period. Applies to other
comments in this patch.

> +        */

...

>  {
> -       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
> -                                                  : EXAR_OFFSET_MPIOSEL_LO;
> +       int addr;
> +
> +       addr = (offset % 16 + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
> +                                                       : EXAR_OFFSET_MPIOSEL_LO;
> +       return offset / 16 ? addr + exar_gpio->slave_offset : addr;

Can we rather have something like

  unsigned int pin = exar->first_pin + (offset % 16);
  unsigned int slave = offset / 16;

  addr = pin / 8 ? ...;
  return addr + (slave ? ... : 0);

?

>  }
>
>  static unsigned int
>  exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
>  {
> -       return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
> -                                                  : EXAR_OFFSET_MPIOLVL_LO;
> +       int addr;
> +
> +       addr = (offset % 16 + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
> +                                                       : EXAR_OFFSET_MPIOLVL_LO;
> +       return offset / 16 ? addr + exar_gpio->slave_offset : addr;

In the similar way as above.

>  }

...

> +       if (pcidev->device & 0xf000) {

GENMASK()

> +               /*
> +                * xr17v354 or xr17v358 slaves have the same amount of
> +                * MPIOs as the master
> +                */
> +               ngpios += ngpios;
> +
> +               /*
> +                * The last 4 bits of the master's PCI Device ID is
> +                * the number of its UART channels
> +                */
> +               exar_gpio->slave_offset = (pcidev->device & 0xf) *

GENMASK()

> +                               EXAR_UART_CHANNEL_SIZE;
> +       }

-- 
With Best Regards,
Andy Shevchenko