lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 5/5] gpio: pca953x: Add support for PCAL6534
From
Date
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@collabora.com>
> >
> > 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?
>

\
 
 \ /
  Last update: 2022-09-06 16:45    [W:1.499 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site