[PATCH v2 0/3] Add support for AK09918

[PATCH v2 0/3] Add support for AK09918

[Barnabás Czémán]

Add support for AK09918 which is register and scaling compatible with
AK09912.

It was tested in Xiaomi Redmi 5 Plus (vince).

magnetometer@c {
  compatible = "asahi-kasei,ak09918";
  reg = <0x0c>;
  vdd-supply = <&pm8953_l6>;
  mount-matrix = "1", "0", "0",
                 "0", "1", "0",
                 "0", "0", "1";
};

Add a fix for data reading according to datasheet [1] (9.4.3.2.)
ST2 register should be read out after read measurment data as third step.
ST2 read out is required and get correct value during measurment data
read.

[1] https://www.akm.com/content/dam/documents/products/electronic-compass/ak09918c/ak09918c-en-datasheet.pdf

Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
Changes in v2:
- Remove unnecessary ak09918 compatbile.
- Link to v1: https://lore.kernel.org/r/20240805-ak09918-v1-0-70837eebd7d8@mainlining.org

---
Barnabás Czémán (1):
      iio: magnetometer: ak8975: Fix reading for ak099xx sensors

Danila Tikhonov (2):
      dt-bindings: iio: imu: magnetometer: Add ak09118
      iio: magnetometer: ak8975: Add AK09118 support

 .../iio/magnetometer/asahi-kasei,ak8975.yaml       |  1 +
 drivers/iio/magnetometer/Kconfig                   |  2 +-
 drivers/iio/magnetometer/ak8975.c                  | 65 ++++++++++++++++------
 3 files changed, 51 insertions(+), 17 deletions(-)
---
base-commit: d6dbc9f56c3a70e915625b6f1887882c23dc5c91
change-id: 20240805-ak09918-4a6cfef91c32

Best regards,
-- 
Barnabás Czémán <barnabas.czeman@mainlining.org>

[PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Barnabás Czémán]

ST2 register read should be placed after read measurment data,
because it will get correct values after it.

Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
 drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index dd466c5fa621..925d76062b3e 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct ak8975_data *data,
 	if (ret < 0)
 		return ret;
 
-	/* This will be executed only for non-interrupt based waiting case */
-	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
-		ret = i2c_smbus_read_byte_data(client,
-					       data->def->ctrl_regs[ST2]);
-		if (ret < 0) {
-			dev_err(&client->dev, "Error in reading ST2\n");
-			return ret;
-		}
-		if (ret & (data->def->ctrl_masks[ST2_DERR] |
-			   data->def->ctrl_masks[ST2_HOFL])) {
-			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
+	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
 }
 
 /* Retrieve raw flux value for one of the x, y, or z axis.  */
@@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	ret = i2c_smbus_read_i2c_block_data_or_emulated(
 			client, def->data_regs[index],
 			sizeof(rval), (u8*)&rval);
