| 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
| ||
|
Message-ID: <CAHp75VfZmmDGJJ5wxM8-pbqo+npOSZrPtyJnQhuGadLUYod=3A@mail.gmail.com>
Date: Thu, 1 Sep 2022 00:02:18 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Martyn Welch <martyn.welch@...labora.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 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.
...
> #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
Powered by blists - more mailing lists