[PATCH 1/3] IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed

[PATCH 1/3] IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed

[Nikolaus Voss]

Currently, the driver bails out if not explicitly referred to in
DT or ACPI tables. This prevents fallback mechanisms from coming
into effect, e.g. I2C device ID table match via DT or ACPI
PRP0001 HID. However DT/ACPI enum should take precedence over
the fallback, so evaluate that first.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 drivers/iio/accel/st_accel_i2c.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
index 6bdec8c451e0..e360da407027 100644
--- a/drivers/iio/accel/st_accel_i2c.c
+++ b/drivers/iio/accel/st_accel_i2c.c
@@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
 
-static int st_accel_i2c_probe(struct i2c_client *client,
-						const struct i2c_device_id *id)
+static int st_accel_i2c_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
 	struct st_sensor_data *adata;
@@ -156,14 +155,18 @@ static int st_accel_i2c_probe(struct i2c_client *client,
 					 client->name, sizeof(client->name));
 	} else if (ACPI_HANDLE(&client->dev)) {
 		ret = st_sensors_match_acpi_device(&client->dev);
-		if ((ret < 0) || (ret >= ST_ACCEL_MAX))
-			return -ENODEV;
-
-		strlcpy(client->name, st_accel_id_table[ret].name,
+		if ((ret >= 0) && (ret < ST_ACCEL_MAX))
+			strlcpy(client->name, st_accel_id_table[ret].name,
 				sizeof(client->name));
-	} else if (!id)
-		return -ENODEV;
+	}
 
+	/*
+	 * If OF and ACPI enumeration failed, there could still be platform
+	 * information via fallback enumeration or explicit instantiation, so
+	 * check if id table has been matched via client->name.
+	 */
+	if (!client->name)
+		return -ENODEV;
 
 	st_sensors_i2c_configure(indio_dev, client, adata);
 
@@ -187,7 +190,7 @@ static struct i2c_driver st_accel_driver = {
 		.of_match_table = of_match_ptr(st_accel_of_match),
 		.acpi_match_table = ACPI_PTR(st_accel_acpi_match),
 	},
-	.probe = st_accel_i2c_probe,
+	.probe_new = st_accel_i2c_probe,
 	.remove = st_accel_i2c_remove,
 	.id_table = st_accel_id_table,
 };
-- 
2.17.1

[PATCH 2/3] IIO: st_sensors_i2c.c: Don't print error on failed ACPI match

[Nikolaus Voss]

If there is a ACPI node for a i2c device but HID/CID doesn't match, there
could still be a ACPI_DT_NAMESPACE_HID / PRP0001 HID entry which is
matched against the i2c device ID table, so don't print an error
message.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 drivers/iio/common/st_sensors/st_sensors_i2c.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/iio/common/st_sensors/st_sensors_i2c.c
index b81e48e9f27e..0c1a77f9b08c 100644
--- a/drivers/iio/common/st_sensors/st_sensors_i2c.c
+++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
@@ -87,10 +87,9 @@ int st_sensors_match_acpi_device(struct device *dev)
 
 	if (ACPI_HANDLE(dev)) {
 		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
-		if (!acpi_id) {
-			dev_err(dev, "No driver data\n");
+		if (!acpi_id)
 			return -EINVAL;
-		}
+
 		driver_data = acpi_id->driver_data;
 	}
 	return driver_data;
-- 
2.17.1

[PATCH 3/3] IIO: st_accel.h: sync DT and I2C device ID table strings

[Nikolaus Voss]

I2C device ID table strings should really match the DT compatible
strings (without the manufacturer prefix) to avoid confusion. This is
especially reasonable when using ACPI PRP0001 HID /DT compatibility
entries along with the DT compatible property in DSD which is
used as a modalias (with manufacturer prefix stripped off) by the ACPI
layer and thus as i2c_board_info->type by the I2C layer.

Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
---
 drivers/iio/accel/st_accel.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/accel/st_accel.h b/drivers/iio/accel/st_accel.h
index 2f931e4837e5..be4a4a41f849 100644
--- a/drivers/iio/accel/st_accel.h
+++ b/drivers/iio/accel/st_accel.h
@@ -37,23 +37,23 @@ enum st_accel_type {
 	ST_ACCEL_MAX,
 };
 
