[PATCH 0/2] ACPI / scan: Create platform device for CPLM3218 ACPI nodes

[PATCH 0/2] ACPI / scan: Create platform device for CPLM3218 ACPI nodes

[Hans de Goede]

Hi All,

Here is a patch series to deal with the way jow the CM3281
ambient-light-sensor is described in the ACPI tables of Asus
T100TA and T100CHI devices:

 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
 {
     Name (SBUF, ResourceTemplate ()
     {
         I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
         {
             0x00000033,
         }
     })
     Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
 }

Notice that the first entry is the SMBus Alert Response Address, this
is actually somewhat useful as on this sensor we must read a byte
from that address once to clear an alert which seems to be set on
power-on; and without doing this the sensor will not respond on its
actual address. Taking care of this is left up to the cm32181 driver
(I will Cc you on the patch series for that).

This series uses the i2c-multi-instantiate code to instantiate
i2c-clients for both addresses.

Note this series touches files under both drivers/apci and
drivers/platform/x86. IIRC in the past i2c-multi-instantiate changes
were merged through Rafael's tree because of this.

Andy or Darren may we have your Acked-by for merging this through
Rafael's tree?

Regards,

Hans

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

[Hans de Goede]

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().

This is a problem when there is only an IRQ for 1 of the clients described
in the ACPI device we are instantiating clients for. If we unconditionally
pass the fwnode, then i2c_acpi_get_irq() will assign the same IRQ to all
clients instantiated, leading to kernel-oopses like this (BSG1160 device):

[   27.340557] genirq: Flags mismatch irq 76. 00002001 (bmc150_magn_event) vs. 00000001 (bmc150_accel_event)
[   27.340567] Call Trace:
...

So we cannot simply always pass the fwnode. This commit adds a PASS_FWNODE
flag, which can be used to pass the fwnode in cases where we do not have
the IRQ problem and the driver for the instantiated client(s) needs access
to the fwnode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 6acc8457866e..dcafb1a29d17 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -20,6 +20,8 @@
 #define IRQ_RESOURCE_GPIO	1
 #define IRQ_RESOURCE_APIC	2
 
+#define PASS_FWNODE		BIT(2)
+
 struct i2c_inst_data {
 	const char *type;
 	unsigned int flags;
@@ -93,6 +95,10 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
 			 inst_data[i].type, i);
 		board_info.dev_name = name;
+
+		if (inst_data[i].flags & PASS_FWNODE)
+			board_info.fwnode = dev->fwnode;
+
 		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
 		case IRQ_RESOURCE_GPIO:
 			ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
-- 
2.26.0

[PATCH 2/2] ACPI / scan: Create platform device for CPLM3218 ACPI nodes

[Hans de Goede]

Some CPLM3218 ACPI nodes also put the SMBus Alert Response Address (0x0c)
in their ACPI resource table; and they put it there as the first entry,
here is an example from the CPLM3218 device in the DSDT of an Asus T100TA:

 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
 {
     Name (SBUF, ResourceTemplate ()
     {
         I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
         {
             0x00000033,
         }
     })
     Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
 }

The actual I2C address of the sensor in this case is the 0x48 address
from the second resource-table entry. On some other devices
(e.g. HP X2 Bay Trail models, Acer SW5-012) the CPLM3218 node contains
only 1 I2C resource.

Add the CPLM3218 to the I2C multi instantiate list, so that the
i2c-multi-instantiate.c driver can handle it.

Note in the case where there are 2 I2C resources we simply instatiate
i2c-clients for both and let the cm32181 driver figure out that the
first one is not the one it wants.

Doing things this way is actually desirable because on devices where
there are 2 I2C resources it seems that we first need to do a SMBus
read of the 0x0c address before the sensor will respond to I2C transfers
on its actual address.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c                          | 1 +
 drivers/platform/x86/i2c-multi-instantiate.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 6d3448895382..937d72fc212c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1544,6 +1544,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
 		{"BSG1160", },
 		{"BSG2150", },
+		{"CPLM3218", },
 		{"INT33FE", },
 		{"INT3515", },
 		{}
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index dcafb1a29d17..e1cdc44e6f57 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -180,6 +180,12 @@ static const struct i2c_inst_data int3515_data[]  = {
 	{}
 };
 