+	ret = i2c_smbus_read_byte_data(client,
+				       data->def->ctrl_regs[ST2]);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error in reading ST2\n");
+		goto exit;
+	}
+
+	if (ret & (data->def->ctrl_masks[ST2_DERR] |
+		   data->def->ctrl_masks[ST2_HOFL])) {
+		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	if (ret < 0)
 		goto exit;
 

-- 
2.46.0

[PATCH v2 2/3] dt-bindings: iio: imu: magnetometer: Add ak09118

[Barnabás Czémán]

From: Danila Tikhonov <danila@jiaxyga.com>

Document asahi-kasei,ak09918 compatible.

Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
 .../devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml         | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
index 9790f75fc669..583cdd2fad7e 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
+++ b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
@@ -18,6 +18,7 @@ properties:
           - asahi-kasei,ak09911
           - asahi-kasei,ak09912
           - asahi-kasei,ak09916
+          - asahi-kasei,ak09918
       - enum:
           - ak8975
           - ak8963

-- 
2.46.0

[PATCH v2 3/3] iio: magnetometer: ak8975: Add AK09118 support

[Barnabás Czémán]

From: Danila Tikhonov <danila@jiaxyga.com>

Add additional AK09118 to the magnetometer driver which has the same
register mapping and scaling as the AK09112 device.

Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
 drivers/iio/magnetometer/Kconfig  |  2 +-
 drivers/iio/magnetometer/ak8975.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index cd2917d71904..8eb718f5e50f 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -39,7 +39,7 @@ config AK8975
 	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Asahi Kasei AK8975, AK8963,
-	  AK09911, AK09912 or AK09916 3-Axis Magnetometer.
+	  AK09911, AK09912, AK09916 or AK09918 3-Axis Magnetometer.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called ak8975.
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 925d76062b3e..9871fea67ae3 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -78,6 +78,7 @@
  */
 #define AK09912_REG_WIA1		0x00
 #define AK09912_REG_WIA2		0x01
+#define AK09918_DEVICE_ID		0x0C
 #define AK09916_DEVICE_ID		0x09
 #define AK09912_DEVICE_ID		0x04
 #define AK09911_DEVICE_ID		0x05
@@ -209,6 +210,7 @@ enum asahi_compass_chipset {
 	AK09911,
 	AK09912,
 	AK09916,
+	AK09918,
 };
 
 enum ak_ctrl_reg_addr {
@@ -371,6 +373,31 @@ static const struct ak_def ak_def_array[] = {
 			AK09912_REG_HXL,
 			AK09912_REG_HYL,
 			AK09912_REG_HZL},
+	},
+	[AK09918] = {
+		.type = AK09918,
+		.raw_to_gauss = ak09912_raw_to_gauss,
+		.range = 32752,
+		.ctrl_regs = {
+			AK09912_REG_ST1,
+			AK09912_REG_ST2,
+			AK09912_REG_CNTL2,
+			AK09912_REG_ASAX,
+			AK09912_MAX_REGS},
+		.ctrl_masks = {
+			AK09912_REG_ST1_DRDY_MASK,
+			AK09912_REG_ST2_HOFL_MASK,
+			0,
+			AK09912_REG_CNTL2_MODE_MASK},
+		.ctrl_modes = {
+			AK09912_REG_CNTL_MODE_POWER_DOWN,
+			AK09912_REG_CNTL_MODE_ONCE,
+			AK09912_REG_CNTL_MODE_SELF_TEST,
+			AK09912_REG_CNTL_MODE_FUSE_ROM},
+		.data_regs = {
+			AK09912_REG_HXL,
+			AK09912_REG_HYL,
+			AK09912_REG_HZL},
 	}
 };
 