-#define H3LIS331DL_ACCEL_DEV_NAME	"h3lis331dl_accel"
-#define LIS3LV02DL_ACCEL_DEV_NAME	"lis3lv02dl_accel"
-#define LSM303DLHC_ACCEL_DEV_NAME	"lsm303dlhc_accel"
-#define LIS3DH_ACCEL_DEV_NAME		"lis3dh"
-#define LSM330D_ACCEL_DEV_NAME		"lsm330d_accel"
-#define LSM330DL_ACCEL_DEV_NAME		"lsm330dl_accel"
-#define LSM330DLC_ACCEL_DEV_NAME	"lsm330dlc_accel"
-#define LIS331DL_ACCEL_DEV_NAME		"lis331dl_accel"
-#define LIS331DLH_ACCEL_DEV_NAME	"lis331dlh"
-#define LSM303DL_ACCEL_DEV_NAME		"lsm303dl_accel"
-#define LSM303DLH_ACCEL_DEV_NAME	"lsm303dlh_accel"
-#define LSM303DLM_ACCEL_DEV_NAME	"lsm303dlm_accel"
-#define LSM330_ACCEL_DEV_NAME		"lsm330_accel"
-#define LSM303AGR_ACCEL_DEV_NAME	"lsm303agr_accel"
-#define LIS2DH12_ACCEL_DEV_NAME		"lis2dh12_accel"
+#define H3LIS331DL_ACCEL_DEV_NAME	"h3lis331dl-accel"
+#define LIS3LV02DL_ACCEL_DEV_NAME	"lis3lv02dl-accel"
+#define LSM303DLHC_ACCEL_DEV_NAME	"lsm303dlhc-accel"
+#define LIS3DH_ACCEL_DEV_NAME		"lis3dh-accel"
+#define LSM330D_ACCEL_DEV_NAME		"lsm330d-accel"
+#define LSM330DL_ACCEL_DEV_NAME		"lsm330dl-accel"
+#define LSM330DLC_ACCEL_DEV_NAME	"lsm330dlc-accel"
+#define LIS331DL_ACCEL_DEV_NAME		"lis331dl-accel"
+#define LIS331DLH_ACCEL_DEV_NAME	"lis331dlh-accel"
+#define LSM303DL_ACCEL_DEV_NAME		"lsm303dl-accel"
+#define LSM303DLH_ACCEL_DEV_NAME	"lsm303dlh-accel"
+#define LSM303DLM_ACCEL_DEV_NAME	"lsm303dlm-accel"
+#define LSM330_ACCEL_DEV_NAME		"lsm330-accel"
+#define LSM303AGR_ACCEL_DEV_NAME	"lsm303agr-accel"
+#define LIS2DH12_ACCEL_DEV_NAME		"lis2dh12-accel"
 #define LIS3L02DQ_ACCEL_DEV_NAME	"lis3l02dq"
-#define LNG2DM_ACCEL_DEV_NAME		"lng2dm"
+#define LNG2DM_ACCEL_DEV_NAME		"lng2dm-accel"
 #define LIS2DW12_ACCEL_DEV_NAME		"lis2dw12"
 #define LIS3DHH_ACCEL_DEV_NAME		"lis3dhh"
 
-- 
2.17.1

[PATCH 0/3] IIO: st_sensors_i2c: improve device enumeration

[Nikolaus Voss]

When trying to instantiate a st_accel_i2c device from an ACPI based
system, I ran into some problems:

For my device, there is no ACPI match table entry, so rather than
creating /allocating a new ACPI HID for the device, I wanted to use an
existing DT table compatible entry via creating an ACPI_DT_NAMESPACE_HID
/PRP0001 HID ACPI entry (see Documentation/acpi/enumeration.txt).

This did not work because st_accel_i2c.c bails out if there is a ACPI
node but no ACPI table match instead of looking for a match from one of
the fallback mechanisms (patch 1).

