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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Sep 1, 2022 at 12:41 PM Qingtao Cao <qingtao.cao.au@xxxxxxxxx> 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