Re: [PATCH v2 3/3 UPDATED] i2c / ACPI: add ACPI enumeration support

[Bjorn Helgaas]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/