Patch 2 removes an error message when a ACPI node exists but no table
entry is found (this doesn't need to be fatal because of the fallback).

Patch 3 syncs the strings in the I2C device table (which is used by the
I2C core for fallback matching) with the DT compatible strings, so a
PRP0001 entry can use the same compatible strings as a corresponding
DT entry. As far as I can see, the old I2C table strings aren't used
in mainline, so renaming them should be safe.

Nikolaus Voss (3):
  IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed
  IIO: st_sensors_i2c.c: Don't print error on failed ACPI match
  IIO: st_accel.h: sync DT and I2C device ID table strings

 drivers/iio/accel/st_accel.h                  | 32 +++++++++----------
 drivers/iio/accel/st_accel_i2c.c              | 21 ++++++------
 .../iio/common/st_sensors/st_sensors_i2c.c    |  5 ++-
 3 files changed, 30 insertions(+), 28 deletions(-)

-- 
2.17.1

Re: [PATCH 0/3] IIO: st_sensors_i2c: improve device enumeration

[Andy Shevchenko]

On Fri, Jun 29, 2018 at 1:30 PM, Nikolaus Voss
<nikolaus.voss@loewensteinmedical.de> wrote:
> When trying to instantiate a st_accel_i2c device from an ACPI based
> system, I ran into some problems:
>
> For my device, there is no ACPI match table entry, so rather than
> creating /allocating a new ACPI HID for the device, I wanted to use an
> existing DT table compatible entry via creating an ACPI_DT_NAMESPACE_HID
> /PRP0001 HID ACPI entry (see Documentation/acpi/enumeration.txt).
>
> This did not work because st_accel_i2c.c bails out if there is a ACPI
> node but no ACPI table match instead of looking for a match from one of
> the fallback mechanisms (patch 1).
>
> Patch 2 removes an error message when a ACPI node exists but no table
> entry is found (this doesn't need to be fatal because of the fallback).
>
> Patch 3 syncs the strings in the I2C device table (which is used by the
> I2C core for fallback matching) with the DT compatible strings, so a
> PRP0001 entry can use the same compatible strings as a corresponding
> DT entry. As far as I can see, the old I2C table strings aren't used
> in mainline, so renaming them should be safe.
>

I'm not sure I understand how ->probe_new() is supposed to work
against i2c_id_table, but I don't care for legacy platform data
anyway.

What I would like to point to is device_get_match_data() API which
should simplify / unify the case how you get driver data.

> Nikolaus Voss (3):
>   IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed
>   IIO: st_sensors_i2c.c: Don't print error on failed ACPI match
>   IIO: st_accel.h: sync DT and I2C device ID table strings
>
>  drivers/iio/accel/st_accel.h                  | 32 +++++++++----------
>  drivers/iio/accel/st_accel_i2c.c              | 21 ++++++------
>  .../iio/common/st_sensors/st_sensors_i2c.c    |  5 ++-
>  3 files changed, 30 insertions(+), 28 deletions(-)
>
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed

[Jonathan Cameron]

On Fri, 29 Jun 2018 10:10:10 +0200
Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:

> Currently, the driver bails out if not explicitly referred to in
> DT or ACPI tables. This prevents fallback mechanisms from coming
> into effect, e.g. I2C device ID table match via DT or ACPI
> PRP0001 HID. However DT/ACPI enum should take precedence over
> the fallback, so evaluate that first.
> 
> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>

Is the change to probe_new actually related to the rest of the change?

I can't immediately see why...  If not I would prefer that as a separate
change.

> ---
>  drivers/iio/accel/st_accel_i2c.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
> index 6bdec8c451e0..e360da407027 100644
> --- a/drivers/iio/accel/st_accel_i2c.c
> +++ b/drivers/iio/accel/st_accel_i2c.c
> @@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>  
> -static int st_accel_i2c_probe(struct i2c_client *client,
> -						const struct i2c_device_id *id)
> +static int st_accel_i2c_probe(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev;
>  	struct st_sensor_data *adata;
> @@ -156,14 +155,18 @@ static int st_accel_i2c_probe(struct i2c_client *client,
>  					 client->name, sizeof(client->name));
>  	} else if (ACPI_HANDLE(&client->dev)) {
>  		ret = st_sensors_match_acpi_device(&client->dev);
> -		if ((ret < 0) || (ret >= ST_ACCEL_MAX))
> -			return -ENODEV;
> -
> -		strlcpy(client->name, st_accel_id_table[ret].name,
> +		if ((ret >= 0) && (ret < ST_ACCEL_MAX))
> +			strlcpy(client->name, st_accel_id_table[ret].name,
>  				sizeof(client->name));
> -	} else if (!id)
> -		return -ENODEV;
> +	}
>  
> +	/*
> +	 * If OF and ACPI enumeration failed, there could still be platform
> +	 * information via fallback enumeration or explicit instantiation, so
> +	 * check if id table has been matched via client->name.
> +	 */
> +	if (!client->name)
> +		return -ENODEV;
>  
>  	st_sensors_i2c_configure(indio_dev, client, adata);
>  
> @@ -187,7 +190,7 @@ static struct i2c_driver st_accel_driver = {
>  		.of_match_table = of_match_ptr(st_accel_of_match),
>  		.acpi_match_table = ACPI_PTR(st_accel_acpi_match),
>  	},
> -	.probe = st_accel_i2c_probe,
> +	.probe_new = st_accel_i2c_probe,
>  	.remove = st_accel_i2c_remove,
>  	.id_table = st_accel_id_table,
>  };

Re: [PATCH 3/3] IIO: st_accel.h: sync DT and I2C device ID table strings

[Jonathan Cameron]

On Fri, 29 Jun 2018 10:45:54 +0200
Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:

> I2C device ID table strings should really match the DT compatible
> strings (without the manufacturer prefix) to avoid confusion. This is
> especially reasonable when using ACPI PRP0001 HID /DT compatibility
> entries along with the DT compatible property in DSD which is
> used as a modalias (with manufacturer prefix stripped off) by the ACPI
> layer and thus as i2c_board_info->type by the I2C layer.
> 
> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>

Nice to have I agree.  However, it's an ABI change as this is exposed
via
/sys/bus/iio/devices/iio:\deviceN/name and is used by lots of scripts
etc to identify the device.  So we are stuck with it.

There is a reason we've kept this mess here for quite some time.

Jonathan

> ---
>  drivers/iio/accel/st_accel.h | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel.h b/drivers/iio/accel/st_accel.h
> index 2f931e4837e5..be4a4a41f849 100644
> --- a/drivers/iio/accel/st_accel.h
> +++ b/drivers/iio/accel/st_accel.h
> @@ -37,23 +37,23 @@ enum st_accel_type {
>  	ST_ACCEL_MAX,
>  };
>  
> -#define H3LIS331DL_ACCEL_DEV_NAME	"h3lis331dl_accel"
> -#define LIS3LV02DL_ACCEL_DEV_NAME	"lis3lv02dl_accel"
> -#define LSM303DLHC_ACCEL_DEV_NAME	"lsm303dlhc_accel"
> -#define LIS3DH_ACCEL_DEV_NAME		"lis3dh"
> -#define LSM330D_ACCEL_DEV_NAME		"lsm330d_accel"
> -#define LSM330DL_ACCEL_DEV_NAME		"lsm330dl_accel"
> -#define LSM330DLC_ACCEL_DEV_NAME	"lsm330dlc_accel"
> -#define LIS331DL_ACCEL_DEV_NAME		"lis331dl_accel"
> -#define LIS331DLH_ACCEL_DEV_NAME	"lis331dlh"
> -#define LSM303DL_ACCEL_DEV_NAME		"lsm303dl_accel"
> -#define LSM303DLH_ACCEL_DEV_NAME	"lsm303dlh_accel"
> -#define LSM303DLM_ACCEL_DEV_NAME	"lsm303dlm_accel"
> -#define LSM330_ACCEL_DEV_NAME		"lsm330_accel"
> -#define LSM303AGR_ACCEL_DEV_NAME	"lsm303agr_accel"
> -#define LIS2DH12_ACCEL_DEV_NAME		"lis2dh12_accel"
> +#define H3LIS331DL_ACCEL_DEV_NAME	"h3lis331dl-accel"
> +#define LIS3LV02DL_ACCEL_DEV_NAME	"lis3lv02dl-accel"
> +#define LSM303DLHC_ACCEL_DEV_NAME	"lsm303dlhc-accel"
> +#define LIS3DH_ACCEL_DEV_NAME		"lis3dh-accel"
> +#define LSM330D_ACCEL_DEV_NAME		"lsm330d-accel"
> +#define LSM330DL_ACCEL_DEV_NAME		"lsm330dl-accel"
> +#define LSM330DLC_ACCEL_DEV_NAME	"lsm330dlc-accel"
> +#define LIS331DL_ACCEL_DEV_NAME		"lis331dl-accel"
> +#define LIS331DLH_ACCEL_DEV_NAME	"lis331dlh-accel"
> +#define LSM303DL_ACCEL_DEV_NAME		"lsm303dl-accel"
> +#define LSM303DLH_ACCEL_DEV_NAME	"lsm303dlh-accel"
> +#define LSM303DLM_ACCEL_DEV_NAME	"lsm303dlm-accel"
> +#define LSM330_ACCEL_DEV_NAME		"lsm330-accel"
> +#define LSM303AGR_ACCEL_DEV_NAME	"lsm303agr-accel"
> +#define LIS2DH12_ACCEL_DEV_NAME		"lis2dh12-accel"
>  #define LIS3L02DQ_ACCEL_DEV_NAME	"lis3l02dq"
> -#define LNG2DM_ACCEL_DEV_NAME		"lng2dm"
> +#define LNG2DM_ACCEL_DEV_NAME		"lng2dm-accel"
>  #define LIS2DW12_ACCEL_DEV_NAME		"lis2dw12"
>  #define LIS3DHH_ACCEL_DEV_NAME		"lis3dhh"
>  

Re: [PATCH 0/3] IIO: st_sensors_i2c: improve device enumeration

[Nikolaus Voss]

On Fri, 29 Jun 2018, Andy Shevchenko wrote:

> I'm not sure I understand how ->probe_new() is supposed to work
> against i2c_id_table, but I don't care for legacy platform data
> anyway.
>
> What I would like to point to is device_get_match_data() API which
> should simplify / unify the case how you get driver data.

This driver doesn't need any driver data/ platform_data beyond the 
i2c_id_table name (which has already been matched when probe()/ 
probe_new() is called), so strictly neither of_match_table nor 
apci_match_table would be necessary, because i2c DT/ ACPI enumeration also 
matches against i2c_table names.

But thanks for the hint ;-).

