[PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Bastien Nocera]

Instead of using the I2C or ACPI ID to determine which variant of
the chipset to use, determine that from the chip ID.

Under Windows, the same driver is used for those variants and, despite
incorrect ACPI data, it is able to load and operate the accelerometer.

Fixes the accelerometer failing with:
bmc150_accel i2c-BMA250E:00: Invalid chip f8
on the WinBook TW100

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
index 2ec9b56..7df221c 100644
--- a/drivers/iio/accel/bmc150-accel.c
+++ b/drivers/iio/accel/bmc150-accel.c
@@ -151,6 +151,7 @@ struct bmc150_scale_info {
 };
 
 struct bmc150_accel_chip_info {
+	const char *name;
 	u8 chip_id;
 	const struct iio_chan_spec *channels;
 	int num_channels;
@@ -1061,6 +1062,7 @@ enum {
 
 static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 	[bmc150] = {
+		.name = "BMC150A",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
@@ -1070,6 +1072,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
 	[bmi055] = {
+		.name = "BMI055A",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
@@ -1079,6 +1082,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
 	[bma255] = {
+		.name = "BMA0255",
 		.chip_id = 0xFA,
 		.channels = bmc150_accel_channels,
 		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
@@ -1088,6 +1092,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
 	[bma250e] = {
+		.name = "BMA250E",
 		.chip_id = 0xF9,
 		.channels = bma250e_accel_channels,
 		.num_channels = ARRAY_SIZE(bma250e_accel_channels),
@@ -1097,6 +1102,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {306457, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
 	[bma222e] = {
+		.name = "BMA222E",
 		.chip_id = 0xF8,
 		.channels = bma222e_accel_channels,
 		.num_channels = ARRAY_SIZE(bma222e_accel_channels),
@@ -1106,6 +1112,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
 				 {1225831, BMC150_ACCEL_DEF_RANGE_16G} },
 	},
 	[bma280] = {
+		.name = "BMA0280",
 		.chip_id = 0xFB,
 		.channels = bma280_accel_channels,
 		.num_channels = ARRAY_SIZE(bma280_accel_channels),
@@ -1344,20 +1351,6 @@ static irqreturn_t bmc150_accel_irq_handler(int irq, void *private)
 	return IRQ_NONE;
 }
 
-static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
-{
-	const struct acpi_device_id *id;
-
-	id = acpi_match_device(dev->driver->acpi_match_table, dev);
-
-	if (!id)
-		return NULL;
-
-	*data = (int) id->driver_data;
-
-	return dev_name(dev);
-}
-
 static int bmc150_accel_gpio_probe(struct i2c_client *client,
 					struct bmc150_accel_data *data)
 {
@@ -1554,7 +1547,7 @@ static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
 
 static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 {
-	int ret;
+	int ret, i;
 
 	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_CHIP_ID);
 	if (ret < 0) {
@@ -1564,8 +1557,15 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
 	}
 
 	dev_dbg(&data->client->dev, "Chip Id %x\n", ret);
-	if (ret != data->chip_info->chip_id) {
-		dev_err(&data->client->dev, "Invalid chip %x\n", ret);
+	for (i = 0; i < ARRAY_SIZE(bmc150_accel_chip_info_tbl); i++) {
+		if (bmc150_accel_chip_info_tbl[i].chip_id == ret) {
+			data->chip_info = &bmc150_accel_chip_info_tbl[i];
+			break;
+		}
+	}
+
+	if (!data->chip_info) {
+		dev_err(&data->client->dev, "Unsupported chip %x\n", ret);
 		return -ENODEV;
 	}
 
@@ -1618,7 +1618,6 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	struct iio_dev *indio_dev;
 	int ret;
 	const char *name = NULL;
-	int chip_id = 0;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -1628,15 +1627,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
 
-	if (id) {
+	if (id)
 		name = id->name;
-		chip_id = id->driver_data;
-	}
-
-	if (ACPI_HANDLE(&client->dev))
-		name = bmc150_accel_match_acpi_device(&client->dev, &chip_id);
-
-	data->chip_info = &bmc150_accel_chip_info_tbl[chip_id];
 
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
@@ -1647,7 +1639,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = data->chip_info->channels;
 	indio_dev->num_channels = data->chip_info->num_channels;
-	indio_dev->name = name;
+	indio_dev->name = name ? name : data->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmc150_accel_info;
 
-- 
2.4.3

Re: [PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Jonathan Cameron]

On 23/07/15 16:21, Bastien Nocera wrote:
> Instead of using the I2C or ACPI ID to determine which variant of
> the chipset to use, determine that from the chip ID.
> 
> Under Windows, the same driver is used for those variants and, despite
> incorrect ACPI data, it is able to load and operate the accelerometer.
> 
> Fixes the accelerometer failing with:
> bmc150_accel i2c-BMA250E:00: Invalid chip f8
> on the WinBook TW100
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
I'll start by saying I hate that this change is necessary down
at the driver level.

Is there no way to catch it as an ACPI quirk? (I know next to nothing about
ACPI and a quick a google isn't pointing me in the right direction!)
Seems irritating that we have to deal with this but such is life I guess
(anyone want to take the bet that at some point the windows driver will break
 horribly as well for these machines?)

Anyhow, Srinivas, could you also take a look at this one.

I suppose I'll cope with the horribleness :)

> ---
>  drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> index 2ec9b56..7df221c 100644
> --- a/drivers/iio/accel/bmc150-accel.c
> +++ b/drivers/iio/accel/bmc150-accel.c
> @@ -151,6 +151,7 @@ struct bmc150_scale_info {
>  };
>  
>  struct bmc150_accel_chip_info {
> +	const char *name;
>  	u8 chip_id;
>  	const struct iio_chan_spec *channels;
>  	int num_channels;
> @@ -1061,6 +1062,7 @@ enum {
>  
>  static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  	[bmc150] = {
> +		.name = "BMC150A",
>  		.chip_id = 0xFA,
>  		.channels = bmc150_accel_channels,
>  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> @@ -1070,6 +1072,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
>  	},
>  	[bmi055] = {
> +		.name = "BMI055A",
>  		.chip_id = 0xFA,
>  		.channels = bmc150_accel_channels,
>  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> @@ -1079,6 +1082,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
>  	},
>  	[bma255] = {
> +		.name = "BMA0255",
>  		.chip_id = 0xFA,
>  		.channels = bmc150_accel_channels,
>  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> @@ -1088,6 +1092,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
>  	},
>  	[bma250e] = {
> +		.name = "BMA250E",
>  		.chip_id = 0xF9,
>  		.channels = bma250e_accel_channels,
>  		.num_channels = ARRAY_SIZE(bma250e_accel_channels),
> @@ -1097,6 +1102,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  				 {306457, BMC150_ACCEL_DEF_RANGE_16G} },
>  	},
>  	[bma222e] = {
> +		.name = "BMA222E",
>  		.chip_id = 0xF8,
>  		.channels = bma222e_accel_channels,
>  		.num_channels = ARRAY_SIZE(bma222e_accel_channels),
> @@ -1106,6 +1112,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
>  				 {1225831, BMC150_ACCEL_DEF_RANGE_16G} },
>  	},
>  	[bma280] = {
> +		.name = "BMA0280",
>  		.chip_id = 0xFB,
>  		.channels = bma280_accel_channels,
>  		.num_channels = ARRAY_SIZE(bma280_accel_channels),
> @@ -1344,20 +1351,6 @@ static irqreturn_t bmc150_accel_irq_handler(int irq, void *private)
>  	return IRQ_NONE;
>  }
>  
> -static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
> -{
> -	const struct acpi_device_id *id;
> -
> -	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -
> -	if (!id)
> -		return NULL;
> -
> -	*data = (int) id->driver_data;
> -
> -	return dev_name(dev);
> -}
> -
>  static int bmc150_accel_gpio_probe(struct i2c_client *client,
>  					struct bmc150_accel_data *data)
>  {
> @@ -1554,7 +1547,7 @@ static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
>  
>  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  {
> -	int ret;
> +	int ret, i;
>  
>  	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_CHIP_ID);
>  	if (ret < 0) {
> @@ -1564,8 +1557,15 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
>  	}
>  
>  	dev_dbg(&data->client->dev, "Chip Id %x\n", ret);
> -	if (ret != data->chip_info->chip_id) {
> -		dev_err(&data->client->dev, "Invalid chip %x\n", ret);
This isn't really init (was a bit debatable that the previous sanity
check was either but there we are). Ah well.

> +	for (i = 0; i < ARRAY_SIZE(bmc150_accel_chip_info_tbl); i++) {
> +		if (bmc150_accel_chip_info_tbl[i].chip_id == ret) {
> +			data->chip_info = &bmc150_accel_chip_info_tbl[i];
> +			break;
> +		}
> +	}
> +
> +	if (!data->chip_info) {
> +		dev_err(&data->client->dev, "Unsupported chip %x\n", ret);
>  		return -ENODEV;
>  	}
>  
> @@ -1618,7 +1618,6 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	struct iio_dev *indio_dev;
>  	int ret;
>  	const char *name = NULL;
> -	int chip_id = 0;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -1628,15 +1627,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
>  
> -	if (id) {
> +	if (id)
>  		name = id->name;
> -		chip_id = id->driver_data;
> -	}
> -
> -	if (ACPI_HANDLE(&client->dev))
> -		name = bmc150_accel_match_acpi_device(&client->dev, &chip_id);
> -
> -	data->chip_info = &bmc150_accel_chip_info_tbl[chip_id];
>  
>  	ret = bmc150_accel_chip_init(data);
>  	if (ret < 0)
> @@ -1647,7 +1639,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->channels = data->chip_info->channels;
>  	indio_dev->num_channels = data->chip_info->num_channels;
> -	indio_dev->name = name;
> +	indio_dev->name = name ? name : data->chip_info->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmc150_accel_info;
>  
> 

Re: [PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Bastien Nocera]

On Sun, 2015-08-02 at 18:18 +0100, Jonathan Cameron wrote:
> On 23/07/15 16:21, Bastien Nocera wrote:
> > Instead of using the I2C or ACPI ID to determine which variant of
> > the chipset to use, determine that from the chip ID.
> > 
> > Under Windows, the same driver is used for those variants and, 
> > despite
> > incorrect ACPI data, it is able to load and operate the 
> > accelerometer.
> > 
> > Fixes the accelerometer failing with:
> > bmc150_accel i2c-BMA250E:00: Invalid chip f8
> > on the WinBook TW100
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> I'll start by saying I hate that this change is necessary down
> at the driver level.
> 
> Is there no way to catch it as an ACPI quirk? (I know next to nothing 
> about
> ACPI and a quick a google isn't pointing me in the right direction!)

I could add DMI matches, but that seems silly when the new code is
actually simpler.

> Seems irritating that we have to deal with this but such is life I 
> guess
> (anyone want to take the bet that at some point the windows driver 
> will break
>  horribly as well for these machines?)
> 
> Anyhow, Srinivas, could you also take a look at this one.
> 
> I suppose I'll cope with the horribleness :)
> 
> > ---
> >  drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------
> > ------------
> >  1 file changed, 19 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel.c 
> > b/drivers/iio/accel/bmc150-accel.c
> > index 2ec9b56..7df221c 100644
> > --- a/drivers/iio/accel/bmc150-accel.c
> > +++ b/drivers/iio/accel/bmc150-accel.c
> > @@ -151,6 +151,7 @@ struct bmc150_scale_info {
> >  };
> >  
> >  struct bmc150_accel_chip_info {
> > +	const char *name;
> >  	u8 chip_id;
> >  	const struct iio_chan_spec *channels;
> >  	int num_channels;
> > @@ -1061,6 +1062,7 @@ enum {
> >  
> >  static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  	[bmc150] = {
> > +		.name = "BMC150A",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1070,6 +1072,7 @@ static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, 
> > BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bmi055] = {
> > +		.name = "BMI055A",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1079,6 +1082,7 @@ static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, 
> > BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma255] = {
> > +		.name = "BMA0255",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1088,6 +1092,7 @@ static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, 
> > BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma250e] = {
> > +		.name = "BMA250E",
> >  		.chip_id = 0xF9,
> >  		.channels = bma250e_accel_channels,
> >  		.num_channels = 
> > ARRAY_SIZE(bma250e_accel_channels),
> > @@ -1097,6 +1102,7 @@ static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  				 {306457, 
> > BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma222e] = {
> > +		.name = "BMA222E",
> >  		.chip_id = 0xF8,
> >  		.channels = bma222e_accel_channels,
> >  		.num_channels = 
> > ARRAY_SIZE(bma222e_accel_channels),
> > @@ -1106,6 +1112,7 @@ static const struct bmc150_accel_chip_info 
> > bmc150_accel_chip_info_tbl[] = {
> >  				 {1225831, 
> > BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma280] = {
> > +		.name = "BMA0280",
> >  		.chip_id = 0xFB,
> >  		.channels = bma280_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bma280_accel_channels),
> > @@ -1344,20 +1351,6 @@ static irqreturn_t 
> > bmc150_accel_irq_handler(int irq, void *private)
> >  	return IRQ_NONE;
> >  }
> >  
> > -static const char *bmc150_accel_match_acpi_device(struct device 
> > *dev, int *data)
> > -{
> > -	const struct acpi_device_id *id;
> > -
> > -	id = acpi_match_device(dev->driver->acpi_match_table, 
> > dev);
> > -
> > -	if (!id)
> > -		return NULL;
> > -
> > -	*data = (int) id->driver_data;
> > -
> > -	return dev_name(dev);
> > -}
> > -
> >  static int bmc150_accel_gpio_probe(struct i2c_client *client,
> >  					struct bmc150_accel_data 
> > *data)
> >  {
> > @@ -1554,7 +1547,7 @@ static const struct iio_buffer_setup_ops 
> > bmc150_accel_buffer_ops = {
> >  
> >  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> >  {
> > -	int ret;
> > +	int ret, i;
> >  
> >  	ret = i2c_smbus_read_byte_data(data->client, 
> > BMC150_ACCEL_REG_CHIP_ID);
> >  	if (ret < 0) {
> > @@ -1564,8 +1557,15 @@ static int bmc150_accel_chip_init(struct 
> > bmc150_accel_data *data)
> >  	}
> >  
> >  	dev_dbg(&data->client->dev, "Chip Id %x\n", ret);
> > -	if (ret != data->chip_info->chip_id) {
> > -		dev_err(&data->client->dev, "Invalid chip %x\n", 
> > ret);
> This isn't really init (was a bit debatable that the previous sanity
> check was either but there we are). Ah well.
> 
> > +	for (i = 0; i < ARRAY_SIZE(bmc150_accel_chip_info_tbl); 
> > i++) {
> > +		if (bmc150_accel_chip_info_tbl[i].chip_id == ret) 
> > {
> > +			data->chip_info = 
> > &bmc150_accel_chip_info_tbl[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!data->chip_info) {
> > +		dev_err(&data->client->dev, "Unsupported chip 
> > %x\n", ret);
> >  		return -ENODEV;
> >  	}
> >  
> > @@ -1618,7 +1618,6 @@ static int bmc150_accel_probe(struct 
> > i2c_client *client,
> >  	struct iio_dev *indio_dev;
> >  	int ret;
> >  	const char *name = NULL;
> > -	int chip_id = 0;
> >  
> >  	indio_dev = devm_iio_device_alloc(&client->dev, 
> > sizeof(*data));
> >  	if (!indio_dev)
> > @@ -1628,15 +1627,8 @@ static int bmc150_accel_probe(struct 
> > i2c_client *client,
> >  	i2c_set_clientdata(client, indio_dev);
> >  	data->client = client;
> >  
> > -	if (id) {
> > +	if (id)
> >  		name = id->name;
> > -		chip_id = id->driver_data;
> > -	}
> > -
> > -	if (ACPI_HANDLE(&client->dev))
> > -		name = bmc150_accel_match_acpi_device(&client
> > ->dev, &chip_id);
> > -
> > -	data->chip_info = &bmc150_accel_chip_info_tbl[chip_id];
> >  
> >  	ret = bmc150_accel_chip_init(data);
> >  	if (ret < 0)
> > @@ -1647,7 +1639,7 @@ static int bmc150_accel_probe(struct 
> > i2c_client *client,
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->channels = data->chip_info->channels;
> >  	indio_dev->num_channels = data->chip_info->num_channels;
> > -	indio_dev->name = name;
> > +	indio_dev->name = name ? name : data->chip_info->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->info = &bmc150_accel_info;
> >  
> > 
> 

Re: [PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Srinivas Pandruvada]

On Sun, 2015-08-02 at 18:18 +0100, Jonathan Cameron wrote:
> On 23/07/15 16:21, Bastien Nocera wrote:
> > Instead of using the I2C or ACPI ID to determine which variant of
> > the chipset to use, determine that from the chip ID.
> > 
> > Under Windows, the same driver is used for those variants and, despite
> > incorrect ACPI data, it is able to load and operate the accelerometer.
> > 
> > Fixes the accelerometer failing with:
> > bmc150_accel i2c-BMA250E:00: Invalid chip f8
> > on the WinBook TW100
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> I'll start by saying I hate that this change is necessary down
> at the driver level.
> 
> Is there no way to catch it as an ACPI quirk? (I know next to nothing about
> ACPI and a quick a google isn't pointing me in the right direction!)
> Seems irritating that we have to deal with this but such is life I guess
> (anyone want to take the bet that at some point the windows driver will break
>  horribly as well for these machines?)
> 
> Anyhow, Srinivas, could you also take a look at this one.
> 
> I suppose I'll cope with the horribleness :)
We have seen where manufactures replaces a part for some reason, without
modifying ACPI tables. So matching chip id makes sense logically. But
even this is not fail proof. Some parts have same chip id with different
features (clones), in that case ACPI match makes more sense.
There is a way to patch ACPI tables but that also need to be based on
some DMI information.
I am not aware of such issue with Bosch parts. So I think this would be
OK to use chip id here instead of acpi id here. But what about using
this only when the current logic fails? In this way we can still have
another entry in the table for clones, if required.

Thanks,
Srinivas
> 
> > ---
> >  drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
> >  1 file changed, 19 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> > index 2ec9b56..7df221c 100644
> > --- a/drivers/iio/accel/bmc150-accel.c
> > +++ b/drivers/iio/accel/bmc150-accel.c
> > @@ -151,6 +151,7 @@ struct bmc150_scale_info {
> >  };
> >  
> >  struct bmc150_accel_chip_info {
> > +	const char *name;
> >  	u8 chip_id;
> >  	const struct iio_chan_spec *channels;
> >  	int num_channels;
> > @@ -1061,6 +1062,7 @@ enum {
> >  
> >  static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  	[bmc150] = {
> > +		.name = "BMC150A",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1070,6 +1072,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bmi055] = {
> > +		.name = "BMI055A",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1079,6 +1082,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma255] = {
> > +		.name = "BMA0255",
> >  		.chip_id = 0xFA,
> >  		.channels = bmc150_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bmc150_accel_channels),
> > @@ -1088,6 +1092,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  				 {76590, BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma250e] = {
> > +		.name = "BMA250E",
> >  		.chip_id = 0xF9,
> >  		.channels = bma250e_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bma250e_accel_channels),
> > @@ -1097,6 +1102,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  				 {306457, BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma222e] = {
> > +		.name = "BMA222E",
> >  		.chip_id = 0xF8,
> >  		.channels = bma222e_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bma222e_accel_channels),
> > @@ -1106,6 +1112,7 @@ static const struct bmc150_accel_chip_info bmc150_accel_chip_info_tbl[] = {
> >  				 {1225831, BMC150_ACCEL_DEF_RANGE_16G} },
> >  	},
> >  	[bma280] = {
> > +		.name = "BMA0280",
> >  		.chip_id = 0xFB,
> >  		.channels = bma280_accel_channels,
> >  		.num_channels = ARRAY_SIZE(bma280_accel_channels),
> > @@ -1344,20 +1351,6 @@ static irqreturn_t bmc150_accel_irq_handler(int irq, void *private)
> >  	return IRQ_NONE;
> >  }
> >  
> > -static const char *bmc150_accel_match_acpi_device(struct device *dev, int *data)
> > -{
> > -	const struct acpi_device_id *id;
> > -
> > -	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> > -
> > -	if (!id)
> > -		return NULL;
> > -
> > -	*data = (int) id->driver_data;
> > -
> > -	return dev_name(dev);
> > -}
> > -
> >  static int bmc150_accel_gpio_probe(struct i2c_client *client,
> >  					struct bmc150_accel_data *data)
> >  {
> > @@ -1554,7 +1547,7 @@ static const struct iio_buffer_setup_ops bmc150_accel_buffer_ops = {
> >  
> >  static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> >  {
> > -	int ret;
> > +	int ret, i;
> >  
> >  	ret = i2c_smbus_read_byte_data(data->client, BMC150_ACCEL_REG_CHIP_ID);
> >  	if (ret < 0) {
> > @@ -1564,8 +1557,15 @@ static int bmc150_accel_chip_init(struct bmc150_accel_data *data)
> >  	}
> >  
> >  	dev_dbg(&data->client->dev, "Chip Id %x\n", ret);
> > -	if (ret != data->chip_info->chip_id) {
> > -		dev_err(&data->client->dev, "Invalid chip %x\n", ret);
> This isn't really init (was a bit debatable that the previous sanity
> check was either but there we are). Ah well.
> 
> > +	for (i = 0; i < ARRAY_SIZE(bmc150_accel_chip_info_tbl); i++) {
> > +		if (bmc150_accel_chip_info_tbl[i].chip_id == ret) {
> > +			data->chip_info = &bmc150_accel_chip_info_tbl[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!data->chip_info) {
> > +		dev_err(&data->client->dev, "Unsupported chip %x\n", ret);
> >  		return -ENODEV;
> >  	}
> >  
> > @@ -1618,7 +1618,6 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  	struct iio_dev *indio_dev;
> >  	int ret;
> >  	const char *name = NULL;
> > -	int chip_id = 0;
> >  
> >  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >  	if (!indio_dev)
> > @@ -1628,15 +1627,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  	i2c_set_clientdata(client, indio_dev);
> >  	data->client = client;
> >  
> > -	if (id) {
> > +	if (id)
> >  		name = id->name;
> > -		chip_id = id->driver_data;
> > -	}
> > -
> > -	if (ACPI_HANDLE(&client->dev))
> > -		name = bmc150_accel_match_acpi_device(&client->dev, &chip_id);
> > -
> > -	data->chip_info = &bmc150_accel_chip_info_tbl[chip_id];
> >  
> >  	ret = bmc150_accel_chip_init(data);
> >  	if (ret < 0)
> > @@ -1647,7 +1639,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->channels = data->chip_info->channels;
> >  	indio_dev->num_channels = data->chip_info->num_channels;
> > -	indio_dev->name = name;
> > +	indio_dev->name = name ? name : data->chip_info->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->info = &bmc150_accel_info;
> >  
> > 
> 

Re: [PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Bastien Nocera]

On Mon, 2015-08-03 at 13:09 -0700, Srinivas Pandruvada wrote:
> On Sun, 2015-08-02 at 18:18 +0100, Jonathan Cameron wrote:
> > On 23/07/15 16:21, Bastien Nocera wrote:
> > > Instead of using the I2C or ACPI ID to determine which variant of
> > > the chipset to use, determine that from the chip ID.
> > > 
> > > Under Windows, the same driver is used for those variants and, 
> > > despite
> > > incorrect ACPI data, it is able to load and operate the 
> > > accelerometer.
> > > 
> > > Fixes the accelerometer failing with:
> > > bmc150_accel i2c-BMA250E:00: Invalid chip f8
> > > on the WinBook TW100
> > > 
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > I'll start by saying I hate that this change is necessary down
> > at the driver level.
> > 
> > Is there no way to catch it as an ACPI quirk? (I know next to 
> > nothing about
> > ACPI and a quick a google isn't pointing me in the right 
> > direction!)
> > Seems irritating that we have to deal with this but such is life I 
> > guess
> > (anyone want to take the bet that at some point the windows driver 
> > will break
> >  horribly as well for these machines?)
> > 
> > Anyhow, Srinivas, could you also take a look at this one.
> > 
> > I suppose I'll cope with the horribleness :)
> We have seen where manufactures replaces a part for some reason, 
> without
> modifying ACPI tables. So matching chip id makes sense logically. But
> even this is not fail proof. Some parts have same chip id with 
> different
> features (clones), in that case ACPI match makes more sense.
> There is a way to patch ACPI tables but that also need to be based on
> some DMI information.
> I am not aware of such issue with Bosch parts. So I think this would 
> be
> OK to use chip id here instead of acpi id here. But what about using
> this only when the current logic fails? In this way we can still have
> another entry in the table for clones, if required.

For clones which don't advertise the right chip ID, I'd just have a
separate quirk table, "use this chip ID for that hardware".

Seems simpler, and I'm happy to write that code when we encounter the
problem.

Re: [PATCH 2/2] iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

[Jonathan Cameron]

On 04/08/15 10:58, Bastien Nocera wrote:
> On Mon, 2015-08-03 at 13:09 -0700, Srinivas Pandruvada wrote:
>> On Sun, 2015-08-02 at 18:18 +0100, Jonathan Cameron wrote:
>>> On 23/07/15 16:21, Bastien Nocera wrote:
>>>> Instead of using the I2C or ACPI ID to determine which variant of
>>>> the chipset to use, determine that from the chip ID.
>>>>
>>>> Under Windows, the same driver is used for those variants and, 
>>>> despite
>>>> incorrect ACPI data, it is able to load and operate the 
>>>> accelerometer.
>>>>
>>>> Fixes the accelerometer failing with:
>>>> bmc150_accel i2c-BMA250E:00: Invalid chip f8
>>>> on the WinBook TW100
>>>>
>>>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>>> I'll start by saying I hate that this change is necessary down
>>> at the driver level.
>>>
>>> Is there no way to catch it as an ACPI quirk? (I know next to 
>>> nothing about
>>> ACPI and a quick a google isn't pointing me in the right 
>>> direction!)
>>> Seems irritating that we have to deal with this but such is life I 
>>> guess
>>> (anyone want to take the bet that at some point the windows driver 
>>> will break
>>>  horribly as well for these machines?)
>>>
>>> Anyhow, Srinivas, could you also take a look at this one.
>>>
>>> I suppose I'll cope with the horribleness :)
>> We have seen where manufactures replaces a part for some reason, 
>> without
>> modifying ACPI tables. So matching chip id makes sense logically. But
>> even this is not fail proof. Some parts have same chip id with 
>> different
>> features (clones), in that case ACPI match makes more sense.
>> There is a way to patch ACPI tables but that also need to be based on
>> some DMI information.
>> I am not aware of such issue with Bosch parts. So I think this would 
>> be
>> OK to use chip id here instead of acpi id here. But what about using
>> this only when the current logic fails? In this way we can still have
>> another entry in the table for clones, if required.
> 
> For clones which don't advertise the right chip ID, I'd just have a
> separate quirk table, "use this chip ID for that hardware".
> 
> Seems simpler, and I'm happy to write that code when we encounter the
> problem.
I'm convinced.  Applied to the togreg branch of iio.git (too large to push
out as a fix at this point in the cycle as it's not fixing a regression).

Note that it didn't apply cleanly so please take a quick look to check
I haven't messed it up!

Will be pushed out as testing as soon as my local sanity check build is
done so the autobuilders can hit it harder.

Thanks,

Jonathan
> --
> 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
>