[PATCH 0/3] Init device ids from ACPI of_compatible

[PATCH 0/3] Init device ids from ACPI of_compatible

[Crestez Dan Leonard]

When using devicetree stuff like i2c_client.name or spi_device.modalias
is initialized to the first DT compatible id with the vendor prefix
stripped. Since some drivers rely on this try to replicate it when using
ACPI with DT ids.

Drivers rely on these ids in order to differentiate between hardware variants
supported by the same driver. As far as I can tell the alternative would be for
drivers to dig into ACPI_COMPANION manually somehow?

This also makes it so that the i2c_device_id parameter passed to probe is
non-NULL when matching with ACPI and DT ids.

The patches only touch the ACPI-specific parts of the i2c and spi core.

These patches are on top of v4.7-rc3. I tested this using Octavin Purdila's
patches for "acpi overlays" but this series does not depend on that. Patch 2
has a conflict with acpi overlays because acpi_i2c_add_device was split into
multiple functions.

Here is an example .dsl for an SPI accelerometer connected to minnowboard max:

Device (ACCL)
{
    Name (_ADR, Zero)
    Name (_HID, "PRP0001")
    Name (_CID, "PRP0001")
    Name (_UID, One)

    Method (_CRS, 0, Serialized)
    {
	Name (RBUF, ResourceTemplate ()
	{
	    SPISerialBus(1, PolarityLow, FourWireMode, 16,
		    ControllerInitiated, 1000000, ClockPolarityLow,
		    ClockPhaseFirst, "\\_SB.SPI1",)
	    GpioInt (Edge, ActiveHigh, Exclusive, PullDown, 0x0000,
		     "\\_SB.GPO2", 0x00, ResourceConsumer, , )
	    { // Pin list
		    1
	    }
	})
	Return (RBUF)
    }
    Name (_DSD, Package ()
    {
	ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
	Package ()
	{
	    Package () {"compatible", "st,lis3dh"},
	}
    })
}

The driver for that device supports tens of models is not aware of ACPI.

Like to prev thread: https://www.spinics.net/lists/linux-acpi/msg66469.html
Like to acpi overlays: https://www.spinics.net/lists/linux-acpi/msg66638.html

Crestez Dan Leonard (3):
  acpi: Export acpi_of_modalias equiv of of_modalias_node
  acpi i2c: Initialize info.type from of_compatible
  acpi spi: Initialize modalias from of_compatible

 drivers/acpi/bus.c      | 36 ++++++++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c  |  8 +++++++-
 drivers/spi/spi.c       | 10 +++++++++-
 include/acpi/acpi_bus.h |  1 +
 4 files changed, 53 insertions(+), 2 deletions(-)

-- 
2.5.5

[PATCH 1/3] acpi: Export acpi_of_modalias equiv of of_modalias_node

[Crestez Dan Leonard]

When using devicetree stuff like i2c_client.name or spi_device.modalias
is initialized to the first DT compatible id with the vendor prefix
stripped. Since some drivers rely on this try to replicate it when using
ACPI with DT ids.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/acpi/bus.c      | 36 ++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 262ca31..4208e0d 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -606,6 +606,42 @@ static bool acpi_of_match_device(struct acpi_device *adev,
 	return false;
 }
 
+/**
+ * acpi_of_modalias - Like of_modalias_node for ACPI with DT ids
+ * @adev:	ACPI device object to match.
+ * @outstr:	Pointer to buffer for result
+ * @outlen:	Length of outstr value
+ *
+ * If we have a DT id set outstr to the first compatible string with the vendor
+ * prefix stripped, just like of_modalias_node does for devicetree.
+ *
+ * Returns 0 on success or negative errno on failure.
+ */
+int acpi_of_modalias(struct acpi_device *adev, char *outstr, int outlen)
+{
+	const union acpi_object *of_compatible;
+	const union acpi_object *obj;
+	const char *str, *chr;
+
+	of_compatible = adev->data.of_compatible;
+	if (!of_compatible)
+		return -ENODEV;
+
+	if (of_compatible->type == ACPI_TYPE_PACKAGE)
+		obj = of_compatible->package.elements;
+	else /* Must be ACPI_TYPE_STRING. */
+		obj = of_compatible;
+
+	str = obj->string.pointer;
+	chr = strchr(str, ',');
+	if (chr)
+		str = chr + 1;
+	strlcpy(outstr, str, outlen);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_of_modalias);
+
 static bool __acpi_match_device_cls(const struct acpi_device_id *id,
 				    struct acpi_hardware_id *hwid)
 {
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 788c6c35..e6523de 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -515,6 +515,7 @@ void acpi_bus_trim(struct acpi_device *start);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids);
+int acpi_of_modalias(struct acpi_device *adev, char *outstr, int outlen);
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 
-- 
2.5.5

