| Message ID | 20250111-mainlining-mc3510c-v1-3-57be503addf8@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: mc3230: add mount matrix, of match and mc3510c support | expand |
On 1/11/25 10:11 PM, Vasiliy Doylov via B4 Relay wrote: > From: Vasiliy Doylov <nekodevelopper@gmail.com> > > This commit integrates support for the mc3510c into the mc3230 driver. > > Tested on Huawei MediaPad T3 10 (huawei-agassi) > > Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com> > --- > drivers/iio/accel/mc3230.c | 55 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 44 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c > index 3cad6f2d7a2a79df38f90e5656763f6ed019a920..ebbb96c658d87a83007c7c3c7212ce9ebf039963 100644 > --- a/drivers/iio/accel/mc3230.c > +++ b/drivers/iio/accel/mc3230.c > @@ -22,20 +22,41 @@ > #define MC3230_MODE_OPCON_STANDBY 0x03 > > #define MC3230_REG_CHIP_ID 0x18 > -#define MC3230_CHIP_ID 0x01 > - > #define MC3230_REG_PRODUCT_CODE 0x3b > -#define MC3230_PRODUCT_CODE 0x19 > > /* > * The accelerometer has one measurement range: > * > * -1.5g - +1.5g (8-bit, signed) > * > - * scale = (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 > */ > > -static const int mc3230_nscale = 115411765; > +enum mc3xxx_chips { > + MC3230, > + MC3510C, > +}; > + > +struct mc3xxx_chip_info { > + const char *name; > + const u8 chip_id; > + const u8 product_code; > + const int scale; > +}; The struct members are usually ordered alphabetically. Also, const specifiers for u8s and int are redundant, you will only want it for the pointer, usually. > + > +static struct mc3xxx_chip_info mc3xxx_chip_info_tbl[] = { > + [MC3230] = { > + .name = "mc3230", > + .chip_id = 0x01, > + .product_code = 0x19, > + .scale = 115411765, // (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 /* */ style comments are preferred. Also, it should be above the .scale to not make the line so long (even if the line length requirement is met). > + }, > + [MC3510C] = { > + .name = "mc3510c", > + .chip_id = 0x23, > + .product_code = 0x10, > + .scale = 625000000, // Was obtained empirically Same here. > + }, > +}; > > #define MC3230_CHANNEL(reg, axis) { \ > .type = IIO_ACCEL, \ > @@ -50,6 +71,7 @@ static const int mc3230_nscale = 115411765; > struct mc3230_data { > struct i2c_client *client; > struct iio_mount_matrix orientation; > + const struct mc3xxx_chip_info *chip_info; Same here, order alphabetically. > }; > > static const struct iio_mount_matrix * > @@ -111,7 +133,7 @@ static int mc3230_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > case IIO_CHAN_INFO_SCALE: > *val = 0; > - *val2 = mc3230_nscale; > + *val2 = data->chip_info->scale; > return IIO_VAL_INT_PLUS_NANO; > default: > return -EINVAL; > @@ -127,15 +149,23 @@ static int mc3230_probe(struct i2c_client *client) > int ret; > struct iio_dev *indio_dev; > struct mc3230_data *data; > + const struct mc3xxx_chip_info *chip_info; > > + chip_info = i2c_get_match_data(client); > /* First check chip-id and product-id */ > ret = i2c_smbus_read_byte_data(client, MC3230_REG_CHIP_ID); > - if (ret != MC3230_CHIP_ID) > + if (ret != chip_info->chip_id) { > + dev_err(&client->dev, > + "chip id check fail: 0x%x != 0x%x !\n", ret, chip_info->chip_id); > return (ret < 0) ? ret : -ENODEV; > + } > > ret = i2c_smbus_read_byte_data(client, MC3230_REG_PRODUCT_CODE); > - if (ret != MC3230_PRODUCT_CODE) > + if (ret != chip_info->product_code) { > + dev_err(&client->dev, > + "product code check fail: 0x%x != 0x%x !\n", ret, chip_info->product_code); > return (ret < 0) ? ret : -ENODEV; > + } > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > if (!indio_dev) { > @@ -145,10 +175,11 @@ static int mc3230_probe(struct i2c_client *client) > > data = iio_priv(indio_dev); > data->client = client; > + data->chip_info = chip_info; > i2c_set_clientdata(client, indio_dev); > > indio_dev->info = &mc3230_info; > - indio_dev->name = "mc3230"; > + indio_dev->name = chip_info->name; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = mc3230_channels; > indio_dev->num_channels = ARRAY_SIZE(mc3230_channels); > @@ -200,13 +231,15 @@ static int mc3230_resume(struct device *dev) > static DEFINE_SIMPLE_DEV_PM_OPS(mc3230_pm_ops, mc3230_suspend, mc3230_resume); > > static const struct i2c_device_id mc3230_i2c_id[] = { > - { "mc3230" }, > + { "mc3230", (kernel_ulong_t)&mc3xxx_chip_info_tbl[MC3230] }, > + { "mc3510c", (kernel_ulong_t)&mc3xxx_chip_info_tbl[MC3510C] }, > {} > }; > MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id); > > static const struct of_device_id mc3230_of_match[] = { > - { .compatible = "mcube,mc3230" }, > + { .compatible = "mcube,mc3230", &mc3xxx_chip_info_tbl[MC3230] }, > + { .compatible = "mcube,mc3510c", &mc3xxx_chip_info_tbl[MC3510C] }, > { }, > }; > MODULE_DEVICE_TABLE(of, mc3230_of_match);
On Sun, 12 Jan 2025 01:04:34 +0200 Markuss Broks <markuss.broks@gmail.com> wrote: > On 1/11/25 10:11 PM, Vasiliy Doylov via B4 Relay wrote: > > From: Vasiliy Doylov <nekodevelopper@gmail.com> > > > > This commit integrates support for the mc3510c into the mc3230 driver. > > > > Tested on Huawei MediaPad T3 10 (huawei-agassi) > > > > Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com> > > --- > > drivers/iio/accel/mc3230.c | 55 ++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 44 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c > > index 3cad6f2d7a2a79df38f90e5656763f6ed019a920..ebbb96c658d87a83007c7c3c7212ce9ebf039963 100644 > > --- a/drivers/iio/accel/mc3230.c > > +++ b/drivers/iio/accel/mc3230.c > > @@ -22,20 +22,41 @@ > > #define MC3230_MODE_OPCON_STANDBY 0x03 > > > > #define MC3230_REG_CHIP_ID 0x18 > > -#define MC3230_CHIP_ID 0x01 > > - > > #define MC3230_REG_PRODUCT_CODE 0x3b > > -#define MC3230_PRODUCT_CODE 0x19 > > > > /* > > * The accelerometer has one measurement range: > > * > > * -1.5g - +1.5g (8-bit, signed) > > * > > - * scale = (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 > > */ > > > > -static const int mc3230_nscale = 115411765; > > +enum mc3xxx_chips { > > + MC3230, > > + MC3510C, > > +}; > > + > > +struct mc3xxx_chip_info { > > + const char *name; > > + const u8 chip_id; > > + const u8 product_code; > > + const int scale; > > +}; > The struct members are usually ordered alphabetically. Also, const > specifiers for u8s and int are redundant, you will only want it for the > pointer, usually. No they are not usually ordered alphabetically (in kernel code anyway) Much more important characteristics apply when choosing structure ordering. 1) Comprehensibility - keep related items next to each other. 2) Slight potential performance benefit from frequently accessed items as first entry. 3) Padding concerns. pahole will help but generally it is easy to work out from first principles. In that order. Sure you can do alphabetical if none of the above apply, but it is far from a critical factor. Const specifiers here are harmless as anotation but not necessary as you say. Jonathan
On Sat, 11 Jan 2025 23:11:08 +0300 Vasiliy Doylov via B4 Relay <devnull+nekodevelopper.gmail.com@kernel.org> wrote: > From: Vasiliy Doylov <nekodevelopper@gmail.com> > > This commit integrates support for the mc3510c into the mc3230 driver. > > Tested on Huawei MediaPad T3 10 (huawei-agassi) > > Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com> General approach to this sort of change is a first 'no operation' patch that refactors the driver to allow for multiple device support, and a second patch that adds the support. In this case the second patch is very simple though so I don't mind that much. Mostly looks good, but a few things that are non obvious the first time you write a patch like this. Mostly avoiding pitfalls we have fallen down in the past ;) Jonathan > --- > drivers/iio/accel/mc3230.c | 55 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 44 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c > index 3cad6f2d7a2a79df38f90e5656763f6ed019a920..ebbb96c658d87a83007c7c3c7212ce9ebf039963 100644 > --- a/drivers/iio/accel/mc3230.c > +++ b/drivers/iio/accel/mc3230.c > @@ -22,20 +22,41 @@ > #define MC3230_MODE_OPCON_STANDBY 0x03 > > #define MC3230_REG_CHIP_ID 0x18 > -#define MC3230_CHIP_ID 0x01 > - > #define MC3230_REG_PRODUCT_CODE 0x3b > -#define MC3230_PRODUCT_CODE 0x19 > > /* > * The accelerometer has one measurement range: > * > * -1.5g - +1.5g (8-bit, signed) > * > - * scale = (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 > */ > > -static const int mc3230_nscale = 115411765; > +enum mc3xxx_chips { In IIO drivers avoid use of wild cards in naming of anything. They go wrong far too often as other parts match the coding and drivers start supporting additional devices that don't Name everything after the first supported part unless it applies only to a different device, in which case name it after the first device it applies to. This is a hard learned lesson over the years! > + MC3230, > + MC3510C, > +}; > + > +struct mc3xxx_chip_info { > + const char *name; > + const u8 chip_id; > + const u8 product_code; > + const int scale; > +}; > + > +static struct mc3xxx_chip_info mc3xxx_chip_info_tbl[] = { > + [MC3230] = { > + .name = "mc3230", > + .chip_id = 0x01, > + .product_code = 0x19, > + .scale = 115411765, // (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 As noted in Markuss' review /* */ for comments preferred for consistency reasons. > + }, > + [MC3510C] = { > + .name = "mc3510c", > + .chip_id = 0x23, > + .product_code = 0x10, > + .scale = 625000000, // Was obtained empirically > + }, > +}; We used to do this table thing a lot, but have over time come to conclusion it is much clearer just to have separate named structures and no enum. struct mxc3230_chip_info mx3230_chip_info = { }; struct mxc3230_chip_info mx3510c_chip_info = { }; etc > > #define MC3230_CHANNEL(reg, axis) { \ > .type = IIO_ACCEL, \ > @@ -50,6 +71,7 @@ static const int mc3230_nscale = 115411765; > struct mc3230_data { > struct i2c_client *client; > struct iio_mount_matrix orientation; > + const struct mc3xxx_chip_info *chip_info; > }; > > static const struct iio_mount_matrix * > @@ -111,7 +133,7 @@ static int mc3230_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > case IIO_CHAN_INFO_SCALE: > *val = 0; > - *val2 = mc3230_nscale; > + *val2 = data->chip_info->scale; > return IIO_VAL_INT_PLUS_NANO; > default: > return -EINVAL; > @@ -127,15 +149,23 @@ static int mc3230_probe(struct i2c_client *client) > int ret; > struct iio_dev *indio_dev; > struct mc3230_data *data; > + const struct mc3xxx_chip_info *chip_info; > > + chip_info = i2c_get_match_data(client); Whilst very unlikely to fail (it won't), usual convention is to check the chip_info is not NULL anyway. Maybe in future that function will gain paths that more likely to fail than today so good to be paranoid on this one. We aren't completely consistent on this, so some drivers may not check it. > /* First check chip-id and product-id */ > ret = i2c_smbus_read_byte_data(client, MC3230_REG_CHIP_ID); > - if (ret != MC3230_CHIP_ID) > + if (ret != chip_info->chip_id) { > + dev_err(&client->dev, > + "chip id check fail: 0x%x != 0x%x !\n", ret, chip_info->chip_id); dev_info() and do not fail on this. Also, indent the message to align below the & Hard matches against chip IDs break the concept of Device Tree fallback compatibles. If a new device is released that is backwards compatible with an older one we want the driver to work. It is fine to print a message thought to say we don't recognise it. > return (ret < 0) ? ret : -ENODEV; > + } > > ret = i2c_smbus_read_byte_data(client, MC3230_REG_PRODUCT_CODE); > - if (ret != MC3230_PRODUCT_CODE) > + if (ret != chip_info->product_code) { > + dev_err(&client->dev, > + "product code check fail: 0x%x != 0x%x !\n", ret, chip_info->product_code); > return (ret < 0) ? ret : -ENODEV; As above. > + }
diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c index 3cad6f2d7a2a79df38f90e5656763f6ed019a920..ebbb96c658d87a83007c7c3c7212ce9ebf039963 100644 --- a/drivers/iio/accel/mc3230.c +++ b/drivers/iio/accel/mc3230.c @@ -22,20 +22,41 @@ #define MC3230_MODE_OPCON_STANDBY 0x03 #define MC3230_REG_CHIP_ID 0x18 -#define MC3230_CHIP_ID 0x01 - #define MC3230_REG_PRODUCT_CODE 0x3b -#define MC3230_PRODUCT_CODE 0x19 /* * The accelerometer has one measurement range: * * -1.5g - +1.5g (8-bit, signed) * - * scale = (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 */ -static const int mc3230_nscale = 115411765; +enum mc3xxx_chips { + MC3230, + MC3510C, +}; + +struct mc3xxx_chip_info { + const char *name; + const u8 chip_id; + const u8 product_code; + const int scale; +}; + +static struct mc3xxx_chip_info mc3xxx_chip_info_tbl[] = { + [MC3230] = { + .name = "mc3230", + .chip_id = 0x01, + .product_code = 0x19, + .scale = 115411765, // (1.5 + 1.5) * 9.81 / (2^8 - 1) = 0.115411765 + }, + [MC3510C] = { + .name = "mc3510c", + .chip_id = 0x23, + .product_code = 0x10, + .scale = 625000000, // Was obtained empirically + }, +}; #define MC3230_CHANNEL(reg, axis) { \ .type = IIO_ACCEL, \ @@ -50,6 +71,7 @@ static const int mc3230_nscale = 115411765; struct mc3230_data { struct i2c_client *client; struct iio_mount_matrix orientation; + const struct mc3xxx_chip_info *chip_info; }; static const struct iio_mount_matrix * @@ -111,7 +133,7 @@ static int mc3230_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: *val = 0; - *val2 = mc3230_nscale; + *val2 = data->chip_info->scale; return IIO_VAL_INT_PLUS_NANO; default: return -EINVAL; @@ -127,15 +149,23 @@ static int mc3230_probe(struct i2c_client *client) int ret; struct iio_dev *indio_dev; struct mc3230_data *data; + const struct mc3xxx_chip_info *chip_info; + chip_info = i2c_get_match_data(client); /* First check chip-id and product-id */ ret = i2c_smbus_read_byte_data(client, MC3230_REG_CHIP_ID); - if (ret != MC3230_CHIP_ID) + if (ret != chip_info->chip_id) { + dev_err(&client->dev, + "chip id check fail: 0x%x != 0x%x !\n", ret, chip_info->chip_id); return (ret < 0) ? ret : -ENODEV; + } ret = i2c_smbus_read_byte_data(client, MC3230_REG_PRODUCT_CODE); - if (ret != MC3230_PRODUCT_CODE) + if (ret != chip_info->product_code) { + dev_err(&client->dev, + "product code check fail: 0x%x != 0x%x !\n", ret, chip_info->product_code); return (ret < 0) ? ret : -ENODEV; + } indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); if (!indio_dev) { @@ -145,10 +175,11 @@ static int mc3230_probe(struct i2c_client *client) data = iio_priv(indio_dev); data->client = client; + data->chip_info = chip_info; i2c_set_clientdata(client, indio_dev); indio_dev->info = &mc3230_info; - indio_dev->name = "mc3230"; + indio_dev->name = chip_info->name; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = mc3230_channels; indio_dev->num_channels = ARRAY_SIZE(mc3230_channels); @@ -200,13 +231,15 @@ static int mc3230_resume(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(mc3230_pm_ops, mc3230_suspend, mc3230_resume); static const struct i2c_device_id mc3230_i2c_id[] = { - { "mc3230" }, + { "mc3230", (kernel_ulong_t)&mc3xxx_chip_info_tbl[MC3230] }, + { "mc3510c", (kernel_ulong_t)&mc3xxx_chip_info_tbl[MC3510C] }, {} }; MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id); static const struct of_device_id mc3230_of_match[] = { - { .compatible = "mcube,mc3230" }, + { .compatible = "mcube,mc3230", &mc3xxx_chip_info_tbl[MC3230] }, + { .compatible = "mcube,mc3510c", &mc3xxx_chip_info_tbl[MC3510C] }, { }, }; MODULE_DEVICE_TABLE(of, mc3230_of_match);