Niko

Re: [PATCH 1/3] IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed

[Nikolaus Voss]

On Sat, 30 Jun 2018, Jonathan Cameron wrote:

> On Fri, 29 Jun 2018 10:10:10 +0200
> Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:
>
>> Currently, the driver bails out if not explicitly referred to in
>> DT or ACPI tables. This prevents fallback mechanisms from coming
>> into effect, e.g. I2C device ID table match via DT or ACPI
>> PRP0001 HID. However DT/ACPI enum should take precedence over
>> the fallback, so evaluate that first.
>>
>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>
> Is the change to probe_new actually related to the rest of the change?
>
> I can't immediately see why...  If not I would prefer that as a separate
> change.

Well, it is, because the id table pointer of the old probe() is not 
used any more.

>
>> ---
>>  drivers/iio/accel/st_accel_i2c.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
>> index 6bdec8c451e0..e360da407027 100644
>> --- a/drivers/iio/accel/st_accel_i2c.c
>> +++ b/drivers/iio/accel/st_accel_i2c.c
>> @@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
>>
>> -static int st_accel_i2c_probe(struct i2c_client *client,
>> -						const struct i2c_device_id *id)
>> +static int st_accel_i2c_probe(struct i2c_client *client)
>>  {
>>  	struct iio_dev *indio_dev;
>>  	struct st_sensor_data *adata;
>> @@ -156,14 +155,18 @@ static int st_accel_i2c_probe(struct i2c_client *client,
>>  					 client->name, sizeof(client->name));
>>  	} else if (ACPI_HANDLE(&client->dev)) {
>>  		ret = st_sensors_match_acpi_device(&client->dev);
>> -		if ((ret < 0) || (ret >= ST_ACCEL_MAX))
>> -			return -ENODEV;
>> -
>> -		strlcpy(client->name, st_accel_id_table[ret].name,
>> +		if ((ret >= 0) && (ret < ST_ACCEL_MAX))
>> +			strlcpy(client->name, st_accel_id_table[ret].name,
>>  				sizeof(client->name));
>> -	} else if (!id)
>> -		return -ENODEV;
>> +	}
>>
>> +	/*
>> +	 * If OF and ACPI enumeration failed, there could still be platform
>> +	 * information via fallback enumeration or explicit instantiation, so
>> +	 * check if id table has been matched via client->name.
>> +	 */
>> +	if (!client->name)
>> +		return -ENODEV;
>>
>>  	st_sensors_i2c_configure(indio_dev, client, adata);
>>
>> @@ -187,7 +190,7 @@ static struct i2c_driver st_accel_driver = {
>>  		.of_match_table = of_match_ptr(st_accel_of_match),
>>  		.acpi_match_table = ACPI_PTR(st_accel_acpi_match),
>>  	},
>> -	.probe = st_accel_i2c_probe,
>> +	.probe_new = st_accel_i2c_probe,
>>  	.remove = st_accel_i2c_remove,
>>  	.id_table = st_accel_id_table,
>>  };
>
>