@@ -452,6 +479,7 @@ static int ak8975_who_i_am(struct i2c_client *client,
 	/*
 	 * Signature for each device:
 	 * Device   |  WIA1      |  WIA2
+	 * AK09918  |  DEVICE_ID_|  AK09918_DEVICE_ID
 	 * AK09916  |  DEVICE_ID_|  AK09916_DEVICE_ID
 	 * AK09912  |  DEVICE_ID |  AK09912_DEVICE_ID
 	 * AK09911  |  DEVICE_ID |  AK09911_DEVICE_ID
@@ -484,6 +512,10 @@ static int ak8975_who_i_am(struct i2c_client *client,
 		if (wia_val[1] == AK09916_DEVICE_ID)
 			return 0;
 		break;
+	case AK09918:
+		if (wia_val[1] == AK09918_DEVICE_ID)
+			return 0;
+		break;
 	default:
 		dev_err(&client->dev, "Type %d unknown\n", type);
 	}
@@ -1066,6 +1098,7 @@ static const struct i2c_device_id ak8975_id[] = {
 	{"ak09911", (kernel_ulong_t)&ak_def_array[AK09911] },
 	{"ak09912", (kernel_ulong_t)&ak_def_array[AK09912] },
 	{"ak09916", (kernel_ulong_t)&ak_def_array[AK09916] },
+	{"ak09918", (kernel_ulong_t)&ak_def_array[AK09918] },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ak8975_id);
@@ -1081,6 +1114,7 @@ static const struct of_device_id ak8975_of_match[] = {
 	{ .compatible = "ak09912", .data = &ak_def_array[AK09912] },
 	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
 	{ .compatible = "ak09916", .data = &ak_def_array[AK09916] },
+	{ .compatible = "asahi-kasei,ak09918", .data = &ak_def_array[AK09918] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ak8975_of_match);

-- 
2.46.0

Re: [PATCH v2 2/3] dt-bindings: iio: imu: magnetometer: Add ak09118

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]

On Tue, Aug 06, 2024 at 08:10:19AM +0200, Barnabás Czémán wrote:
> From: Danila Tikhonov <danila@jiaxyga.com>
> 
> Document asahi-kasei,ak09918 compatible.

Please explain what makes this device incompatible with those already in
the binding and why a fallback is not suitable.

Thanks,
Conor.

> 
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
>  .../devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml         | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> index 9790f75fc669..583cdd2fad7e 100644
> --- a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> @@ -18,6 +18,7 @@ properties:
>            - asahi-kasei,ak09911
>            - asahi-kasei,ak09912
>            - asahi-kasei,ak09916
> +          - asahi-kasei,ak09918
>        - enum:
>            - ak8975
>            - ak8963
> 
> -- 
> 2.46.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] dt-bindings: iio: imu: magnetometer: Add ak09118

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

On Tue, Aug 06, 2024 at 05:00:44PM +0100, Conor Dooley wrote:
> On Tue, Aug 06, 2024 at 08:10:19AM +0200, Barnabás Czémán wrote:
> > From: Danila Tikhonov <danila@jiaxyga.com>
> > 
> > Document asahi-kasei,ak09918 compatible.
> 
> Please explain what makes this device incompatible with those already in
> the binding and why a fallback is not suitable.

From the driver patch:
| Add additional AK09118 to the magnetometer driver which has the same
| register mapping and scaling as the AK09112 device.

Why isnt a fallback suitable here?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Jonathan Cameron]

On Tue, 06 Aug 2024 08:10:18 +0200
Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:

Hi Barnabás,

Welcome to IIO.

> ST2 register read should be placed after read measurment data,
> because it will get correct values after it.

What is the user visible result of this? Do we detect errors when none
are there?  Do we have a datasheet reference for the status being
update on the read command, not after the trigger?
>
Needs a Fixes tag to let us know how far to backport the fix.

A few comments inline.
 
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
>  drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index dd466c5fa621..925d76062b3e 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct ak8975_data *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* This will be executed only for non-interrupt based waiting case */
> -	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
> -		ret = i2c_smbus_read_byte_data(client,
> -					       data->def->ctrl_regs[ST2]);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "Error in reading ST2\n");
> -			return ret;
> -		}
> -		if (ret & (data->def->ctrl_masks[ST2_DERR] |
> -			   data->def->ctrl_masks[ST2_HOFL])) {
> -			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
> -			return -EINVAL;
> -		}
> -	}
> -
This completely removes the check from the _fill_buffer() path

> -	return 0;
> +	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
returning a positive value here is unusual enough you should add a comment for
the function + use that return value.

>  }
>  
>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
> @@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>  	ret = i2c_smbus_read_i2c_block_data_or_emulated(
>  			client, def->data_regs[index],
>  			sizeof(rval), (u8*)&rval);
No longer gated on ret & data->def->ctrl_masks[ST1_DRDY] which seems unintentional.

Still need a check on ret here.

