| Message ID | 20180520132857.8103-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | ACPI/i2c Enumerate several instances out of one fwnode | expand |
On Sun, 20 May 2018 15:28:48 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi All, > > This series really consists of 2 series, patches 1-5 add support for > interesting ACPI tables which describe multiple i2c chips in a single > fwnode, sometimes multiple cases of the same chip on different addresses, > sometimes a bunch of related chips. > > Andy Shevchenko has come up with the solution of adding a quirk based > on the ACPI HID of the fwnode for these devices which makes the > drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices > for each I2cSerialBusV2 in the fwnode. I agree with him that this is > the best (least ugly) solution for this. > > I've been testing this solution on a device if mine which needs a solution > for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode > with a HID of BSG1160 which describes 3 different i2c sensors in an accel / > magneto / gyro sensor cluster on the tablet. This has let to some extra > prep. patches and some fixes to Andy's patches. > > Patches 6-9 use the new functionality creating one i2c-client per > I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and > are posted as part of this series to show how this functionality can be > used. > > Assuming everyone is ok with this series (I'm not expecting anyone to be > really happy about the need for this), then I suggest that patches 1-6 > get merged togther through either the ACPI or the i2c tree, I guess the > i2c tree would make somewhat more sense, since most patches are there. > > Then once those are accepted patches 7-9 can be merged into the iio tree, > there is no compile time dependency between the 2, so these can be merged > separately. Note merging 7-9 before there is agreement that this is the > right way to fix this is probably not a good idea. It's hideous, but I can live with it as better than anything else anyone has come up with. I just hope we don't get a huge number of these 'interesting' ACPI cases going forwards. Jonathan > > Regards, > > Hans > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote: > Hi All, > > This series really consists of 2 series, patches 1-5 add support for > interesting ACPI tables which describe multiple i2c chips in a single > fwnode, sometimes multiple cases of the same chip on different > addresses, > sometimes a bunch of related chips. > > Andy Shevchenko has come up with the solution of adding a quirk based > on the ACPI HID of the fwnode for these devices which makes the > drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client > devices > for each I2cSerialBusV2 in the fwnode. I agree with him that this is > the best (least ugly) solution for this. > > I've been testing this solution on a device if mine which needs a > solution > for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / > fwnode > with a HID of BSG1160 which describes 3 different i2c sensors in an > accel / > magneto / gyro sensor cluster on the tablet. This has let to some > extra > prep. patches and some fixes to Andy's patches. Thanks for taking care! > Patches 6-9 use the new functionality creating one i2c-client per > I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work > and > are posted as part of this series to show how this functionality can > be > used. I suppose it's better to do an "MFD" type of IIO driver for that chip. Check, for example, drivers/iio/imu/bmi160/bmi160_core.c > > Assuming everyone is ok with this series (I'm not expecting anyone to > be > really happy about the need for this), then I suggest that patches 1-6 > get merged togther through either the ACPI or the i2c tree, I guess > the > i2c tree would make somewhat more sense, since most patches are there. > > Then once those are accepted patches 7-9 can be merged into the iio > tree, > there is no compile time dependency between the 2, so these can be > merged > separately. Note merging 7-9 before there is agreement that this is > the > right way to fix this is probably not a good idea.
Hi, On 21-05-18 11:19, Andy Shevchenko wrote: > On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote: >> Hi All, >> >> This series really consists of 2 series, patches 1-5 add support for >> interesting ACPI tables which describe multiple i2c chips in a single >> fwnode, sometimes multiple cases of the same chip on different >> addresses, >> sometimes a bunch of related chips. >> >> Andy Shevchenko has come up with the solution of adding a quirk based >> on the ACPI HID of the fwnode for these devices which makes the >> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client >> devices >> for each I2cSerialBusV2 in the fwnode. I agree with him that this is >> the best (least ugly) solution for this. >> >> I've been testing this solution on a device if mine which needs a >> solution >> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / >> fwnode >> with a HID of BSG1160 which describes 3 different i2c sensors in an >> accel / >> magneto / gyro sensor cluster on the tablet. This has let to some >> extra >> prep. patches and some fixes to Andy's patches. > > Thanks for taking care! > >> Patches 6-9 use the new functionality creating one i2c-client per >> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work >> and >> are posted as part of this series to show how this functionality can >> be >> used. > > I suppose it's better to do an "MFD" type of IIO driver for that chip. > Check, for example, drivers/iio/imu/bmi160/bmi160_core.c That seems to be a single chip listening on a single i2c address / spi chip-select. In the BSG1160 case the 3 sensors are listening on 3 different i2c addresses. We could use the drivers/mfd framework, but the we get platform devices and we would need to patch all 3 existing drivers to support platform bindings and get a regmap from there (converting them to regmap where necessary). Regards, Hans > >> >> Assuming everyone is ok with this series (I'm not expecting anyone to >> be >> really happy about the need for this), then I suggest that patches 1-6 >> get merged togther through either the ACPI or the i2c tree, I guess >> the >> i2c tree would make somewhat more sense, since most patches are there. >> >> Then once those are accepted patches 7-9 can be merged into the iio >> tree, >> there is no compile time dependency between the 2, so these can be >> merged >> separately. Note merging 7-9 before there is agreement that this is >> the >> right way to fix this is probably not a good idea. >
On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: > On 21-05-18 11:19, Andy Shevchenko wrote: > > > Patches 6-9 use the new functionality creating one i2c-client per > > > I2cSerialBusV2 resource to make the sensor cluster on the HP X2 > > > work > > > and > > > are posted as part of this series to show how this functionality > > > can > > > be > > > used. > > > > I suppose it's better to do an "MFD" type of IIO driver for that > > chip. > > Check, for example, drivers/iio/imu/bmi160/bmi160_core.c > > That seems to be a single chip listening on a single i2c address / spi > chip-select. Ooops, wrong reference. > In the BSG1160 case the 3 sensors are listening on 3 different i2c > addresses. There is a Bosh magnetometer + accelerometer chip (BMC150). We have just two independent drivers for them. Luckily for ACPI they have different IDs (on the platforms where it's used like that). So, my series targeting the series of same IPs under one device... > We could use the drivers/mfd framework, but the we get platform > devices > and we would need to patch all 3 existing drivers to support platform > bindings and get a regmap from there (converting them to regmap where > necessary). ...and in your case MFD sounds better. Though why do you need to have a common regmap?
On 05/20/2018 06:23 PM, Jonathan Cameron wrote: > On Sun, 20 May 2018 15:28:48 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi All, >> >> This series really consists of 2 series, patches 1-5 add support for >> interesting ACPI tables which describe multiple i2c chips in a single >> fwnode, sometimes multiple cases of the same chip on different addresses, >> sometimes a bunch of related chips. >> >> Andy Shevchenko has come up with the solution of adding a quirk based >> on the ACPI HID of the fwnode for these devices which makes the >> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices >> for each I2cSerialBusV2 in the fwnode. I agree with him that this is >> the best (least ugly) solution for this. >> >> I've been testing this solution on a device if mine which needs a solution >> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode >> with a HID of BSG1160 which describes 3 different i2c sensors in an accel / >> magneto / gyro sensor cluster on the tablet. This has let to some extra >> prep. patches and some fixes to Andy's patches. >> >> Patches 6-9 use the new functionality creating one i2c-client per >> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and >> are posted as part of this series to show how this functionality can be >> used. >> >> Assuming everyone is ok with this series (I'm not expecting anyone to be >> really happy about the need for this), then I suggest that patches 1-6 >> get merged togther through either the ACPI or the i2c tree, I guess the >> i2c tree would make somewhat more sense, since most patches are there. >> >> Then once those are accepted patches 7-9 can be merged into the iio tree, >> there is no compile time dependency between the 2, so these can be merged >> separately. Note merging 7-9 before there is agreement that this is the >> right way to fix this is probably not a good idea. > > It's hideous, but I can live with it as better than anything else anyone > has come up with. I just hope we don't get a huge number of these > 'interesting' ACPI cases going forwards. It's ACPI, you know this is just the beginning ;)
On 05/21/2018 03:13 PM, Andy Shevchenko wrote: > On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >> On 21-05-18 11:19, Andy Shevchenko wrote: > >>>> Patches 6-9 use the new functionality creating one i2c-client per >>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>> work >>>> and >>>> are posted as part of this series to show how this functionality >>>> can >>>> be >>>> used. >>> >>> I suppose it's better to do an "MFD" type of IIO driver for that >>> chip. >>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >> >> That seems to be a single chip listening on a single i2c address / spi >> chip-select. > > Ooops, wrong reference. > >> In the BSG1160 case the 3 sensors are listening on 3 different i2c >> addresses. > > There is a Bosh magnetometer + accelerometer chip (BMC150). We have just > two independent drivers for them. Luckily for ACPI they have different > IDs (on the platforms where it's used like that). > > So, my series targeting the series of same IPs under one device... > >> We could use the drivers/mfd framework, but the we get platform >> devices >> and we would need to patch all 3 existing drivers to support platform >> bindings and get a regmap from there (converting them to regmap where >> necessary). > > ...and in your case MFD sounds better. Though why do you need to have a > common regmap? I'm not convinced MFD is the right place. You wouldn't really utilize anything of the MFD subsystem. And in a sense it is not a multi-function device. It's just multiple devices that are described by the same firmware description table entry. But I think some kind of board driver might be useful here that translates the ACPI description into something more reasonable. I.e. bind to the ACPI ID and then instantiate the 3 child I2C devices on the same bus. Those do not have to be platform drivers and you do not have to use regmap. The current approach adds board specific workarounds to each of the device drivers. It might be easier to have that managed in a central place. The problem with ACPI is that the description in the tables is often for vendor device drivers that ship together with the hardware. If you want to use the same tables with upstream drivers some kind of translation table might be required.
Hi, On 21-05-18 15:13, Andy Shevchenko wrote: > On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >> On 21-05-18 11:19, Andy Shevchenko wrote: > >>>> Patches 6-9 use the new functionality creating one i2c-client per >>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>> work >>>> and >>>> are posted as part of this series to show how this functionality >>>> can >>>> be >>>> used. >>> >>> I suppose it's better to do an "MFD" type of IIO driver for that >>> chip. >>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >> >> That seems to be a single chip listening on a single i2c address / spi >> chip-select. > > Ooops, wrong reference. > >> In the BSG1160 case the 3 sensors are listening on 3 different i2c >> addresses. > > There is a Bosh magnetometer + accelerometer chip (BMC150). We have just > two independent drivers for them. Luckily for ACPI they have different > IDs (on the platforms where it's used like that). > > So, my series targeting the series of same IPs under one device... > >> We could use the drivers/mfd framework, but the we get platform >> devices >> and we would need to patch all 3 existing drivers to support platform >> bindings and get a regmap from there (converting them to regmap where >> necessary). > > ...and in your case MFD sounds better. Though why do you need to have a > common regmap? Not a common regmap, but a regmap per i2c-address of course, but we need to have the MFD driver create these regmaps because the MFD-child devices are platform_devices which know nothing about i2c. And then we need to add platform_device / driver support to 3 iio devices and have that code retrieve the regmap. And if not all drivers are using regmap (I did not check) convert some of them to regmap first. There really is no MFD device here, as the need to create separate regmaps shows, usually the whole purpose of the MFD framework is to share a single i2c-client / regmap between multiple drivers because the i2c device has multiple function blocks. So the MFD framework really is a _very_ poor fit here. To put this differently, I can rewrite the DSDT so that things just work without needing any kernel modification at all, the problem is in the *enumeration* here, not in multiple separate function blocks sharing a single address space. And using your proposed quirk support for enumeration multiple i2c-clients from a single acpi_device fixes the enumeration problem. Regards, Hans
Hi, On 21-05-18 15:31, Lars-Peter Clausen wrote: > On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>> On 21-05-18 11:19, Andy Shevchenko wrote: >> >>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>> work >>>>> and >>>>> are posted as part of this series to show how this functionality >>>>> can >>>>> be >>>>> used. >>>> >>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>> chip. >>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>> >>> That seems to be a single chip listening on a single i2c address / spi >>> chip-select. >> >> Ooops, wrong reference. >> >>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>> addresses. >> >> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >> two independent drivers for them. Luckily for ACPI they have different >> IDs (on the platforms where it's used like that). >> >> So, my series targeting the series of same IPs under one device... >> >>> We could use the drivers/mfd framework, but the we get platform >>> devices >>> and we would need to patch all 3 existing drivers to support platform >>> bindings and get a regmap from there (converting them to regmap where >>> necessary). >> >> ...and in your case MFD sounds better. Though why do you need to have a >> common regmap? > > I'm not convinced MFD is the right place. You wouldn't really utilize > anything of the MFD subsystem. And in a sense it is not a multi-function > device. It's just multiple devices that are described by the same firmware > description table entry. > > But I think some kind of board driver might be useful here that translates > the ACPI description into something more reasonable. I.e. bind to the ACPI > ID and then instantiate the 3 child I2C devices on the same bus. Those do > not have to be platform drivers and you do not have to use regmap. > > The current approach adds board specific workarounds to each of the device > drivers. It might be easier to have that managed in a central place. Right, I considered that, and I'm actually doing pretty much that for a somewhat similar ACPI case, see: drivers/platform/x86/intel_cht_int33fe.c But there things were more complicated and we also needed to attach device-properties, while at the same time we were also somewhat lucky, because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode and we only care about 2-4, so we can have an i2c-driver in platform/drivers/x86 bind to the 1st resource and then have it instantiate i2c clients for I2cSerialBusV2 resources 2-4. The problem with the BSG1160 case is that we want to also have an iio driver bind to the first i2c-client and that will not work if an i2c-driver in platform/drivers/x86 binds to the first i2c-client and the i2c-subsys will rightfully not let us create another i2c-client at the same address. About the "board specific workarounds for each of the drivers", I could check if they are all checking an id register and if so if I could just let all 3 of them try to bind without issues. This will likely still require a change to log the id not matching add a less severe log-level. Regards, Hans
Hi, On 21-05-18 15:40, Hans de Goede wrote: > Hi, > > On 21-05-18 15:31, Lars-Peter Clausen wrote: >> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>> >>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>> work >>>>>> and >>>>>> are posted as part of this series to show how this functionality >>>>>> can >>>>>> be >>>>>> used. >>>>> >>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>> chip. >>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>> >>>> That seems to be a single chip listening on a single i2c address / spi >>>> chip-select. >>> >>> Ooops, wrong reference. >>> >>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>> addresses. >>> >>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >>> two independent drivers for them. Luckily for ACPI they have different >>> IDs (on the platforms where it's used like that). >>> >>> So, my series targeting the series of same IPs under one device... >>> >>>> We could use the drivers/mfd framework, but the we get platform >>>> devices >>>> and we would need to patch all 3 existing drivers to support platform >>>> bindings and get a regmap from there (converting them to regmap where >>>> necessary). >>> >>> ...and in your case MFD sounds better. Though why do you need to have a >>> common regmap? >> >> I'm not convinced MFD is the right place. You wouldn't really utilize >> anything of the MFD subsystem. And in a sense it is not a multi-function >> device. It's just multiple devices that are described by the same firmware >> description table entry. >> >> But I think some kind of board driver might be useful here that translates >> the ACPI description into something more reasonable. I.e. bind to the ACPI >> ID and then instantiate the 3 child I2C devices on the same bus. Those do >> not have to be platform drivers and you do not have to use regmap. >> >> The current approach adds board specific workarounds to each of the device >> drivers. It might be easier to have that managed in a central place. > > Right, I considered that, and I'm actually doing pretty much that for > a somewhat similar ACPI case, see: > > drivers/platform/x86/intel_cht_int33fe.c > > But there things were more complicated and we also needed to attach > device-properties, while at the same time we were also somewhat lucky, > because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode > and we only care about 2-4, so we can have an i2c-driver in > platform/drivers/x86 bind to the 1st resource and then have it > instantiate i2c clients for I2cSerialBusV2 resources 2-4. > > The problem with the BSG1160 case is that we want to also have an > iio driver bind to the first i2c-client and that will not work > if an i2c-driver in platform/drivers/x86 binds to the first > i2c-client and the i2c-subsys will rightfully not let us create another > i2c-client at the same address. > > About the "board specific workarounds for each of the drivers", I could > check if they are all checking an id register and if so if I could just > let all 3 of them try to bind without issues. This will likely still > require a change to log the id not matching add a less severe log-level. p.s. Also there seems to be a pattern here where this is happening more often, e.g. see also: https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl Search for BOSC0200 to find a single Device() blurb describing 2 bma250 accelerometers at 2 different addresses. And having to write a whole new driver each time this happens is going to become tedious pretty quick and also seems undesirable. Just adding a HID to an id-table OTOH for each case seems like a better (less sucking) solution. So I think we should not focus too much on the BSG1160 example and more try to come up with a generic solution for this as Andy has done. Regards, Hans
On 05/21/2018 03:44 PM, Hans de Goede wrote: > Hi, > > On 21-05-18 15:40, Hans de Goede wrote: >> Hi, >> >> On 21-05-18 15:31, Lars-Peter Clausen wrote: >>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>>> >>>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>>> work >>>>>>> and >>>>>>> are posted as part of this series to show how this functionality >>>>>>> can >>>>>>> be >>>>>>> used. >>>>>> >>>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>>> chip. >>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>>> >>>>> That seems to be a single chip listening on a single i2c address / spi >>>>> chip-select. >>>> >>>> Ooops, wrong reference. >>>> >>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>>> addresses. >>>> >>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >>>> two independent drivers for them. Luckily for ACPI they have different >>>> IDs (on the platforms where it's used like that). >>>> >>>> So, my series targeting the series of same IPs under one device... >>>> >>>>> We could use the drivers/mfd framework, but the we get platform >>>>> devices >>>>> and we would need to patch all 3 existing drivers to support platform >>>>> bindings and get a regmap from there (converting them to regmap where >>>>> necessary). >>>> >>>> ...and in your case MFD sounds better. Though why do you need to have a >>>> common regmap? >>> >>> I'm not convinced MFD is the right place. You wouldn't really utilize >>> anything of the MFD subsystem. And in a sense it is not a multi-function >>> device. It's just multiple devices that are described by the same firmware >>> description table entry. >>> >>> But I think some kind of board driver might be useful here that translates >>> the ACPI description into something more reasonable. I.e. bind to the ACPI >>> ID and then instantiate the 3 child I2C devices on the same bus. Those do >>> not have to be platform drivers and you do not have to use regmap. >>> >>> The current approach adds board specific workarounds to each of the device >>> drivers. It might be easier to have that managed in a central place. >> >> Right, I considered that, and I'm actually doing pretty much that for >> a somewhat similar ACPI case, see: >> >> drivers/platform/x86/intel_cht_int33fe.c >> >> But there things were more complicated and we also needed to attach >> device-properties, while at the same time we were also somewhat lucky, >> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode >> and we only care about 2-4, so we can have an i2c-driver in >> platform/drivers/x86 bind to the 1st resource and then have it >> instantiate i2c clients for I2cSerialBusV2 resources 2-4. >> >> The problem with the BSG1160 case is that we want to also have an >> iio driver bind to the first i2c-client and that will not work >> if an i2c-driver in platform/drivers/x86 binds to the first >> i2c-client and the i2c-subsys will rightfully not let us create another >> i2c-client at the same address. >> >> About the "board specific workarounds for each of the drivers", I could >> check if they are all checking an id register and if so if I could just >> let all 3 of them try to bind without issues. This will likely still >> require a change to log the id not matching add a less severe log-level. > > p.s. > > Also there seems to be a pattern here where this is happening more > often, e.g. see also: > > https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl > Search for BOSC0200 to find a single Device() blurb describing > 2 bma250 accelerometers at 2 different addresses. > > And having to write a whole new driver each time this happens is > going to become tedious pretty quick and also seems undesirable. > > Just adding a HID to an id-table OTOH for each case seems like a > better (less sucking) solution. I'd use the same argument to argue for the opposite. The fact that is is a common occurrence means it should not be handled in the device driver, because it means you'll end up having to add quirks for each and every vendor binding. E.g. if you look at the example you provided there is also a mounting matrix and calibration data for each of the two sensors. You need a way to map those to the individual devices. > > So I think we should not focus too much on the BSG1160 example > and more try to come up with a generic solution for this as > Andy has done. I agree that a generic solution is the right approach, but I do not think that adding lots of individual quirks to device drivers is a generic solution. Maybe we can teach the I2C framework about these hub nodes, so that the device for the hub itself does not prevent the children from binding to their I2C addresses. You are already patching the I2C core anyway.
Hi, On 21-05-18 17:07, Lars-Peter Clausen wrote: > On 05/21/2018 03:44 PM, Hans de Goede wrote: >> Hi, >> >> On 21-05-18 15:40, Hans de Goede wrote: >>> Hi, >>> >>> On 21-05-18 15:31, Lars-Peter Clausen wrote: >>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>>>> >>>>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>>>> work >>>>>>>> and >>>>>>>> are posted as part of this series to show how this functionality >>>>>>>> can >>>>>>>> be >>>>>>>> used. >>>>>>> >>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>>>> chip. >>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>>>> >>>>>> That seems to be a single chip listening on a single i2c address / spi >>>>>> chip-select. >>>>> >>>>> Ooops, wrong reference. >>>>> >>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>>>> addresses. >>>>> >>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >>>>> two independent drivers for them. Luckily for ACPI they have different >>>>> IDs (on the platforms where it's used like that). >>>>> >>>>> So, my series targeting the series of same IPs under one device... >>>>> >>>>>> We could use the drivers/mfd framework, but the we get platform >>>>>> devices >>>>>> and we would need to patch all 3 existing drivers to support platform >>>>>> bindings and get a regmap from there (converting them to regmap where >>>>>> necessary). >>>>> >>>>> ...and in your case MFD sounds better. Though why do you need to have a >>>>> common regmap? >>>> >>>> I'm not convinced MFD is the right place. You wouldn't really utilize >>>> anything of the MFD subsystem. And in a sense it is not a multi-function >>>> device. It's just multiple devices that are described by the same firmware >>>> description table entry. >>>> >>>> But I think some kind of board driver might be useful here that translates >>>> the ACPI description into something more reasonable. I.e. bind to the ACPI >>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do >>>> not have to be platform drivers and you do not have to use regmap. >>>> >>>> The current approach adds board specific workarounds to each of the device >>>> drivers. It might be easier to have that managed in a central place. >>> >>> Right, I considered that, and I'm actually doing pretty much that for >>> a somewhat similar ACPI case, see: >>> >>> drivers/platform/x86/intel_cht_int33fe.c >>> >>> But there things were more complicated and we also needed to attach >>> device-properties, while at the same time we were also somewhat lucky, >>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode >>> and we only care about 2-4, so we can have an i2c-driver in >>> platform/drivers/x86 bind to the 1st resource and then have it >>> instantiate i2c clients for I2cSerialBusV2 resources 2-4. >>> >>> The problem with the BSG1160 case is that we want to also have an >>> iio driver bind to the first i2c-client and that will not work >>> if an i2c-driver in platform/drivers/x86 binds to the first >>> i2c-client and the i2c-subsys will rightfully not let us create another >>> i2c-client at the same address. >>> >>> About the "board specific workarounds for each of the drivers", I could >>> check if they are all checking an id register and if so if I could just >>> let all 3 of them try to bind without issues. This will likely still >>> require a change to log the id not matching add a less severe log-level. >> >> p.s. >> >> Also there seems to be a pattern here where this is happening more >> often, e.g. see also: >> >> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl >> Search for BOSC0200 to find a single Device() blurb describing >> 2 bma250 accelerometers at 2 different addresses. >> >> And having to write a whole new driver each time this happens is >> going to become tedious pretty quick and also seems undesirable. >> >> Just adding a HID to an id-table OTOH for each case seems like a >> better (less sucking) solution. > > I'd use the same argument to argue for the opposite. The fact that is is a > common occurrence means it should not be handled in the device driver, > because it means you'll end up having to add quirks for each and every > vendor binding. > > E.g. if you look at the example you provided there is also a mounting matrix > and calibration data for each of the two sensors. You need a way to map > those to the individual devices. > >> >> So I think we should not focus too much on the BSG1160 example >> and more try to come up with a generic solution for this as >> Andy has done. > > I agree that a generic solution is the right approach, but I do not think > that adding lots of individual quirks to device drivers is a generic solution. > > Maybe we can teach the I2C framework about these hub nodes, so that the > device for the hub itself does not prevent the children from binding to > their I2C addresses. You are already patching the I2C core anyway. Ok, so thinking more about this I think that we indeed need to solve this differently. Another argument here is to also not pollute the i2c core with a whole bunch of extra code, just to handle these corner cases. So my idea is to have an i2c-driver under platform/x86 which deals with these special cases where we want multiple i2c-clients instantiated from a single ACPI fwnode. The idea is to have a bool no_address_busy_check in i2c_board_info, with a big fat comment that it is special and should be avoided, which disables the i2c_check_addr_busy() check in i2c_new_device(). This instantiation driver will use per ACPI-HID driver_data pointing to an array of: struct give_my_type_a_proper_name { const char *type; int irq_index; } The probe will then iterate over this array, stopping at a NULL type pointer and instantiate i2c_clients for each entry in the array using type as i2c_board_info.type and requesting an interrupt from the ACPI fwnode resources using irq_index, except when irq_index is -1 (and setting the special no_address_busy_check bool for the first instantiation). The idea is that by having a generic instantiation loop for this driven by per ACPI-HID driver_data we have a generic solution, while at the same time having this isolated in a driver which can be modular and only loaded when one of the special ACPI HIDs is encountered. So how does this sound ? I will give you all some time to reply and assuming no one shoots this down try to implement this say next weekend. Heikki, would this also work for your "INT3515" HID case? Regards, Hans
On Mon, May 21, 2018 at 09:12:38PM +0200, Hans de Goede wrote: > Hi, > > On 21-05-18 17:07, Lars-Peter Clausen wrote: > > On 05/21/2018 03:44 PM, Hans de Goede wrote: > > > Hi, > > > > > > On 21-05-18 15:40, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 21-05-18 15:31, Lars-Peter Clausen wrote: > > > > > On 05/21/2018 03:13 PM, Andy Shevchenko wrote: > > > > > > On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: > > > > > > > On 21-05-18 11:19, Andy Shevchenko wrote: > > > > > > > > > > > > > > > Patches 6-9 use the new functionality creating?? one i2c-client per > > > > > > > > > I2cSerialBusV2 resource to make the sensor cluster on the HP X2 > > > > > > > > > work > > > > > > > > > and > > > > > > > > > are posted as part of this series to show how this functionality > > > > > > > > > can > > > > > > > > > be > > > > > > > > > used. > > > > > > > > > > > > > > > > I suppose it's better to do an "MFD" type of IIO driver for that > > > > > > > > chip. > > > > > > > > Check, for example, drivers/iio/imu/bmi160/bmi160_core.c > > > > > > > > > > > > > > That seems to be a single chip listening on a single i2c address / spi > > > > > > > chip-select. > > > > > > > > > > > > Ooops, wrong reference. > > > > > > > > > > > > > In the BSG1160 case the 3 sensors are listening on 3 different i2c > > > > > > > addresses. > > > > > > > > > > > > There is a Bosh magnetometer + accelerometer chip (BMC150). We have just > > > > > > two independent drivers for them. Luckily for ACPI they have different > > > > > > IDs (on the platforms where it's used like that). > > > > > > > > > > > > So, my series targeting the series of same IPs under one device... > > > > > > > > > > > > > We could use the drivers/mfd framework, but the we get platform > > > > > > > devices > > > > > > > and we would need to patch all 3 existing drivers to support platform > > > > > > > bindings and get a regmap from there (converting them to regmap where > > > > > > > necessary). > > > > > > > > > > > > ...and in your case MFD sounds better. Though why do you need to have a > > > > > > common regmap? > > > > > > > > > > I'm not convinced MFD is the right place. You wouldn't really utilize > > > > > anything of the MFD subsystem. And in a sense it is not a multi-function > > > > > device. It's just multiple devices that are described by the same firmware > > > > > description table entry. > > > > > > > > > > But I think some kind of board driver might be useful here that translates > > > > > the ACPI description into something more reasonable. I.e. bind to the ACPI > > > > > ID and then instantiate the 3 child I2C devices on the same bus. Those do > > > > > not have to be platform drivers and you do not have to use regmap. > > > > > > > > > > The current approach adds board specific workarounds to each of the device > > > > > drivers. It might be easier to have that managed in a central place. > > > > > > > > Right, I considered that, and I'm actually doing pretty much that for > > > > a somewhat similar ACPI case, see: > > > > > > > > drivers/platform/x86/intel_cht_int33fe.c > > > > > > > > But there things were more complicated and we also needed to attach > > > > device-properties, while at the same time we were also somewhat lucky, > > > > because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode > > > > and we only care about 2-4, so we can have an i2c-driver in > > > > platform/drivers/x86 bind to the 1st resource and then have it > > > > instantiate i2c clients for I2cSerialBusV2 resources 2-4. > > > > > > > > The problem with the BSG1160 case is that we want to also have an > > > > iio driver bind to the first i2c-client and that will not work > > > > if an i2c-driver in platform/drivers/x86 binds to the first > > > > i2c-client and the i2c-subsys will rightfully not let us create another > > > > i2c-client at the same address. > > > > > > > > About the "board specific workarounds for each of the drivers", I could > > > > check if they are all checking an id register and if so if I could just > > > > let all 3 of them try to bind without issues. This will likely still > > > > require a change to log the id not matching add a less severe log-level. > > > > > > p.s. > > > > > > Also there seems to be a pattern here where this is happening more > > > often, e.g. see also: > > > > > > https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl > > > Search for BOSC0200 to find a single Device() blurb describing > > > 2 bma250 accelerometers at 2 different addresses. > > > > > > And having to write a whole new driver each time this happens is > > > going to become tedious pretty quick and also seems undesirable. > > > > > > Just adding a HID to an id-table OTOH for each case seems like a > > > better (less sucking) solution. > > > > I'd use the same argument to argue for the opposite. The fact that is is a > > common occurrence means it should not be handled in the device driver, > > because it means you'll end up having to add quirks for each and every > > vendor binding. > > > > E.g. if you look at the example you provided there is also a mounting matrix > > and calibration data for each of the two sensors. You need a way to map > > those to the individual devices. > > > > > > > > So I think we should not focus too much on the BSG1160 example > > > and more try to come up with a generic solution for this as > > > Andy has done. > > > > I agree that a generic solution is the right approach, but I do not think > > that adding lots of individual quirks to device drivers is a generic solution. > > > > Maybe we can teach the I2C framework about these hub nodes, so that the > > device for the hub itself does not prevent the children from binding to > > their I2C addresses. You are already patching the I2C core anyway. > > Ok, so thinking more about this I think that we indeed need to solve this > differently. Another argument here is to also not pollute the i2c core > with a whole bunch of extra code, just to handle these corner cases. > > So my idea is to have an i2c-driver under platform/x86 which deals with > these special cases where we want multiple i2c-clients instantiated > from a single ACPI fwnode. > > The idea is to have a bool no_address_busy_check in i2c_board_info, > with a big fat comment that it is special and should be avoided, > which disables the i2c_check_addr_busy() check in i2c_new_device(). > > This instantiation driver will use per ACPI-HID driver_data > pointing to an array of: > > struct give_my_type_a_proper_name { > const char *type; > int irq_index; > } > > The probe will then iterate over this array, stopping at a NULL type > pointer and instantiate i2c_clients for each entry in the array > using type as i2c_board_info.type and requesting an interrupt > from the ACPI fwnode resources using irq_index, except when irq_index > is -1 (and setting the special no_address_busy_check bool for the > first instantiation). > > The idea is that by having a generic instantiation loop for this > driven by per ACPI-HID driver_data we have a generic solution, > while at the same time having this isolated in a driver which > can be modular and only loaded when one of the special ACPI HIDs > is encountered. > > So how does this sound ? I will give you all some time to reply > and assuming no one shoots this down try to implement this say > next weekend. > > Heikki, would this also work for your "INT3515" HID case? I'm sure it will. I'll test it once you are done. Thanks,
On Mon, 21 May 2018 21:12:38 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 21-05-18 17:07, Lars-Peter Clausen wrote: > > On 05/21/2018 03:44 PM, Hans de Goede wrote: > >> Hi, > >> > >> On 21-05-18 15:40, Hans de Goede wrote: > >>> Hi, > >>> > >>> On 21-05-18 15:31, Lars-Peter Clausen wrote: > >>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: > >>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: > >>>>>> On 21-05-18 11:19, Andy Shevchenko wrote: > >>>>> > >>>>>>>> Patches 6-9 use the new functionality creating one i2c-client per > >>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 > >>>>>>>> work > >>>>>>>> and > >>>>>>>> are posted as part of this series to show how this functionality > >>>>>>>> can > >>>>>>>> be > >>>>>>>> used. > >>>>>>> > >>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that > >>>>>>> chip. > >>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c > >>>>>> > >>>>>> That seems to be a single chip listening on a single i2c address / spi > >>>>>> chip-select. > >>>>> > >>>>> Ooops, wrong reference. > >>>>> > >>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c > >>>>>> addresses. > >>>>> > >>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just > >>>>> two independent drivers for them. Luckily for ACPI they have different > >>>>> IDs (on the platforms where it's used like that). > >>>>> > >>>>> So, my series targeting the series of same IPs under one device... > >>>>> > >>>>>> We could use the drivers/mfd framework, but the we get platform > >>>>>> devices > >>>>>> and we would need to patch all 3 existing drivers to support platform > >>>>>> bindings and get a regmap from there (converting them to regmap where > >>>>>> necessary). > >>>>> > >>>>> ...and in your case MFD sounds better. Though why do you need to have a > >>>>> common regmap? > >>>> > >>>> I'm not convinced MFD is the right place. You wouldn't really utilize > >>>> anything of the MFD subsystem. And in a sense it is not a multi-function > >>>> device. It's just multiple devices that are described by the same firmware > >>>> description table entry. > >>>> > >>>> But I think some kind of board driver might be useful here that translates > >>>> the ACPI description into something more reasonable. I.e. bind to the ACPI > >>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do > >>>> not have to be platform drivers and you do not have to use regmap. > >>>> > >>>> The current approach adds board specific workarounds to each of the device > >>>> drivers. It might be easier to have that managed in a central place. > >>> > >>> Right, I considered that, and I'm actually doing pretty much that for > >>> a somewhat similar ACPI case, see: > >>> > >>> drivers/platform/x86/intel_cht_int33fe.c > >>> > >>> But there things were more complicated and we also needed to attach > >>> device-properties, while at the same time we were also somewhat lucky, > >>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode > >>> and we only care about 2-4, so we can have an i2c-driver in > >>> platform/drivers/x86 bind to the 1st resource and then have it > >>> instantiate i2c clients for I2cSerialBusV2 resources 2-4. > >>> > >>> The problem with the BSG1160 case is that we want to also have an > >>> iio driver bind to the first i2c-client and that will not work > >>> if an i2c-driver in platform/drivers/x86 binds to the first > >>> i2c-client and the i2c-subsys will rightfully not let us create another > >>> i2c-client at the same address. > >>> > >>> About the "board specific workarounds for each of the drivers", I could > >>> check if they are all checking an id register and if so if I could just > >>> let all 3 of them try to bind without issues. This will likely still > >>> require a change to log the id not matching add a less severe log-level. > >> > >> p.s. > >> > >> Also there seems to be a pattern here where this is happening more > >> often, e.g. see also: > >> > >> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl > >> Search for BOSC0200 to find a single Device() blurb describing > >> 2 bma250 accelerometers at 2 different addresses. > >> > >> And having to write a whole new driver each time this happens is > >> going to become tedious pretty quick and also seems undesirable. > >> > >> Just adding a HID to an id-table OTOH for each case seems like a > >> better (less sucking) solution. > > > > I'd use the same argument to argue for the opposite. The fact that is is a > > common occurrence means it should not be handled in the device driver, > > because it means you'll end up having to add quirks for each and every > > vendor binding. > > > > E.g. if you look at the example you provided there is also a mounting matrix > > and calibration data for each of the two sensors. You need a way to map > > those to the individual devices. > > > >> > >> So I think we should not focus too much on the BSG1160 example > >> and more try to come up with a generic solution for this as > >> Andy has done. > > > > I agree that a generic solution is the right approach, but I do not think > > that adding lots of individual quirks to device drivers is a generic solution. > > > > Maybe we can teach the I2C framework about these hub nodes, so that the > > device for the hub itself does not prevent the children from binding to > > their I2C addresses. You are already patching the I2C core anyway. > > Ok, so thinking more about this I think that we indeed need to solve this > differently. Another argument here is to also not pollute the i2c core > with a whole bunch of extra code, just to handle these corner cases. > > So my idea is to have an i2c-driver under platform/x86 which deals with > these special cases where we want multiple i2c-clients instantiated > from a single ACPI fwnode. Don't make it x86 specific (sooner or later someone will do something similar on an ARM or other platform), other than that it will be interesting to see how this pans out. One corner case as well that looks much the same is the package in package parts. They really are kind of "one device", so not really broken ACPI in the same way most of these cases are. I looked through the ACPI 6.2 spec and concluded that these cases were already broken. If the opportunity arises I'll have a chat with some people more heavily involved in that standard than I am and see if we can strengthen this. May make no difference to what Vendors do but we can try. We can pragmatically handle them with the same interface, but it seems a little clunky. For DT we have the option to have odd bindings that bind the two internal parts separately but we clearly can't control that for ACPI. > > The idea is to have a bool no_address_busy_check in i2c_board_info, > with a big fat comment that it is special and should be avoided, > which disables the i2c_check_addr_busy() check in i2c_new_device(). > > This instantiation driver will use per ACPI-HID driver_data > pointing to an array of: > > struct give_my_type_a_proper_name { > const char *type; > int irq_index; > } > > The probe will then iterate over this array, stopping at a NULL type > pointer and instantiate i2c_clients for each entry in the array > using type as i2c_board_info.type and requesting an interrupt > from the ACPI fwnode resources using irq_index, except when irq_index > is -1 (and setting the special no_address_busy_check bool for the > first instantiation). > > The idea is that by having a generic instantiation loop for this > driven by per ACPI-HID driver_data we have a generic solution, > while at the same time having this isolated in a driver which > can be modular and only loaded when one of the special ACPI HIDs > is encountered. > > So how does this sound ? I will give you all some time to reply > and assuming no one shoots this down try to implement this say > next weekend. > > Heikki, would this also work for your "INT3515" HID case? > > Regards, > > Hans > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/21/2018 09:12 PM, Hans de Goede wrote: > Hi, > > On 21-05-18 17:07, Lars-Peter Clausen wrote: >> On 05/21/2018 03:44 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 21-05-18 15:40, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 21-05-18 15:31, Lars-Peter Clausen wrote: >>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>>>>> >>>>>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>>>>> work >>>>>>>>> and >>>>>>>>> are posted as part of this series to show how this functionality >>>>>>>>> can >>>>>>>>> be >>>>>>>>> used. >>>>>>>> >>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>>>>> chip. >>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>>>>> >>>>>>> That seems to be a single chip listening on a single i2c address / spi >>>>>>> chip-select. >>>>>> >>>>>> Ooops, wrong reference. >>>>>> >>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>>>>> addresses. >>>>>> >>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >>>>>> two independent drivers for them. Luckily for ACPI they have different >>>>>> IDs (on the platforms where it's used like that). >>>>>> >>>>>> So, my series targeting the series of same IPs under one device... >>>>>> >>>>>>> We could use the drivers/mfd framework, but the we get platform >>>>>>> devices >>>>>>> and we would need to patch all 3 existing drivers to support platform >>>>>>> bindings and get a regmap from there (converting them to regmap where >>>>>>> necessary). >>>>>> >>>>>> ...and in your case MFD sounds better. Though why do you need to have a >>>>>> common regmap? >>>>> >>>>> I'm not convinced MFD is the right place. You wouldn't really utilize >>>>> anything of the MFD subsystem. And in a sense it is not a multi-function >>>>> device. It's just multiple devices that are described by the same firmware >>>>> description table entry. >>>>> >>>>> But I think some kind of board driver might be useful here that translates >>>>> the ACPI description into something more reasonable. I.e. bind to the ACPI >>>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do >>>>> not have to be platform drivers and you do not have to use regmap. >>>>> >>>>> The current approach adds board specific workarounds to each of the device >>>>> drivers. It might be easier to have that managed in a central place. >>>> >>>> Right, I considered that, and I'm actually doing pretty much that for >>>> a somewhat similar ACPI case, see: >>>> >>>> drivers/platform/x86/intel_cht_int33fe.c >>>> >>>> But there things were more complicated and we also needed to attach >>>> device-properties, while at the same time we were also somewhat lucky, >>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode >>>> and we only care about 2-4, so we can have an i2c-driver in >>>> platform/drivers/x86 bind to the 1st resource and then have it >>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4. >>>> >>>> The problem with the BSG1160 case is that we want to also have an >>>> iio driver bind to the first i2c-client and that will not work >>>> if an i2c-driver in platform/drivers/x86 binds to the first >>>> i2c-client and the i2c-subsys will rightfully not let us create another >>>> i2c-client at the same address. >>>> >>>> About the "board specific workarounds for each of the drivers", I could >>>> check if they are all checking an id register and if so if I could just >>>> let all 3 of them try to bind without issues. This will likely still >>>> require a change to log the id not matching add a less severe log-level. >>> >>> p.s. >>> >>> Also there seems to be a pattern here where this is happening more >>> often, e.g. see also: >>> >>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl >>> Search for BOSC0200 to find a single Device() blurb describing >>> 2 bma250 accelerometers at 2 different addresses. >>> >>> And having to write a whole new driver each time this happens is >>> going to become tedious pretty quick and also seems undesirable. >>> >>> Just adding a HID to an id-table OTOH for each case seems like a >>> better (less sucking) solution. >> >> I'd use the same argument to argue for the opposite. The fact that is is a >> common occurrence means it should not be handled in the device driver, >> because it means you'll end up having to add quirks for each and every >> vendor binding. >> >> E.g. if you look at the example you provided there is also a mounting matrix >> and calibration data for each of the two sensors. You need a way to map >> those to the individual devices. >> >>> >>> So I think we should not focus too much on the BSG1160 example >>> and more try to come up with a generic solution for this as >>> Andy has done. >> >> I agree that a generic solution is the right approach, but I do not think >> that adding lots of individual quirks to device drivers is a generic >> solution. >> >> Maybe we can teach the I2C framework about these hub nodes, so that the >> device for the hub itself does not prevent the children from binding to >> their I2C addresses. You are already patching the I2C core anyway. > > Ok, so thinking more about this I think that we indeed need to solve this > differently. Another argument here is to also not pollute the i2c core > with a whole bunch of extra code, just to handle these corner cases. > > So my idea is to have an i2c-driver under platform/x86 which deals with > these special cases where we want multiple i2c-clients instantiated > from a single ACPI fwnode. > > The idea is to have a bool no_address_busy_check in i2c_board_info, > with a big fat comment that it is special and should be avoided, > which disables the i2c_check_addr_busy() check in i2c_new_device(). Ideally we'd be able to register the hub as an address-less device and then have each of the sensors bind to their respective I2C addresses while still making sure that there are no address duplicates. Maybe is possible to re-use part of the I2C MUX infrastructure and have the hub register itself as some kind of MUX device and the sensors as children to the hub. This way the sensors would still be grouped in the device hierarchy.
Hi, On 22-05-18 13:40, Lars-Peter Clausen wrote: > On 05/21/2018 09:12 PM, Hans de Goede wrote: >> Hi, >> >> On 21-05-18 17:07, Lars-Peter Clausen wrote: >>> On 05/21/2018 03:44 PM, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 21-05-18 15:40, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 21-05-18 15:31, Lars-Peter Clausen wrote: >>>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>>>>>> >>>>>>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>>>>>> work >>>>>>>>>> and >>>>>>>>>> are posted as part of this series to show how this functionality >>>>>>>>>> can >>>>>>>>>> be >>>>>>>>>> used. >>>>>>>>> >>>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>>>>>> chip. >>>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>>>>>> >>>>>>>> That seems to be a single chip listening on a single i2c address / spi >>>>>>>> chip-select. >>>>>>> >>>>>>> Ooops, wrong reference. >>>>>>> >>>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>>>>>> addresses. >>>>>>> >>>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just >>>>>>> two independent drivers for them. Luckily for ACPI they have different >>>>>>> IDs (on the platforms where it's used like that). >>>>>>> >>>>>>> So, my series targeting the series of same IPs under one device... >>>>>>> >>>>>>>> We could use the drivers/mfd framework, but the we get platform >>>>>>>> devices >>>>>>>> and we would need to patch all 3 existing drivers to support platform >>>>>>>> bindings and get a regmap from there (converting them to regmap where >>>>>>>> necessary). >>>>>>> >>>>>>> ...and in your case MFD sounds better. Though why do you need to have a >>>>>>> common regmap? >>>>>> >>>>>> I'm not convinced MFD is the right place. You wouldn't really utilize >>>>>> anything of the MFD subsystem. And in a sense it is not a multi-function >>>>>> device. It's just multiple devices that are described by the same firmware >>>>>> description table entry. >>>>>> >>>>>> But I think some kind of board driver might be useful here that translates >>>>>> the ACPI description into something more reasonable. I.e. bind to the ACPI >>>>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do >>>>>> not have to be platform drivers and you do not have to use regmap. >>>>>> >>>>>> The current approach adds board specific workarounds to each of the device >>>>>> drivers. It might be easier to have that managed in a central place. >>>>> >>>>> Right, I considered that, and I'm actually doing pretty much that for >>>>> a somewhat similar ACPI case, see: >>>>> >>>>> drivers/platform/x86/intel_cht_int33fe.c >>>>> >>>>> But there things were more complicated and we also needed to attach >>>>> device-properties, while at the same time we were also somewhat lucky, >>>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode >>>>> and we only care about 2-4, so we can have an i2c-driver in >>>>> platform/drivers/x86 bind to the 1st resource and then have it >>>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4. >>>>> >>>>> The problem with the BSG1160 case is that we want to also have an >>>>> iio driver bind to the first i2c-client and that will not work >>>>> if an i2c-driver in platform/drivers/x86 binds to the first >>>>> i2c-client and the i2c-subsys will rightfully not let us create another >>>>> i2c-client at the same address. >>>>> >>>>> About the "board specific workarounds for each of the drivers", I could >>>>> check if they are all checking an id register and if so if I could just >>>>> let all 3 of them try to bind without issues. This will likely still >>>>> require a change to log the id not matching add a less severe log-level. >>>> >>>> p.s. >>>> >>>> Also there seems to be a pattern here where this is happening more >>>> often, e.g. see also: >>>> >>>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl >>>> Search for BOSC0200 to find a single Device() blurb describing >>>> 2 bma250 accelerometers at 2 different addresses. >>>> >>>> And having to write a whole new driver each time this happens is >>>> going to become tedious pretty quick and also seems undesirable. >>>> >>>> Just adding a HID to an id-table OTOH for each case seems like a >>>> better (less sucking) solution. >>> >>> I'd use the same argument to argue for the opposite. The fact that is is a >>> common occurrence means it should not be handled in the device driver, >>> because it means you'll end up having to add quirks for each and every >>> vendor binding. >>> >>> E.g. if you look at the example you provided there is also a mounting matrix >>> and calibration data for each of the two sensors. You need a way to map >>> those to the individual devices. >>> >>>> >>>> So I think we should not focus too much on the BSG1160 example >>>> and more try to come up with a generic solution for this as >>>> Andy has done. >>> >>> I agree that a generic solution is the right approach, but I do not think >>> that adding lots of individual quirks to device drivers is a generic >>> solution. >>> >>> Maybe we can teach the I2C framework about these hub nodes, so that the >>> device for the hub itself does not prevent the children from binding to >>> their I2C addresses. You are already patching the I2C core anyway. >> >> Ok, so thinking more about this I think that we indeed need to solve this >> differently. Another argument here is to also not pollute the i2c core >> with a whole bunch of extra code, just to handle these corner cases. >> >> So my idea is to have an i2c-driver under platform/x86 which deals with >> these special cases where we want multiple i2c-clients instantiated >> from a single ACPI fwnode. >> >> The idea is to have a bool no_address_busy_check in i2c_board_info, >> with a big fat comment that it is special and should be avoided, >> which disables the i2c_check_addr_busy() check in i2c_new_device(). > > Ideally we'd be able to register the hub as an address-less device and then > have each of the sensors bind to their respective I2C addresses while still > making sure that there are no address duplicates. > > Maybe is possible to re-use part of the I2C MUX infrastructure and have the > hub register itself as some kind of MUX device and the sensors as children > to the hub. This way the sensors would still be grouped in the device hierarchy. There is no hub, i2c topology wise there are simply 3 separate i2c devices which for some reason got lumped together in a single ACPI fwnode. Representing this as a different topology then it actually is seems counter-productive. I do not know the physicial topology in the HP x2 case, but in the Lenovo Yoga 11e case there are 2 separate sensors, one in the base and one in the display, lumped together in a single ACPI fwnode. Regards, Hans
On 05/22/2018 01:55 PM, Hans de Goede wrote: > Hi, > > On 22-05-18 13:40, Lars-Peter Clausen wrote: >> On 05/21/2018 09:12 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 21-05-18 17:07, Lars-Peter Clausen wrote: >>>> On 05/21/2018 03:44 PM, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 21-05-18 15:40, Hans de Goede wrote: >>>>>> Hi, >>>>>> >>>>>> On 21-05-18 15:31, Lars-Peter Clausen wrote: >>>>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote: >>>>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote: >>>>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote: >>>>>>>> >>>>>>>>>>> Patches 6-9 use the new functionality creating one i2c-client per >>>>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 >>>>>>>>>>> work >>>>>>>>>>> and >>>>>>>>>>> are posted as part of this series to show how this functionality >>>>>>>>>>> can >>>>>>>>>>> be >>>>>>>>>>> used. >>>>>>>>>> >>>>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that >>>>>>>>>> chip. >>>>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c >>>>>>>>> >>>>>>>>> That seems to be a single chip listening on a single i2c address / spi >>>>>>>>> chip-select. >>>>>>>> >>>>>>>> Ooops, wrong reference. >>>>>>>> >>>>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c >>>>>>>>> addresses. >>>>>>>> >>>>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have >>>>>>>> just >>>>>>>> two independent drivers for them. Luckily for ACPI they have different >>>>>>>> IDs (on the platforms where it's used like that). >>>>>>>> >>>>>>>> So, my series targeting the series of same IPs under one device... >>>>>>>> >>>>>>>>> We could use the drivers/mfd framework, but the we get platform >>>>>>>>> devices >>>>>>>>> and we would need to patch all 3 existing drivers to support platform >>>>>>>>> bindings and get a regmap from there (converting them to regmap where >>>>>>>>> necessary). >>>>>>>> >>>>>>>> ...and in your case MFD sounds better. Though why do you need to have a >>>>>>>> common regmap? >>>>>>> >>>>>>> I'm not convinced MFD is the right place. You wouldn't really utilize >>>>>>> anything of the MFD subsystem. And in a sense it is not a multi-function >>>>>>> device. It's just multiple devices that are described by the same >>>>>>> firmware >>>>>>> description table entry. >>>>>>> >>>>>>> But I think some kind of board driver might be useful here that >>>>>>> translates >>>>>>> the ACPI description into something more reasonable. I.e. bind to the >>>>>>> ACPI >>>>>>> ID and then instantiate the 3 child I2C devices on the same bus. >>>>>>> Those do >>>>>>> not have to be platform drivers and you do not have to use regmap. >>>>>>> >>>>>>> The current approach adds board specific workarounds to each of the >>>>>>> device >>>>>>> drivers. It might be easier to have that managed in a central place. >>>>>> >>>>>> Right, I considered that, and I'm actually doing pretty much that for >>>>>> a somewhat similar ACPI case, see: >>>>>> >>>>>> drivers/platform/x86/intel_cht_int33fe.c >>>>>> >>>>>> But there things were more complicated and we also needed to attach >>>>>> device-properties, while at the same time we were also somewhat lucky, >>>>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode >>>>>> and we only care about 2-4, so we can have an i2c-driver in >>>>>> platform/drivers/x86 bind to the 1st resource and then have it >>>>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4. >>>>>> >>>>>> The problem with the BSG1160 case is that we want to also have an >>>>>> iio driver bind to the first i2c-client and that will not work >>>>>> if an i2c-driver in platform/drivers/x86 binds to the first >>>>>> i2c-client and the i2c-subsys will rightfully not let us create another >>>>>> i2c-client at the same address. >>>>>> >>>>>> About the "board specific workarounds for each of the drivers", I could >>>>>> check if they are all checking an id register and if so if I could just >>>>>> let all 3 of them try to bind without issues. This will likely still >>>>>> require a change to log the id not matching add a less severe log-level. >>>>> >>>>> p.s. >>>>> >>>>> Also there seems to be a pattern here where this is happening more >>>>> often, e.g. see also: >>>>> >>>>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl >>>>> Search for BOSC0200 to find a single Device() blurb describing >>>>> 2 bma250 accelerometers at 2 different addresses. >>>>> >>>>> And having to write a whole new driver each time this happens is >>>>> going to become tedious pretty quick and also seems undesirable. >>>>> >>>>> Just adding a HID to an id-table OTOH for each case seems like a >>>>> better (less sucking) solution. >>>> >>>> I'd use the same argument to argue for the opposite. The fact that is is a >>>> common occurrence means it should not be handled in the device driver, >>>> because it means you'll end up having to add quirks for each and every >>>> vendor binding. >>>> >>>> E.g. if you look at the example you provided there is also a mounting >>>> matrix >>>> and calibration data for each of the two sensors. You need a way to map >>>> those to the individual devices. >>>> >>>>> >>>>> So I think we should not focus too much on the BSG1160 example >>>>> and more try to come up with a generic solution for this as >>>>> Andy has done. >>>> >>>> I agree that a generic solution is the right approach, but I do not think >>>> that adding lots of individual quirks to device drivers is a generic >>>> solution. >>>> >>>> Maybe we can teach the I2C framework about these hub nodes, so that the >>>> device for the hub itself does not prevent the children from binding to >>>> their I2C addresses. You are already patching the I2C core anyway. >>> >>> Ok, so thinking more about this I think that we indeed need to solve this >>> differently. Another argument here is to also not pollute the i2c core >>> with a whole bunch of extra code, just to handle these corner cases. >>> >>> So my idea is to have an i2c-driver under platform/x86 which deals with >>> these special cases where we want multiple i2c-clients instantiated >>> from a single ACPI fwnode. >>> >>> The idea is to have a bool no_address_busy_check in i2c_board_info, >>> with a big fat comment that it is special and should be avoided, >>> which disables the i2c_check_addr_busy() check in i2c_new_device(). >> >> Ideally we'd be able to register the hub as an address-less device and then >> have each of the sensors bind to their respective I2C addresses while still >> making sure that there are no address duplicates. >> >> Maybe is possible to re-use part of the I2C MUX infrastructure and have the >> hub register itself as some kind of MUX device and the sensors as children >> to the hub. This way the sensors would still be grouped in the device >> hierarchy. > > There is no hub, i2c topology wise there are simply 3 separate i2c devices > which for some reason got lumped together in a single ACPI fwnode. Representing > this as a different topology then it actually is seems counter-productive. > > I do not know the physicial topology in the HP x2 case, but in the > Lenovo Yoga 11e case there are 2 separate sensors, one in the base and > one in the display, lumped together in a single ACPI fwnode. Hm, OK. I was assuming that there was a good reason why they are lumped together, forming some sort of virtual hub. If they produce uncorrelated datastreams it does not really matter.
On Sunday, May 20, 2018 3:28:48 PM CEST Hans de Goede wrote: > Hi All, > > This series really consists of 2 series, patches 1-5 add support for > interesting ACPI tables which describe multiple i2c chips in a single > fwnode, sometimes multiple cases of the same chip on different addresses, > sometimes a bunch of related chips. > > Andy Shevchenko has come up with the solution of adding a quirk based > on the ACPI HID of the fwnode for these devices which makes the > drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices > for each I2cSerialBusV2 in the fwnode. I agree with him that this is > the best (least ugly) solution for this. > > I've been testing this solution on a device if mine which needs a solution > for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode > with a HID of BSG1160 which describes 3 different i2c sensors in an accel / > magneto / gyro sensor cluster on the tablet. This has let to some extra > prep. patches and some fixes to Andy's patches. > > Patches 6-9 use the new functionality creating one i2c-client per > I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and > are posted as part of this series to show how this functionality can be > used. > > Assuming everyone is ok with this series (I'm not expecting anyone to be > really happy about the need for this), then I suggest that patches 1-6 > get merged togther through either the ACPI or the i2c tree, I guess the > i2c tree would make somewhat more sense, since most patches are there. > > Then once those are accepted patches 7-9 can be merged into the iio tree, > there is no compile time dependency between the 2, so these can be merged > separately. Note merging 7-9 before there is agreement that this is the > right way to fix this is probably not a good idea. From the discussion I gather that this series will be updated. Thanks, Rafael
Hi, On 24-05-18 10:55, Rafael J. Wysocki wrote: > On Sunday, May 20, 2018 3:28:48 PM CEST Hans de Goede wrote: >> Hi All, >> >> This series really consists of 2 series, patches 1-5 add support for >> interesting ACPI tables which describe multiple i2c chips in a single >> fwnode, sometimes multiple cases of the same chip on different addresses, >> sometimes a bunch of related chips. >> >> Andy Shevchenko has come up with the solution of adding a quirk based >> on the ACPI HID of the fwnode for these devices which makes the >> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices >> for each I2cSerialBusV2 in the fwnode. I agree with him that this is >> the best (least ugly) solution for this. >> >> I've been testing this solution on a device if mine which needs a solution >> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode >> with a HID of BSG1160 which describes 3 different i2c sensors in an accel / >> magneto / gyro sensor cluster on the tablet. This has let to some extra >> prep. patches and some fixes to Andy's patches. >> >> Patches 6-9 use the new functionality creating one i2c-client per >> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and >> are posted as part of this series to show how this functionality can be >> used. >> >> Assuming everyone is ok with this series (I'm not expecting anyone to be >> really happy about the need for this), then I suggest that patches 1-6 >> get merged togther through either the ACPI or the i2c tree, I guess the >> i2c tree would make somewhat more sense, since most patches are there. >> >> Then once those are accepted patches 7-9 can be merged into the iio tree, >> there is no compile time dependency between the 2, so these can be merged >> separately. Note merging 7-9 before there is agreement that this is the >> right way to fix this is probably not a good idea. > > From the discussion I gather that this series will be updated. More like thrown away and rewritten, but yes this is obsolete. Regards, Hans