Re: [PATCH 3/3] i2c: designware: Turn models back to enumerated values

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Sep 18, 2025 at 04:13:01PM +0200, Jean Delvare wrote:
> There are 4 flag bits reserved to store the device model. All
> accesses to this information is properly masked with MODEL_MASK to
> extract only these bits and compare them with a given model value.
> 
> However the model definitions isn't an enumeration as you would
> expect. Instead each model uses a separate flag, meaning that the
> reserved space is already exhausted with the 4 models which have been
> defined so far.
> 
> The error seems to originate from commit a5df4c14b9a9 ("i2c:
> designware: Switch header to use BIT() and GENMASK()") where:
> 
> define MODEL_MSCC_OCELOT      0x00000100
> define MODEL_BAIKAL_BT1       0x00000200
> 
> was erroneously converted to:

I don't think "erroneously" is correct word here. The code before that commit
as you mentioned starts with a bit set, rather than from 0. I would argue that
the intention was to use a bitmask instead of plain number.

> define MODEL_MSCC_OCELOT      BIT(8)
> define MODEL_BAIKAL_BT1       BIT(9)
> 
> While numerically equivalent, conceptually it wasn't, and it caused
> the models added later to get bit-based definitions instead of
> continuing with the next enumerated value (0x00000300).
> 
> Turn back these definitions to enumerated values to clear the
> confusion, avoid future mistakes, and free some space for more models
> to be supported in the future.

...

> -#define MODEL_MSCC_OCELOT			BIT(8)
> -#define MODEL_BAIKAL_BT1			BIT(9)
> -#define MODEL_AMD_NAVI_GPU			BIT(10)
> -#define MODEL_WANGXUN_SP			BIT(11)
> +#define MODEL_MSCC_OCELOT			(1 << 8)
> +#define MODEL_BAIKAL_BT1			(2 << 8)
> +#define MODEL_AMD_NAVI_GPU			(3 << 8)
> +#define MODEL_WANGXUN_SP			(4 << 8)
>  #define MODEL_MASK				GENMASK(11, 8)

Taking above into consideration, why can't we start them from 0?

-- 
With Best Regards,
Andy Shevchenko