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

[Martyn Welch <martyn.welch@xxxxxxxxxxxxx>]

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:
> > Looking at this again, why not to make PCAL bit as a part of the
> > type,
> > so you will have the check below much easier, like masking for PCAL
> > bit
> > and that's it.
> 
> On Wed, 26 Oct, 2022 at 17:32:03 +0000, Andy Shevchenko wrote:
> > Consider avoiding the change of the ID tables in almost every patch
> > you
> > have. This can be achiever by carefully allocating bits for types
> > and
> > define the actual HW via bit ORed masks.
> 
> 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.
> 
> 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.

Martyn

> ---
> Best Regards,
> Levente