| Message ID | 20121116172828.GY17774@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, 16 Nov 2012 19:28:28 +0200, Mika Westerberg wrote: > On Fri, Nov 16, 2012 at 05:47:53PM +0100, Jean Delvare wrote: > > Hi Mika, > > > > On Fri, 16 Nov 2012 17:23:32 +0200, Mika Westerberg wrote: > > > Here's the updated version where we handle 10-bit addresses properly > > > (hopefully). > > > (...) > > > +static int acpi_i2c_find_device(struct device *dev, acpi_handle *handle) > > > +{ > > > (...) > > > + memset(&i2c_find, 0, sizeof(i2c_find)); > > > + i2c_find.addr = client->addr; > > > + if (client->flags & I2C_CLIENT_TEN) > > > + i2c_find.access_mode = ACPI_I2C_10BIT_MODE; > > > > This works because you are lucky and ACPI_I2C_7BIT_MODE has value 0, > > but for correctness I'd rather add: > > > > else > > i2c_find.access_mode = ACPI_I2C_7BIT_MODE; > > Right, sorry about that. The patch below should address this as well. > (...) Yes, looks good.
On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > ... > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Date: Mon, 10 Sep 2012 12:12:32 +0300 > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate > and configure the I2C slave devices behind the I2C controller. This patch > adds helper functions to support I2C slave enumeration. > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() > in order to get its slave devices enumerated, created and bound to the > corresponding ACPI handle. I must admit I don't understand the strategy here. Likely it's only because I haven't been paying enough attention, but I'll ask anyway in case anybody else is similarly confused. The callchain when we enumerate these slave devices looks like this: acpi_i2c_register_devices(struct i2c_adapter *) acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) acpi_i2c_add_device acpi_bus_get_device acpi_bus_get_status acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) <find IRQ, addr> acpi_dev_free_resources i2c_new_device client = kzalloc client->dev = ... device_register(&client->dev) Is the ACPI namespace in question something like the following? Device { # i2C master, i.e., the i2c_adapter _HID PNPmmmm Device { # I2C slave 1, i.e., a client _HID PNPsss1 _CRS SerialBus/I2C addr addr1, mode mode1 IRQ irq1 } Device { # I2C slave 2 _HID PNPsss2 _CRS SerialBus/I2C addr addr2, mode mode2 IRQ irq2 } } _CRS is a device configuration method, so I would expect that it exists within the scope of a Device() object. The way I'm used to this working is for a driver to specify "I know about PNPsss1 devices." But it looks like acpi_i2c_register() walks the namespace below an i2c master device, registering a new i2c device (a slave) for every ACPI device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C descriptor. It seems like you're basically claiming those devices nodes based on the contents of their _CRS, not based on their PNP IDs, which seems strange to me. We have to be able to hook device enumeration into udev so we can autoload drivers. It's obvious how to do that with _HID and _CID -- we just emit a uevent saying "we found a new device with PNP IDs x,y,z". I don't see how to do anything similar based on the _CRS contents. Again, probably I'm completely missing the point here, and I'm sorry to be dense. I guess this isn't really "enumeration" -- the ACPI core has previously walked this namespace and built acpi_devices for the Device() nodes, and I think it emits uevents at that time. So this is more of a "claim" than an "enumerate." But the Device() node for the I2C slave still exists, and it has _HID/_CID, doesn't it? Do we use that _HID anywhere? In any event, after acpi_i2c_register(), I think we have a set of i2c_client devices (with the above namespace, I assume we'd have two of them). I guess acpi_i2c_find_device() is useful now -- it looks like it takes a "struct device *" (e.g., &client->dev from a struct i2c_client), and and gives you back the acpi_handle corresponding to it? Here's the callchain of that path: acpi_i2c_find_device(struct device *) # acpi_i2c_bus.find_device i2c_verify_client(dev) acpi_walk_namespace acpi_i2c_find_child acpi_bus_get_device acpi_bus_get_status acpi_dev_get_resources(..., acpi_i2c_find_child_address, ...) acpi_i2c_find_child_address found if (SERIAL_BUS && SERIAL_TYPE_I2C && slave_address == xx) acpi_dev_free_resource_list *handle = handle That seems like an awful lot of work to do just to map a struct device * back to the acpi_handle. But I don't have any suggestion; just that observation. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 16, 2012 at 11:46:40PM -0700, Bjorn Helgaas wrote: > On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > ... > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Date: Mon, 10 Sep 2012 12:12:32 +0300 > > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support > > > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate > > and configure the I2C slave devices behind the I2C controller. This patch > > adds helper functions to support I2C slave enumeration. > > > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() > > in order to get its slave devices enumerated, created and bound to the > > corresponding ACPI handle. > > I must admit I don't understand the strategy here. Likely it's only > because I haven't been paying enough attention, but I'll ask anyway in > case anybody else is similarly confused. > > The callchain when we enumerate these slave devices looks like this: > > acpi_i2c_register_devices(struct i2c_adapter *) > acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) > acpi_i2c_add_device > acpi_bus_get_device > acpi_bus_get_status > acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) > <find IRQ, addr> > acpi_dev_free_resources > i2c_new_device > client = kzalloc > client->dev = ... > device_register(&client->dev) > > Is the ACPI namespace in question something like the following? > > Device { # i2C master, i.e., the i2c_adapter > _HID PNPmmmm > Device { # I2C slave 1, i.e., a client > _HID PNPsss1 > _CRS > SerialBus/I2C addr addr1, mode mode1 > IRQ irq1 > } > Device { # I2C slave 2 > _HID PNPsss2 > _CRS > SerialBus/I2C addr addr2, mode mode2 > IRQ irq2 > } > } Yes. > _CRS is a device configuration method, so I would expect that it > exists within the scope of a Device() object. The way I'm used to > this working is for a driver to specify "I know about PNPsss1 > devices." Yes. > But it looks like acpi_i2c_register() walks the namespace below an i2c > master device, registering a new i2c device (a slave) for every ACPI > device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C > descriptor. It seems like you're basically claiming those devices > nodes based on the contents of their _CRS, not based on their PNP IDs, > which seems strange to me. Yes, if we only matched the PNP IDs we would get bunch of PNP devices which certainly doesn't help us to reuse the existing I2C drivers. So instead of creating a new glue driver for ACPI or PNP device we added this enumeration method that then creates the I2C devices, just like DT does. > We have to be able to hook device enumeration into udev so we can > autoload drivers. It's obvious how to do that with _HID and _CID -- > we just emit a uevent saying "we found a new device with PNP IDs > x,y,z". I don't see how to do anything similar based on the _CRS > contents. Again, probably I'm completely missing the point here, and > I'm sorry to be dense. If you remember the concrete example, I gave you few mails back, it shows how we are planning to the matching in an existing (or new driver). So we match with _HID and _CID but that matching is done in the core of each bus in question (platform, I2C and SPI). > I guess this isn't really "enumeration" -- the ACPI core has > previously walked this namespace and built acpi_devices for the > Device() nodes, and I think it emits uevents at that time. So this is > more of a "claim" than an "enumerate." But the Device() node for the > I2C slave still exists, and it has _HID/_CID, doesn't it? Do we use > that _HID anywhere? The matching is done using those _HID and _CIDs in the ACPI namespace, we just now allow any driver to match any device using and not limit it to ACPI device drivers (this is done with the help of drv->acpi_match_table). > In any event, after acpi_i2c_register(), I think we have a set of > i2c_client devices (with the above namespace, I assume we'd have two > of them). I guess acpi_i2c_find_device() is useful now -- it looks > like it takes a "struct device *" (e.g., &client->dev from a struct > i2c_client), and and gives you back the acpi_handle corresponding to > it? > > Here's the callchain of that path: > > acpi_i2c_find_device(struct device *) # acpi_i2c_bus.find_device > i2c_verify_client(dev) > acpi_walk_namespace > acpi_i2c_find_child > acpi_bus_get_device > acpi_bus_get_status > acpi_dev_get_resources(..., acpi_i2c_find_child_address, ...) > acpi_i2c_find_child_address > found if (SERIAL_BUS && SERIAL_TYPE_I2C && slave_address == xx) > acpi_dev_free_resource_list > *handle = handle > > That seems like an awful lot of work to do just to map a struct device > * back to the acpi_handle. But I don't have any suggestion; just that > observation. We had similar discussion internally about not using that drivers/acpi/glue.c but we decided to use it for now, even if it really backwards and makes things hard (like you observed as well). A much better way would be just to assign the handle once we make the device but it seemed not to be that simple after all. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 17, 2012 at 10:03:54AM +0200, Mika Westerberg wrote: > On Fri, Nov 16, 2012 at 11:46:40PM -0700, Bjorn Helgaas wrote: > > On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > ... > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Date: Mon, 10 Sep 2012 12:12:32 +0300 > > > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support > > > > > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate > > > and configure the I2C slave devices behind the I2C controller. This patch > > > adds helper functions to support I2C slave enumeration. > > > > > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() > > > in order to get its slave devices enumerated, created and bound to the > > > corresponding ACPI handle. > > > > I must admit I don't understand the strategy here. Likely it's only > > because I haven't been paying enough attention, but I'll ask anyway in > > case anybody else is similarly confused. > > > > The callchain when we enumerate these slave devices looks like this: > > > > acpi_i2c_register_devices(struct i2c_adapter *) > > acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) > > acpi_i2c_add_device > > acpi_bus_get_device > > acpi_bus_get_status > > acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) > > <find IRQ, addr> > > acpi_dev_free_resources > > i2c_new_device > > client = kzalloc > > client->dev = ... > > device_register(&client->dev) > > > > Is the ACPI namespace in question something like the following? > > > > Device { # i2C master, i.e., the i2c_adapter > > _HID PNPmmmm > > Device { # I2C slave 1, i.e., a client > > _HID PNPsss1 > > _CRS > > SerialBus/I2C addr addr1, mode mode1 > > IRQ irq1 > > } > > Device { # I2C slave 2 > > _HID PNPsss2 > > _CRS > > SerialBus/I2C addr addr2, mode mode2 > > IRQ irq2 > > } > > } > > Yes. > > > _CRS is a device configuration method, so I would expect that it > > exists within the scope of a Device() object. The way I'm used to > > this working is for a driver to specify "I know about PNPsss1 > > devices." > > Yes. > > > But it looks like acpi_i2c_register() walks the namespace below an i2c > > master device, registering a new i2c device (a slave) for every ACPI > > device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C > > descriptor. It seems like you're basically claiming those devices > > nodes based on the contents of their _CRS, not based on their PNP IDs, > > which seems strange to me. > > Yes, if we only matched the PNP IDs we would get bunch of PNP devices which > certainly doesn't help us to reuse the existing I2C drivers. So instead of > creating a new glue driver for ACPI or PNP device we added this enumeration > method that then creates the I2C devices, just like DT does. In other words, what this whole thing is trying to achieve is something along the lines of: - Instead of making PNP or ACPI devices out of every device in the ACPI namespace we use the resources returned by the _CRS method for a given device as a hint of what type of device it is. - If we find I2CSerialBus() we assume it is an I2C device and create i2c_device (and i2c_client) and register this to the I2C core. - If we find SPISerialBus() we assume it is a SPI device and create corresponding spidevice and register it to the SPI core. - Devices that don't have a bus are represented as platform devices (based on the table in drivers/acpi/scan.c). The reason for this is that most of the SoC devices have already platform driver so we can easily reuse the existing drivers. The implementation follows the Device Tree as much as possible so that adding support for DT and ACPI to a driver would be similar and thus easy for people who know either method. An alternative would be to create PNP or ACPI glue drivers for each device that then create the corresponding real device like platform or I2C which means that we need add much more lines of unnecessary code to the kernel compared to adding the ACPI/PNP IDs to the driver which takes only few lines of code. We still allow more complex configuration with the means of dev->acpi_handle. So for example if driver needs to call _DSM in order to retrieve some parameters for the device it can do so with the help of dev->acpi_handle. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 17, 2012 at 2:55 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Sat, Nov 17, 2012 at 10:03:54AM +0200, Mika Westerberg wrote: >> On Fri, Nov 16, 2012 at 11:46:40PM -0700, Bjorn Helgaas wrote: >> > On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg >> > <mika.westerberg@linux.intel.com> wrote: >> > > ... >> > > From: Mika Westerberg <mika.westerberg@linux.intel.com> >> > > Date: Mon, 10 Sep 2012 12:12:32 +0300 >> > > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support >> > > >> > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate >> > > and configure the I2C slave devices behind the I2C controller. This patch >> > > adds helper functions to support I2C slave enumeration. >> > > >> > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() >> > > in order to get its slave devices enumerated, created and bound to the >> > > corresponding ACPI handle. >> > >> > I must admit I don't understand the strategy here. Likely it's only >> > because I haven't been paying enough attention, but I'll ask anyway in >> > case anybody else is similarly confused. >> > >> > The callchain when we enumerate these slave devices looks like this: >> > >> > acpi_i2c_register_devices(struct i2c_adapter *) >> > acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) >> > acpi_i2c_add_device >> > acpi_bus_get_device >> > acpi_bus_get_status >> > acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) >> > <find IRQ, addr> >> > acpi_dev_free_resources >> > i2c_new_device >> > client = kzalloc >> > client->dev = ... >> > device_register(&client->dev) >> > >> > Is the ACPI namespace in question something like the following? >> > >> > Device { # i2C master, i.e., the i2c_adapter >> > _HID PNPmmmm >> > Device { # I2C slave 1, i.e., a client >> > _HID PNPsss1 >> > _CRS >> > SerialBus/I2C addr addr1, mode mode1 >> > IRQ irq1 >> > } >> > Device { # I2C slave 2 >> > _HID PNPsss2 >> > _CRS >> > SerialBus/I2C addr addr2, mode mode2 >> > IRQ irq2 >> > } >> > } >> >> Yes. >> >> > _CRS is a device configuration method, so I would expect that it >> > exists within the scope of a Device() object. The way I'm used to >> > this working is for a driver to specify "I know about PNPsss1 >> > devices." >> >> Yes. >> >> > But it looks like acpi_i2c_register() walks the namespace below an i2c >> > master device, registering a new i2c device (a slave) for every ACPI >> > device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C >> > descriptor. It seems like you're basically claiming those devices >> > nodes based on the contents of their _CRS, not based on their PNP IDs, >> > which seems strange to me. >> >> Yes, if we only matched the PNP IDs we would get bunch of PNP devices which >> certainly doesn't help us to reuse the existing I2C drivers. So instead of >> creating a new glue driver for ACPI or PNP device we added this enumeration >> method that then creates the I2C devices, just like DT does. > > In other words, what this whole thing is trying to achieve is something > along the lines of: > > - Instead of making PNP or ACPI devices out of every device in the > ACPI namespace we use the resources returned by the _CRS > method for a given device as a hint of what type of device it is. > > - If we find I2CSerialBus() we assume it is an I2C device and > create i2c_device (and i2c_client) and register this to the I2C > core. > > - If we find SPISerialBus() we assume it is a SPI device and create > corresponding spidevice and register it to the SPI core. > > - Devices that don't have a bus are represented as platform devices > (based on the table in drivers/acpi/scan.c). The reason for this > is that most of the SoC devices have already platform driver so > we can easily reuse the existing drivers. Using _CRS contents to infer the device type feels like a mistake to me. It doesn't generalize to arbitrary devices. I don't think it's the intent of the spec, which seems clearly to be "start with the _HID/_CID to identify devices," so it violates the principle of least surprise. I'm not sure it's even safe to rely on _CRS being useful until after the OS runs _SRS. Sec 6.2 of the spec (ACPI 5.0) says the OS uses _PRS to determine what resources the device needs and _SRS to assign them, and it *may* use _CRS to learn any current assignments (I know this doesn't match current Linux behavior very well). I interpret that to mean the device may be disabled and return nothing in _CRS until after the OS evaluates _SRS to enable the device. I think it will make it harder to reason about and refactor ACPI because it's "unusual." For example, the acpi_i2c_register_devices -> acpi_i2c_add_device path allocates a new struct device (in struct i2c_client) and registers it. Now we have a struct device in struct acpi_device, in struct pnp_dev, *and* in struct i2c_client, and all refer to the same thing. What does that mean? The sysfs picture seems confusing to me. I assume you mean the acpi_platform_device_ids[] table you added with 91e56878058. Having a table of IDs that are treated specially by the core is a bit of a concern for me because it means we need to add things to it every time a new platform device comes along. The patch didn't include clear criteria for deciding what qualifies. For example, I don't know whether PCI host bridges would qualify as platform devices. I guess maybe they would, because they don't have a bus (though of course they have acpi_bus_type like all other ACPI devices)? > The implementation follows the Device Tree as much as possible so that > adding support for DT and ACPI to a driver would be similar and thus easy > for people who know either method. > > An alternative would be to create PNP or ACPI glue drivers for each device > that then create the corresponding real device like platform or I2C which > means that we need add much more lines of unnecessary code to the kernel > compared to adding the ACPI/PNP IDs to the driver which takes only few > lines of code. > > We still allow more complex configuration with the means of > dev->acpi_handle. So for example if driver needs to call _DSM in order to > retrieve some parameters for the device it can do so with the help of > dev->acpi_handle. I think the benefit here is that you can merely point .acpi_match_table at an acpi_device_id[] table, then use platform_get_resource() as a generic way to get resources, whether the platform device came from OF, ACPI, etc. The alternative would be to add, e.g., a PNP driver with a .probe() method that uses pnp_get_resource(). That's not very much code, but it is more, even if the .probe() method just calls a device registration function that's shared across bus types. That benefit seems like a great thing, and my question then is why wouldn't we just do it across the board and make platform devices for *all* ACPI devices without having the I2C and SPI special cases? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 19, 2012 03:49:25 PM Bjorn Helgaas wrote: > On Sat, Nov 17, 2012 at 2:55 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Sat, Nov 17, 2012 at 10:03:54AM +0200, Mika Westerberg wrote: > >> On Fri, Nov 16, 2012 at 11:46:40PM -0700, Bjorn Helgaas wrote: > >> > On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg > >> > <mika.westerberg@linux.intel.com> wrote: > >> > > ... > >> > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > >> > > Date: Mon, 10 Sep 2012 12:12:32 +0300 > >> > > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support > >> > > > >> > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate > >> > > and configure the I2C slave devices behind the I2C controller. This patch > >> > > adds helper functions to support I2C slave enumeration. > >> > > > >> > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() > >> > > in order to get its slave devices enumerated, created and bound to the > >> > > corresponding ACPI handle. > >> > > >> > I must admit I don't understand the strategy here. Likely it's only > >> > because I haven't been paying enough attention, but I'll ask anyway in > >> > case anybody else is similarly confused. > >> > > >> > The callchain when we enumerate these slave devices looks like this: > >> > > >> > acpi_i2c_register_devices(struct i2c_adapter *) > >> > acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) > >> > acpi_i2c_add_device > >> > acpi_bus_get_device > >> > acpi_bus_get_status > >> > acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) > >> > <find IRQ, addr> > >> > acpi_dev_free_resources > >> > i2c_new_device > >> > client = kzalloc > >> > client->dev = ... > >> > device_register(&client->dev) > >> > > >> > Is the ACPI namespace in question something like the following? > >> > > >> > Device { # i2C master, i.e., the i2c_adapter > >> > _HID PNPmmmm > >> > Device { # I2C slave 1, i.e., a client > >> > _HID PNPsss1 > >> > _CRS > >> > SerialBus/I2C addr addr1, mode mode1 > >> > IRQ irq1 > >> > } > >> > Device { # I2C slave 2 > >> > _HID PNPsss2 > >> > _CRS > >> > SerialBus/I2C addr addr2, mode mode2 > >> > IRQ irq2 > >> > } > >> > } > >> > >> Yes. > >> > >> > _CRS is a device configuration method, so I would expect that it > >> > exists within the scope of a Device() object. The way I'm used to > >> > this working is for a driver to specify "I know about PNPsss1 > >> > devices." > >> > >> Yes. > >> > >> > But it looks like acpi_i2c_register() walks the namespace below an i2c > >> > master device, registering a new i2c device (a slave) for every ACPI > >> > device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C > >> > descriptor. It seems like you're basically claiming those devices > >> > nodes based on the contents of their _CRS, not based on their PNP IDs, > >> > which seems strange to me. > >> > >> Yes, if we only matched the PNP IDs we would get bunch of PNP devices which > >> certainly doesn't help us to reuse the existing I2C drivers. So instead of > >> creating a new glue driver for ACPI or PNP device we added this enumeration > >> method that then creates the I2C devices, just like DT does. > > > > In other words, what this whole thing is trying to achieve is something > > along the lines of: > > > > - Instead of making PNP or ACPI devices out of every device in the > > ACPI namespace we use the resources returned by the _CRS > > method for a given device as a hint of what type of device it is. > > > > - If we find I2CSerialBus() we assume it is an I2C device and > > create i2c_device (and i2c_client) and register this to the I2C > > core. > > > > - If we find SPISerialBus() we assume it is a SPI device and create > > corresponding spidevice and register it to the SPI core. > > > > - Devices that don't have a bus are represented as platform devices > > (based on the table in drivers/acpi/scan.c). The reason for this > > is that most of the SoC devices have already platform driver so > > we can easily reuse the existing drivers. > > Using _CRS contents to infer the device type feels like a mistake to > me. It doesn't generalize to arbitrary devices. I don't think it's > the intent of the spec, which seems clearly to be "start with the > _HID/_CID to identify devices," so it violates the principle of least > surprise. This is a chicken-and-egg problem, kind of. Namely, we need a struct i2c_device (in this particular case) for a driver to bind to, before the driver will use its device IDs to identify it. :-) > I'm not sure it's even safe to rely on _CRS being useful until after > the OS runs _SRS. Sec 6.2 of the spec (ACPI 5.0) says the OS uses > _PRS to determine what resources the device needs and _SRS to assign > them, and it *may* use _CRS to learn any current assignments (I know > this doesn't match current Linux behavior very well). Ideally, we'll do all those things some time in the future. We'll then make sure that acpi_i2c_register_devices() doesn't run before we've done them. > I interpret that to mean the device may be disabled and return nothing in > _CRS until after the OS evaluates _SRS to enable the device. Yes, that may be the case in theory. No, I haven't seen any evidence that it happens in practice. > I think it will make it harder to reason about and refactor ACPI > because it's "unusual." For example, the acpi_i2c_register_devices -> > acpi_i2c_add_device path allocates a new struct device (in struct > i2c_client) and registers it. Now we have a struct device in struct > acpi_device, in struct pnp_dev, *and* in struct i2c_client, and all > refer to the same thing. What does that mean? The sysfs picture > seems confusing to me. We don't want that in pnp_dev and we'll make this one go away going forward. Also, we don't want drivers to bind to the one in struct acpi_device, so that struct acpi_device only is a representation of an ACPI device node (analogously to PCI). The fact that it is visible through sysfs doesn't seem to hurt (for PCI it even helps sometimes :-)). > I assume you mean the acpi_platform_device_ids[] table you added with > 91e56878058. Having a table of IDs that are treated specially by the > core is a bit of a concern for me because it means we need to add > things to it every time a new platform device comes along. That's a safety measure so that adding things handled by the new code is under tight control to start with. We're going to get rid of it at one point, when we decide it's safe. > The patch didn't include clear criteria for deciding what qualifies. For > example, I don't know whether PCI host bridges would qualify as > platform devices. I guess maybe they would, because they don't have a > bus (though of course they have acpi_bus_type like all other ACPI > devices)? They would in principle, but they are kind of special. We may want to have a special bus type for them. I would like to make acpi_bus_type go away in the future, if possible, because it's a source of major confusion (as this very thread shows clearly :-)). > > The implementation follows the Device Tree as much as possible so that > > adding support for DT and ACPI to a driver would be similar and thus easy > > for people who know either method. > > > > An alternative would be to create PNP or ACPI glue drivers for each device > > that then create the corresponding real device like platform or I2C which > > means that we need add much more lines of unnecessary code to the kernel > > compared to adding the ACPI/PNP IDs to the driver which takes only few > > lines of code. > > > > We still allow more complex configuration with the means of > > dev->acpi_handle. So for example if driver needs to call _DSM in order to > > retrieve some parameters for the device it can do so with the help of > > dev->acpi_handle. > > I think the benefit here is that you can merely point > .acpi_match_table at an acpi_device_id[] table, then use > platform_get_resource() as a generic way to get resources, whether the > platform device came from OF, ACPI, etc. The alternative would be to > add, e.g., a PNP driver with a .probe() method that uses > pnp_get_resource(). That's not very much code, but it is more, even > if the .probe() method just calls a device registration function > that's shared across bus types. > > That benefit seems like a great thing, and my question then is why > wouldn't we just do it across the board and make platform devices for > *all* ACPI devices without having the I2C and SPI special cases? Simply because there are existing I2C and SPI drivers for some IP blocks are represented by those ACPI device nodes and we want to use them to handles those devices. :-) Thanks, Rafael
On Monday, November 19, 2012 03:49:25 PM Bjorn Helgaas wrote: > On Sat, Nov 17, 2012 at 2:55 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Sat, Nov 17, 2012 at 10:03:54AM +0200, Mika Westerberg wrote: > >> On Fri, Nov 16, 2012 at 11:46:40PM -0700, Bjorn Helgaas wrote: > >> > On Fri, Nov 16, 2012 at 10:28 AM, Mika Westerberg > >> > <mika.westerberg@linux.intel.com> wrote: > >> > > ... > >> > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > >> > > Date: Mon, 10 Sep 2012 12:12:32 +0300 > >> > > Subject: [PATCH] i2c / ACPI: add ACPI enumeration support > >> > > > >> > > ACPI 5 introduced I2cSerialBus resource that makes it possible to enumerate > >> > > and configure the I2C slave devices behind the I2C controller. This patch > >> > > adds helper functions to support I2C slave enumeration. > >> > > > >> > > An ACPI enabled I2C controller driver only needs to call acpi_i2c_register_devices() > >> > > in order to get its slave devices enumerated, created and bound to the > >> > > corresponding ACPI handle. > >> > > >> > I must admit I don't understand the strategy here. Likely it's only > >> > because I haven't been paying enough attention, but I'll ask anyway in > >> > case anybody else is similarly confused. > >> > > >> > The callchain when we enumerate these slave devices looks like this: > >> > > >> > acpi_i2c_register_devices(struct i2c_adapter *) > >> > acpi_walk_namespace(adapter->dev.acpi_handle, acpi_i2c_add_device) > >> > acpi_i2c_add_device > >> > acpi_bus_get_device > >> > acpi_bus_get_status > >> > acpi_dev_get_resources(..., acpi_i2c_add_resource, ...) > >> > <find IRQ, addr> > >> > acpi_dev_free_resources > >> > i2c_new_device > >> > client = kzalloc > >> > client->dev = ... > >> > device_register(&client->dev) > >> > > >> > Is the ACPI namespace in question something like the following? > >> > > >> > Device { # i2C master, i.e., the i2c_adapter > >> > _HID PNPmmmm > >> > Device { # I2C slave 1, i.e., a client > >> > _HID PNPsss1 > >> > _CRS > >> > SerialBus/I2C addr addr1, mode mode1 > >> > IRQ irq1 > >> > } > >> > Device { # I2C slave 2 > >> > _HID PNPsss2 > >> > _CRS > >> > SerialBus/I2C addr addr2, mode mode2 > >> > IRQ irq2 > >> > } > >> > } > >> > >> Yes. > >> > >> > _CRS is a device configuration method, so I would expect that it > >> > exists within the scope of a Device() object. The way I'm used to > >> > this working is for a driver to specify "I know about PNPsss1 > >> > devices." > >> > >> Yes. > >> > >> > But it looks like acpi_i2c_register() walks the namespace below an i2c > >> > master device, registering a new i2c device (a slave) for every ACPI > >> > device node with a _CRS method that contains a SERIAL_BUS/TYPE_I2C > >> > descriptor. It seems like you're basically claiming those devices > >> > nodes based on the contents of their _CRS, not based on their PNP IDs, > >> > which seems strange to me. > >> > >> Yes, if we only matched the PNP IDs we would get bunch of PNP devices which > >> certainly doesn't help us to reuse the existing I2C drivers. So instead of > >> creating a new glue driver for ACPI or PNP device we added this enumeration > >> method that then creates the I2C devices, just like DT does. > > > > In other words, what this whole thing is trying to achieve is something > > along the lines of: > > > > - Instead of making PNP or ACPI devices out of every device in the > > ACPI namespace we use the resources returned by the _CRS > > method for a given device as a hint of what type of device it is. > > > > - If we find I2CSerialBus() we assume it is an I2C device and > > create i2c_device (and i2c_client) and register this to the I2C > > core. > > > > - If we find SPISerialBus() we assume it is a SPI device and create > > corresponding spidevice and register it to the SPI core. > > > > - Devices that don't have a bus are represented as platform devices > > (based on the table in drivers/acpi/scan.c). The reason for this > > is that most of the SoC devices have already platform driver so > > we can easily reuse the existing drivers. > > Using _CRS contents to infer the device type feels like a mistake to > me. It doesn't generalize to arbitrary devices. I don't think it's > the intent of the spec, which seems clearly to be "start with the > _HID/_CID to identify devices," so it violates the principle of least > surprise. > > I'm not sure it's even safe to rely on _CRS being useful until after > the OS runs _SRS. Sec 6.2 of the spec (ACPI 5.0) says the OS uses > _PRS to determine what resources the device needs and _SRS to assign > them, and it *may* use _CRS to learn any current assignments (I know > this doesn't match current Linux behavior very well). I interpret > that to mean the device may be disabled and return nothing in _CRS > until after the OS evaluates _SRS to enable the device. > > I think it will make it harder to reason about and refactor ACPI > because it's "unusual." For example, the acpi_i2c_register_devices -> > acpi_i2c_add_device path allocates a new struct device (in struct > i2c_client) and registers it. Now we have a struct device in struct > acpi_device, in struct pnp_dev, *and* in struct i2c_client, and all > refer to the same thing. What does that mean? The sysfs picture > seems confusing to me. > > I assume you mean the acpi_platform_device_ids[] table you added with > 91e56878058. Having a table of IDs that are treated specially by the > core is a bit of a concern for me because it means we need to add > things to it every time a new platform device comes along. The patch > didn't include clear criteria for deciding what qualifies. The current criterion is: If we know for a fact that the given device ID matches a piece of hardware that we have an existing platform driver for (e.g. an SPI controller), we'll add it to that table. For now, that's about it (although the "vision" is to extend that to other situations and, ideally, to get rid of that table in the future). All of that is about avoiding the need to have two different drivers for the same piece of hardware, just because once it is used in a system with an ACPI BIOS, and some other time it is used in a system where we get device information from Device Trees. And yes, that applies to PCI host bridges too. :-) Thanks, Rafael
On Mon, Nov 19, 2012 at 03:49:25PM -0700, Bjorn Helgaas wrote: > I think the benefit here is that you can merely point > .acpi_match_table at an acpi_device_id[] table, then use > platform_get_resource() as a generic way to get resources, whether the > platform device came from OF, ACPI, etc. The alternative would be to > add, e.g., a PNP driver with a .probe() method that uses > pnp_get_resource(). That's not very much code, but it is more, even > if the .probe() method just calls a device registration function > that's shared across bus types. > > That benefit seems like a great thing, and my question then is why > wouldn't we just do it across the board and make platform devices for > *all* ACPI devices without having the I2C and SPI special cases? That wouldn't be any better than having a PNP or ACPI device. We must still create the corresponding I2C or SPI device in order to have a driver that can plug into I2C or SPI core. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 119d58d..0300bf6 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -181,6 +181,12 @@ config ACPI_DOCK This driver supports ACPI-controlled docking stations and removable drive bays such as the IBM Ultrabay and the Dell Module Bay. +config ACPI_I2C + def_tristate I2C + depends on I2C + help + ACPI I2C enumeration support. + config ACPI_PROCESSOR tristate "Processor" select THERMAL diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 7289828..2a4502b 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_ACPI_HED) += hed.o obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o obj-$(CONFIG_ACPI_BGRT) += bgrt.o +obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o # processor has its own "processor." module_param namespace processor-y := processor_driver.o processor_throttling.o diff --git a/drivers/acpi/acpi_i2c.c b/drivers/acpi/acpi_i2c.c new file mode 100644 index 0000000..5783d93 --- /dev/null +++ b/drivers/acpi/acpi_i2c.c @@ -0,0 +1,216 @@ +/* + * ACPI I2C enumeration support + * + * Copyright (C) 2012, Intel Corporation + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/export.h> +#include <linux/i2c.h> +#include <linux/ioport.h> + +ACPI_MODULE_NAME("i2c"); + +static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data) +{ + struct acpi_resource_i2c_serialbus *sb; + struct i2c_board_info *info = data; + + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) + return 0; + + sb = &ares->data.i2c_serial_bus; + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) + return 0; + + info->addr = sb->slave_address; + if (sb->access_mode == ACPI_I2C_10BIT_MODE) + info->flags |= I2C_CLIENT_TEN; + + return 1; +} + +static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, + void *data, void **return_value) +{ + struct i2c_adapter *adapter = data; + struct resource_list_entry *rentry; + struct list_head resource_list; + struct i2c_board_info info; + struct acpi_device *adev; + int ret; + + if (acpi_bus_get_device(handle, &adev)) + return AE_OK; + if (acpi_bus_get_status(adev) || !adev->status.present) + return AE_OK; + + memset(&info, 0, sizeof(info)); + + INIT_LIST_HEAD(&resource_list); + ret = acpi_dev_get_resources(adev, &resource_list, + acpi_i2c_add_resource, &info); + if (ret < 0) + return AE_OK; + + list_for_each_entry(rentry, &resource_list, node) { + struct resource *r = &rentry->res; + + if (resource_type(r) == IORESOURCE_IRQ) { + info.irq = r->start; + break; + } + } + + acpi_dev_free_resource_list(&resource_list); + + if (!info.addr) + return AE_OK; + + strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type)); + if (!i2c_new_device(adapter, &info)) { + dev_err(&adapter->dev, + "failed to add I2C device %s from ACPI\n", + dev_name(&adev->dev)); + } + + return AE_OK; +} + +/** + * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter + * @adapter: pointer to adapter + * + * Enumerate all I2C slave devices behind this adapter by walking the ACPI + * namespace. When a device is found it will be added to the Linux device + * model and bound to the corresponding ACPI handle. + */ +void acpi_i2c_register_devices(struct i2c_adapter *adapter) +{ + acpi_handle handle; + acpi_status status; + + handle = adapter->dev.acpi_handle; + if (!handle) + return; + + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, + acpi_i2c_add_device, NULL, + adapter, NULL); + if (ACPI_FAILURE(status)) + dev_warn(&adapter->dev, "failed to enumerate I2C slaves\n"); +} +EXPORT_SYMBOL_GPL(acpi_i2c_register_devices); + +struct acpi_i2c_find { + acpi_handle handle; + u16 addr; + u8 access_mode; + bool found; +}; + +static int acpi_i2c_find_child_address(struct acpi_resource *ares, void *data) +{ + struct acpi_resource_i2c_serialbus *sb; + struct acpi_i2c_find *i2c_find = data; + + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) + return 1; + + sb = &ares->data.i2c_serial_bus; + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) + return 1; + + if (sb->slave_address == i2c_find->addr && + sb->access_mode == i2c_find->access_mode) + i2c_find->found = true; + + return 1; +} + +static acpi_status acpi_i2c_find_child(acpi_handle handle, u32 level, + void *data, void **return_value) +{ + struct acpi_i2c_find *i2c_find = data; + struct list_head resource_list; + struct acpi_device *adev; + int ret; + + if (acpi_bus_get_device(handle, &adev)) + return AE_OK; + if (acpi_bus_get_status(adev) || !adev->status.present) + return AE_OK; + + INIT_LIST_HEAD(&resource_list); + ret = acpi_dev_get_resources(adev, &resource_list, + acpi_i2c_find_child_address, i2c_find); + if (ret < 0) + return AE_OK; + + acpi_dev_free_resource_list(&resource_list); + + if (i2c_find->found) { + i2c_find->handle = handle; + return AE_CTRL_TERMINATE; + } + return AE_OK; +} + +static int acpi_i2c_find_device(struct device *dev, acpi_handle *handle) +{ + struct acpi_i2c_find i2c_find; + struct i2c_adapter *adapter; + struct i2c_client *client; + acpi_handle parent; + acpi_status status; + + client = i2c_verify_client(dev); + if (!client) + return -ENODEV; + + adapter = client->adapter; + if (!adapter) + return -ENODEV; + + parent = adapter->dev.acpi_handle; + if (!parent) + return -ENODEV; + + memset(&i2c_find, 0, sizeof(i2c_find)); + i2c_find.addr = client->addr; + i2c_find.access_mode = client->flags & I2C_CLIENT_TEN ? + ACPI_I2C_10BIT_MODE : ACPI_I2C_7BIT_MODE; + + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, parent, 1, + acpi_i2c_find_child, NULL, + &i2c_find, NULL); + if (ACPI_FAILURE(status) || !i2c_find.handle) + return -ENODEV; + + *handle = i2c_find.handle; + return 0; +} + +static struct acpi_bus_type acpi_i2c_bus = { + .bus = &i2c_bus_type, + .find_device = acpi_i2c_find_device, +}; + +void acpi_i2c_bus_register(void) +{ + register_acpi_bus_type(&acpi_i2c_bus); +} +EXPORT_SYMBOL_GPL(acpi_i2c_bus_register); + +void acpi_i2c_bus_unregister(void) +{ + unregister_acpi_bus_type(&acpi_i2c_bus); +} +EXPORT_SYMBOL_GPL(acpi_i2c_bus_unregister); diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index a7edf98..7385de2f 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -39,6 +39,8 @@ #include <linux/irqflags.h> #include <linux/rwsem.h> #include <linux/pm_runtime.h> +#include <linux/acpi.h> +#include <linux/acpi_i2c.h> #include <asm/uaccess.h> #include "i2c-core.h" @@ -78,6 +80,10 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv) if (of_driver_match_device(dev, drv)) return 1; + /* Then ACPI style match */ + if (acpi_driver_match_device(dev, drv)) + return 1; + driver = to_i2c_driver(drv); /* match on an id table if there is one */ if (driver->id_table) @@ -1298,6 +1304,8 @@ static int __init i2c_init(void) retval = i2c_add_driver(&dummy_driver); if (retval) goto class_err; + + acpi_i2c_bus_register(); return 0; class_err: @@ -1311,6 +1319,8 @@ bus_err: static void __exit i2c_exit(void) { + acpi_i2c_bus_unregister(); + i2c_del_driver(&dummy_driver); #ifdef CONFIG_I2C_COMPAT class_compat_unregister(i2c_adapter_compat_class); diff --git a/include/linux/acpi_i2c.h b/include/linux/acpi_i2c.h new file mode 100644 index 0000000..cf2233f --- /dev/null +++ b/include/linux/acpi_i2c.h @@ -0,0 +1,27 @@ +/* + * ACPI I2C enumeration support + * + * Copyright (C) 2012, Intel Corporation + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef LINUX_ACPI_I2C_H +#define LINUX_ACPI_I2C_H + +struct i2c_adapter; + +#if IS_ENABLED(CONFIG_ACPI_I2C) +extern void acpi_i2c_register_devices(struct i2c_adapter *adap); +extern void acpi_i2c_bus_register(void); +extern void acpi_i2c_bus_unregister(void); +#else +static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {} +static inline void acpi_i2c_bus_register(void) {} +static inline void acpi_i2c_bus_unregister(void) {} +#endif /* IS_ENABLED(CONFIG_ACPI_I2C) */ + +#endif /* LINUX_ACPI_I2C_H */