Patchwork [v1] ACPI / bus: Respect PRP0001 when retrieving device match data

login
register
mail settings
Submitter Andy Shevchenko
Date Feb. 25, 2019, 2:12 p.m.
Message ID <20190225141247.32576-1-andriy.shevchenko@linux.intel.com>
Download mbox | patch
Permalink /patch/735107/
State New
Headers show

Comments

Andy Shevchenko - Feb. 25, 2019, 2:12 p.m.
In case of PRP0001 the compatible string may have additional data affiliated
with the device. When we call device_get_match_data() on such device, we will
get nothing since currently acpi_device_get_match_data() doesn't respect
PRP0001.

To fix above, try acpi_of_match_device() if there is no ACPI table in the
driver.

Anyway, note that the device is supposed to get its own proper ACPI ID.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Mika Westerberg - Feb. 25, 2019, 2:29 p.m.
On Mon, Feb 25, 2019 at 05:12:47PM +0300, Andy Shevchenko wrote:
> In case of PRP0001 the compatible string may have additional data affiliated
> with the device. When we call device_get_match_data() on such device, we will
> get nothing since currently acpi_device_get_match_data() doesn't respect
> PRP0001.
> 
> To fix above, try acpi_of_match_device() if there is no ACPI table in the
> driver.
> 
> Anyway, note that the device is supposed to get its own proper ACPI ID.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/bus.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 147f6c7ea59c..c2730118e95b 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -799,10 +799,25 @@ const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
>  }
>  EXPORT_SYMBOL_GPL(acpi_match_device);
>  
> +static const void *acpi_device_get_match_data_of(const struct device *dev)

Maybe call it acpi_of_device_get_match_data() instead following
acpi_of_match device and the like?

> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	const struct of_device_id *match = NULL;
> +
> +	acpi_of_match_device(adev, dev->driver->of_match_table, &match);
> +	if (!match)
> +		return NULL;
> +
> +	return match->data;

Hm, why not write it like:

	if (!acpi_of_match_device(adev, dev->driver->of_match_table, &match))
		return NULL;
	return match->data;

instead?

> +}
> +
>  const void *acpi_device_get_match_data(const struct device *dev)
>  {
>  	const struct acpi_device_id *match;
>  
> +	if (!dev->driver->acpi_match_table)

If a driver does not have acpi_match_table is it expected to be
compatible with PRP0001 or should you check for adev->data.of_compatible
here as well?

> +		return acpi_device_get_match_data_of(dev);
> +
>  	match = acpi_match_device(dev->driver->acpi_match_table, dev);
>  	if (!match)
>  		return NULL;
> -- 
> 2.20.1
Andy Shevchenko - Feb. 25, 2019, 2:57 p.m.
On Mon, Feb 25, 2019 at 04:29:00PM +0200, Mika Westerberg wrote:
> On Mon, Feb 25, 2019 at 05:12:47PM +0300, Andy Shevchenko wrote:
> > In case of PRP0001 the compatible string may have additional data affiliated
> > with the device. When we call device_get_match_data() on such device, we will
> > get nothing since currently acpi_device_get_match_data() doesn't respect
> > PRP0001.
> > 
> > To fix above, try acpi_of_match_device() if there is no ACPI table in the
> > driver.
> > 
> > Anyway, note that the device is supposed to get its own proper ACPI ID.

> > +static const void *acpi_device_get_match_data_of(const struct device *dev)
> 
> Maybe call it acpi_of_device_get_match_data() instead following
> acpi_of_match device and the like?

Makes sense.

> > +{
> > +	struct acpi_device *adev = ACPI_COMPANION(dev);
> > +	const struct of_device_id *match = NULL;
> > +
> > +	acpi_of_match_device(adev, dev->driver->of_match_table, &match);
> > +	if (!match)
> > +		return NULL;
> > +
> > +	return match->data;
> 
> Hm, why not write it like:
> 
> 	if (!acpi_of_match_device(adev, dev->driver->of_match_table, &match))
> 		return NULL;
> 	return match->data;
> 
> instead?

Sadly the acpi_of_match_device() description doesn't say what return value
means. Nevertheless, reading its code shows that your suggestion will work.

> > +}
> > +
> >  const void *acpi_device_get_match_data(const struct device *dev)
> >  {
> >  	const struct acpi_device_id *match;
> >  
> > +	if (!dev->driver->acpi_match_table)
> 
> If a driver does not have acpi_match_table is it expected to be
> compatible with PRP0001 or should you check for adev->data.of_compatible
> here as well?

It's checked anyway in acpi_of_match_device(). I see no point for the check
duplication.

> 
> > +		return acpi_device_get_match_data_of(dev);
> > +
> >  	match = acpi_match_device(dev->driver->acpi_match_table, dev);
> >  	if (!match)
> >  		return NULL;
> > -- 
> > 2.20.1
Mika Westerberg - Feb. 25, 2019, 3:03 p.m.
On Mon, Feb 25, 2019 at 04:57:15PM +0200, Andy Shevchenko wrote:
> > > +	if (!dev->driver->acpi_match_table)
> > 
> > If a driver does not have acpi_match_table is it expected to be
> > compatible with PRP0001 or should you check for adev->data.of_compatible
> > here as well?
> 
> It's checked anyway in acpi_of_match_device(). I see no point for the check
> duplication.

Indeed, then it does not make sense. Thanks for the clarification.

Patch

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 147f6c7ea59c..c2730118e95b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -799,10 +799,25 @@  const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
+static const void *acpi_device_get_match_data_of(const struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	const struct of_device_id *match = NULL;
+
+	acpi_of_match_device(adev, dev->driver->of_match_table, &match);
+	if (!match)
+		return NULL;
+
+	return match->data;
+}
+
 const void *acpi_device_get_match_data(const struct device *dev)
 {
 	const struct acpi_device_id *match;
 
+	if (!dev->driver->acpi_match_table)
+		return acpi_device_get_match_data_of(dev);
+
 	match = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!match)
 		return NULL;