Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux