Re: [PATCH v2 5/5] gpio: pca953x: Add support for PCAL6534

[Martyn Welch <martyn.welch@xxxxxxxxxxxxx>]

On Tue, 2022-09-06 at 15:24 +0300, Andy Shevchenko wrote:
> On Tue, Sep 06, 2022 at 09:28:19AM +0100, Martyn Welch wrote:
> > From: Martyn Welch <martyn.welch@xxxxxxxxxxxxx>
> > 
> > Add support for the NXP PCAL6534. This device is 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. To accommodate ths,
> > callback
> > functions have been added to allow alterate implementations of
> > pca953x_recalc_addr() and pca953x_check_register() for the
> > PCAL6534.
> 
> 
> This looks much cleaner!
> 
> ...
> 
> > @@ -107,6 +109,7 @@ static const struct i2c_device_id pca953x_id[]
> > = {
> >         { "tca9539", 16 | PCA953X_TYPE | PCA_INT, },
> >         { "tca9554", 8  | PCA953X_TYPE | PCA_INT, },
> >         { "xra1202", 8  | PCA953X_TYPE },
> > +
> >         { }
> 
> Missed Diodes?
> 

Dropped as it is expected for the DTBs of devices with a pi4ioe5v6534q
also have a compatible for pcal6534. hence it's not needed in the
driver (at least until someone finds a difference which needs to be
explicitly handled for one and not the other).

> >  };
> >  MODULE_DEVICE_TABLE(i2c, pca953x_id);
> 
> ...
> 
> > +       u8 (*recalc_addr)(struct pca953x_chip *chip, int reg , int
> > off);
> > +       bool (*check_reg)(struct pca953x_chip *chip, unsigned int
> > reg,
> > +                         u32 checkbank);
> 
> I would think of splitting this change. Like in a separate patch you
> simply
> create this interface and only add what you need in the next one.
> 

Can do, though I didn't feel you were particularly fussed about me
having split that out...

> ...
> 
> > +static bool pcal6534_check_register(struct pca953x_chip *chip,
> > unsigned int reg,
> > +                                   u32 checkbank)
> > +{
> > +       int bank;
> > +       int offset;
> > +
> > +       if (reg > 0x2f) {
> 
> I guess code read and generation wise the
> 
>         if (reg >= 0x30) {
> 
> is slightly better.

OK.

> 
> > +               /*
> > +                * 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));
> 
> Parentheses are not needed fur multiplication, but if you insist...
> 



> > +               bank += 8;
> 
> > +       } else if (reg > 0x53) {
> 
> In the similar way...
> 
> > +               /* Handle lack of reserved registers after output
> > port
> > +                * configuration register to form a bank.
> > +                */
> 
> Comment style
> 
> /*
>  * Handle...
>  */
> 

ack

> > +               int temp = reg - 0x54;
> > +
> > +               bank = temp / NBANK(chip);
> > +               offset = temp - (bank * NBANK(chip));
> > +               bank += 16;
> > +       } else {
> > +               bank = reg / NBANK(chip);
> > +               offset = reg - (bank * NBANK(chip));
> > +       }
> > +
> > +       /* Register is not in the matching bank. */
> > +       if (!(BIT(bank) & checkbank))
> > +               return false;
> > +
> > +       /* Register is not within allowed range of bank. */
> > +       if (offset >= NBANK(chip))
> > +               return false;
> > +
> > +       return true;
> > +}
> 
> ...
> 
> > -       u8 regaddr = pinctrl | addr | (off / BANK_SZ);
> >  
> > -       return regaddr;
> > +       return pinctrl | addr | (off / BANK_SZ);
> 
> Stray change, or anything I have missed?
> 

Yeah, can remove that change...

> ...
> 
> > +/* The PCAL6534 and compatible chips have altered bank alignment
> > that doesn't
> > + * fit within the bit shifting scheme used for other devices.
> > + */
> 
> Comment style.
> 
> ...
> 
> > @@ -1240,6 +1335,7 @@ static const struct of_device_id
> > pca953x_dt_ids[] = {
> >  
> >         { .compatible = "nxp,pcal6416", .data = OF_953X(16,
> > PCA_LATCH_INT), },
> >         { .compatible = "nxp,pcal6524", .data = OF_953X(24,
> > PCA_LATCH_INT), },
> > +       { .compatible = "nxp,pcal6534", .data = OF_653X(34,
> > PCA_LATCH_INT), },
> >         { .compatible = "nxp,pcal9535", .data = OF_953X(16,
> > PCA_LATCH_INT), },
> >         { .compatible = "nxp,pcal9554b", .data = OF_953X( 8,
> > PCA_LATCH_INT), },
> >         { .compatible = "nxp,pcal9555a", .data = OF_953X(16,
> > PCA_LATCH_INT), },
> 
> Do you decide to drop Diodes compatible from the code?
>