Re: [PATCH 1/2] platform/x86: i2c-multi-instantiate: Add flag for passing fwnode

[Hans de Goede]

Hi,

On 4/27/20 7:55 PM, Hans de Goede wrote:
> Hi,
>
> On 4/27/20 7:33 PM, Andy Shevchenko wrote:
> > On Mon, Apr 27, 2020 at 6:06 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > > On 4/27/20 3:18 PM, Andy Shevchenko wrote:
> > > > On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > > > > On 4/26/20 7:59 PM, Andy Shevchenko wrote:
> > > > > > On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >
> > > > > > > In some cases the driver for the i2c_client-s which i2c-multi-instantiate
> > > > > > > instantiates may need access some fields / methods from to the ACPI fwnode
> > > > > > > for which i2c_clients are being instantiated.
> > > > > > >
> > > > > > > An example of this are CPLM3218 ACPI device-s. These contain CPM0 and
> > > > > > > CPM1 packages with various information (e.g. register init values) which
> > > > > > > the driver needs.
> > > > > > >
> > > > > > > Passing the fwnode through the i2c_board_info struct also gives the
> > > > > > > i2c-core access to it, and if we do not pass an IRQ then the i2c-core
> > > > > > > will use the fwnode to get an IRQ, see i2c_acpi_get_irq().
> > > > > >
> > > > > > I'm wondering, can we rather do it in the same way like we do for
> > > > > > GPIO/APIC case here.
> > > > > > Introduce IRQ_RESOURCE_SHARED (or so) and
> > > > > >
> > > > > > case _SHARED:
> > > > > > ÂÂÂ irq = i2c_acpi_get_irq();
> > > > > > ...
> > > > > >
> > > > > > ?
> > > > >
> > > > > I think you are miss-understanding the problem. The problem is not that
> > > > > we want to share the IRQ, the problem is that we want to pass the single
> > > > > IRQ in the resources to only 1 of the instantiated I2C-clients. But if we
> > > > > do not pass an IRQ (we leave it at 0) and we do pass the fwnode then
> > > > > i2c-core-base.c will see that there is an ACPI-node attached to the
> > > > > device and will call i2c_acpi_get_irq().
> > > >
> > > > Do we know ahead which device should take IRQ resource and which should not?
> > > > Can we use current _NONE flag for them?
> > >
> > > The problem is not internal to i2c-multi-instantiate.c, the problem
> > > (once we pass a fwnode) is the API between i2c-multi-instantiate.c and
> > > the i2c-core. For the IRQ_RESOURCE_NONE case i2c-multi-instantiate.c
> > > sets board_info.irq to 0, which is the correct way to specify that
> > > we do not have an IRQ, but if don't pass an IRQ then the i2c-core
> > > will try to find one itself. And once we pass the fwnode, then
> > > the "try to find one itself" code will call i2c_acpi_get_irq()
> > > and find the same IRQ for clients we instantiate, leading to
> > > the earlier mentioned IRQ conflict.
> >
> > I'm missing something here. Why we need to pass an fwnode in the first place?
> > Seems you would like to access to methods from the driver.
>
> Right, the cm32181 code needs access to the CPM0 and CPM1 ACPI
> objects, which requires access to the fwnode.
>
> > But if you simple enumerate the driver in ACPI multi-instantiate won't
> > be needed. >
> > As far as I understand, the actual driver consumes *both* IÂC
> > resources. It's not a multi-instantiate in this case.
>
> On systems where there are 2 resources, the driver only attaches
> to the second resouce. It does detect when it gets called for
> the first resource (it detects the ARA address) and then returns
> -ENODEV.
>
> Another approach might be for the driver to call i2c_acpi_new_device
> itself when it detects the ARA address, but that is quite ugly, then
> we get:
>
> -ACPI subsys instantiates i2c-client
>  Â-cm32181_probe
>  Â -cm32181_probe instantiates i2c-client for second resource
>  ÂÂ -cm32181 probe (for second resource)
>  ÂÂ -cm32181 probe returns 0
>  Â -cm32181 probe returns -ENODEV
>
> So the end result is the same (2 clients instantiated, one
> bound to the cm32181 driver). But the nested probe calls to me
> look quite ugly and since this solution actually still does
> multi-instantiation using i2c-multi-inst seems like the more
> clean solution to me.

Ok so thinking more about this, you are right. The ACPI
resources describe a single chip here, so this really
should not use the i2c-multi-instantiate code.

Self-nack for this series.

I still think we may want to eventually pass the fwnode
through to the clients instantiated from the
i2c-multi-instantiate code, so my below proposal still stands:

> Note that we need to likely solve the fwnode passing problem
> sooner or later anyways. One of these days a driver for an
> i2c-client instantiated by the i2c-multi-inst code is going
> to need access to some methods or objects from the ACPI
> device.
>
> Since you do not like the PASS_FWNODE flag, one solution
> would be this change:
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index a66912782064..365864e8bfd5 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -341,12 +341,12 @@ static int i2c_device_probe(struct device *dev)
>  ÂÂÂÂÂÂÂÂ if (irq == -EPROBE_DEFER)
>  ÂÂÂÂÂÂÂÂÂÂÂÂ return irq;
>
> -ÂÂÂÂÂÂÂ if (irq < 0)
> -ÂÂÂÂÂÂÂÂÂÂÂ irq = 0;
> -
>  ÂÂÂÂÂÂÂÂ client->irq = irq;
>  ÂÂÂÂ }
>
> +ÂÂÂ if (client->irq < 0)
> +ÂÂÂÂÂÂÂ client->irq = 0;
> +
>  ÂÂÂÂ /*
>  ÂÂÂÂÂ * An I2C ID table is not mandatory, if and only if, a suitable OF
>  ÂÂÂÂÂ * or ACPI ID table is supplied for the probing device.
>
>
> This allows us to set board_info.irq to -ENOENT in the i2c-multi-inst
> code, causing the core to skip trying to get the irq from the fwnode
> itself, while still making drivers see 0 as irq value (which they
> expect when there is no irq).
>
> With this change i2c-multi-inst can pass the fwnode unconditionally.

Regards,

Hans