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

[Jiawen Wu <jiawenwu@xxxxxxxxxxxxxx>]

On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@xxxxxxxxx wrote:
> Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti:
> > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > with SFP.
> >
> > Introduce the property "i2c-dw-flags" to match device data for software
> > node case. Since IO resource was mapped on the ethernet driver, add a model
> > quirk to get resource from platform info.
> >
> > 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.
> 
> Thanks for an update, my comments below.
> 
> ...
> 
> >  		goto done_nolock;
> >  	}
> >
> > +	if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) {
> > +		ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num);
> > +		goto done_nolock;
> > +	}
> 
> 	switch (dev->flags & MODEL_MASK) {
> 	case AMD:
> 		...
> 	case WANGXUN:
> 		...
> 	default:
> 		break;
> 	}
> 
> ...
> 
> > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev->dev);
> > +	struct resource *r;
> > +
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!r)
> > +		return -ENODEV;
> > +
> > +	dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> > +
> > +	return PTR_ERR_OR_ZERO(dev->base);
> > +}
> 
> Redundant. See below.
> 
> ...
> 
> >  	case MODEL_BAIKAL_BT1:
> >  		ret = bt1_i2c_request_regs(dev);
> >  		break;
> > +	case MODEL_WANGXUN_SP:
> > +		ret = txgbe_i2c_request_regs(dev);
> 
> How is it different to...
> 
> > +		break;
> >  	default:
> >  		dev->base = devm_platform_ioremap_resource(pdev, 0);
> 
> ...this one?
> 

devm_platform_ioremap_resource() has one more devm_request_mem_region()
operation than devm_ioremap(). By my test, this memory cannot be re-requested,
only re-mapped.

> ...
> 
> >  	dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
> 
> > +	if (!dev->flags)
> 
> No need to check this. Just define priorities (I would go with above to be
> higher priority).
> 
> > +		device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags);
> 
> Needs to be added to the Device Tree bindings I believe.
> 
> But wait, don't we have other ways to detect your hardware at probe time and
> initialize flags respectively?
> 

I2C is connected to our NIC chip with no PCI ID, so I register a platform device for it.
Please see the 4/9 patch. Software nodes are used to pass the device structure but
no DT and ACPI. I haven't found another way to initialize flags yet, other than the
platform data used in the previous patch (it seems to be an obsolete way).

> --
> With Best Regards,
> Andy Shevchenko
> 
> 
>