[PATCH 0/4] Add support for AF8133J magnetometer

[PATCH 0/4] Add support for AF8133J magnetometer

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

This series adds support for AF8133J magnetometer sensor. It's a simple
3-axis sensor with two sensitivity options and not much else to it.

This sensor is used on both Pinephone and Pinephone Pro. DT patches
adding it will come later, once this driver is merged.

Please take a look. :)

Thank you very much,
	Ondřej Jirman

Icenowy Zheng (3):
  dt-bindings: vendor-prefix: Add prefix for Voltafield
  dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J
  iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

Ondrej Jirman (1):
  MAINTAINERS: Add an entry for AF8133J driver

 .../iio/magnetometer/voltafield,af8133j.yaml  |  58 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/magnetometer/Kconfig              |  12 +
 drivers/iio/magnetometer/Makefile             |   1 +
 drivers/iio/magnetometer/af8133j.c            | 525 ++++++++++++++++++
 6 files changed, 604 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
 create mode 100644 drivers/iio/magnetometer/af8133j.c

-- 
2.43.0

[PATCH 1/4] dt-bindings: vendor-prefix: Add prefix for Voltafield

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

Voltafile Technology Corp. is a company that produces MEMS sensors.

Add a DT vendor prefix for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Ondřej Jirman <megi@xff.cz>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1a0dc04f1db4..82e9f64c90ff 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1534,6 +1534,8 @@ patternProperties:
     description: VoCore Studio
   "^voipac,.*":
     description: Voipac Technologies s.r.o.
+  "^voltafield,.*":
+    description: Voltafield Technology Corp.
   "^vot,.*":
     description: Vision Optical Technology Co., Ltd.
   "^vxt,.*":
-- 
2.43.0

Re: [PATCH 1/4] dt-bindings: vendor-prefix: Add prefix for Voltafield

[Krzysztof Kozlowski]

On 11/02/2024 21:51, Ondřej Jirman wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
> 
> Voltafile Technology Corp. is a company that produces MEMS sensors.
> 
> Add a DT vendor prefix for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Ondřej Jirman <megi@xff.cz>
> ---

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

Best regards,
Krzysztof

[PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
Technology Corp, with dual power supplies (one for core and one for I/O)
and active-low reset pin.

The sensor has configurable range 1.2 - 2.2 mT and a software controlled
standby mode.

Add a device tree binding for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Ondřej Jirman <megi@xff.cz>
---
 .../iio/magnetometer/voltafield,af8133j.yaml  | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
new file mode 100644
index 000000000000..ab56c0f798ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Voltafield AF8133J magnetometer sensor
+
+maintainers:
+  - Ondřej Jirman <megi@xff.cz>
+
+properties:
+  compatible:
+    - voltafield,af8133j
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description: |
+      an pin needed for AF8133J to set the reset state. This should be usually
+      active low.
+
+  avdd-supply:
+    description: |
+      an optional regulator that needs to be on to provide AVDD power (Working
+      power, usually 3.3V) to the sensor.
+
+  dvdd-supply:
+    description: |
+      an optional regulator that needs to be on to provide DVDD power (Digital
+      IO power, 1.8V~AVDD) to the sensor.
+
+  mount-matrix:
+    description: an optional 3x3 mounting rotation matrix.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@1c {
+            compatible = "voltafield,af8133j";
+            reg = <0x1c>;
+            avdd-supply = <&reg_dldo1>;
+            dvdd-supply = <&reg_dldo1>;
+            reset-gpios = <&pio 1 1 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.43.0

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Rob Herring]

On Sun, 11 Feb 2024 21:51:58 +0100, Ondřej Jirman wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
> 
> Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
> Technology Corp, with dual power supplies (one for core and one for I/O)
> and active-low reset pin.
> 
> The sensor has configurable range 1.2 - 2.2 mT and a software controlled
> standby mode.
> 
> Add a device tree binding for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Ondřej Jirman <megi@xff.cz>
> ---
>  .../iio/magnetometer/voltafield,af8133j.yaml  | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: properties:compatible: ['voltafield,af8133j'] is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: properties:compatible: ['voltafield,af8133j'] is not of type 'object', 'boolean'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: ignoring, error in schema: properties: compatible
Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.example.dtb: /example-0/i2c/magnetometer@1c: failed to match any schema with compatible: ['voltafield,af8133j']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240211205211.2890931-3-megi@xff.cz

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Jonathan Cameron]

On Sun, 11 Feb 2024 21:51:58 +0100
Ondřej Jirman <megi@xff.cz> wrote:

> From: Icenowy Zheng <icenowy@aosc.io>

Title doesn't need to mention binding (it's implicit from the dt-bindings bit
dt-bindings: iio: magnetometer: Add Voltafield AF8133J

> 
> Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
> Technology Corp, with dual power supplies (one for core and one for I/O)
> and active-low reset pin.
> 
> The sensor has configurable range 1.2 - 2.2 mT and a software controlled
> standby mode.
> 
> Add a device tree binding for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Ondřej Jirman <megi@xff.cz>

Hi Ondřej

A few quick comments.


> ---
>  .../iio/magnetometer/voltafield,af8133j.yaml  | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> new file mode 100644
> index 000000000000..ab56c0f798ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Voltafield AF8133J magnetometer sensor
> +
> +maintainers:
> +  - Ondřej Jirman <megi@xff.cz>
> +
> +properties:
> +  compatible:
> +    - voltafield,af8133j
Test your bindings (as described in the bot message).
    const: voltafield,af8133j

> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: |
No need for the | as the formatting doesn't need to be preserved.

> +      an pin needed for AF8133J to set the reset state. This should be usually

A pin

> +      active low.
> +
> +  avdd-supply:
> +    description: |
> +      an optional regulator that needs to be on to provide AVDD power (Working

An optional (if it were optional, A regulator if not)

> +      power, usually 3.3V) to the sensor.
Seems unlikely the power supply is optional (though specifying it in DT might be -
however see below).

> +
> +  dvdd-supply:
> +    description: |
> +      an optional regulator that needs to be on to provide DVDD power (Digital
> +      IO power, 1.8V~AVDD) to the sensor.
> +
> +  mount-matrix:
> +    description: an optional 3x3 mounting rotation matrix.
> +
> +required:
> +  - compatible
> +  - reg

Any power supply that is required for operation should be listed here (even though
we can rely on the stub supplies if it isn't in the DT).
I used to think this wasn't necessary, so lots of bindings upstream don't yet
have it.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        magnetometer@1c {
> +            compatible = "voltafield,af8133j";
> +            reg = <0x1c>;
> +            avdd-supply = <&reg_dldo1>;
> +            dvdd-supply = <&reg_dldo1>;
> +            reset-gpios = <&pio 1 1 GPIO_ACTIVE_LOW>;
> +        };
> +    };

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Ondřej Jirman]

Hi Jonathan,

thanks for the feedback.

