Re: [RFC PATCH 2/3] gpio: pca953x: Describe register maps with enums

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Jan 30, 2023 at 09:59:48PM +0100, Levente Révész wrote:
> The driver supports 8 chip types, 6 of which have extended registers
> with various functions, e.g. pull-up and pull-down bias for the gpio
> lines or interrupt masking. To allow supporting these functions, the
> driver has to be able to calculate the addresses of these registers.
> 
> Replace the register maps with an enum for each chip type. These do not
> contain the same numeric values as the old defines, but the new address
> calculating functions (in the next patch) use them appropriately.
> 
> Add currently used registers to struct pca953x_reg_config.
> 
> Create a reg_config struct for each chip type.

...

> +enum xra120x_reg {
> +};

>  static const struct pca953x_reg_config pca953x_regs = {
> +};

Make those enums and reg_config definitions to be sorted by their
respective names.

...

> +	case TYPE_PCA950X:
> +		registers = BIT(PCA950X_REG_INPUT) |
> +			    BIT(PCA950X_REG_OUTPUT) |
> +			    BIT(PCA950X_REG_INVERT) |
> +			    BIT(PCA950X_REG_DIRECTION) |
> +			    BIT(PCA950X_REG_INT_MASK);
> +		break;

Can't it be simplified if you define something like REG_MAX in each of
the enums and use here simply GENMASK(MAX, 0); ?

...

> +	case TYPE_PCA950X:
> +		registers = BIT(PCA950X_REG_OUTPUT) |
> +			    BIT(PCA950X_REG_INVERT) |
> +			    BIT(PCA950X_REG_DIRECTION) |
> +			    BIT(PCA950X_REG_INT_MASK);

Something similar, maybe with a definition of the volatile registers?

> +		break;

...

>  	if (chip->type == TYPE_PCA957X) {
> -		chip->regs = &pca957x_regs;
>  		ret = device_pca957x_init(chip, invert);
>  	} else {
> -		chip->regs = &pca953x_regs;
>  		ret = device_pca95xx_init(chip, invert);
>  	}

After this the {} may be dropped as well.

-- 
With Best Regards,
Andy Shevchenko