Re: [PATCH 3/3] IIO: st_accel.h: sync DT and I2C device ID table strings

[Nikolaus Voss]

On Sat, 30 Jun 2018, Jonathan Cameron wrote:

> On Fri, 29 Jun 2018 10:45:54 +0200
> Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:
>
>> I2C device ID table strings should really match the DT compatible
>> strings (without the manufacturer prefix) to avoid confusion. This is
>> especially reasonable when using ACPI PRP0001 HID /DT compatibility
>> entries along with the DT compatible property in DSD which is
>> used as a modalias (with manufacturer prefix stripped off) by the ACPI
>> layer and thus as i2c_board_info->type by the I2C layer.
>>
>> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>
>
> Nice to have I agree.  However, it's an ABI change as this is exposed
> via
> /sys/bus/iio/devices/iio:\deviceN/name and is used by lots of scripts
> etc to identify the device.  So we are stuck with it.
>
> There is a reason we've kept this mess here for quite some time.

Ok, I feared there could be some reason ;-). Tell me, if you want me to 
respin anything...

Niko

Re: [PATCH 1/3] IIO: st_accel_i2c.c: Use fallback if DT/ACPI enum failed

[Jonathan Cameron]

On Mon, 2 Jul 2018 08:53:00 +0200
Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:

> On Sat, 30 Jun 2018, Jonathan Cameron wrote:
> 
> > On Fri, 29 Jun 2018 10:10:10 +0200
> > Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote:
> >  
> >> Currently, the driver bails out if not explicitly referred to in
> >> DT or ACPI tables. This prevents fallback mechanisms from coming
> >> into effect, e.g. I2C device ID table match via DT or ACPI
> >> PRP0001 HID. However DT/ACPI enum should take precedence over
> >> the fallback, so evaluate that first.
> >>
> >> Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de>  
> >
> > Is the change to probe_new actually related to the rest of the change?
> >
> > I can't immediately see why...  If not I would prefer that as a separate
> > change.  
> 
> Well, it is, because the id table pointer of the old probe() is not 
> used any more.
Hmm. Fair enough, kind of incidental cleanup rather than anything more.