On Mon, Feb 12, 2024 at 11:47:38AM +0000, Jonathan Cameron wrote:
> On Sun, 11 Feb 2024 21:51:58 +0100
> Ondřej Jirman <megi@xff.cz> wrote:
> 
> > From: Icenowy Zheng <icenowy@aosc.io>
> 
> Title doesn't need to mention binding (it's implicit from the dt-bindings bit
> dt-bindings: iio: magnetometer: Add Voltafield AF8133J
> 
> > 
> > Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
> > Technology Corp, with dual power supplies (one for core and one for I/O)
> > and active-low reset pin.
> > 
> > The sensor has configurable range 1.2 - 2.2 mT and a software controlled
> > standby mode.
> > 
> > Add a device tree binding for it.
> > 
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > Signed-off-by: Ondřej Jirman <megi@xff.cz>
> 
> Hi Ondřej
> 
> A few quick comments.
> 
> 
> > ---
> >  .../iio/magnetometer/voltafield,af8133j.yaml  | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> > new file mode 100644
> > index 000000000000..ab56c0f798ad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Voltafield AF8133J magnetometer sensor
> > +
> > +maintainers:
> > +  - Ondřej Jirman <megi@xff.cz>
> > +
> > +properties:
> > +  compatible:
> > +    - voltafield,af8133j
> Test your bindings (as described in the bot message).
>     const: voltafield,af8133j

I did, but didn't notice that DT_SCHEMA_FILES= didn't work as expected when
provided with full path to the bindings file. :)

Just using DT_SCHEMA_FILES=voltafield,af8133j.yaml works better and catches this
issue.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: |
> No need for the | as the formatting doesn't need to be preserved.
> 
> > +      an pin needed for AF8133J to set the reset state. This should be usually
> 
> A pin
> 
> > +      active low.
> > +
> > +  avdd-supply:
> > +    description: |
> > +      an optional regulator that needs to be on to provide AVDD power (Working
> 
> An optional (if it were optional, A regulator if not)
> 
> > +      power, usually 3.3V) to the sensor.
> Seems unlikely the power supply is optional (though specifying it in DT might be -
> however see below).
> 
> > +
> > +  dvdd-supply:
> > +    description: |
> > +      an optional regulator that needs to be on to provide DVDD power (Digital
> > +      IO power, 1.8V~AVDD) to the sensor.
> > +
> > +  mount-matrix:
> > +    description: an optional 3x3 mounting rotation matrix.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> Any power supply that is required for operation should be listed here (even though
> we can rely on the stub supplies if it isn't in the DT).
> I used to think this wasn't necessary, so lots of bindings upstream don't yet
> have it.

By stub supply you mean dummy supply created when the *-supply property is not
specified in DT? Or something else?

Because DTC_CHK prints a warning if I make the proerty required in bindings and
not specify it in DT

arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2b.dtb: magnetometer@1c: 'avdd-supply' is a required property
	from schema $id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#


kind regards,
	o.

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        magnetometer@1c {
> > +            compatible = "voltafield,af8133j";
> > +            reg = <0x1c>;
> > +            avdd-supply = <&reg_dldo1>;
> > +            dvdd-supply = <&reg_dldo1>;
> > +            reset-gpios = <&pio 1 1 GPIO_ACTIVE_LOW>;
> > +        };
> > +    };
> 

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Jonathan Cameron]

> > > +
> > > +  dvdd-supply:
> > > +    description: |
> > > +      an optional regulator that needs to be on to provide DVDD power (Digital
> > > +      IO power, 1.8V~AVDD) to the sensor.
> > > +
> > > +  mount-matrix:
> > > +    description: an optional 3x3 mounting rotation matrix.
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg  
> > 
> > Any power supply that is required for operation should be listed here (even though
> > we can rely on the stub supplies if it isn't in the DT).
> > I used to think this wasn't necessary, so lots of bindings upstream don't yet
> > have it.  
> 
> By stub supply you mean dummy supply created when the *-supply property is not
> specified in DT? Or something else?

Ah yes. I got the term wrong.
> 
> Because DTC_CHK prints a warning if I make the proerty required in bindings and
> not specify it in DT
> 
> arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2b.dtb: magnetometer@1c: 'avdd-supply' is a required property
> 	from schema $id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#

Provide one, or don't worry about that warning. 

Various discussions have taken place on this over time and end
result is bindings should require them to differentiate from power
supplies that are actually optional.

Jonathan

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Conor Dooley]

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

On Wed, Feb 14, 2024 at 04:31:16PM +0000, Jonathan Cameron wrote:
> 
> > > > +
> > > > +  dvdd-supply:
> > > > +    description: |
> > > > +      an optional regulator that needs to be on to provide DVDD power (Digital
> > > > +      IO power, 1.8V~AVDD) to the sensor.
> > > > +
> > > > +  mount-matrix:
> > > > +    description: an optional 3x3 mounting rotation matrix.
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg  
> > > 
> > > Any power supply that is required for operation should be listed here (even though
> > > we can rely on the stub supplies if it isn't in the DT).
> > > I used to think this wasn't necessary, so lots of bindings upstream don't yet
> > > have it.  
> > 
> > By stub supply you mean dummy supply created when the *-supply property is not
> > specified in DT? Or something else?
> 
> Ah yes. I got the term wrong.
> > 
> > Because DTC_CHK prints a warning if I make the proerty required in bindings and
> > not specify it in DT
> > 
> > arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2b.dtb: magnetometer@1c: 'avdd-supply' is a required property
> > 	from schema $id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
> 
> Provide one, or don't worry about that warning. 

For the sake of the platform maintainer, please choose option 1.

Thanks,
Conor.

> Various discussions have taken place on this over time and end
> result is bindings should require them to differentiate from power
> supplies that are actually optional.
> 
> Jonathan
> 
> 

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

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[Ondřej Jirman]

On Wed, Feb 14, 2024 at 04:31:16PM +0000, Jonathan Cameron wrote:
> 
> > > > +
> > > > +  dvdd-supply:
> > > > +    description: |
> > > > +      an optional regulator that needs to be on to provide DVDD power (Digital
> > > > +      IO power, 1.8V~AVDD) to the sensor.
> > > > +
> > > > +  mount-matrix:
> > > > +    description: an optional 3x3 mounting rotation matrix.
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg  
> > > 
> > > Any power supply that is required for operation should be listed here (even though
> > > we can rely on the stub supplies if it isn't in the DT).
> > > I used to think this wasn't necessary, so lots of bindings upstream don't yet
> > > have it.  
> > 
> > By stub supply you mean dummy supply created when the *-supply property is not
> > specified in DT? Or something else?
> 
> Ah yes. I got the term wrong.
> > 
> > Because DTC_CHK prints a warning if I make the proerty required in bindings and
> > not specify it in DT
> > 
> > arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2b.dtb: magnetometer@1c: 'avdd-supply' is a required property
> > 	from schema $id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
> 
> Provide one, or don't worry about that warning. 
> 
> Various discussions have taken place on this over time and end
> result is bindings should require them to differentiate from power
> supplies that are actually optional.

Ok. Good to know. :)

regards,
	o.

> Jonathan
> 
> 

Re: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J

[kernel test robot]

Hi Ondřej,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.8-rc4 next-20240212]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ond-ej-Jirman/dt-bindings-vendor-prefix-Add-prefix-for-Voltafield/20240212-045510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240211205211.2890931-3-megi%40xff.cz
patch subject: [PATCH 2/4] dt-bindings: iio: magnetometer: Add DT binding for Voltafield AF8133J
:::::: branch date: 11 hours ago
:::::: commit date: 11 hours ago
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240212/202402121531.EoXy0HWe-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202402121531.EoXy0HWe-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: properties:compatible: ['voltafield,af8133j'] is not of type 'object', 'boolean'
   	from schema $id: http://json-schema.org/draft-07/schema#
>> Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: properties:compatible: ['voltafield,af8133j'] is not of type 'object', 'boolean'
   	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
--
>> Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml: ignoring, error in schema: properties: compatible

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/4] MAINTAINERS: Add an entry for AF8133J driver

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

As I am submitting the driver and have the device to test. I'll maintain
the driver.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dc5ca7a042b5..cc691f61a77e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -579,6 +579,12 @@ F:	drivers/iio/accel/adxl372.c
 F:	drivers/iio/accel/adxl372_i2c.c
 F:	drivers/iio/accel/adxl372_spi.c
 
+AF8133J THREE-AXIS MAGNETOMETER DRIVER
+M:	Ondřej Jirman <megi@xff.cz>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
+F:	drivers/iio/magnetometer/af8133j.c
+
 AF9013 MEDIA DRIVER
 L:	linux-media@vger.kernel.org
 S:	Orphan
-- 
2.43.0

Re: [PATCH 3/4] MAINTAINERS: Add an entry for AF8133J driver

[Krzysztof Kozlowski]

On 11/02/2024 21:51, Ondřej Jirman wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> As I am submitting the driver and have the device to test. I'll maintain
> the driver.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc5ca7a042b5..cc691f61a77e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -579,6 +579,12 @@ F:	drivers/iio/accel/adxl372.c
>  F:	drivers/iio/accel/adxl372_i2c.c
>  F:	drivers/iio/accel/adxl372_spi.c
>  
> +AF8133J THREE-AXIS MAGNETOMETER DRIVER
> +M:	Ondřej Jirman <megi@xff.cz>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
> +F:	drivers/iio/magnetometer/af8133j.c

Your patchset is not correctly ordered. There is no such file here.

Best regards,
Krzysztof

[PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

AF8133J is a simple I2C-connected magnetometer, without interrupts.

Add a simple IIO driver for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Dalton Durst <dalton@ubports.com>
Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/iio/magnetometer/Kconfig   |  12 +
 drivers/iio/magnetometer/Makefile  |   1 +
 drivers/iio/magnetometer/af8133j.c | 525 +++++++++++++++++++++++++++++
 3 files changed, 538 insertions(+)
 create mode 100644 drivers/iio/magnetometer/af8133j.c

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 38532d840f2a..cd2917d71904 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -6,6 +6,18 @@
 
 menu "Magnetometer sensors"
 
+config AF8133J
+	tristate "Voltafield AF8133J 3-Axis Magnetometer"
+	depends on I2C
+	depends on OF
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for Voltafield AF8133J I2C-based
+	  3-axis magnetometer chip.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called af8133j.
+
 config AK8974
 	tristate "Asahi Kasei AK8974 3-Axis Magnetometer"
 	depends on I2C
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index b1c784ea71c8..ec5c46fbf999 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AF8133J)	+= af8133j.o
 obj-$(CONFIG_AK8974)	+= ak8974.o
 obj-$(CONFIG_AK8975)	+= ak8975.o
 obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
