On Sat, Nov 17, 2012 at 2:55 AM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
>> > > ...
>> > > From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
