RE: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jiawen Wu <jiawenwu@xxxxxxxxxxxxxx>]

On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote:
> On 5/15/23 12:24, andy.shevchenko@xxxxxxxxx wrote:
> > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
> >> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> >> with SFP.
> >>
> >> Introduce the property "snps,i2c-platform" to match device data for Wangxun
> >> in software node case. Since IO resource was mapped on the ethernet driver,
> >> add a model quirk to get regmap from parent device.
> >>
> >> The exists IP limitations are dealt as workarounds:
> >> - IP does not support interrupt mode, it works on polling mode.
> >> - Additionally set FIFO depth address the chip issue.
> >
> > ...
> >
> >>   	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> >> +	if (device_property_present(&pdev->dev, "snps,i2c-platform"))
> >> +		dev->flags |= MODEL_WANGXUN_SP;
> >
> > What I meant here is to use device_property_present() _iff_ you have decided to
> > go with the _vendor-specific_ property name.
> >
> > Otherwise it should be handled differently, i.e. with reading the actual value
> > of that property. Hence it should correspond the model enum, which you need to
> > declare in the Device Tree bindings before use.
> >
> > So, either
> >
> > 	if (device_property_present(&pdev->dev, "wx,..."))
> > 		dev->flags |= MODEL_WANGXUN_SP;
> >
> > or
> >
> > 	if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
> > 	// you now have to distinguish that there is no model set in driver data
> > 		u32 model;
> >
> > 		ret = device_property_read_u32(dev, "snps,i2c-platform");
> > 		if (ret) {
> > 			...handle error...
> > 		}
> > 		dev->flags |= model
> >
> I'm not a device tree expert but I wonder would it be possible somehow
> combine this and compatible properties in dw_i2c_of_match[]? They set
> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.

Maybe the table could be changed to match device property, instead of relying
on DT only. Or device_get_match_data() could be also implemented in
software node case?

> 
> Then I'm thinking is "snps,i2c-platform" descriptive enough name for a
> model and does it confuse with "snps,designware-i2c" compatible property?

I'd like to change the name back to "wx,i2c-snps-model" for the specific one.