[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-gpio
Subject:    Re: [RFC PATCH net-next v5 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC
From:       Andy Shevchenko <andy.shevchenko () gmail ! com>
Date:       2023-04-27 10:42:28
Message-ID: CAHp75Vf54dkq9t9qt0KFjkUyj6sYrYxa8n70NxYiQX_XFJpx-Q () mail ! gmail ! com
[Download RAW message or body]

On Thu, Apr 27, 2023 at 9:58 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> On Thursday, April 27, 2023 1:57 PM, Andy Shevchenko wrote:
> > On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
> > > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@gmail.com 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?

If it's in the middle there are two (maybe more, but I can't right now
come up with) possible solutions:
1/ mother driver can provide a regmap, then in the children drivers
the dev_get_regmap(pdev->dev.parent) will return it (see
intel_soc_pmic_*.c set of drivers, IIRC they work with this schema in
mind)
2/ in the mother driver you need to open code remapping resource in a
way that it does ioremap and request regions separately.

The problem with your current approach is that you request a region in
the driver which does not handle that part of IO space and belongs to
another one. It's a clear layering violation. (Note, we already have
one big driver that does something like this and we have to learn from
it not to be trapped by making the same mistake).


-- 
With Best Regards,
Andy Shevchenko
[prev in list] [next in list] [prev in thread] [next in thread]