> +	ret = i2c_smbus_read_byte_data(client,
> +				       data->def->ctrl_regs[ST2]);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Error in reading ST2\n");
> +		goto exit;
> +	}
> +
> +	if (ret & (data->def->ctrl_masks[ST2_DERR] |
> +		   data->def->ctrl_masks[ST2_HOFL])) {
> +		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>  	if (ret < 0)
>  		goto exit;

And this one ends up redundant I think which suggests to me the
code is inserted a few lines early.

>  
> 

Re: [PATCH v2 3/3] iio: magnetometer: ak8975: Add AK09118 support

[Jonathan Cameron]

On Tue, 06 Aug 2024 08:10:20 +0200
Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:

> From: Danila Tikhonov <danila@jiaxyga.com>
> 
> Add additional AK09118 to the magnetometer driver which has the same
> register mapping and scaling as the AK09112 device.
> 
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>

It definitely seems like a fallback compatible is suitable, but we
will still want the more precise match in the driver so that we can
avoid printing an info message to say we saw an unexpected ID.

Comments on that inline.

> ---
>  drivers/iio/magnetometer/Kconfig  |  2 +-
>  drivers/iio/magnetometer/ak8975.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index cd2917d71904..8eb718f5e50f 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -39,7 +39,7 @@ config AK8975
>  	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Asahi Kasei AK8975, AK8963,
> -	  AK09911, AK09912 or AK09916 3-Axis Magnetometer.
> +	  AK09911, AK09912, AK09916 or AK09918 3-Axis Magnetometer.
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called ak8975.
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 925d76062b3e..9871fea67ae3 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -78,6 +78,7 @@
>   */
>  #define AK09912_REG_WIA1		0x00
>  #define AK09912_REG_WIA2		0x01
> +#define AK09918_DEVICE_ID		0x0C
>  #define AK09916_DEVICE_ID		0x09
>  #define AK09912_DEVICE_ID		0x04
>  #define AK09911_DEVICE_ID		0x05
> @@ -209,6 +210,7 @@ enum asahi_compass_chipset {
>  	AK09911,
>  	AK09912,
>  	AK09916,
> +	AK09918,
>  };
>  
>  enum ak_ctrl_reg_addr {
> @@ -371,6 +373,31 @@ static const struct ak_def ak_def_array[] = {
>  			AK09912_REG_HXL,
>  			AK09912_REG_HYL,
>  			AK09912_REG_HZL},
> +	},
> +	[AK09918] = {
> +		.type = AK09918,
> +		.raw_to_gauss = ak09912_raw_to_gauss,
> +		.range = 32752,
> +		.ctrl_regs = {
> +			AK09912_REG_ST1,
> +			AK09912_REG_ST2,
> +			AK09912_REG_CNTL2,
> +			AK09912_REG_ASAX,
> +			AK09912_MAX_REGS},
> +		.ctrl_masks = {
> +			AK09912_REG_ST1_DRDY_MASK,
> +			AK09912_REG_ST2_HOFL_MASK,
> +			0,
> +			AK09912_REG_CNTL2_MODE_MASK},
> +		.ctrl_modes = {
> +			AK09912_REG_CNTL_MODE_POWER_DOWN,
> +			AK09912_REG_CNTL_MODE_ONCE,
> +			AK09912_REG_CNTL_MODE_SELF_TEST,
> +			AK09912_REG_CNTL_MODE_FUSE_ROM},
> +		.data_regs = {
> +			AK09912_REG_HXL,
> +			AK09912_REG_HYL,
> +			AK09912_REG_HZL},
>  	}
>  };
>  
> @@ -452,6 +479,7 @@ static int ak8975_who_i_am(struct i2c_client *client,
>  	/*
>  	 * Signature for each device:
>  	 * Device   |  WIA1      |  WIA2
> +	 * AK09918  |  DEVICE_ID_|  AK09918_DEVICE_ID
>  	 * AK09916  |  DEVICE_ID_|  AK09916_DEVICE_ID
>  	 * AK09912  |  DEVICE_ID |  AK09912_DEVICE_ID
>  	 * AK09911  |  DEVICE_ID |  AK09911_DEVICE_ID
> @@ -484,6 +512,10 @@ static int ak8975_who_i_am(struct i2c_client *client,
>  		if (wia_val[1] == AK09916_DEVICE_ID)
>  			return 0;
>  		break;
> +	case AK09918:
> +		if (wia_val[1] == AK09918_DEVICE_ID)
> +			return 0;
> +		break;
Whilst you are changing this code, please make it accept an unknown ID
with at most a dev_info() print. We used to get this wrong in IIO, but
this flexibility in matching Who Am I values is necessary to enable
fallback compatibles in DT.

Those allow you to use a newer DT with an older driver. It will
use the fallback ID to probe, so will get the ak09912 who am I value
and fail to probe currently.  We want it to succeed if they are
completely compatible other than this ID.

>  	default:
>  		dev_err(&client->dev, "Type %d unknown\n", type);
>  	}
> @@ -1066,6 +1098,7 @@ static const struct i2c_device_id ak8975_id[] = {
>  	{"ak09911", (kernel_ulong_t)&ak_def_array[AK09911] },
>  	{"ak09912", (kernel_ulong_t)&ak_def_array[AK09912] },
>  	{"ak09916", (kernel_ulong_t)&ak_def_array[AK09916] },
> +	{"ak09918", (kernel_ulong_t)&ak_def_array[AK09918] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, ak8975_id);
> @@ -1081,6 +1114,7 @@ static const struct of_device_id ak8975_of_match[] = {
>  	{ .compatible = "ak09912", .data = &ak_def_array[AK09912] },
>  	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
>  	{ .compatible = "ak09916", .data = &ak_def_array[AK09916] },
> +	{ .compatible = "asahi-kasei,ak09918", .data = &ak_def_array[AK09918] },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ak8975_of_match);
> 

Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Barnabás Czémán]

On August 6, 2024 6:19:25 PM GMT+02:00, Jonathan Cameron <jic23@kernel.org> wrote:
>On Tue, 06 Aug 2024 08:10:18 +0200
>Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:
>
>Hi Barnabás,
>
>Welcome to IIO.
>
>> ST2 register read should be placed after read measurment data,
>> because it will get correct values after it.
>
>What is the user visible result of this? Do we detect errors when none
>are there?  Do we have a datasheet reference for the status being
>update on the read command, not after the trigger?

Second read will fail. In the datasheet ST2 comes after measurment data read. Here is some explanation from datasheet.

"When ST2 register is read, AK09918 judges that data reading is finished. Stored measurement data is
protected during data reading and data is not updated. By reading ST2 register, this protection is
released. It is required to read ST2 register after data reading."

So if ST2 is read before measurment it will stuck at protected mode.
>>
>Needs a Fixes tag to let us know how far to backport the fix.
I think it is broken since 09912 was added but i cannot verify i have only devices with 09918.
>
>A few comments inline.
> 
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>>  drivers/iio/magnetometer/ak8975.c | 31 +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
>> index dd466c5fa621..925d76062b3e 100644
>> --- a/drivers/iio/magnetometer/ak8975.c
>> +++ b/drivers/iio/magnetometer/ak8975.c
>> @@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct ak8975_data *data,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	/* This will be executed only for non-interrupt based waiting case */
>> -	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
>> -		ret = i2c_smbus_read_byte_data(client,
>> -					       data->def->ctrl_regs[ST2]);
>> -		if (ret < 0) {
>> -			dev_err(&client->dev, "Error in reading ST2\n");
>> -			return ret;
>> -		}
>> -		if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> -			   data->def->ctrl_masks[ST2_HOFL])) {
>> -			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> -			return -EINVAL;
>> -		}
>> -	}
>> -
>This completely removes the check from the _fill_buffer() path
>
>> -	return 0;
>> +	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
>returning a positive value here is unusual enough you should add a comment for
>the function + use that return value.
>
>>  }
>>  
>>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
>> @@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>>  	ret = i2c_smbus_read_i2c_block_data_or_emulated(
>>  			client, def->data_regs[index],
>>  			sizeof(rval), (u8*)&rval);
>No longer gated on ret & data->def->ctrl_masks[ST1_DRDY] which seems unintentional.
>
>Still need a check on ret here.
>
>> +	ret = i2c_smbus_read_byte_data(client,
>> +				       data->def->ctrl_regs[ST2]);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Error in reading ST2\n");
>> +		goto exit;
>> +	}
>> +
>> +	if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> +		   data->def->ctrl_masks[ST2_HOFL])) {
>> +		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>>  	if (ret < 0)
>>  		goto exit;
>
>And this one ends up redundant I think which suggests to me the
>code is inserted a few lines early.
>
>>  
>> 
>

Re: [PATCH v2 2/3] dt-bindings: iio: imu: magnetometer: Add ak09118

[Krzysztof Kozlowski]

On 06/08/2024 08:10, Barnabás Czémán wrote:
> From: Danila Tikhonov <danila@jiaxyga.com>
> 
> Document asahi-kasei,ak09918 compatible.
> 
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/3] iio: magnetometer: ak8975: Add AK09118 support