Re: [PATCH 1/3] acpi: Export acpi_of_modalias equiv of of_modalias_node

[Mika Westerberg]

On Wed, Jun 15, 2016 at 09:30:27PM +0300, Crestez Dan Leonard wrote:
> +int acpi_of_modalias(struct acpi_device *adev, char *outstr, int outlen)

Why not size_t for outlen?

[PATCH 2/3] acpi i2c: Initialize info.type from of_compatible

[Crestez Dan Leonard]

When using devicetree i2c_board_info.type is set to the compatible
string with the vendor prefix removed. For I2C devices described via
ACPI the i2c_board_info.type string is set to the ACPI device name. When
using ACPI and DT ids this string ends up something like "PRP0001:00".

If the of_compatible property is present try to use that instead. This
makes it easier to instantiate i2c drivers through ACPI with DT ids.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/i2c/i2c-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index af11b65..5ab1fb9 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -188,8 +188,14 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 
 	acpi_dev_free_resource_list(&resource_list);
 
+	if (adev->data.of_compatible) {
+		ret = acpi_of_modalias(adev, info.type, sizeof(info.type));
+		if (ret)
+			return -EINVAL;
+	} else
+		strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
+
 	adev->power.flags.ignore_parent = true;
-	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
 		adev->power.flags.ignore_parent = false;
 		dev_err(&adapter->dev,
-- 
2.5.5

Re: [PATCH 2/3] acpi i2c: Initialize info.type from of_compatible

[Mika Westerberg]

On Wed, Jun 15, 2016 at 09:30:28PM +0300, Crestez Dan Leonard wrote:
> When using devicetree i2c_board_info.type is set to the compatible
> string with the vendor prefix removed. For I2C devices described via
> ACPI the i2c_board_info.type string is set to the ACPI device name. When
> using ACPI and DT ids this string ends up something like "PRP0001:00".
> 
> If the of_compatible property is present try to use that instead. This
> makes it easier to instantiate i2c drivers through ACPI with DT ids.

Adding Jarkko -- he was the original author of I2C/SPI device naming in
ACPI. Since this only affects devices with PRP0001 in their _HID/_CID,
it should not break anything.

> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/i2c/i2c-core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index af11b65..5ab1fb9 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -188,8 +188,14 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
>  
>  	acpi_dev_free_resource_list(&resource_list);
>  
> +	if (adev->data.of_compatible) {

We may want to add acpi_is_of_compatible(adev) or similar at some point.

> +		ret = acpi_of_modalias(adev, info.type, sizeof(info.type));
> +		if (ret)
> +			return -EINVAL;
> +	} else
> +		strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));

Please use {} in the else branch as well. See Documentation/CodingStyle.