+static const struct i2c_inst_data cplm3218_data[]  = {
+	{ "cm32181", PASS_FWNODE },
+	{ "cm32181", PASS_FWNODE },
+	{}
+};
+
 /*
  * Note new device-ids must also be added to i2c_multi_instantiate_ids in
  * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
@@ -187,6 +193,7 @@ static const struct i2c_inst_data int3515_data[]  = {
 static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
 	{ "BSG1160", (unsigned long)bsg1160_data },
 	{ "BSG2150", (unsigned long)bsg2150_data },
+	{ "CPLM3218", (unsigned long)cplm3218_data },
 	{ "INT3515", (unsigned long)int3515_data },
 	{ }
 };
-- 
2.26.0

Re: [PATCH 2/2] ACPI / scan: Create platform device for CPLM3218 ACPI nodes

[Rafael J. Wysocki]

On Sun, Apr 26, 2020 at 12:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some CPLM3218 ACPI nodes also put the SMBus Alert Response Address (0x0c)
> in their ACPI resource table; and they put it there as the first entry,
> here is an example from the CPLM3218 device in the DSDT of an Asus T100TA:
>
>  Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>  {
>      Name (SBUF, ResourceTemplate ()
>      {
>          I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
>              AddressingMode7Bit, "\\_SB.I2C3",
>              0x00, ResourceConsumer, , Exclusive,
>              )
>          I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
>              AddressingMode7Bit, "\\_SB.I2C3",
>              0x00, ResourceConsumer, , Exclusive,
>              )
>          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
>          {
>              0x00000033,
>          }
>      })
>      Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
>  }
>
> The actual I2C address of the sensor in this case is the 0x48 address
> from the second resource-table entry. On some other devices
> (e.g. HP X2 Bay Trail models, Acer SW5-012) the CPLM3218 node contains
> only 1 I2C resource.
>
> Add the CPLM3218 to the I2C multi instantiate list, so that the
> i2c-multi-instantiate.c driver can handle it.
>
> Note in the case where there are 2 I2C resources we simply instatiate
> i2c-clients for both and let the cm32181 driver figure out that the
> first one is not the one it wants.
>
> Doing things this way is actually desirable because on devices where
> there are 2 I2C resources it seems that we first need to do a SMBus
> read of the 0x0c address before the sensor will respond to I2C transfers
> on its actual address.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the scan.c change and I'm expecting platform/x86 to take care of
this series.

Thanks!

> ---
>  drivers/acpi/scan.c                          | 1 +
>  drivers/platform/x86/i2c-multi-instantiate.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 6d3448895382..937d72fc212c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1544,6 +1544,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>         static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
>                 {"BSG1160", },
>                 {"BSG2150", },
> +               {"CPLM3218", },
>                 {"INT33FE", },
>                 {"INT3515", },
>                 {}
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index dcafb1a29d17..e1cdc44e6f57 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -180,6 +180,12 @@ static const struct i2c_inst_data int3515_data[]  = {
>         {}
>  };
>
> +static const struct i2c_inst_data cplm3218_data[]  = {
> +       { "cm32181", PASS_FWNODE },
> +       { "cm32181", PASS_FWNODE },
> +       {}
> +};
> +
>  /*
>   * Note new device-ids must also be added to i2c_multi_instantiate_ids in
>   * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> @@ -187,6 +193,7 @@ static const struct i2c_inst_data int3515_data[]  = {
>  static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
>         { "BSG1160", (unsigned long)bsg1160_data },
>         { "BSG2150", (unsigned long)bsg2150_data },
> +       { "CPLM3218", (unsigned long)cplm3218_data },
>         { "INT3515", (unsigned long)int3515_data },
>         { }
>  };
> --
> 2.26.0
>

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

[Andy Shevchenko]

On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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();
...

?

>
> This is a problem when there is only an IRQ for 1 of the clients described
> in the ACPI device we are instantiating clients for. If we unconditionally
> pass the fwnode, then i2c_acpi_get_irq() will assign the same IRQ to all
> clients instantiated, leading to kernel-oopses like this (BSG1160 device):
>
> [   27.340557] genirq: Flags mismatch irq 76. 00002001 (bmc150_magn_event) vs. 00000001 (bmc150_accel_event)
> [   27.340567] Call Trace:
> ...
>
> So we cannot simply always pass the fwnode. This commit adds a PASS_FWNODE
> flag, which can be used to pass the fwnode in cases where we do not have
> the IRQ problem and the driver for the instantiated client(s) needs access
> to the fwnode.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/i2c-multi-instantiate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index 6acc8457866e..dcafb1a29d17 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -20,6 +20,8 @@
>  #define IRQ_RESOURCE_GPIO      1
>  #define IRQ_RESOURCE_APIC      2
>
> +#define PASS_FWNODE            BIT(2)
> +
>  struct i2c_inst_data {
>         const char *type;
>         unsigned int flags;
> @@ -93,6 +95,10 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>                 snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
>                          inst_data[i].type, i);
>                 board_info.dev_name = name;
> +
> +               if (inst_data[i].flags & PASS_FWNODE)
> +                       board_info.fwnode = dev->fwnode;
> +
>                 switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
>                 case IRQ_RESOURCE_GPIO:
>                         ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
> --
> 2.26.0
>


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 4/26/20 7:59 PM, Andy Shevchenko wrote:
> On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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().

So the solution is definitely not calling i2c_acpi_get_irq() inside
i2c-multi-instantiate.c we want to avoid the i2c_acpi_get_irq(),
leaving the other 2 clients for the BSG1160 device without an IRQ
and thus avoiding the IRQ mismatch (it is a mismatch because the
drivers do not set the shared flag; and that is ok, we do not want
to share the IRQ, it is just for the accelerometer AFAIK).

Regards,

Hans


> 
>>
>> This is a problem when there is only an IRQ for 1 of the clients described
>> in the ACPI device we are instantiating clients for. If we unconditionally
>> pass the fwnode, then i2c_acpi_get_irq() will assign the same IRQ to all
>> clients instantiated, leading to kernel-oopses like this (BSG1160 device):
>>
>> [   27.340557] genirq: Flags mismatch irq 76. 00002001 (bmc150_magn_event) vs. 00000001 (bmc150_accel_event)
>> [   27.340567] Call Trace:
>> ...
>>
>> So we cannot simply always pass the fwnode. This commit adds a PASS_FWNODE
>> flag, which can be used to pass the fwnode in cases where we do not have
>> the IRQ problem and the driver for the instantiated client(s) needs access
>> to the fwnode.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/platform/x86/i2c-multi-instantiate.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
>> index 6acc8457866e..dcafb1a29d17 100644
>> --- a/drivers/platform/x86/i2c-multi-instantiate.c
>> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
>> @@ -20,6 +20,8 @@
>>   #define IRQ_RESOURCE_GPIO      1
>>   #define IRQ_RESOURCE_APIC      2
>>
>> +#define PASS_FWNODE            BIT(2)
>> +
>>   struct i2c_inst_data {
>>          const char *type;
>>          unsigned int flags;
>> @@ -93,6 +95,10 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>>                  snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
>>                           inst_data[i].type, i);
>>                  board_info.dev_name = name;
>> +
>> +               if (inst_data[i].flags & PASS_FWNODE)
>> +                       board_info.fwnode = dev->fwnode;
>> +
>>                  switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
>>                  case IRQ_RESOURCE_GPIO:
>>                          ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
>> --
>> 2.26.0
>>
> 
> 

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

[Andy Shevchenko]

On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 4/26/20 7:59 PM, Andy Shevchenko wrote:
> > On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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?

> So the solution is definitely not calling i2c_acpi_get_irq() inside
> i2c-multi-instantiate.c we want to avoid the i2c_acpi_get_irq(),
> leaving the other 2 clients for the BSG1160 device without an IRQ
> and thus avoiding the IRQ mismatch (it is a mismatch because the
> drivers do not set the shared flag; and that is ok, we do not want
> to share the IRQ, it is just for the accelerometer AFAIK).

> >> This is a problem when there is only an IRQ for 1 of the clients described
> >> in the ACPI device we are instantiating clients for. If we unconditionally
> >> pass the fwnode, then i2c_acpi_get_irq() will assign the same IRQ to all
> >> clients instantiated, leading to kernel-oopses like this (BSG1160 device):
> >>
> >> [   27.340557] genirq: Flags mismatch irq 76. 00002001 (bmc150_magn_event) vs. 00000001 (bmc150_accel_event)
> >> [   27.340567] Call Trace:
> >> ...
> >>
> >> So we cannot simply always pass the fwnode. This commit adds a PASS_FWNODE
> >> flag, which can be used to pass the fwnode in cases where we do not have
> >> the IRQ problem and the driver for the instantiated client(s) needs access
> >> to the fwnode.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/platform/x86/i2c-multi-instantiate.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> >> index 6acc8457866e..dcafb1a29d17 100644
> >> --- a/drivers/platform/x86/i2c-multi-instantiate.c
> >> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> >> @@ -20,6 +20,8 @@
> >>   #define IRQ_RESOURCE_GPIO      1
> >>   #define IRQ_RESOURCE_APIC      2
> >>
> >> +#define PASS_FWNODE            BIT(2)
> >> +
> >>   struct i2c_inst_data {
> >>          const char *type;
> >>          unsigned int flags;
> >> @@ -93,6 +95,10 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
> >>                  snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
> >>                           inst_data[i].type, i);
> >>                  board_info.dev_name = name;
> >> +
> >> +               if (inst_data[i].flags & PASS_FWNODE)
> >> +                       board_info.fwnode = dev->fwnode;
> >> +
> >>                  switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
> >>                  case IRQ_RESOURCE_GPIO:
> >>                          ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
> >> --
> >> 2.26.0
> >>
> >
> >
>


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 4/27/20 3:18 PM, Andy Shevchenko wrote:
> On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 4/26/20 7:59 PM, Andy Shevchenko wrote:
>>> On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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.

<adding Wolfram + i2c lists to the Cc>

We could set board_info.irq to -ENOENT to indicate that there should
not be an irq. But that will get passed to various i2c-drivers, many of
which check for an irq like this:

	if (client->irq) {
		...
	}

We can avoid this, without needing to change all the drivers
by making the i2c-core check for board_info.irq < 0 to skip its
own "try to find IRQ" code and then set client->irq to 0 after
that check, rather then setting it to board_info.irq = -ENOENT.

If we do that then we can unconditionally pass the fwnode in
the i2c-multi-instantiate code.

Regards,

Hans





>> So the solution is definitely not calling i2c_acpi_get_irq() inside
>> i2c-multi-instantiate.c we want to avoid the i2c_acpi_get_irq(),
>> leaving the other 2 clients for the BSG1160 device without an IRQ
>> and thus avoiding the IRQ mismatch (it is a mismatch because the
>> drivers do not set the shared flag; and that is ok, we do not want
>> to share the IRQ, it is just for the accelerometer AFAIK).
> 
>>>> This is a problem when there is only an IRQ for 1 of the clients described
>>>> in the ACPI device we are instantiating clients for. If we unconditionally
>>>> pass the fwnode, then i2c_acpi_get_irq() will assign the same IRQ to all
>>>> clients instantiated, leading to kernel-oopses like this (BSG1160 device):
>>>>
>>>> [   27.340557] genirq: Flags mismatch irq 76. 00002001 (bmc150_magn_event) vs. 00000001 (bmc150_accel_event)
>>>> [   27.340567] Call Trace:
>>>> ...
>>>>
>>>> So we cannot simply always pass the fwnode. This commit adds a PASS_FWNODE
>>>> flag, which can be used to pass the fwnode in cases where we do not have
>>>> the IRQ problem and the driver for the instantiated client(s) needs access
>>>> to the fwnode.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    drivers/platform/x86/i2c-multi-instantiate.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
>>>> index 6acc8457866e..dcafb1a29d17 100644
>>>> --- a/drivers/platform/x86/i2c-multi-instantiate.c
>>>> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
>>>> @@ -20,6 +20,8 @@
>>>>    #define IRQ_RESOURCE_GPIO      1
>>>>    #define IRQ_RESOURCE_APIC      2
>>>>
>>>> +#define PASS_FWNODE            BIT(2)
>>>> +
>>>>    struct i2c_inst_data {
>>>>           const char *type;
>>>>           unsigned int flags;
>>>> @@ -93,6 +95,10 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>>>>                   snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
>>>>                            inst_data[i].type, i);
>>>>                   board_info.dev_name = name;
>>>> +
>>>> +               if (inst_data[i].flags & PASS_FWNODE)
>>>> +                       board_info.fwnode = dev->fwnode;
>>>> +
>>>>                   switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
>>>>                   case IRQ_RESOURCE_GPIO:
>>>>                           ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
>>>> --
>>>> 2.26.0
>>>>
>>>
>>>
>>
> 
> 

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

[Andy Shevchenko]

On Mon, Apr 27, 2020 at 6:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/27/20 3:18 PM, Andy Shevchenko wrote:
> > On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 4/26/20 7:59 PM, Andy Shevchenko wrote:
> >>> On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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.
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.


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 4/27/20 7:33 PM, Andy Shevchenko wrote:
> On Mon, Apr 27, 2020 at 6:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 4/27/20 3:18 PM, Andy Shevchenko wrote:
>>> On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 4/26/20 7:59 PM, Andy Shevchenko wrote:
>>>>> On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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.

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

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@redhat.com> wrote:
>>> On 4/27/20 3:18 PM, Andy Shevchenko wrote:
>>>> On Mon, Apr 27, 2020 at 3:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> On 4/26/20 7:59 PM, Andy Shevchenko wrote:
>>>>>> On Sun, Apr 26, 2020 at 1:47 PM Hans de Goede <hdegoede@redhat.com> 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