[Krzysztof Kozlowski]

On 06/08/2024 08:10, Barnabás Czémán wrote:
>  };
>  MODULE_DEVICE_TABLE(i2c, ak8975_id);
> @@ -1081,6 +1114,7 @@ static const struct of_device_id ak8975_of_match[] = {
>  	{ .compatible = "ak09912", .data = &ak_def_array[AK09912] },
>  	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
>  	{ .compatible = "ak09916", .data = &ak_def_array[AK09916] },
> +	{ .compatible = "asahi-kasei,ak09918", .data = &ak_def_array[AK09918] },

I think you might need to rebase on top of Jonathan's branches...

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[barnabas.czeman]

On 2024-08-06 18:19, Jonathan Cameron wrote:
> On Tue, 06 Aug 2024 08:10:18 +0200
> Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:
> 
> Hi Barnabás,
> 
> Welcome to IIO.
> 
>> ST2 register read should be placed after read measurment data,
>> because it will get correct values after it.
> 
> What is the user visible result of this? Do we detect errors when none
> are there?  Do we have a datasheet reference for the status being
> update on the read command, not after the trigger?
>> 
> Needs a Fixes tag to let us know how far to backport the fix.
> 
> A few comments inline.
> 
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>>  drivers/iio/magnetometer/ak8975.c | 31 
>> +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/iio/magnetometer/ak8975.c 
>> b/drivers/iio/magnetometer/ak8975.c
>> index dd466c5fa621..925d76062b3e 100644
>> --- a/drivers/iio/magnetometer/ak8975.c
>> +++ b/drivers/iio/magnetometer/ak8975.c
>> @@ -692,22 +692,7 @@ static int ak8975_start_read_axis(struct 
>> ak8975_data *data,
>>  	if (ret < 0)
>>  		return ret;
>> 
>> -	/* This will be executed only for non-interrupt based waiting case 
>> */
>> -	if (ret & data->def->ctrl_masks[ST1_DRDY]) {
>> -		ret = i2c_smbus_read_byte_data(client,
>> -					       data->def->ctrl_regs[ST2]);
>> -		if (ret < 0) {
>> -			dev_err(&client->dev, "Error in reading ST2\n");
>> -			return ret;
>> -		}
>> -		if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> -			   data->def->ctrl_masks[ST2_HOFL])) {
>> -			dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> -			return -EINVAL;
>> -		}
>> -	}
>> -
> This completely removes the check from the _fill_buffer() path
> 
>> -	return 0;
>> +	return !(ret & data->def->ctrl_masks[ST1_DRDY]);
> returning a positive value here is unusual enough you should add a 
> comment for
> the function + use that return value.
> 
>>  }
>> 
>>  /* Retrieve raw flux value for one of the x, y, or z axis.  */
>> @@ -731,6 +716,20 @@ static int ak8975_read_axis(struct iio_dev 
>> *indio_dev, int index, int *val)
>>  	ret = i2c_smbus_read_i2c_block_data_or_emulated(
>>  			client, def->data_regs[index],
>>  			sizeof(rval), (u8*)&rval);
> No longer gated on ret & data->def->ctrl_masks[ST1_DRDY] which seems 
> unintentional.
It is checked exactly before the measurement data read, it is the return 
value of ak8975_start_read_axis.
The read section should be ST1 -> measurement -> ST2, exactly the same 
can be found in the datasheets.
> 
> Still need a check on ret here.
> 
>> +	ret = i2c_smbus_read_byte_data(client,
>> +				       data->def->ctrl_regs[ST2]);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Error in reading ST2\n");
>> +		goto exit;
>> +	}
>> +
>> +	if (ret & (data->def->ctrl_masks[ST2_DERR] |
>> +		   data->def->ctrl_masks[ST2_HOFL])) {
>> +		dev_err(&client->dev, "ST2 status error 0x%x\n", ret);
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>>  	if (ret < 0)
>>  		goto exit;
> 
> And this one ends up redundant I think which suggests to me the
> code is inserted a few lines early.
> 
>> 
>> 

Re: [PATCH v2 1/3] iio: magnetometer: ak8975: Fix reading for ak099xx sensors

[Jonathan Cameron]

On Tue, 06 Aug 2024 19:54:56 +0200
Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:

> On August 6, 2024 6:19:25 PM GMT+02:00, Jonathan Cameron <jic23@kernel.org> wrote:
> >On Tue, 06 Aug 2024 08:10:18 +0200
> >Barnabás Czémán <barnabas.czeman@mainlining.org> wrote:
> >
> >Hi Barnabás,
> >
> >Welcome to IIO.
> >  
> >> ST2 register read should be placed after read measurment data,
> >> because it will get correct values after it.  
> >
> >What is the user visible result of this? Do we detect errors when none
> >are there?  Do we have a datasheet reference for the status being
> >update on the read command, not after the trigger?  
> 
> Second read will fail. In the datasheet ST2 comes after measurment data read. Here is some explanation from datasheet.
> 
> "When ST2 register is read, AK09918 judges that data reading is finished. Stored measurement data is
> protected during data reading and data is not updated. By reading ST2 register, this protection is
> released. It is required to read ST2 register after data reading."
> 
Thanks. Please add more of that detail to the patch description for v3.

> So if ST2 is read before measurment it will stuck at protected mode.
> >>  
> >Needs a Fixes tag to let us know how far to backport the fix.  
> I think it is broken since 09912 was added but i cannot verify i have only devices with 09918.
> >
I wasn't meaning devices, but rather what patch broke the kernel code.
It might be the original driver introduction.

If we can add a Fixes tag that makes it much easier for stable + distributions
to work out whether to pick the fix up or not.

Re: [PATCH v2 3/3] iio: magnetometer: ak8975: Add AK09118 support

[Barnabás Czémán]

On August 7, 2024 8:04:19 AM GMT+02:00, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>On 06/08/2024 08:10, Barnabás Czémán wrote:
>>  };
>>  MODULE_DEVICE_TABLE(i2c, ak8975_id);
>> @@ -1081,6 +1114,7 @@ static const struct of_device_id ak8975_of_match[] = {
>>  	{ .compatible = "ak09912", .data = &ak_def_array[AK09912] },
>>  	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
>>  	{ .compatible = "ak09916", .data = &ak_def_array[AK09916] },
>> +	{ .compatible = "asahi-kasei,ak09918", .data = &ak_def_array[AK09918] },
>
>I think you might need to rebase on top of Jonathan's branches...
>
Which barnch?
>Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
>Best regards,
>Krzysztof
>