J

> 
> >  
> >> ---
> >>  drivers/iio/accel/st_accel_i2c.c | 21 ++++++++++++---------
> >>  1 file changed, 12 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
> >> index 6bdec8c451e0..e360da407027 100644
> >> --- a/drivers/iio/accel/st_accel_i2c.c
> >> +++ b/drivers/iio/accel/st_accel_i2c.c
> >> @@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, st_accel_id_table);
> >>
> >> -static int st_accel_i2c_probe(struct i2c_client *client,
> >> -						const struct i2c_device_id *id)
> >> +static int st_accel_i2c_probe(struct i2c_client *client)
> >>  {
> >>  	struct iio_dev *indio_dev;
> >>  	struct st_sensor_data *adata;
> >> @@ -156,14 +155,18 @@ static int st_accel_i2c_probe(struct i2c_client *client,
> >>  					 client->name, sizeof(client->name));
> >>  	} else if (ACPI_HANDLE(&client->dev)) {
> >>  		ret = st_sensors_match_acpi_device(&client->dev);
> >> -		if ((ret < 0) || (ret >= ST_ACCEL_MAX))
> >> -			return -ENODEV;
> >> -
> >> -		strlcpy(client->name, st_accel_id_table[ret].name,
> >> +		if ((ret >= 0) && (ret < ST_ACCEL_MAX))
> >> +			strlcpy(client->name, st_accel_id_table[ret].name,
> >>  				sizeof(client->name));
> >> -	} else if (!id)
> >> -		return -ENODEV;
> >> +	}
> >>
> >> +	/*
> >> +	 * If OF and ACPI enumeration failed, there could still be platform
> >> +	 * information via fallback enumeration or explicit instantiation, so
> >> +	 * check if id table has been matched via client->name.
> >> +	 */
> >> +	if (!client->name)
> >> +		return -ENODEV;
> >>
> >>  	st_sensors_i2c_configure(indio_dev, client, adata);
> >>
> >> @@ -187,7 +190,7 @@ static struct i2c_driver st_accel_driver = {
> >>  		.of_match_table = of_match_ptr(st_accel_of_match),
> >>  		.acpi_match_table = ACPI_PTR(st_accel_acpi_match),
> >>  	},
> >> -	.probe = st_accel_i2c_probe,
> >> +	.probe_new = st_accel_i2c_probe,
> >>  	.remove = st_accel_i2c_remove,
> >>  	.id_table = st_accel_id_table,
> >>  };  
> >
> >  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] IIO: st_sensors_i2c: improve device enumeration

