[PATCH 0/4] iio: accel: mc3230: add mount matrix, of match and mc3510c support

[PATCH 0/4] iio: accel: mc3230: add mount matrix, of match and mc3510c support

[Vasiliy Doylov via B4 Relay]

Changes includes:
- Add mount matrix handling
- Add match table to work with DT
- Add MC3510C support

MC3510C use same registors as MC3230, but different value scale.

Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
---
Vasiliy Doylov (4):
      iio: accel: mc3230: add mount matrix support
      iio: accel: mc3230: add OF match table
      iio: accel: mc3230: add mc3510c support
      dt-bindings: iio: accel: mc3230: document mc3510c

 .../devicetree/bindings/trivial-devices.yaml       |  2 +
 drivers/iio/accel/mc3230.c                         | 88 ++++++++++++++++++----
 2 files changed, 76 insertions(+), 14 deletions(-)
---
base-commit: 2b88851f583d3c4e40bcd40cfe1965241ec229dd
change-id: 20250111-mainlining-mc3510c-564702fba487

Best regards,
-- 
Vasiliy Doylov <nekodevelopper@gmail.com>

[PATCH 1/4] iio: accel: mc3230: add mount matrix support

[Vasiliy Doylov via B4 Relay]

From: Vasiliy Doylov <nekodevelopper@gmail.com>

This patch allows to read a mount-matrix device tree
property and report to user-space or in-kernel iio
clients.

Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
---
 drivers/iio/accel/mc3230.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
index caa40a14a6316acae3a972f0ebe0b325db96eb44..48787c0494ae6f0ef1d4d22bc5a4608035cbe123 100644
--- a/drivers/iio/accel/mc3230.c
+++ b/drivers/iio/accel/mc3230.c
@@ -44,18 +44,34 @@ static const int mc3230_nscale = 115411765;
 	.channel2 = IIO_MOD_##axis,	\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.ext_info = mc3230_ext_info, \
 }
 
+struct mc3230_data {
+	struct i2c_client *client;
+	struct iio_mount_matrix orientation;
+};
+
+static const struct iio_mount_matrix *
+mc3230_get_mount_matrix(const struct iio_dev *indio_dev,
+			const struct iio_chan_spec *chan)
+{
+	struct mc3230_data *data = iio_priv(indio_dev);
+
+	return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info mc3230_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, mc3230_get_mount_matrix),
+	{ }
+};
+
 static const struct iio_chan_spec mc3230_channels[] = {
 	MC3230_CHANNEL(MC3230_REG_XOUT, X),
 	MC3230_CHANNEL(MC3230_REG_YOUT, Y),
 	MC3230_CHANNEL(MC3230_REG_ZOUT, Z),
 };
 
-struct mc3230_data {
-	struct i2c_client *client;
-};
-
 static int mc3230_set_opcon(struct mc3230_data *data, int opcon)
 {
 	int ret;
@@ -141,6 +157,10 @@ static int mc3230_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
+	if (ret)
+		return ret;
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "device_register failed\n");

-- 
2.47.1

[PATCH 2/4] iio: accel: mc3230: add OF match table

[Vasiliy Doylov via B4 Relay]

From: Vasiliy Doylov <nekodevelopper@gmail.com>

This will make the driver probe-able via device-tree.
While the I2C match table may be sufficient, this should extend the cover
of this driver being probed by other methods.

Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
---
 drivers/iio/accel/mc3230.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
index 48787c0494ae6f0ef1d4d22bc5a4608035cbe123..3cad6f2d7a2a79df38f90e5656763f6ed019a920 100644
--- a/drivers/iio/accel/mc3230.c
+++ b/drivers/iio/accel/mc3230.c
@@ -205,10 +205,17 @@ static const struct i2c_device_id mc3230_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id);
 
