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, -- heikki
