linux-kernel - Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ff085325437dbe1bd5397b40ecfe4697c5658f33.camel@collabora.com>
Date:   Thu, 01 Sep 2022 18:18:10 +0100
From:   Martyn Welch <martyn.welch@...labora.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Collabora Kernel ML <kernel@...labora.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and
 compatible

On Thu, 2022-09-01 at 00:02 +0300, Andy Shevchenko wrote:
> On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch
> <martyn.welch@...labora.com> wrote:
> > 
> > Add support for the NXP PCAL6534 and Diodes Inc. PI4IOE5V6534Q.
> > These
> > devices, which have identical register layouts and features, are
> > broadly a
> > 34-bit version of the PCAL6524.
> > 
> > However, whilst the registers are broadly what you'd expect for a
> > 34-bit
> > version of the PCAL6524, the spacing of the registers has been
> > compacted. This has the unfortunate effect of breaking the bit
> > shift
> > based mechanism that is employed to work out register locations
> > used by
> > the other chips supported by this driver, resulting in special
> > handling
> > needing to be introduced in pca953x_recalc_addr() and
> > pca953x_check_register().
> 
> This still needs an alternative deep review. I'll do my best to get
> into it sooner than later.
> 

Thanks much appreciated.

...

> >  static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg,
> > int off)
> >  {
> > -       int bank_shift = pca953x_bank_shift(chip);
> > -       int addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > -       int pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > -       u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> > +       int addr;
> > +       int pinctrl;
> > +       u8 regaddr;
> > +
> > +       if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
> > +               /* The PCAL6534 and compatible chips have altered
> > bank alignment that doesn't
> > +                * fit within the bit shifting scheme used for
> > other devices.
> > +                */
> > +               addr = (reg & PCAL_GPIO_MASK) * NBANK(chip);
> > +
> > +               switch (reg) {
> > +               case PCAL953X_OUT_STRENGTH:
> > +               case PCAL953X_IN_LATCH:
> > +               case PCAL953X_PULL_EN:
> > +               case PCAL953X_PULL_SEL:
> > +               case PCAL953X_INT_MASK:
> > +               case PCAL953X_INT_STAT:
> > +               case PCAL953X_OUT_CONF:
> > +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > + 0x20;
> > +                       break;
> > +               case PCAL6524_INT_EDGE:
> > +               case PCAL6524_INT_CLR:
> > +               case PCAL6524_IN_STATUS:
> > +               case PCAL6524_OUT_INDCONF:
> > +               case PCAL6524_DEBOUNCE:
> > +                       pinctrl = ((reg & PCAL_PINCTRL_MASK) >> 1)
> > + 0x1c;
> > +                       break;
> > +               }
> > +               regaddr = pinctrl + addr + (off / BANK_SZ);
> > +       } else {
> > +               int bank_shift = pca953x_bank_shift(chip);
> > +
> > +               addr = (reg & PCAL_GPIO_MASK) << bank_shift;
> > +               pinctrl = (reg & PCAL_PINCTRL_MASK) << 1;
> > +               regaddr = pinctrl | addr | (off / BANK_SZ);
> > +       }
> 
> Looking at all these, why not add the callbacks for recalc_addr and
> check_reg and assign them in the ->probe()?
> 

Yeah, that sounds like a good plan. I'll look into doing that.

Martyn