> +
>  	adev->power.flags.ignore_parent = true;
> -	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
>  	if (!i2c_new_device(adapter, &info)) {
>  		adev->power.flags.ignore_parent = false;
>  		dev_err(&adapter->dev,
> -- 
> 2.5.5

Re: [PATCH 2/3] acpi i2c: Initialize info.type from of_compatible

[Crestez Dan Leonard]

On 06/16/2016 11:11 AM, Mika Westerberg wrote:
> On Wed, Jun 15, 2016 at 09:30:28PM +0300, Crestez Dan Leonard wrote:
>> When using devicetree i2c_board_info.type is set to the compatible
>> string with the vendor prefix removed. For I2C devices described via
>> ACPI the i2c_board_info.type string is set to the ACPI device name. When
>> using ACPI and DT ids this string ends up something like "PRP0001:00".
>>
>> If the of_compatible property is present try to use that instead. This
>> makes it easier to instantiate i2c drivers through ACPI with DT ids.
> 
> Adding Jarkko -- he was the original author of I2C/SPI device naming in
> ACPI. Since this only affects devices with PRP0001 in their _HID/_CID,
> it should not break anything.

It's possible to have something like this:

            Name (_HID, "XXX1234")
            Name (_CID, "PRP0001")
            Name (_DSD, Package () {
                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                Package () {
                    Package () {"compatible", "vendor,x1234"},
                }
            })

With these patches the i2c device name changes from "XXX1234:00" to
"x1234". This would happen even if a driver matches the XXX1234 acpi ID.
It's not clear if anyone actually uses that or if this change would be
harmful.

It theory it would be possible to use of_compatible for naming only if
there are no other real IDs. I don't think it's worthwhile.

Re: [PATCH 2/3] acpi i2c: Initialize info.type from of_compatible

[Mika Westerberg]

On Fri, Jun 17, 2016 at 03:07:39PM +0300, Crestez Dan Leonard wrote:
> On 06/16/2016 11:11 AM, Mika Westerberg wrote:
> > On Wed, Jun 15, 2016 at 09:30:28PM +0300, Crestez Dan Leonard wrote:
> >> When using devicetree i2c_board_info.type is set to the compatible
> >> string with the vendor prefix removed. For I2C devices described via
> >> ACPI the i2c_board_info.type string is set to the ACPI device name. When
> >> using ACPI and DT ids this string ends up something like "PRP0001:00".
> >>
> >> If the of_compatible property is present try to use that instead. This
> >> makes it easier to instantiate i2c drivers through ACPI with DT ids.
> > 
> > Adding Jarkko -- he was the original author of I2C/SPI device naming in
> > ACPI. Since this only affects devices with PRP0001 in their _HID/_CID,
> > it should not break anything.
> 
> It's possible to have something like this:
> 
>             Name (_HID, "XXX1234")
>             Name (_CID, "PRP0001")
>             Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                     Package () {"compatible", "vendor,x1234"},
>                 }
>             })
> 
> With these patches the i2c device name changes from "XXX1234:00" to
> "x1234". This would happen even if a driver matches the XXX1234 acpi ID.
> It's not clear if anyone actually uses that or if this change would be
> harmful.

I have not seen a single machine using _DSD with PRP0001 so I don't
think it is a problem.

> It theory it would be possible to use of_compatible for naming only if
> there are no other real IDs. I don't think it's worthwhile.

I agree.

[PATCH 3/3] acpi spi: Initialize modalias from of_compatible

[Crestez Dan Leonard]

When using devicetree spi_device.modalias is set to the compatible
string with the vendor prefix removed. For SPI devices described via
ACPI the i2c_board_info.type string is initialized by acpi_device_hid.
When using ACPI and DT ids this string ends up something like "PRP0001".

Change acpi_register_spi_device to use the of_compatible property if
present. This makes it easier to instantiate spi drivers through ACPI
with DT ids.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/spi/spi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 77e6e45..eef5ac2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1680,11 +1680,19 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 		return AE_OK;
 	}
 
+	if (adev->data.of_compatible) {
+		ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias));
+		if (ret) {
+			spi_dev_put(spi);
+			return AE_NOT_FOUND;
+		}
+	} else
+		strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
+
 	if (spi->irq < 0)
 		spi->irq = acpi_dev_gpio_irq_get(adev, 0);
 
 	adev->power.flags.ignore_parent = true;
-	strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
 	if (spi_add_device(spi)) {
 		adev->power.flags.ignore_parent = false;
 		dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",
-- 
2.5.5

Re: [PATCH 3/3] acpi spi: Initialize modalias from of_compatible

[Mika Westerberg]

On Wed, Jun 15, 2016 at 09:30:29PM +0300, Crestez Dan Leonard wrote:
> When using devicetree spi_device.modalias is set to the compatible
> string with the vendor prefix removed. For SPI devices described via
> ACPI the i2c_board_info.type string is initialized by acpi_device_hid.
> When using ACPI and DT ids this string ends up something like "PRP0001".
> 
> Change acpi_register_spi_device to use the of_compatible property if
> present. This makes it easier to instantiate spi drivers through ACPI
> with DT ids.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/spi/spi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 77e6e45..eef5ac2 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1680,11 +1680,19 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
>  		return AE_OK;
>  	}
>  
> +	if (adev->data.of_compatible) {
> +		ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias));
> +		if (ret) {
> +			spi_dev_put(spi);
> +			return AE_NOT_FOUND;
> +		}
> +	} else
> +		strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));

Same here, use {} in the else branch.

> +
>  	if (spi->irq < 0)
>  		spi->irq = acpi_dev_gpio_irq_get(adev, 0);
>  
>  	adev->power.flags.ignore_parent = true;
> -	strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
>  	if (spi_add_device(spi)) {
>  		adev->power.flags.ignore_parent = false;
>  		dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",
> -- 
> 2.5.5