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

[Levente Révész <levente.revesz@xxxxxxxxxx>]

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?

---
Best Regards,
Levente