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

[Jean Delvare <jdelvare@suse.de>]

Hi Andy,

On Thu, 18 Sep 2025 21:50:55 +0300, Andy Shevchenko wrote:
> 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.

Models are mutually exclusive, so using a bit array to store that
information doesn't make sense. As a matter of fact, the driver code
which makes use of these values clearly assumes that they can't be
combined:

	switch (dev->flags & MODEL_MASK) {
	case MODEL_BAIKAL_BT1:
		ret = bt1_i2c_request_regs(dev);
		break;
	case MODEL_WANGXUN_SP:
		ret = txgbe_i2c_request_regs(dev);
		break;
	(...)
	}

> > 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?

Because model 0 is every other device which is neither MSCC Ocelot nor
Baikal BT1 nor AMD Navi nor Wangxun SP. We could define MODEL_OTHER as
(0 << 8), but there's not reason to ever use it in the code
(switch/case statements simply use default if something needs to be
done for standard devices), which is why nobody bothered defining it so
far.

-- 
Jean Delvare
SUSE L3 Support