+static const struct of_device_id mc3230_of_match[] = {
+	{ .compatible = "mcube,mc3230" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mc3230_of_match);
+
 static struct i2c_driver mc3230_driver = {
 	.driver = {
 		.name = "mc3230",
 		.pm = pm_sleep_ptr(&mc3230_pm_ops),
+		.of_match_table = mc3230_of_match,
 	},
 	.probe		= mc3230_probe,
 	.remove		= mc3230_remove,

-- 
2.47.1

[PATCH 3/4] iio: accel: mc3230: add mc3510c support

[Vasiliy Doylov via B4 Relay]

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;
+};
+
+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);

-- 
2.47.1

[PATCH 4/4] dt-bindings: iio: accel: mc3230: document mc3510c

[Vasiliy Doylov via B4 Relay]

From: Vasiliy Doylov <nekodevelopper@gmail.com>

The MC3510C is a 3 asix digital accelerometer.
It handled by the same driver as MC3230.
Document it as a trivial device.

Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index fadbd3c041c8c39faedfe62874d4eba25a0bf30e..6c34e4c0dcc6df5a4d8edc5effb80980de820db9 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -187,6 +187,8 @@ properties:
           - maxim,max6621
             # mCube 3-axis 8-bit digital accelerometer
           - mcube,mc3230
+            # mCube 3-axis 8-bit digital accelerometer
+          - mcube,mc3510c
             # Measurement Specialities I2C temperature and humidity sensor
           - meas,htu21
             # Measurement Specialities I2C pressure and temperature sensor

-- 
2.47.1

Re: [PATCH 4/4] dt-bindings: iio: accel: mc3230: document mc3510c

[Markuss Broks]

Hey Vasiliy,

On 1/11/25 10:11 PM, Vasiliy Doylov via B4 Relay wrote:
> From: Vasiliy Doylov <nekodevelopper@gmail.com>
>
> The MC3510C is a 3 asix digital accelerometer.
> It handled by the same driver as MC3230.
> Document it as a trivial device.
>
> Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> ---
>   Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index fadbd3c041c8c39faedfe62874d4eba25a0bf30e..6c34e4c0dcc6df5a4d8edc5effb80980de820db9 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -187,6 +187,8 @@ properties:
>             - maxim,max6621
>               # mCube 3-axis 8-bit digital accelerometer
>             - mcube,mc3230
> +            # mCube 3-axis 8-bit digital accelerometer
> +          - mcube,mc3510c
>               # Measurement Specialities I2C temperature and humidity sensor
>             - meas,htu21
>               # Measurement Specialities I2C pressure and temperature sensor

DT bindings changes are supposed to be on top of the series for the ease 
of picking them by DT bindings maintainers.

- Markuss

Re: [PATCH 3/4] iio: accel: mc3230: add mc3510c support

[Markuss Broks]

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

Re: [PATCH 2/4] iio: accel: mc3230: add OF match table

[Markuss Broks]

