Re: [PATCH v2 2/6] gpio: pca953x: Add PCAL953X as a separate chip type

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Oct 27, 2022 at 02:54:38PM +0100, Martyn Welch wrote:
> On Thu, 2022-10-27 at 15:36 +0200, Levente Révész wrote:
> > Thank you for your guidance, it is much appreciated.
> > On Wed, 26 Oct, 2022 at 17:29:17 +0000, Andy Shevchenko wrote:
> > On Wed, 26 Oct, 2022 at 17:32:03 +0000, Andy Shevchenko wrote:

...

> > If I understand this correctly, this should be a sufficient solution:
> > 
> >  #define PCA_PCAL               BIT(9)
> >  #define PCA_TYPE_MASK          GENMASK(15, 12)
> > -#define PCA_CHIP_TYPE(x)       ((x) & PCA_TYPE_MASK)
> > +#define PCA_CHIP_TYPE(x)       (((x) & PCA_TYPE_MASK) >> 11 | ((x) &
> > PCA_PCAL) >> 9)
> > +#define PCA_SET_TYPE(x)        ((x) << 11 & PCA_TYPE_MASK | (x) << 9
> > & PCA_PCAL)
> > 
> > Use 5 bits to encode chip type: bit 15..12 and bit 9.

Easier if you move PCA_PCAL to be bit 11 and extend mask by one bit.


> > PCA_SET_TYPE() shifts bits of the TYPE to the correct positions in
> > the id.
> > PCA_CHIP_TYPE() gathers the bits from the ID to form a nice decimal
> > number.
> > 
> > -#define PCA953X_TYPE           BIT(12)
> > -#define PCA957X_TYPE           BIT(13)
> > -#define PCAL653X_TYPE          BIT(14)
> > +#define PCA953X_TYPE           2
> > +#define PCA953XM_TYPE          6
> > +#define PCAL953X_TYPE          3
> > +#define PCAL653X_TYPE          9
> > +#define PCA957X_TYPE           4
> > 
> > Types are decimal numbers. Values are carefully chosen to leave
> > existing
> > IDs unchanged.
> > 
> >     * 2 instead of            BIT(12), ID mask remains
> > 0b0001'0000'0000'0000
> >     * 4 instead of            BIT(13), ID mask remains
> > 0b0010'0000'0000'0000
> >     * 9 instead of BIT(14) | PCA_PCAL, ID mask remains
> > 0b0100'0010'0000'0000
> >     * 3 instead of BIT(12) | PCA_PCAL, ID mask remains
> > 0b0001'0010'0000'0000
> > 
> > PCAL chips have odd IDs, which means the PCA_PCAL bit will be set.
> > 
> > It seems more natural now to have a separate type, PCA953XM_TYPE for
> > pca9505,
> > pca9506 and pca9698 with the [M]ask register. Only PCA953X type chips
> > can have
> > this register anyway.
> > 
> > What do you think?
> > 
> 
> I don't like the idea of encoding whether a device is PCA or PCAL into
> the bit offsets. Primarily because it's massively opaque, making
> successfully adding support for another device that much more prone to
> issues, especially if that person isn't already intimately familiar
> with the driver. I'd also like to point out that not all PCAL can be
> handled the same. The register layout of the PCAL653X_TYPE needs to be
> handled differently from the PCAL953X_TYPE.

PCA_PCAL is about having latched interrupts, it doesn't suggest the layout. So,
somebody needs to take a pen and draw the current list of possible combinations
of the features and layouts and then we can see what can be done as a type and
what's not.

-- 
With Best Regards,
Andy Shevchenko