Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/25/20 11:55 AM, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Jeremy Cline <jeremy@jcline.org>
>>
>> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI
>> device. Normally we would handle this by letting the special
>> drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200
>> ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us.
>>
>> But doing so changes the modalias for the first accelerometer
>> (which is already supported and used on many devices) from
>> acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used
>> to load the driver, but is also used by hwdb matches in
>> /lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to
>> userspace by setting the ACCEL_MOUNT_MATRIX udev property.
>>
>> Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c
>> will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop
>> to no longer be set. So switching over to i2c-multi-instantiate.c is
>> not an option.
> 
> I'm wondering if we can meanwhile update hwdb to support
> i2c-multi-instantiate cases in the future and in a few years switch to
> it.

Even if we fix current hwdb entries to match on both, then there
is no guarantee newly added entries will also contain the new match.

Now with the code to get the matrix from the ACPI tables new entries
should happen less often, but I saw at least one model where the ACPI
provided matrix appears to be wrong (if the ACPI matrix was always
correct then breaking hwdb would not really be an issue).

So I don't think this is going to work and all in all it feels like
a lot of work for little gain.


>> Changes by Hans de Goede:
>> -Add explanation to the commit message why i2c-multi-instantiate.c
>>  cannot be used
>> -Also set the dev_name, fwnode and irq i2c_board_info struct members
>>  for the 2nd client
> 
> ...
> 
>> +       ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /*
>> +        * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI
>> +        * device, try instantiating a second i2c_client for an I2cSerialBusV2
>> +        * ACPI resource with index 1. The !id check avoids recursion when
>> +        * bmc150_accel_probe() gets called for the second client.
>> +        */
> 
>> +       if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) {
> 
>> +               struct i2c_board_info board_info = {
>> +                       .type = "bmc150_accel",
>> +                       /* The 2nd accel sits in the base of 2-in-1s */
> 
>> +                       .dev_name = "BOSC0200:base",
> 
> Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion
> with ACPI device naming schema? (Or was it on purpose?)

So with the ':' the end result is:

[root@localhost ~]# cd /sys/bus/i2c/devices/
[root@localhost devices]# ls | cat
6-0050
i2c-0
i2c-1
i2c-2
i2c-3
i2c-4
i2c-5
i2c-6
i2c-BOSC0200:00
i2c-BOSC0200:base
i2c-WCOM50BD:00

Which looks nice and consistent, which is why I went with the ':'
and since base is not a number, there is no chance on conflicting with
ACPI device names (it does look somewhat like an ACPI device name, but
it is an ACPI enumerated device, so ...).

Anyways if there is a strong preference for changing this to a '.'
I would be happy to make this change.

> And this seems to be the only device in the system, second as this is
> not allowed as far as I understand. Right?

I don't understand what you are trying to say here, sorry.

> But theoretically I can
> create an ACPI SSDT with quite similar excerpt and sensor and
> enumerate it via ConfigFS (I understand that is quite unlikely).


> 
>> +                       .fwnode = client->dev.fwnode,
>> +                       .irq = -ENOENT,
>> +               };
>> +               struct i2c_client *second_dev;
>> +
>> +               second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>> +               if (!IS_ERR(second_dev))
>> +                       bmc150_set_second_device(second_dev);
>> +       }
> 
> ...
> 
>>  static int bmc150_accel_remove(struct i2c_client *client)
>>  {
>> +       struct i2c_client *second_dev = bmc150_get_second_device(client);
> 
>> +       if (second_dev)
> 
> Redundant.

True, I will fix this for v2, once the ':' vs '.' thing is settled.

Regards,

Hans



> 
>> +               i2c_unregister_device(second_dev);
>> +
>>         return bmc150_accel_core_remove(&client->dev);
>>  }
>