Re: [PATCH 3/4] mfd: dln2: add support for ACPI

[Octavian Purdila <octavian.purdila@xxxxxxxxx>]

On Thu, Jan 22, 2015 at 4:00 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>

Hi Rafael,

Thanks for reviewing the patch.

> On Tuesday, December 16, 2014 06:12:32 PM Octavian Purdila wrote:
> > This patch adds support to load a custom ACPI table that describes
> > devices connected via the DLN2 USB to I2C/SPI/GPIO bridge.
> >
> > The ACPI table can be loaded either externally (from QEMU or with
> > CONFIG_ACPI_CUSTOM_DSDT) or it can be loaded as firmware file with the
> > name dln2.aml. The driver looks for an ACPI device entry with _HID set
> > to "DLN20000" and makes it the ACPI companion for DLN2 USB
> > sub-drivers.
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
> > ---
> >  Documentation/acpi/dln2-acpi.txt |  62 ++++++++++++++++++
> >  drivers/mfd/dln2.c               | 134 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 196 insertions(+)
> >  create mode 100644 Documentation/acpi/dln2-acpi.txt
> >
> > diff --git a/Documentation/acpi/dln2-acpi.txt b/Documentation/acpi/dln2-acpi.txt
> > new file mode 100644
> > index 0000000..d76605f
> > --- /dev/null
> > +++ b/Documentation/acpi/dln2-acpi.txt
> > @@ -0,0 +1,62 @@
> > +Diolan DLN2 custom APCI table
> > +
> > +The Diolan DLN2 is an USB to I2C/SPI/GPIO bridge and as such it can be used to
> > +connect to various I2C or SPI devices. Because these busses lack an enumeration
> > +protocol, the driver obtains various information about the device (such as I2C
> > +address and GPIO pins) from either ACPI or device tree.
> > +
> > +To allow enumerating devices and their properties via ACPI, the Diolan
> > +driver looks for an ACPI tree with the root _HID set to "DLN20000". If
> > +it finds such an ACPI object it will set the ACPI companion to the
> > +DLN2 MFD driver and from their it will be propagated to all its
> > +sub-devices (I2C, GPIO, SPI).
> > +
> > +The user can either load the custom DSDT table with three methods:
>
> s/DSDT/ACPI/

OK.

> > +
> > +1. Via QEMU (see -acpitable)
> > +
> > +2. Via the CONFIG_ACPI_CUSTOM_DSDT kernel config option (see
> > +Documentation/acpi/dsdt-override.txt)
> > +
> > +3. By placing the custom DSDT in the firmware paths in a file name
> > +dln2.aml.
>
> Surely SSDT?
>

Correct.

<snip>

> > diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> > index f9c4a0b..93f6d1d 100644
> > --- a/drivers/mfd/dln2.c
> > +++ b/drivers/mfd/dln2.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/dln2.h>
> >  #include <linux/rculist.h>
> > +#include <linux/acpi.h>
> > +#include <linux/firmware.h>
> >
>
> OK, so correct me if I'm wrong.
>
> When the (USB) dnl2 device is probed, it is supposed to find an ACPI companion
> for itself and if it's not there, the driver will try to load a custom SSDT from
> a firmware blob in the hope that the companion will be there?
>
> So if I'm not wrong, why is this not broken?
>
> It is not sufficient to call ACPI_COMPANION_SET(dev, ai->dev) to set the device's
> companion.  acpi_bind_one() needs to be run in addition to that, but acpi_bind_one()
> is not to be called from drivers.  It is called by the core automatically during
> device registration and ACPI_COMPANION_SET() is to be used *before* that, not after.
>
> So if I'm not missing anything, the design here is entirely backwards and we
> need to talk about how to implement it correctly at the design level in the
> first place.
>

The idea here is to set the companion for the MFD sub-devices. Mika's
commit "mfd: Add ACPI support" propagates the parent's companion to
the MFD sub-devices, so it is sufficient to set the ACPI companion to
the USB device.

Then, the companion will be propagated to the sub-devices after which
acpi_bind_one() will be called for the sub-devices from
mfd_add_devices (via platform_device_add -> device_add ->
platform_notify).

It is true that the USB dev will have its ACPI companion set without
having acpi_bind_one called but I do not see any harm in that. Even
though acpi_unbind_one() is called it will not find the USB dev on the
physical node list so no put_device() imbalance is caused.

> And no, "Let's come up with a patch that sort of works, throw it at the maintainers
> and see what happens" is not an acceptable approach, sorry.

This patch is based on your feedback of the previous RFC patch set:

On Thu, Dec 11, 2014 at 11:44 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:

> it seems to me that it would be much more straightforward to check for the existence
> of the "DLN20000" device when you are about to register DLN2 USB sub-devices
> and set it directly as an ACPI companion for them if present.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html