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.