On 1/11/25 10:11 PM, Vasiliy Doylov via B4 Relay wrote:
> From: Vasiliy Doylov <nekodevelopper@gmail.com>
>
> This will make the driver probe-able via device-tree.
> While the I2C match table may be sufficient, this should extend the cover
> of this driver being probed by other methods.
>
> Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> ---
>   drivers/iio/accel/mc3230.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
> index 48787c0494ae6f0ef1d4d22bc5a4608035cbe123..3cad6f2d7a2a79df38f90e5656763f6ed019a920 100644
> --- a/drivers/iio/accel/mc3230.c
> +++ b/drivers/iio/accel/mc3230.c
> @@ -205,10 +205,17 @@ static const struct i2c_device_id mc3230_i2c_id[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id);
>   
> +static const struct of_device_id mc3230_of_match[] = {
> +	{ .compatible = "mcube,mc3230" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mc3230_of_match);
> +
>   static struct i2c_driver mc3230_driver = {
>   	.driver = {
>   		.name = "mc3230",
>   		.pm = pm_sleep_ptr(&mc3230_pm_ops),
> +		.of_match_table = mc3230_of_match,
Should also be alphabetic over here.
>   	},
>   	.probe		= mc3230_probe,
>   	.remove		= mc3230_remove,

Re: [PATCH 2/4] iio: accel: mc3230: add OF match table

[Jonathan Cameron]

On Sun, 12 Jan 2025 01:07:44 +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 will make the driver probe-able via device-tree.
> > While the I2C match table may be sufficient, this should extend the cover
> > of this driver being probed by other methods.
> >
> > Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> > ---
> >   drivers/iio/accel/mc3230.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
> > index 48787c0494ae6f0ef1d4d22bc5a4608035cbe123..3cad6f2d7a2a79df38f90e5656763f6ed019a920 100644
> > --- a/drivers/iio/accel/mc3230.c
> > +++ b/drivers/iio/accel/mc3230.c
> > @@ -205,10 +205,17 @@ static const struct i2c_device_id mc3230_i2c_id[] = {
> >   };
> >   MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id);
> >   
> > +static const struct of_device_id mc3230_of_match[] = {
> > +	{ .compatible = "mcube,mc3230" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mc3230_of_match);
> > +
> >   static struct i2c_driver mc3230_driver = {
> >   	.driver = {
> >   		.name = "mc3230",
> >   		.pm = pm_sleep_ptr(&mc3230_pm_ops),
> > +		.of_match_table = mc3230_of_match,  
> Should also be alphabetic over here.
Why?

I'm on board with reordering this to be closer to the definitions
in struct device_driver, but alphabetic doesn't make much sense
in general for filling structures.

So I agree with reorder but not for that reason.

Jonathan

> >   	},
> >   	.probe		= mc3230_probe,
> >   	.remove		= mc3230_remove,  

Re: [PATCH 2/4] iio: accel: mc3230: add OF match table

[Jonathan Cameron]

On Sat, 11 Jan 2025 23:11:07 +0300
Vasiliy Doylov via B4 Relay <devnull+nekodevelopper.gmail.com@kernel.org> wrote:

> From: Vasiliy Doylov <nekodevelopper@gmail.com>
> 
> This will make the driver probe-able via device-tree.

Why is it not today?  I2C has fallbacks to match against
the compatible with the vendor prefix stripped off.
For module autoprobing it will fall back to the driver name.
So are you actually seeing a problem probing this in a DT
set up?

I'm not against the change, which I think reflects best practice
as there may be mc3230 devices from other vendors with different
intended drivers.

Jonathan
 

> While the I2C match table may be sufficient, this should extend the cover
> of this driver being probed by other methods.
> 
> Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> ---
>  drivers/iio/accel/mc3230.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
> index 48787c0494ae6f0ef1d4d22bc5a4608035cbe123..3cad6f2d7a2a79df38f90e5656763f6ed019a920 100644
> --- a/drivers/iio/accel/mc3230.c
> +++ b/drivers/iio/accel/mc3230.c
> @@ -205,10 +205,17 @@ static const struct i2c_device_id mc3230_i2c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, mc3230_i2c_id);
>  
> +static const struct of_device_id mc3230_of_match[] = {
> +	{ .compatible = "mcube,mc3230" },
> +	{ },
Better to have no comma after a terminating entry. We should
not make it easy for people to add stuff after this (as it
makes no sense if they do!)

Jonathan

> +};
> +MODULE_DEVICE_TABLE(of, mc3230_of_match);
> +
>  static struct i2c_driver mc3230_driver = {
>  	.driver = {
>  		.name = "mc3230",
>  		.pm = pm_sleep_ptr(&mc3230_pm_ops),
> +		.of_match_table = mc3230_of_match,
>  	},
>  	.probe		= mc3230_probe,
>  	.remove		= mc3230_remove,
> 

Re: [PATCH 1/4] iio: accel: mc3230: add mount matrix support

[Jonathan Cameron]

On Sat, 11 Jan 2025 23:11:06 +0300
Vasiliy Doylov via B4 Relay <devnull+nekodevelopper.gmail.com@kernel.org> wrote:

> From: Vasiliy Doylov <nekodevelopper@gmail.com>
> 
> This patch allows to read a mount-matrix device tree
> property and report to user-space or in-kernel iio
> clients.

Trivial, but wrap patch descriptions to 75 chars (slightly
shorter than patches because tooling tends to indent it a bit!)

Otherwise this looks good to me.

Jonathan


> 
> Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> ---
>  drivers/iio/accel/mc3230.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/mc3230.c b/drivers/iio/accel/mc3230.c
> index caa40a14a6316acae3a972f0ebe0b325db96eb44..48787c0494ae6f0ef1d4d22bc5a4608035cbe123 100644
> --- a/drivers/iio/accel/mc3230.c
> +++ b/drivers/iio/accel/mc3230.c
> @@ -44,18 +44,34 @@ static const int mc3230_nscale = 115411765;
>  	.channel2 = IIO_MOD_##axis,	\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.ext_info = mc3230_ext_info, \
>  }
>  
> +struct mc3230_data {
> +	struct i2c_client *client;
> +	struct iio_mount_matrix orientation;
> +};
> +
> +static const struct iio_mount_matrix *
> +mc3230_get_mount_matrix(const struct iio_dev *indio_dev,
> +			const struct iio_chan_spec *chan)
> +{
> +	struct mc3230_data *data = iio_priv(indio_dev);
> +
> +	return &data->orientation;
> +}
> +
> +static const struct iio_chan_spec_ext_info mc3230_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, mc3230_get_mount_matrix),
> +	{ }
> +};
> +
>  static const struct iio_chan_spec mc3230_channels[] = {
>  	MC3230_CHANNEL(MC3230_REG_XOUT, X),
>  	MC3230_CHANNEL(MC3230_REG_YOUT, Y),
>  	MC3230_CHANNEL(MC3230_REG_ZOUT, Z),
>  };
>  
> -struct mc3230_data {
> -	struct i2c_client *client;
> -};
> -
>  static int mc3230_set_opcon(struct mc3230_data *data, int opcon)
>  {
>  	int ret;
> @@ -141,6 +157,10 @@ static int mc3230_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
> +	if (ret)
> +		return ret;
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "device_register failed\n");
> 

