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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 1 Sep 2022 22:03:51 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Qingtao Cao <qingtao.cao.au@...il.com>
Cc:     nathan@...hanrossi.com, Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] gpio: exar: access MPIO registers on slave chips

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