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: <20220817112818.787771-1-martyn.welch@collabora.com>
In-Reply-To: <20220817112818.787771-1-martyn.welch@collabora.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 20 Aug 2022 01:35:30 +0300
Message-ID: <CAHp75Vc1bnUGt66LYRAVuJP+OnhLTaU=AN1JdvfiH44O9_eO8g@mail.gmail.com>
Subject: Re: [RFC PATCH] gpio: pca953x: Support for pcal6534
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 Wed, Aug 17, 2022 at 2:29 PM Martyn Welch <martyn.welch@collabora.com> wrote:
>
> The pcal6534[1] is a 34-bit I/O expander with more than a passing
> resemblance to the pcal6524[2] currently supported by the gpio-pca953x
> driver, however whilst the registers seem to functionally match
> perfectly, the alignment of the register banks in the chips address
> space do not follow the pattern expected by the existing driver. For

does not

> instance, as the chip provides 34 I/O, which requires bannks of 5 8-bit
> registers to provide input state, output state, etc. as do the 40 I/O
> variants, however the 40 I/O variants layout the banks of registers on
> 8-byte boundaries, whilst the pcal6534 does not space out the banks at
> all. Additionally the extended functionality starts at 30h rather than
> 40h and I suspect there will be other similar differences that I've not
> yet discovered.

The below shouldn't be in the commit message, but rather in the
comments (after the cutter '---' line below). And due to these two
paragraphs I consider this as an RFC (and it is luckily marked like
this), so, Bart, please do not apply this, we need more eyes and
datasheet reading before going on this.

> I suspect that this may add some additional complexity to the driver and
> I'm not sure whether this will be welcome. I've done a few cursory
> searches to see if there are other chips which follow the pattern of the
> pcal6534 and have so far only found the pi4ioe5v6534q[3], which appears
> to be funcitonaly identical to the pcal6534.
>
> I'm currently wondering whether a submission to extend the pcal6534
> is likely to be deemed acceptable. If so whether something like the

so, whether

> attached approach would be OK, or whether anyone has better ideas on how
> to achieve this. Alternatively I'd be happy to create a new driver to
> support the pcal6534 if that's deemed more appropriate.

> [1] https://www.nxp.com/docs/en/data-sheet/PCAL6534.pdf
> [2] https://www.nxp.com/docs/en/data-sheet/PCAL6524.pdf
> [3] https://www.diodes.com/assets/Datasheets/PI4IOE5V6534Q.pdf

Convert these to Datasheet: tags.

...

>  #define PCA957X_TYPE           BIT(13)
>  #define PCA_TYPE_MASK          GENMASK(15, 12)
>
> +#define PCAL6534_ALIGN         BIT(16)

I believe it should be a chip TYPE.

...

>         { "xra1202", 8  | PCA953X_TYPE },

> +       { "pi4ioe5v6534q", 34 | PCA953X_TYPE | PCA_LATCH_INT | PCAL6534_ALIGN, },

What's this and why is it not ordered?

...

> -       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 (chip->driver_data & PCAL6534_ALIGN) {
> +               bank = (reg & REG_ADDR_MASK) / NBANK(chip);
> +               offset = reg - (bank * NBANK(chip));
> +       } else {
> +               int bank_shift = pca953x_bank_shift(chip);
> +               bank = (reg & REG_ADDR_MASK) >> bank_shift;
> +               offset = reg & (BIT(bank_shift) - 1);
> +       }

I'm wondering if it can be moved to bank_shift()  and possibly a new
helper to get an offset.

...

> +       for (i = 0; i < NBANK(chip); i++) {
>                 value[i] = bitmap_get_value8(val, i * BANK_SZ);
> +               dev_err(&chip->client->dev, "value[%d] = %x\n", i, value[i]);
> +       }
> +       dev_err(&chip->client->dev, "regaddr: %x\n", regaddr);

dev_err() ?!

...

> +       { .compatible = "diodes,pi4ioe5v6534q", .data = OF_953X(34, PCA_LATCH_INT | PCAL6534_ALIGN), },

As per above.

-- 
With Best Regards,
Andy Shevchenko