Re: [PATCH 3/4] iio: accel: mc3230: add mc3510c support

[Jonathan Cameron]

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

Re: [PATCH 3/4] iio: accel: mc3230: add mc3510c support

[Jonathan Cameron]

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.

> +	}

Re: [PATCH 4/4] dt-bindings: iio: accel: mc3230: document mc3510c

[Jonathan Cameron]

On Sun, 12 Jan 2025 00:46:09 +0200
Markuss Broks <markuss.broks@gmail.com> wrote:

> Hey Vasiliy,
> 
> On 1/11/25 10:11 PM, Vasiliy Doylov via B4 Relay wrote:
> > From: Vasiliy Doylov <nekodevelopper@gmail.com>
> >
> > The MC3510C is a 3 asix digital accelerometer.
> > It handled by the same driver as MC3230.
> > Document it as a trivial device.
> >
> > Signed-off-by: Vasiliy Doylov <nekodevelopper@gmail.com>
> > ---
> >   Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index fadbd3c041c8c39faedfe62874d4eba25a0bf30e..6c34e4c0dcc6df5a4d8edc5effb80980de820db9 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -187,6 +187,8 @@ properties:
> >             - maxim,max6621
> >               # mCube 3-axis 8-bit digital accelerometer
> >             - mcube,mc3230
> > +            # mCube 3-axis 8-bit digital accelerometer
> > +          - mcube,mc3510c
> >               # Measurement Specialities I2C temperature and humidity sensor
> >             - meas,htu21
> >               # Measurement Specialities I2C pressure and temperature sensor  
> 
> DT bindings changes are supposed to be on top of the series for the ease 
> of picking them by DT bindings maintainers.
True they should be earlier, but not that the DT binding maintainers
pick them up.  They review them and then they almost always go with the
patch through the subsystem tree in question.

Jonathan

> 
> - Markuss
>