| Message ID | 20180807080539.17811-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI bugfix + i2c-multi-instantiate pseudo driver | expand |
On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote: > Some devices have multiple I2cSerialBus resources and for things to > work > an i2c-client must be instantiated for each, each with its own > i2c_device_id. > > Normally we only instantiate an i2c-client for the first resource, > using > the ACPI HID as id. > > This commit adds a list of HIDs of devices, which need multiple i2c- > clients > instantiated from a single fwnode, to > acpi_device_enumeration_by_parent and > makes acpi_device_enumeration_by_parent return false for these devices > so > that a platform device will be instantiated. > > This allows the drivers/platform/x86/i2c-multi-instantiate.c driver, > which > knows which i2c_device_id to use for each resource, to bind to the > fwnode > and initiate an i2c-client for each resource. > > + /* > + * These devices have multiple I2cSerialBus resources and an > i2c-client > + * must be instantiated for each, each with its own > i2c_device_id. > + * Normally we only instantiate an i2c-client for the first > resource, > + * using the ACPI HID as id. These special cases are handled by > the > + * drivers/platform/x86/i2c-multi-instantiate.c driver, which > knows > + * which i2c_device_id to use for each resource. > + */ > + static const struct acpi_device_id i2c_multi_instantiate_ids[] = > { > + {"BSG1160", 0}, > + {"", 0}, > + }; Style nits: - can we move it outside of function? - terminator better without comma - is this existing style in the file and / or files in this folder for IDs? (I mean unnecessary 0:s and empty string?
Hi, On 07-08-18 13:19, Andy Shevchenko wrote: > On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote: >> Some devices have multiple I2cSerialBus resources and for things to >> work >> an i2c-client must be instantiated for each, each with its own >> i2c_device_id. >> >> Normally we only instantiate an i2c-client for the first resource, >> using >> the ACPI HID as id. >> >> This commit adds a list of HIDs of devices, which need multiple i2c- >> clients >> instantiated from a single fwnode, to >> acpi_device_enumeration_by_parent and >> makes acpi_device_enumeration_by_parent return false for these devices >> so >> that a platform device will be instantiated. >> >> This allows the drivers/platform/x86/i2c-multi-instantiate.c driver, >> which >> knows which i2c_device_id to use for each resource, to bind to the >> fwnode >> and initiate an i2c-client for each resource. >> > >> + /* >> + * These devices have multiple I2cSerialBus resources and an >> i2c-client >> + * must be instantiated for each, each with its own >> i2c_device_id. >> + * Normally we only instantiate an i2c-client for the first >> resource, >> + * using the ACPI HID as id. These special cases are handled by >> the >> + * drivers/platform/x86/i2c-multi-instantiate.c driver, which >> knows >> + * which i2c_device_id to use for each resource. >> + */ >> + static const struct acpi_device_id i2c_multi_instantiate_ids[] = >> { >> + {"BSG1160", 0}, >> + {"", 0}, >> + }; > > Style nits: > - can we move it outside of function? Sure, but there are 2 existing users of an array of acpi_device_id-s combined with an acpi_match_device_ids() call and both have the array inside the function, so for consistency it seems better to keep it where it is. > - terminator better without comma Agreed, will fix for v4. > - is this existing style in the file and / or files in this folder for > IDs? (I mean unnecessary 0:s and empty string? It seems that all variants one can come up with are already used inside this single file. I agree that less is more, so I will change this to: static const struct acpi_device_id i2c_multi_instantiate_ids[] = { {"BSG1160", }, {} }; For v4. Regards, Hans
On Tue, 2018-08-07 at 13:29 +0200, Hans de Goede wrote: > Hi, > > On 07-08-18 13:19, Andy Shevchenko wrote: > > On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote: > > > + /* > > > + * These devices have multiple I2cSerialBus resources and an > > > i2c-client > > > + * must be instantiated for each, each with its own > > > i2c_device_id. > > > + * Normally we only instantiate an i2c-client for the first > > > resource, > > > + * using the ACPI HID as id. These special cases are handled by > > > the > > > + * drivers/platform/x86/i2c-multi-instantiate.c driver, which > > > knows > > > + * which i2c_device_id to use for each resource. > > > + */ > > > + static const struct acpi_device_id i2c_multi_instantiate_ids[] = > > > { > > > + {"BSG1160", 0}, > > > + {"", 0}, > > > + }; > > > > Style nits: > > - can we move it outside of function? > > Sure, but there are 2 existing users of an array of acpi_device_id-s > combined with an acpi_match_device_ids() call and both have the array > inside the function, so for consistency it seems better to keep it > where it is. Hmm... OK. > > - is this existing style in the file and / or files in this folder > > for > > IDs? (I mean unnecessary 0:s and empty string? > > It seems that all variants one can come up with are already used > inside > this single file. Ah, that's sad. > I agree that less is more, so I will change this to: > > static const struct acpi_device_id > i2c_multi_instantiate_ids[] = { > {"BSG1160", }, > {} > }; In case if it mimics already existing style, looks quite good to me (otherwise perhaps comma inside {} can also be removed). > > For v4. Does it make sense to test v3 on your opinion? Or better to wait for v4?
Hi, On 07-08-18 13:49, Andy Shevchenko wrote: > On Tue, 2018-08-07 at 13:29 +0200, Hans de Goede wrote: >> Hi, >> >> On 07-08-18 13:19, Andy Shevchenko wrote: >>> On Tue, 2018-08-07 at 10:05 +0200, Hans de Goede wrote: > >>>> + /* >>>> + * These devices have multiple I2cSerialBus resources and an >>>> i2c-client >>>> + * must be instantiated for each, each with its own >>>> i2c_device_id. >>>> + * Normally we only instantiate an i2c-client for the first >>>> resource, >>>> + * using the ACPI HID as id. These special cases are handled by >>>> the >>>> + * drivers/platform/x86/i2c-multi-instantiate.c driver, which >>>> knows >>>> + * which i2c_device_id to use for each resource. >>>> + */ >>>> + static const struct acpi_device_id i2c_multi_instantiate_ids[] = >>>> { >>>> + {"BSG1160", 0}, >>>> + {"", 0}, >>>> + }; >>> >>> Style nits: >>> - can we move it outside of function? >> >> Sure, but there are 2 existing users of an array of acpi_device_id-s >> combined with an acpi_match_device_ids() call and both have the array >> inside the function, so for consistency it seems better to keep it >> where it is. > > Hmm... OK. > >>> - is this existing style in the file and / or files in this folder >>> for >>> IDs? (I mean unnecessary 0:s and empty string? >> >> It seems that all variants one can come up with are already used >> inside >> this single file. > > Ah, that's sad. > >> I agree that less is more, so I will change this to: >> >> static const struct acpi_device_id >> i2c_multi_instantiate_ids[] = { > >> {"BSG1160", }, >> {} >> }; > > In case if it mimics already existing style, looks quite good to me > (otherwise perhaps comma inside {} can also be removed). > >> >> For v4. > > Does it make sense to test v3 on your opinion? Or better to wait for v4? Sorry for being a bit slow to answer, I'm about to send out v4, so probably best to wait for that now. Note the 2 will be functionally identical, I mainly fixed / clarified commit messages and the MAINTAINERS entry + the small style fixed discussed above. Regards, Hans
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 6799d00dd790..44068066d269 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1540,6 +1540,18 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; + /* + * These devices have multiple I2cSerialBus resources and an i2c-client + * must be instantiated for each, each with its own i2c_device_id. + * Normally we only instantiate an i2c-client for the first resource, + * using the ACPI HID as id. These special cases are handled by the + * drivers/platform/x86/i2c-multi-instantiate.c driver, which knows + * which i2c_device_id to use for each resource. + */ + static const struct acpi_device_id i2c_multi_instantiate_ids[] = { + {"BSG1160", 0}, + {"", 0}, + }; if (acpi_is_indirect_io_slave(device)) return true; @@ -1551,6 +1563,10 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device) fwnode_property_present(&device->fwnode, "baud"))) return true; + /* Instantiate a pdev for the i2c-multi-instantiate drv to bind to */ + if (!acpi_match_device_ids(device, i2c_multi_instantiate_ids)) + return false; + INIT_LIST_HEAD(&resource_list); acpi_dev_get_resources(device, &resource_list, acpi_check_serial_bus_slave,
Some devices have multiple I2cSerialBus resources and for things to work an i2c-client must be instantiated for each, each with its own i2c_device_id. Normally we only instantiate an i2c-client for the first resource, using the ACPI HID as id. This commit adds a list of HIDs of devices, which need multiple i2c-clients instantiated from a single fwnode, to acpi_device_enumeration_by_parent and makes acpi_device_enumeration_by_parent return false for these devices so that a platform device will be instantiated. This allows the drivers/platform/x86/i2c-multi-instantiate.c driver, which knows which i2c_device_id to use for each resource, to bind to the fwnode and initiate an i2c-client for each resource. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/scan.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)