new file mode 100644
index 000000000000..0b03417ba134
--- /dev/null
+++ b/drivers/iio/magnetometer/af8133j.c
@@ -0,0 +1,525 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * af8133j.c - Voltafield AF8133J magnetometer driver
+ *
+ * Copyright 2021 Icenowy Zheng <icenowy@aosc.io>
+ * Copyright 2024 Ondřej Jirman <megi@xff.cz>
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AF8133J_DRV_NAME "af8133j"
+
+#define AF8133J_REG_OUT		0x03
+#define AF8133J_REG_PCODE	0x00
+#define AF8133J_REG_PCODE_VAL	0x5e
+#define AF8133J_REG_STATUS	0x02
+#define AF8133J_REG_STATUS_ACQ	BIT(0)
+#define AF8133J_REG_STATE	0x0a
+#define AF8133J_REG_STATE_STBY	0x00
+#define AF8133J_REG_STATE_WORK	0x01
+#define AF8133J_REG_RANGE	0x0b
+#define AF8133J_REG_RANGE_22G	0x12
+#define AF8133J_REG_RANGE_12G	0x34
+#define AF8133J_REG_SWR		0x11
+#define AF8133J_REG_SWR_PERFORM	0x81
+
+static const char * const af8133j_supply_names[] = {
+	"avdd",
+	"dvdd",
+};
+
+#define AF8133J_NUM_SUPPLIES ARRAY_SIZE(af8133j_supply_names)
+
+struct af8133j_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct iio_mount_matrix orientation;
+
+	struct gpio_desc *reset_gpiod;
+	struct regulator_bulk_data supplies[AF8133J_NUM_SUPPLIES];
+
+	bool powered;
+	u8 range;
+};
+
+enum af8133j_axis {
+	AXIS_X = 0,
+	AXIS_Y,
+	AXIS_Z,
+};
+
+static struct iio_mount_matrix *
+af8133j_get_mount_matrix(struct iio_dev *indio_dev,
+			 const struct iio_chan_spec *chan)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info af8133j_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, af8133j_get_mount_matrix),
+	{ }
+};
+
+#define AF8133J_CHANNEL(_si, _axis) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_ ## _axis, \
+	.address = AXIS_ ## _axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
+	.ext_info = af8133j_ext_info, \
+	.scan_index = _si, \
+	.scan_type = { \
+		.sign = 's', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+static const struct iio_chan_spec af8133j_channels[] = {
+	AF8133J_CHANNEL(0, X),
+	AF8133J_CHANNEL(1, Y),
+	AF8133J_CHANNEL(2, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static int af8133j_product_check(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
+	if (ret < 0) {
+		dev_err(dev, "Error reading product code (%d)\n", ret);
+		return ret;
+	}
+
+	if (val != AF8133J_REG_PCODE_VAL) {
+		dev_err(dev, "Invalid product code (0x%02x)\n", val);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int af8133j_reset(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	if (data->reset_gpiod) {
+		/* If we have GPIO reset line, use it */
+		gpiod_set_value_cansleep(data->reset_gpiod, 1);
+		udelay(10);
+		gpiod_set_value_cansleep(data->reset_gpiod, 0);
+	} else {
+		/* Otherwise use software reset */
+		ret = regmap_write(data->regmap, AF8133J_REG_SWR,
+				   AF8133J_REG_SWR_PERFORM);
+		if (ret < 0) {
+			dev_err(dev, "Failed to reset the chip\n");
+			return ret;
+		}
+	}
+
+	/* Wait for reset to finish */
+	usleep_range(1000, 1100);
+
+	/* Restore range setting */
+	if (data->range == AF8133J_REG_RANGE_22G) {
+		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int af8133j_power_up(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	if (data->powered)
+		return 0;
+
+	ret = regulator_bulk_enable(AF8133J_NUM_SUPPLIES, data->supplies);
+	if (ret) {
+		dev_err(dev, "Could not enable regulators\n");
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(data->reset_gpiod, 0);
+
+	/* Wait for power on reset */
+	usleep_range(15000, 16000);
+
+	data->powered = true;
+	return 0;
+}
+
+static void af8133j_power_down(struct af8133j_data *data)
+{
+	if (!data->powered)
+		return;
+
+	gpiod_set_value_cansleep(data->reset_gpiod, 1);
+	regulator_bulk_disable(AF8133J_NUM_SUPPLIES, data->supplies);
+	data->powered = false;
+}
+
+static int af8133j_take_measurement(struct af8133j_data *data)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_write(data->regmap,
+			   AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
+	if (ret < 0)
+		return ret;
+
+	/* The datasheet says "Mesaure Time <1.5ms" */
+	ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
+				       val & AF8133J_REG_STATUS_ACQ,
+				       500, 1500);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(data->regmap,
+			   AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, "failed to power on\n");
+		return ret;
+	}
+
+	mutex_lock(&data->mutex);
+
+	ret = af8133j_take_measurement(data);
+	if (ret == 0)
+		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
+				       buf, sizeof(__le16) * 3);
+
+	mutex_unlock(&data->mutex);
+
+	pm_runtime_mark_last_busy(dev);
+	if (pm_runtime_put_autosuspend(dev))
+		dev_err(dev, "failed to power off\n");
+
+	return ret;
+}
+
+static const int af8133j_scales[][2] = {
+	[0] = { 0, 366210 }, // 12 gauss
+	[1] = { 0, 671386 }, // 22 gauss
+};
+
+static int af8133j_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+	__le16 buf[3];
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = af8133j_read_measurement(data, buf);
+		if (ret < 0)
+			return ret;
+
+		*val = sign_extend32(le16_to_cpu(buf[chan->address]),
+				     chan->scan_type.realbits - 1);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = af8133j_scales[data->range == AF8133J_REG_RANGE_12G ? 0 : 1][1];
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)af8133j_scales;
+		*length = ARRAY_SIZE(af8133j_scales) * 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_set_scale(struct af8133j_data *data,
+			     unsigned int val, unsigned int val2)
+{
+	u8 range;
+	int ret;
+
+	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
+		range = AF8133J_REG_RANGE_12G;
+	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
+		range = AF8133J_REG_RANGE_22G;
+	else
+		return -EINVAL;
+
+	ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
+	if (ret == 0)
+		data->range = range;
+
+	return ret;
+}
+
+static int af8133j_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		mutex_lock(&data->mutex);
+		ret = af8133j_set_scale(data, val, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const struct iio_info af8133j_info = {
+	.read_raw = af8133j_read_raw,
+	.read_avail = af8133j_read_avail,
+	.write_raw = af8133j_write_raw,
+	.write_raw_get_fmt = af8133j_write_raw_get_fmt,
+};
+
+static irqreturn_t af8133j_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct af8133j_data *data = iio_priv(indio_dev);
+	s64 timestamp = iio_get_time_ns(indio_dev);
+	struct {
+		__le16 values[3];
+		s64 timestamp __aligned(8);
+	} sample;
+	int ret;
+
+	memset(&sample, 0, sizeof(sample));
+
+	ret = af8133j_read_measurement(data, sample.values);
+	if (ret == 0)
+		iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct regmap_config af8133j_regmap_config = {
+	.name = "af8133j_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AF8133J_REG_SWR,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int af8133j_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct af8133j_data *data;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int ret, i;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap initialization failed\n");
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->regmap = regmap;
+	data->range = AF8133J_REG_RANGE_12G;
+	mutex_init(&data->mutex);
+
+	data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpiod))
+		return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
+				     "Failed to get reset gpio\n");
+
+	for (i = 0; i < AF8133J_NUM_SUPPLIES; i++)
+		data->supplies[i].supply = af8133j_supply_names[i];
+	ret = devm_regulator_bulk_get(dev, AF8133J_NUM_SUPPLIES, data->supplies);
+	if (ret)
+		return ret;
+
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
+
+	ret = af8133j_power_up(data);
+	if (ret)
+		return ret;
+
+	ret = af8133j_reset(data);
+	if (ret) {
+		af8133j_power_down(data);
+		return ret;
+	}
+
+	ret = af8133j_product_check(data);
+	if (ret) {
+		af8133j_power_down(data);
+		return ret;
+	}
+
+	af8133j_power_down(data);
+
+	indio_dev->info = &af8133j_info;
+	indio_dev->name = AF8133J_DRV_NAME;
+	indio_dev->channels = af8133j_channels;
+	indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      &af8133j_trigger_handler, NULL);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to setup iio triggered buffer\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register iio device");
+
+	pm_runtime_set_autosuspend_delay(dev, 500);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static void af8133j_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct af8133j_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
+	af8133j_power_down(data);
+}
+
+static int __maybe_unused af8133j_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	af8133j_power_down(data);
+
+	return 0;
+}
+
+static int __maybe_unused af8133j_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct af8133j_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = af8133j_power_up(data);
+	if (ret)
+		return ret;
+
+	ret = af8133j_reset(data);
+	if (ret) {
+		af8133j_power_down(data);
+		return ret;
+	}
+
+	return 0;
+}
+
+const struct dev_pm_ops af8133j_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
+};
+
+static const struct of_device_id af8133j_of_match[] = {
+	{ .compatible = "voltafield,af8133j", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, af8133j_of_match);
+
+static const struct i2c_device_id af8133j_id[] = {
+	{ "af8133j", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, af8133j_id);
+
+static struct i2c_driver af8133j_driver = {
+	.driver = {
+		.name = AF8133J_DRV_NAME,
+		.of_match_table = af8133j_of_match,
+		.pm = &af8133j_pm_ops,
+	},
+	.probe = af8133j_probe,
+	.remove = af8133j_remove,
+	.id_table = af8133j_id,
+};
+
+module_i2c_driver(af8133j_driver);
+
+MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
+MODULE_AUTHOR("Ondřej Jirman <megi@xff.cz>");
+MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[kernel test robot]

Hi Ondřej,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.8-rc4 next-20240212]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ond-ej-Jirman/dt-bindings-vendor-prefix-Add-prefix-for-Voltafield/20240212-045510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240211205211.2890931-5-megi%40xff.cz
patch subject: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer
:::::: branch date: 13 hours ago
:::::: commit date: 13 hours ago
config: openrisc-randconfig-r133-20240212 (https://download.01.org/0day-ci/archive/20240212/202402121800.Dk09l1F8-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240212/202402121800.Dk09l1F8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202402121800.Dk09l1F8-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/magnetometer/af8133j.c:492:25: sparse: sparse: symbol 'af8133j_pm_ops' was not declared. Should it be static?

vim +/af8133j_pm_ops +492 drivers/iio/magnetometer/af8133j.c

5f47c80d82e489 Icenowy Zheng 2024-02-11  491  
5f47c80d82e489 Icenowy Zheng 2024-02-11 @492  const struct dev_pm_ops af8133j_pm_ops = {
5f47c80d82e489 Icenowy Zheng 2024-02-11  493  	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
5f47c80d82e489 Icenowy Zheng 2024-02-11  494  	SET_RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
5f47c80d82e489 Icenowy Zheng 2024-02-11  495  };
5f47c80d82e489 Icenowy Zheng 2024-02-11  496  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Jonathan Cameron]

On Sun, 11 Feb 2024 21:52:00 +0100
Ondřej Jirman <megi@xff.cz> wrote:

> From: Icenowy Zheng <icenowy@aosc.io>
> 
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
> 
> Add a simple IIO driver for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Dalton Durst <dalton@ubports.com>
> Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>

This is a lot of sign offs.  If accurate it menas.

Icenowy wrote teh driver,
Dalton then 'handled' it on the path to Shoji who
then 'handled' it on the path to Ondrej.

That's possible if it's been in various other trees for instance, but
I'd like some more explanation below the --- if that is the case.
Otherwise, maybe Co-developed-by: is appropriate for some of
the above list?

Various comments inline, but looks pretty good in general.

Thanks,

Jonathan

> diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> new file mode 100644
> index 000000000000..0b03417ba134
> --- /dev/null
> +++ b/drivers/iio/magnetometer/af8133j.c
> @@ -0,0 +1,525 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * af8133j.c - Voltafield AF8133J magnetometer driver
> + *
> + * Copyright 2021 Icenowy Zheng <icenowy@aosc.io>
> + * Copyright 2024 Ondřej Jirman <megi@xff.cz>
> + */
> +
> +#include <linux/module.h>

Alphabetical order for these.
It's fine to separate out he IIO ones on their own as you have
done.

> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Don't think you are using this as no custom attrs.


> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AF8133J_DRV_NAME "af8133j"
> +
> +#define AF8133J_REG_OUT		0x03
> +#define AF8133J_REG_PCODE	0x00
> +#define AF8133J_REG_PCODE_VAL	0x5e
> +#define AF8133J_REG_STATUS	0x02
> +#define AF8133J_REG_STATUS_ACQ	BIT(0)
> +#define AF8133J_REG_STATE	0x0a
> +#define AF8133J_REG_STATE_STBY	0x00
> +#define AF8133J_REG_STATE_WORK	0x01
> +#define AF8133J_REG_RANGE	0x0b
> +#define AF8133J_REG_RANGE_22G	0x12
> +#define AF8133J_REG_RANGE_12G	0x34
> +#define AF8133J_REG_SWR		0x11
> +#define AF8133J_REG_SWR_PERFORM	0x81
> +
> +static const char * const af8133j_supply_names[] = {
> +	"avdd",
> +	"dvdd",
> +};
> +
> +#define AF8133J_NUM_SUPPLIES ARRAY_SIZE(af8133j_supply_names)

Just use this inline, or the size of the array of regulators.
No need for this define.


> +static int af8133j_product_check(struct af8133j_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Error reading product code (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	if (val != AF8133J_REG_PCODE_VAL) {
> +		dev_err(dev, "Invalid product code (0x%02x)\n", val);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int af8133j_reset(struct af8133j_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret;
> +
> +	if (data->reset_gpiod) {
> +		/* If we have GPIO reset line, use it */
> +		gpiod_set_value_cansleep(data->reset_gpiod, 1);
> +		udelay(10);
> +		gpiod_set_value_cansleep(data->reset_gpiod, 0);
> +	} else {
> +		/* Otherwise use software reset */
> +		ret = regmap_write(data->regmap, AF8133J_REG_SWR,
> +				   AF8133J_REG_SWR_PERFORM);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to reset the chip\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Wait for reset to finish */
> +	usleep_range(1000, 1100);
> +
> +	/* Restore range setting */
> +	if (data->range == AF8133J_REG_RANGE_22G) {
> +		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int af8133j_power_up(struct af8133j_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret;
> +
> +	if (data->powered)
> +		return 0;
> +
> +	ret = regulator_bulk_enable(AF8133J_NUM_SUPPLIES, data->supplies);
> +	if (ret) {
> +		dev_err(dev, "Could not enable regulators\n");
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(data->reset_gpiod, 0);
> +
> +	/* Wait for power on reset */
> +	usleep_range(15000, 16000);
> +
> +	data->powered = true;

Why is this needed?  The RPM code is reference counted, so I don't think
we should need this.

> +	return 0;
> +}
> +
> +static void af8133j_power_down(struct af8133j_data *data)
> +{
> +	if (!data->powered)
> +		return;
> +
> +	gpiod_set_value_cansleep(data->reset_gpiod, 1);
> +	regulator_bulk_disable(AF8133J_NUM_SUPPLIES, data->supplies);
> +	data->powered = false;
> +}

> +
> +static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "failed to power on\n");
> +		return ret;
> +	}
> +
> +	mutex_lock(&data->mutex);
	scoped_guard(mutex, &data->mutex) {
		ret = af8133j_take_measurement(data);
		if (ret)
			goto error_ret;

		ret = regmap_bulk_read(...)
	}

error_ret:

	pm_runtime_mark_last_busy(dev);


> +
> +	ret = af8133j_take_measurement(data);
> +	if (ret == 0)
> +		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> +				       buf, sizeof(__le16) * 3);
> +
> +	mutex_unlock(&data->mutex);
> +
> +	pm_runtime_mark_last_busy(dev);
> +	if (pm_runtime_put_autosuspend(dev))
> +		dev_err(dev, "failed to power off\n");
I think this will only happen if suspend returns non 0 and yours
doesn't.  What else might cause this?
> +
> +	return ret;
> +}

> +
> +static int af8133j_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +	__le16 buf[3];
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = af8133j_read_measurement(data, buf);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = sign_extend32(le16_to_cpu(buf[chan->address]),
> +				     chan->scan_type.realbits - 1);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = af8133j_scales[data->range == AF8133J_REG_RANGE_12G ? 0 : 1][1];

Line length is a bit long and the ternary makes it less easy to read than would be ideal.
I'd just use an if / else.
		if (data->Range == AF8133J_REG_RANGE_12G)
			*val2 = af8133j_scales[0][1];
		else
			*val2 = af8133j_scales[1][1];
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int af8133j_set_scale(struct af8133j_data *data,
> +			     unsigned int val, unsigned int val2)
> +{
> +	u8 range;
> +	int ret;
> +
> +	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
> +		range = AF8133J_REG_RANGE_12G;
> +	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
> +		range = AF8133J_REG_RANGE_22G;
> +	else
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> +	if (ret == 0)
	if (ret)
		return ret;

	data->range = range;

	return 0;

A little more code, but it is easier to review if we use the same pattern
everywhere.

> +		data->range = range;
> +
> +	return ret;
> +}
> +
> +static int af8133j_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		mutex_lock(&data->mutex);

Consider using scoped_guard().

> +		ret = af8133j_set_scale(data, val, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     long mask)
> +{
> +	return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +static const struct iio_info af8133j_info = {
> +	.read_raw = af8133j_read_raw,
> +	.read_avail = af8133j_read_avail,
> +	.write_raw = af8133j_write_raw,
> +	.write_raw_get_fmt = af8133j_write_raw_get_fmt,
> +};
> +
> +static irqreturn_t af8133j_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +	s64 timestamp = iio_get_time_ns(indio_dev);
> +	struct {
> +		__le16 values[3];
> +		s64 timestamp __aligned(8);
> +	} sample;
> +	int ret;
> +
> +	memset(&sample, 0, sizeof(sample));
> +
> +	ret = af8133j_read_measurement(data, sample.values);
> +	if (ret == 0)
> +		iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
Prefer to keep the error path out of line

	if (ret)
		goto err_ret;

	iio_push_to_...

error_ret:
	iio_trigger...

It's a little more code, but the consistency of code organization makes
for easier review etc.

> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct regmap_config af8133j_regmap_config = {
> +	.name = "af8133j_regmap",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AF8133J_REG_SWR,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int af8133j_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct af8133j_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret, i;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "regmap initialization failed\n");
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +	data->regmap = regmap;
> +	data->range = AF8133J_REG_RANGE_12G;
> +	mutex_init(&data->mutex);
> +
> +	data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpiod))
> +		return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> +				     "Failed to get reset gpio\n");
> +
> +	for (i = 0; i < AF8133J_NUM_SUPPLIES; i++)
> +		data->supplies[i].supply = af8133j_supply_names[i];
> +	ret = devm_regulator_bulk_get(dev, AF8133J_NUM_SUPPLIES, data->supplies);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_read_mount_matrix(dev, &data->orientation);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> +
> +	ret = af8133j_power_up(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = af8133j_reset(data);
> +	if (ret) {
> +		af8133j_power_down(data);
Moving over to this being handled by a devm callback would remove the need
for these manual calls.

> +		return ret;
> +	}
> +
> +	ret = af8133j_product_check(data);
> +	if (ret) {
> +		af8133j_power_down(data);
> +		return ret;
> +	}
> +
> +	af8133j_power_down(data);

Leave this to runtime_pm autosuspend.
Just make sure to set it as active with appropriate get and put to ensure
the autosuspend handling deals with this.


> +
> +	indio_dev->info = &af8133j_info;
> +	indio_dev->name = AF8133J_DRV_NAME;

As below. I'd rather see the string here.

> +	indio_dev->channels = af8133j_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      &af8133j_trigger_handler, NULL);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to setup iio triggered buffer\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_use_autosuspend(dev);

See the comment on this call in the header. You need to undo it manually - or
use devm_pm_runtime_enable()

> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static void af8133j_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	af8133j_power_down(data);

Can normally push these into callbacks using
devm_add_action_or_reset() 
That avoids need for either explicit error handling or a remove()

You power the device down here, but there isn't a matching call to
power it up in probe() (as it is powered down in there - which you
should leave to runtime_pm())




> +}
> +
> +static int __maybe_unused af8133j_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +
> +	af8133j_power_down(data);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused af8133j_runtime_resume(struct device *dev)

No need to do the __maybe_unused with the changes below.
The new way of handling this is to expose them all to the compiler and
let it do dead code removal.

That's what the pm_ptr() magic does for us.



> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct af8133j_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = af8133j_power_up(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = af8133j_reset(data);
> +	if (ret) {
> +		af8133j_power_down(data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct dev_pm_ops af8133j_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)

This set of macros are deprecated.  
Use SYSTEM_SLEEP_PM_OPS etc in combination with pm_ptr()

> +	SET_RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
> +};

> +
> +static struct i2c_driver af8133j_driver = {
> +	.driver = {
> +		.name = AF8133J_DRV_NAME,

I'd prefer to just see the string here and where it used above.
The define just makes the code harder to read.  There is no
particular reason the driver name should match the iio_dev->name
so little advantage in enforcing that via a define.

> +		.of_match_table = af8133j_of_match,
> +		.pm = &af8133j_pm_ops,

pm_ptr()

otherwise you are going to get unused warnings for the struct.


> +	},
> +	.probe = af8133j_probe,
> +	.remove = af8133j_remove,
> +	.id_table = af8133j_id,
> +};
> +
> +module_i2c_driver(af8133j_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_AUTHOR("Ondřej Jirman <megi@xff.cz>");
> +MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Ondřej Jirman]

Hi Jonathan,

thank you for the patch review.

On Mon, Feb 12, 2024 at 01:02:32PM +0000, Jonathan Cameron wrote:
> On Sun, 11 Feb 2024 21:52:00 +0100
> Ondřej Jirman <megi@xff.cz> wrote:
> 
> > From: Icenowy Zheng <icenowy@aosc.io>
> > 
> > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > 
> > Add a simple IIO driver for it.
> > 
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > Signed-off-by: Dalton Durst <dalton@ubports.com>
> > Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> 
> This is a lot of sign offs.  If accurate it menas.
> 
> Icenowy wrote teh driver,
> Dalton then 'handled' it on the path to Shoji who
> then 'handled' it on the path to Ondrej.
> 
> That's possible if it's been in various other trees for instance, but
> I'd like some more explanation below the --- if that is the case.
> Otherwise, maybe Co-developed-by: is appropriate for some of
> the above list?

Icenowy wrote basic driver, initially. Here's some older version with only Icenowy sign off:

https://github.com/Icenowy/linux/commit/468ceb921dae9d75064c46d13c60cab2b42362b3

I picked the patch into my linux tree a few years back from one of the Mobile
Linux distributions, likely Mobian:

https://megous.com/git/linux/commit/?h=af8133j-5.17&id=1afd43b002a02cade051acbe7851101258e60194

So I guess Dalton and/or Shoji added the orientation matrix support, because
that and addition of some error logging is the only difference between pure Icenowy
version and the version with other sign offs.

Then I rewrote large parts of the driver and added a few new features, like
support for changing the scale/range, RPM, and buffered mode.

So I don't know how to reflect this in the tags. :) It passed through all of
these people, and all of them touched it in some way, I think.

> Various comments inline, but looks pretty good in general.
> 
> Thanks,
> 
> Jonathan
> 
> > diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> > new file mode 100644
> > index 000000000000..0b03417ba134
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/af8133j.c
> > @@ -0,0 +1,525 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * af8133j.c - Voltafield AF8133J magnetometer driver
> > + *
> > + * Copyright 2021 Icenowy Zheng <icenowy@aosc.io>
> > + * Copyright 2024 Ondřej Jirman <megi@xff.cz>
> > + */
> > +
> > +#include <linux/module.h>
> 
> Alphabetical order for these.
> It's fine to separate out he IIO ones on their own as you have
> done.
> 
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/regmap.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> 
> Don't think you are using this as no custom attrs.
> 
> 
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define AF8133J_DRV_NAME "af8133j"
> > +
> > +#define AF8133J_REG_OUT		0x03
> > +#define AF8133J_REG_PCODE	0x00
> > +#define AF8133J_REG_PCODE_VAL	0x5e
> > +#define AF8133J_REG_STATUS	0x02
> > +#define AF8133J_REG_STATUS_ACQ	BIT(0)
> > +#define AF8133J_REG_STATE	0x0a
> > +#define AF8133J_REG_STATE_STBY	0x00
> > +#define AF8133J_REG_STATE_WORK	0x01
> > +#define AF8133J_REG_RANGE	0x0b
> > +#define AF8133J_REG_RANGE_22G	0x12
> > +#define AF8133J_REG_RANGE_12G	0x34
> > +#define AF8133J_REG_SWR		0x11
> > +#define AF8133J_REG_SWR_PERFORM	0x81
> > +
> > +static const char * const af8133j_supply_names[] = {
> > +	"avdd",
> > +	"dvdd",
> > +};
> > +
> > +#define AF8133J_NUM_SUPPLIES ARRAY_SIZE(af8133j_supply_names)
> 
> Just use this inline, or the size of the array of regulators.
> No need for this define.
> 
> 
> > +static int af8133j_product_check(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Error reading product code (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (val != AF8133J_REG_PCODE_VAL) {
> > +		dev_err(dev, "Invalid product code (0x%02x)\n", val);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int af8133j_reset(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	int ret;
> > +
> > +	if (data->reset_gpiod) {
> > +		/* If we have GPIO reset line, use it */
> > +		gpiod_set_value_cansleep(data->reset_gpiod, 1);
> > +		udelay(10);
> > +		gpiod_set_value_cansleep(data->reset_gpiod, 0);
> > +	} else {
> > +		/* Otherwise use software reset */
> > +		ret = regmap_write(data->regmap, AF8133J_REG_SWR,
> > +				   AF8133J_REG_SWR_PERFORM);
> > +		if (ret < 0) {
> > +			dev_err(dev, "Failed to reset the chip\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* Wait for reset to finish */
> > +	usleep_range(1000, 1100);
> > +
> > +	/* Restore range setting */
> > +	if (data->range == AF8133J_REG_RANGE_22G) {
> > +		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int af8133j_power_up(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	int ret;
> > +
> > +	if (data->powered)
> > +		return 0;
> > +
> > +	ret = regulator_bulk_enable(AF8133J_NUM_SUPPLIES, data->supplies);
> > +	if (ret) {
> > +		dev_err(dev, "Could not enable regulators\n");
> > +		return ret;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(data->reset_gpiod, 0);
> > +
> > +	/* Wait for power on reset */
> > +	usleep_range(15000, 16000);
> > +
> > +	data->powered = true;
> 
> Why is this needed?  The RPM code is reference counted, so I don't think
> we should need this.

It's here because of code in af8133j_remove just disables RPM and then calls
af8133j_power_down(). I guess it can be done via RPM, too, by disabling
autosuspend and leaving it to RPM callbacks.

> > +	return 0;
> > +}
> > +
> > +static void af8133j_power_down(struct af8133j_data *data)
> > +{
> > +	if (!data->powered)
> > +		return;
> > +
> > +	gpiod_set_value_cansleep(data->reset_gpiod, 1);
> > +	regulator_bulk_disable(AF8133J_NUM_SUPPLIES, data->supplies);
> > +	data->powered = false;
> > +}
> 
> > +
> > +static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power on\n");
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&data->mutex);
> 	scoped_guard(mutex, &data->mutex) {
> 		ret = af8133j_take_measurement(data);
> 		if (ret)
> 			goto error_ret;
> 
> 		ret = regmap_bulk_read(...)
> 	}
> 
> error_ret:
> 
> 	pm_runtime_mark_last_busy(dev);
> 
> 
> > +
> > +	ret = af8133j_take_measurement(data);
> > +	if (ret == 0)
> > +		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> > +				       buf, sizeof(__le16) * 3);
> > +
> > +	mutex_unlock(&data->mutex);
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	if (pm_runtime_put_autosuspend(dev))
> > +		dev_err(dev, "failed to power off\n");
> I think this will only happen if suspend returns non 0 and yours
> doesn't.  What else might cause this?

I don't know, there's quite a deep callflow under
https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L470
with a lot of error paths. I'd say it's very unlikely to get na error here.

I can drop it if you like.

> > +
> > +	return ret;
> > +}
> 
> > +
> > +static int af8133j_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	__le16 buf[3];
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = af8133j_read_measurement(data, buf);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = sign_extend32(le16_to_cpu(buf[chan->address]),
> > +				     chan->scan_type.realbits - 1);
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 0;
> > +		*val2 = af8133j_scales[data->range == AF8133J_REG_RANGE_12G ? 0 : 1][1];
> 
> Line length is a bit long and the ternary makes it less easy to read than would be ideal.
> I'd just use an if / else.
> 		if (data->Range == AF8133J_REG_RANGE_12G)
> 			*val2 = af8133j_scales[0][1];
> 		else
> 			*val2 = af8133j_scales[1][1];
> > +		return IIO_VAL_INT_PLUS_NANO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int af8133j_set_scale(struct af8133j_data *data,
> > +			     unsigned int val, unsigned int val2)
> > +{
> > +	u8 range;
> > +	int ret;
> > +
> > +	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
> > +		range = AF8133J_REG_RANGE_12G;
> > +	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
> > +		range = AF8133J_REG_RANGE_22G;
> > +	else
> > +		return -EINVAL;
> > +
> > +	ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> > +	if (ret == 0)
> 	if (ret)
> 		return ret;
> 
> 	data->range = range;
> 
> 	return 0;
> 
> A little more code, but it is easier to review if we use the same pattern
> everywhere.
> 
> > +		data->range = range;
> > +
> > +	return ret;
> > +}
> > +
> > +static int af8133j_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		mutex_lock(&data->mutex);
> 
> Consider using scoped_guard().
> 
> > +		ret = af8133j_set_scale(data, val, val2);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				     struct iio_chan_spec const *chan,
> > +				     long mask)
> > +{
> > +	return IIO_VAL_INT_PLUS_NANO;
> > +}
> > +
> > +static const struct iio_info af8133j_info = {
> > +	.read_raw = af8133j_read_raw,
> > +	.read_avail = af8133j_read_avail,
> > +	.write_raw = af8133j_write_raw,
> > +	.write_raw_get_fmt = af8133j_write_raw_get_fmt,
> > +};
> > +
> > +static irqreturn_t af8133j_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	s64 timestamp = iio_get_time_ns(indio_dev);
> > +	struct {
> > +		__le16 values[3];
> > +		s64 timestamp __aligned(8);
> > +	} sample;
> > +	int ret;
> > +
> > +	memset(&sample, 0, sizeof(sample));
> > +
> > +	ret = af8133j_read_measurement(data, sample.values);
> > +	if (ret == 0)
> > +		iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
> Prefer to keep the error path out of line
> 
> 	if (ret)
> 		goto err_ret;
> 
> 	iio_push_to_...
> 
> error_ret:
> 	iio_trigger...
> 
> It's a little more code, but the consistency of code organization makes
> for easier review etc.
> 
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const struct regmap_config af8133j_regmap_config = {
> > +	.name = "af8133j_regmap",
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = AF8133J_REG_SWR,
> > +	.cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int af8133j_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct af8133j_data *data;
> > +	struct iio_dev *indio_dev;
> > +	struct regmap *regmap;
> > +	int ret, i;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(dev, PTR_ERR(regmap),
> > +				     "regmap initialization failed\n");
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +	data->regmap = regmap;
> > +	data->range = AF8133J_REG_RANGE_12G;
> > +	mutex_init(&data->mutex);
> > +
> > +	data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(data->reset_gpiod))
> > +		return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > +				     "Failed to get reset gpio\n");
> > +
> > +	for (i = 0; i < AF8133J_NUM_SUPPLIES; i++)
> > +		data->supplies[i].supply = af8133j_supply_names[i];
> > +	ret = devm_regulator_bulk_get(dev, AF8133J_NUM_SUPPLIES, data->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_read_mount_matrix(dev, &data->orientation);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > +
> > +	ret = af8133j_power_up(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = af8133j_reset(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> Moving over to this being handled by a devm callback would remove the need
> for these manual calls.
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = af8133j_product_check(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> > +		return ret;
> > +	}
> > +
> > +	af8133j_power_down(data);
> 
> Leave this to runtime_pm autosuspend.
> Just make sure to set it as active with appropriate get and put to ensure
> the autosuspend handling deals with this.
> 
> 
> > +
> > +	indio_dev->info = &af8133j_info;
> > +	indio_dev->name = AF8133J_DRV_NAME;
> 
> As below. I'd rather see the string here.
> 
> > +	indio_dev->channels = af8133j_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      &af8133j_trigger_handler, NULL);
> > +	if (ret < 0)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "failed to setup iio triggered buffer\n");
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register iio device");
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > +	pm_runtime_use_autosuspend(dev);
> 
> See the comment on this call in the header. You need to undo it manually - or
> use devm_pm_runtime_enable()
> 
> > +	pm_runtime_enable(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void af8133j_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	struct device *dev = &data->client->dev;
> > +
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_set_suspended(dev);
> > +
> > +	af8133j_power_down(data);
> 
> Can normally push these into callbacks using
> devm_add_action_or_reset() 
> That avoids need for either explicit error handling or a remove()
> 
> You power the device down here, but there isn't a matching call to
> power it up in probe() (as it is powered down in there - which you
> should leave to runtime_pm())

Yes, that's the reason for powered tracking in the driver.

> 
> 
> 
> > +}
> > +
> > +static int __maybe_unused af8133j_runtime_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +
> > +	af8133j_power_down(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused af8133j_runtime_resume(struct device *dev)
> 
> No need to do the __maybe_unused with the changes below.
> The new way of handling this is to expose them all to the compiler and
> let it do dead code removal.
> 
> That's what the pm_ptr() magic does for us.
> 
> 
> 
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = af8133j_power_up(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = af8133j_reset(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +const struct dev_pm_ops af8133j_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> 
> This set of macros are deprecated.  
> Use SYSTEM_SLEEP_PM_OPS etc in combination with pm_ptr()
> 
> > +	SET_RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
> > +};
> 
> > +
> > +static struct i2c_driver af8133j_driver = {
> > +	.driver = {
> > +		.name = AF8133J_DRV_NAME,
> 
> I'd prefer to just see the string here and where it used above.
> The define just makes the code harder to read.  There is no
> particular reason the driver name should match the iio_dev->name
> so little advantage in enforcing that via a define.
> 
> > +		.of_match_table = af8133j_of_match,
> > +		.pm = &af8133j_pm_ops,
> 
> pm_ptr()
> 
> otherwise you are going to get unused warnings for the struct.

thanks for all the suggestions. :)

Kind regards,
	o.

> 
> > +	},
> > +	.probe = af8133j_probe,
> > +	.remove = af8133j_remove,
> > +	.id_table = af8133j_id,
> > +};
> > +
> > +module_i2c_driver(af8133j_driver);
> > +
> > +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> > +MODULE_AUTHOR("Ondřej Jirman <megi@xff.cz>");
> > +MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
> > +MODULE_LICENSE("GPL");
> 

Re: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Jonathan Cameron]

On Mon, 12 Feb 2024 16:04:02 +0100
Ondřej Jirman <megi@xff.cz> wrote:

> Hi Jonathan,
> 
> thank you for the patch review.
> 
> On Mon, Feb 12, 2024 at 01:02:32PM +0000, Jonathan Cameron wrote:
> > On Sun, 11 Feb 2024 21:52:00 +0100
> > Ondřej Jirman <megi@xff.cz> wrote:
> >   
> > > From: Icenowy Zheng <icenowy@aosc.io>
> > > 
> > > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > > 
> > > Add a simple IIO driver for it.
> > > 
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > Signed-off-by: Dalton Durst <dalton@ubports.com>
> > > Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> > > Signed-off-by: Ondrej Jirman <megi@xff.cz>  
> > 
> > This is a lot of sign offs.  If accurate it menas.
> > 
> > Icenowy wrote teh driver,
> > Dalton then 'handled' it on the path to Shoji who
> > then 'handled' it on the path to Ondrej.
> > 
> > That's possible if it's been in various other trees for instance, but
> > I'd like some more explanation below the --- if that is the case.
> > Otherwise, maybe Co-developed-by: is appropriate for some of
> > the above list?  
> 
> Icenowy wrote basic driver, initially. Here's some older version with only Icenowy sign off:
> 
> https://github.com/Icenowy/linux/commit/468ceb921dae9d75064c46d13c60cab2b42362b3
Ok. So probably the author should be Icenowy as you have it.
> 
> I picked the patch into my linux tree a few years back from one of the Mobile
> Linux distributions, likely Mobian:
> 
> https://megous.com/git/linux/commit/?h=af8133j-5.17&id=1afd43b002a02cade051acbe7851101258e60194
> 
> So I guess Dalton and/or Shoji added the orientation matrix support, because
> that and addition of some error logging is the only difference between pure Icenowy
> version and the version with other sign offs.
ok.  If we can figure that out, seems like co-developed for them as well is appropriate.

> 
> Then I rewrote large parts of the driver and added a few new features, like
> support for changing the scale/range, RPM, and buffered mode.
Defintely a co-developed for you then!
> 
> So I don't know how to reflect this in the tags. :) It passed through all of
> these people, and all of them touched it in some way, I think.

Lots of co-developed probably most appropriate.  Basically add one for each
SoB other than Iceynow's

> > > +
> > > +static int af8133j_power_up(struct af8133j_data *data)
> > > +{
> > > +	struct device *dev = &data->client->dev;
> > > +	int ret;
> > > +
> > > +	if (data->powered)
> > > +		return 0;
> > > +
> > > +	ret = regulator_bulk_enable(AF8133J_NUM_SUPPLIES, data->supplies);
> > > +	if (ret) {
> > > +		dev_err(dev, "Could not enable regulators\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	gpiod_set_value_cansleep(data->reset_gpiod, 0);
> > > +
> > > +	/* Wait for power on reset */
> > > +	usleep_range(15000, 16000);
> > > +
> > > +	data->powered = true;  
> > 
> > Why is this needed?  The RPM code is reference counted, so I don't think
> > we should need this.  
> 
> It's here because of code in af8133j_remove just disables RPM and then calls
> af8133j_power_down(). I guess it can be done via RPM, too, by disabling
> autosuspend and leaving it to RPM callbacks.

ah. Don't use a flag for that, add a little utility function
that takes it as an explicit parameter.  Make sure you wake the device
up using runtime_pm then disable runtime pm before powering it down manually.

> 
> > > +	return 0;

...


> > > +
> > > +	ret = af8133j_take_measurement(data);
> > > +	if (ret == 0)
> > > +		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> > > +				       buf, sizeof(__le16) * 3);
> > > +
> > > +	mutex_unlock(&data->mutex);
> > > +
> > > +	pm_runtime_mark_last_busy(dev);
> > > +	if (pm_runtime_put_autosuspend(dev))
> > > +		dev_err(dev, "failed to power off\n");  
> > I think this will only happen if suspend returns non 0 and yours
> > doesn't.  What else might cause this?  
> 
> I don't know, there's quite a deep callflow under
> https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L470
> with a lot of error paths. I'd say it's very unlikely to get na error here.
> 
> I can drop it if you like.

I would.  If something odd is going on a developer can easily
add a check back in to debug it.
> 
> > > +
> > > +	return ret;
> > > +}  

...

> >   
> > > +	pm_runtime_enable(dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void af8133j_remove(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > +	struct af8133j_data *data = iio_priv(indio_dev);
> > > +	struct device *dev = &data->client->dev;
> > > +
> > > +	pm_runtime_disable(dev);
> > > +	pm_runtime_set_suspended(dev);
> > > +
> > > +	af8133j_power_down(data);  
> > 
> > Can normally push these into callbacks using
> > devm_add_action_or_reset() 
> > That avoids need for either explicit error handling or a remove()
> > 
> > You power the device down here, but there isn't a matching call to
> > power it up in probe() (as it is powered down in there - which you
> > should leave to runtime_pm())  
> 
> Yes, that's the reason for powered tracking in the driver.
> 
ok.  Try and avoid that and just let runtime pm deal with it for you.

For future reference, crop out anything you have commented on in
a review. It saves on scrolling and reduces chances of stuff being
missed in the dicussion.


Jonathan