Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support

[Bjorn Helgaas <bhelgaas@google.com>]

On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
>> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
>> > configure the SPI slave devices behind the SPI controller. This patch adds
>> > support for this to the SPI core.
>> >
>> > In addition we bind ACPI nodes to SPI devices. This makes it possible for
>> > the slave drivers to get the ACPI handle for further configuration.
>> >
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > ---
>> >  drivers/spi/spi.c |  231 ++++++++++++++++++++++++++++++++++++++++++++++++++++-

>> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data)
>> > +{
>> > +       struct acpi_spi_device_info *info = data;
>> > +       struct acpi_resource_spi_serialbus *sb;
>> > +       struct spi_device *spi = info->spi;
>> > +
>> > +       switch (res->type) {
>> > +       case ACPI_RESOURCE_TYPE_SERIAL_BUS:
>> > +               sb = &res->data.spi_serial_bus;
>> > +               if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
>> > +                       spi->chip_select = sb->device_selection;
>> > +                       spi->max_speed_hz = sb->connection_speed;
>> > +
>> > +                       /* Mode (clock phase/polarity/etc. */
>> > +                       if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
>> > +                               spi->mode |= SPI_CPHA;
>> > +                       if (sb->clock_polarity == ACPI_SPI_START_HIGH)
>> > +                               spi->mode |= SPI_CPOL;
>> > +                       if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
>> > +                               spi->mode |= SPI_CS_HIGH;
>> > +
>> > +                       /*
>> > +                        * The info is valid once we have found the
>> > +                        * SPISerialBus resource.
>> > +                        */
>> > +                       info->valid = true;
>> > +               }
>> > +               break;
>> > +
>> > +       case ACPI_RESOURCE_TYPE_IRQ:
>> > +               info->gsi = res->data.irq.interrupts[0];
>> > +               info->triggering = res->data.irq.triggering;
>> > +               info->polarity = res->data.irq.polarity;
>> > +               break;
>> > +
>> > +       case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>> > +               info->gsi = res->data.extended_irq.interrupts[0];
>> > +               info->triggering = res->data.extended_irq.triggering;
>> > +               info->polarity = res->data.extended_irq.polarity;
>>
>> A driver doesn't seem like the right place for _CRS parsing code.
>
> This is not a driver, however. :-)

OK.  Can you describe what this is?  I assume the picture involves an
SPI master device and one or more SPI slave devices, and the ACPI
namespace tells us something about them, and somehow this code is
related to those devices because it's looking at _CRS for them.

A _CRS method is associated with a Device in the namespace.  What is
that device in this case?  What is its PNP ID?  What driver claims
devices with that ID?

>> > +static void acpi_register_spi_devices(struct spi_master *master)
>> > +{
>> > +       acpi_status status;
>> > +       acpi_handle handle;
>> > +
>> > +       handle = master->dev.acpi_handle;
>> > +       if (!handle)
>> > +               return;
>> > +
>> > +       status = acpi_spi_enumerate(handle, acpi_spi_add_device, master);
>>
>> How does this work with hot-plug?  acpi_spi_enumerate() walks a
>> portion of the namespace.  How do we deal with changes to that part of
>> the namespace?  For example, what happens if this part of the
>> namespace gets pruned because an enclosing device is removed?  Is
>> there a way to discover new SPI devices if they get added?
>
> Yes, there should be a way to do that eventually.  No, we don't have any
> removable SPI devices described by ACPI yet, as far as I know.  So even if
> we added code for that now, we wouldn't be able to test it anyway with any
> real hardware until such devices become available.  I have no idea when that's
> going to happen, though.

I'm not asking about hotplug of devices on an SPI bus.  I'm asking
about hotplug of SPI masters.  For example, let's say you hot-add a
whole node that contains an SPI master.  I assume there's some ACPI
Device node that describes the SPI master, and a hot-add would add a
subtree to the namespace that contains a new SPI master Device.

Bjorn