On Thursday, April 27, 2023 1:57 PM, Andy Shevchenko wrote:
> On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@xxxxxxxxxxxxxx> wrote:
> > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@xxxxxxxxx wrote:
> > > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti:
>
> ...
>
> > > > +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.
>
> Yeah, which makes a point that the mother driver requests a region
> that doesn't belong to it. You need to split that properly in the
> mother driver and avoid requesting it there. Is it feasible? If not,
> why?
The I2C region belongs to the middle part of the total region. It was not considered to
split because the mother driver implement I2C bus master driver itself in the previous
patch. But is it suitable for splitting? After splitting, I get two virtual address, and each
time I read/write to a register, I have to determine which region it belongs to...Right?
>
> ...
>
> > > > 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).
>
> You can share a common data structure between the mother driver and
> her children. In that case you may access it via
> `dev_get_drvdata(pdev.dev->parent)` call.
>
I'd like to try it.
> OTOH, the property, if only Linux (kernel) specific for internal
> usage, should be named accordingly, or be prepared to have one in
> Device Tree / ACPI / etc. Examples: USB dwc3 driver (see "linux,"
> ones), or intel-lpss-pci.c/intel-lpss-acpi.c (see the SPI type).
>
> --
> With Best Regards,
> Andy Shevchenko
>