[Andy Shevchenko]

On Mon, Jul 2, 2018 at 9:41 AM, Nikolaus Voss
<nikolaus.voss@loewensteinmedical.de> wrote:
> On Fri, 29 Jun 2018, Andy Shevchenko wrote:
>
>> I'm not sure I understand how ->probe_new() is supposed to work
>> against i2c_id_table, but I don't care for legacy platform data
>> anyway.
>>
>> What I would like to point to is device_get_match_data() API which
>> should simplify / unify the case how you get driver data.
>
>
> This driver doesn't need any driver data/ platform_data beyond the
> i2c_id_table name (which has already been matched when probe()/ probe_new()
> is called), so strictly neither of_match_table nor apci_match_table would be
> necessary, because i2c DT/ ACPI enumeration also matches against i2c_table
> names.

Looking at the code I see still calls to acpi_match_data() and of_match_data().
Instead nowadays better to use device_get_match_data().

i2c_id_table should be removed from the module device table at least
(though you may continue to use it internally in the driver).

That's my understanding how it should be done.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] IIO: st_sensors_i2c: improve device enumeration

[Nikolaus Voss]

On Mon, 2 Jul 2018, Andy Shevchenko wrote:
> On Mon, Jul 2, 2018 at 9:41 AM, Nikolaus Voss
> <nikolaus.voss@loewensteinmedical.de> wrote:
>> On Fri, 29 Jun 2018, Andy Shevchenko wrote:
>>
>>> I'm not sure I understand how ->probe_new() is supposed to work
>>> against i2c_id_table, but I don't care for legacy platform data
>>> anyway.
>>>
>>> What I would like to point to is device_get_match_data() API which
>>> should simplify / unify the case how you get driver data.
>>
>>
>> This driver doesn't need any driver data/ platform_data beyond the
>> i2c_id_table name (which has already been matched when probe()/ probe_new()
>> is called), so strictly neither of_match_table nor apci_match_table would be
>> necessary, because i2c DT/ ACPI enumeration also matches against i2c_table
>> names.
>
> Looking at the code I see still calls to acpi_match_data() and of_match_data().
> Instead nowadays better to use device_get_match_data().

You are completely right, sorry. I rewrote and reposted the patch.

> i2c_id_table should be removed from the module device table at least
> (though you may continue to use it internally in the driver).

As I understand it, it is still necessary to make 
i2c_register_board_info() work.

Niko