Skip to contentSkip to conversation

iio:accel:bmc150-accel: Use the chip ID to detect sensor variant

PATCH In review Bastien Nocera 3 participants 6 messages Jul 23, 2015 – Aug 8, 2015 IIO

lore
Share & tools
lore
#0 lore raw

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

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

---
 drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 27 deletions(-)
drivers/iio/accel/bmc150-accel.c +19 -27
index 2ec9b56..7df221c 100644
@@ -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;
 
Signature · 1 line
2.4.3
maintainer
#1 lore raw
Quoted 11 lines · 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

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 :)

Quoted 95 lines
---
 drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 27 deletions(-)
drivers/iio/accel/bmc150-accel.c +8 -15
index 2ec9b56..7df221c 100644
@@ -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) {

drivers/iio/accel/bmc150-accel.c static int bmc150_accel_chip_init(struct bmc150_accel_data *data) · line 1568

Quoted 5 lines
drivers/iio/accel/bmc150-accel.c +0 -2
@@ -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.

Quoted 47 lines
drivers/iio/accel/bmc150-accel.c +11 -10
+	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 lore raw
Quoted 13 lines · Jonathan Cameron
Quoted 12 lines · 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

Quoted 7 lines
Quoted 1 line

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.

Quoted 193 lines

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 :)

Quoted 116 lines
---
 drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------
------------
 1 file changed, 19 insertions(+), 27 deletions(-)
drivers/iio/accel/bmc150-accel.c +2 -0
index 2ec9b56..7df221c 100644
@@ -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,
drivers/iio/accel/bmc150-accel.c +1 -0
 		.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,
drivers/iio/accel/bmc150-accel.c +1 -0
 		.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,
drivers/iio/accel/bmc150-accel.c +1 -0
 		.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,
drivers/iio/accel/bmc150-accel.c +0 -0
 		.channels = bma250e_accel_channels,
 		.num_channels = 

ARRAY_SIZE(bma250e_accel_channels),

drivers/iio/accel/bmc150-accel.c +1 -0
@@ -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,
drivers/iio/accel/bmc150-accel.c +0 -0
 		.channels = bma222e_accel_channels,
 		.num_channels = 

ARRAY_SIZE(bma222e_accel_channels),

drivers/iio/accel/bmc150-accel.c +1 -0
@@ -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,
drivers/iio/accel/bmc150-accel.c +0 -14
 		.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)
{

drivers/iio/accel/bmc150-accel.c +1 -1
@@ -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);

drivers/iio/accel/bmc150-accel.c +0 -0
 	if (ret < 0) {

drivers/iio/accel/bmc150-accel.c static int bmc150_accel_chip_init(struct · line 1561

Quoted 7 lines
drivers/iio/accel/bmc150-accel.c +0 -2
@@ -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.

Quoted 56 lines
drivers/iio/accel/bmc150-accel.c +2 -0
+	for (i = 0; i < ARRAY_SIZE(bmc150_accel_chip_info_tbl); 
 i++) {
+		if (bmc150_accel_chip_info_tbl[i].chip_id == ret) 

{

drivers/iio/accel/bmc150-accel.c +1 -0
+			data->chip_info = 

&bmc150_accel_chip_info_tbl[i];

drivers/iio/accel/bmc150-accel.c +6 -0
+			break;
+		}
+	}
+
+	if (!data->chip_info) {
+		dev_err(&data->client->dev, "Unsupported chip 

%x\n", ret);

drivers/iio/accel/bmc150-accel.c +0 -1
 		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));

drivers/iio/accel/bmc150-accel.c +2 -10
 	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;
#3 lore raw
In reply to Jonathan Cameron
Quoted 21 lines · Jonathan Cameron
Quoted 11 lines · 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

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?)

Quoted 3 lines

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

Quoted 152 lines
Quoted 96 lines
---
 drivers/iio/accel/bmc150-accel.c | 46 +++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 27 deletions(-)
drivers/iio/accel/bmc150-accel.c +8 -15
index 2ec9b56..7df221c 100644
@@ -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) {

drivers/iio/accel/bmc150-accel.c static int bmc150_accel_chip_init(struct bmc150_accel_data *data) · line 1568

Quoted 5 lines
drivers/iio/accel/bmc150-accel.c +0 -2
@@ -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.

Quoted 47 lines
drivers/iio/accel/bmc150-accel.c +11 -10
+	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;
 
#4 lore raw
Quoted 31 lines · Srinivas Pandruvada
Quoted 27 lines · Jonathan Cameron
Quoted 8 lines · 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.

Quoted 5 lines

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

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?)

Quoted 3 lines

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

I suppose I'll cope with the horribleness :)

Quoted 13 lines

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.

maintainer
#5 lore raw
Quoted 46 lines · Bastien Nocera
Quoted 31 lines · Srinivas Pandruvada
Quoted 27 lines · Jonathan Cameron
Quoted 8 lines · 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.

Quoted 5 lines

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

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?)

Quoted 3 lines

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

I suppose I'll cope with the horribleness :)

Quoted 13 lines

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.

Quoted 5 lines

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

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html