Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220829133923.1114555-1-martyn.welch@collabora.com> <20220829133923.1114555-5-martyn.welch@collabora.com>
In-Reply-To: <20220829133923.1114555-5-martyn.welch@collabora.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 1 Sep 2022 00:02:18 +0300
Message-ID: <CAHp75VfZmmDGJJ5wxM8-pbqo+npOSZrPtyJnQhuGadLUYod=3A@mail.gmail.com>
Subject: Re: [PATCH 5/5] gpio: pca953x: Add support for PCAL6534 and compatible
To:     Martyn Welch <martyn.welch@collabora.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Collabora Kernel ML <kernel@collabora.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Aug 29, 2022 at 4:52 PM Martyn Welch <martyn.welch@collabora.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.

...

>  #define PCA953X_TYPE           BIT(12)
>  #define PCA957X_TYPE           BIT(13)
> +#define PCAL653X_TYPE          BIT(14)
>  #define PCA_TYPE_MASK          GENMASK(15, 12)
>
> +

Stray change.

>  #define PCA_CHIP_TYPE(x)       ((x) & PCA_TYPE_MASK)

...

>  static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
>                                    u32 checkbank)
>  {
> -       int bank_shift = pca953x_bank_shift(chip);
> -       int bank = (reg & REG_ADDR_MASK) >> bank_shift;
> -       int offset = reg & (BIT(bank_shift) - 1);
> +       int bank;
> +       int offset;
> +
> +       if (PCA_CHIP_TYPE(chip->driver_data) == PCAL653X_TYPE) {
> +               if (reg > 0x2f) {
> +                       /*
> +                        * Reserved block between 14h and 2Fh does not align on
> +                        * expected bank boundaries like other devices.
> +                        */
> +                       int temp = reg - 0x30;
> +
> +                       bank = temp / NBANK(chip);
> +                       offset = temp - (bank * NBANK(chip));
> +                       bank += 8;
> +               } else if (reg > 0x53) {
> +                       /* Handle lack of reserved registers after output port
> +                        * configuration register to form a bank.
> +                        */
> +                       int temp = reg - 0x54;
> +
> +                       bank = temp / NBANK(chip);
> +                       offset = temp - (bank * NBANK(chip));
> +                       bank += 16;

Can we rather put this into a separate function?

> +               } else {
> +                       bank = reg / NBANK(chip);
> +                       offset = reg - (bank * NBANK(chip));
> +               }
> +       } else {
> +               int bank_shift = pca953x_bank_shift(chip);
>
> -       /* Special PCAL extended register check. */
> -       if (reg & REG_ADDR_EXT) {
> -               if (!(chip->driver_data & PCA_PCAL))
> -                       return false;
> -               bank += 8;
> +               bank = (reg & REG_ADDR_MASK) >> bank_shift;
> +               offset = reg & (BIT(bank_shift) - 1);
> +
> +               /* Special PCAL extended register check. */
> +               if (reg & REG_ADDR_EXT) {
> +                       if (!(chip->driver_data & PCA_PCAL))
> +                               return false;
> +                       bank += 8;
> +               }
>         }

All the same, split this to be like

if (current)
 do_current_things
else
 do_new_type

...

>  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()?

-- 
With Best Regards,
Andy Shevchenko