Re: [RFC PATCH 1/3] gpio: pca953x: Replace chip type flags with a type enum

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Jan 30, 2023 at 09:59:42PM +0100, Levente Révész wrote:
> The driver supports 8 chip types. The chip type is encoded in
> driver_data so that different chip types can be handled appropriately.
> 
> Encoding the type information in separate bit flags was not too
> straightforward, and it didn't make it possible to encode 8 chip
> types.
> 
> Replace bit flags with a pca953x_chip_type enum. Encode this enum in
> driver_data as a number. Add missing chip types based on chip functions
> and register layout.

...

> +/*
> + * driver_data
> + *
> + *   31    24 23    16 15     8 7      0
> + *   xxxxxxxx xxxxxxxx xxxx____ ________
> + *                         ^^^^ ^^^^^^^^
> + *                         |    number of gpio lines
> + *                         chip type
> + */

Not sure we need this as it's self explanatory from the below masks.
Moreover, driver_data is 64-bit or 32-bit depending on the build.

> +#define PCA_GPIO_MASK		GENMASK(7, 0)
> +#define PCA_TYPE_MASK		GENMASK(11, 8)

...

> +enum pca953x_chip_type {

> +	TYPE_PCA953X_NOINT,
> +	TYPE_PCA953X,

Hmm... Since you change this anyway, I think I would distinguish them better.
Perhaps

	TYPE_PCA953X_INT,
	TYPE_PCA953X_NOINT,

?

In any case, please add a kernel doc to this enum to explain each type briefly.

> +	TYPE_PCA950X,

Can we make sorted? (I.e. move this before PCA953X)

> +	TYPE_PCAL953X,
> +	TYPE_PCAL652X,
> +	TYPE_PCAL653X,
> +	TYPE_PCA957X,

Can we make sorted? (I.e. move this after PCA953X)

> +	TYPE_XRA120X,

	TYPE_MAX

> +};

static_assert((PCA_TYPE_MASK + 1) >= (TYPE_MAX << 8));

-- 
With Best Regards,
Andy Shevchenko