[PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[Jonathan Albrieux]

Convert documentation from txt format to yaml. Add documentation about
reset-gpio. 

Deassert reset on ak8975_power_on, assert reset on ak8975_power_off.

Without reset's deassertion during ak8975_power_on, driver's probe fails
on ak8975_who_i_am while checking for device identity for AK09911 chip

AK09911 has an active low reset gpio to handle register's reset.
AK09911 datasheed says that, if not used, reset pin should be connected
to VID. This patch emulates this situation

Jonathan Albrieux (3):
  dt-bindings: iio: magnetometer: ak8975: convert txt format to yaml
  dt-bindings: iio: magnetometer: ak8975: add gpio reset support
  iio: magnetometer: ak8975: Add gpio reset support

 .../bindings/iio/magnetometer/ak8975.txt      | 30 --------
 .../bindings/iio/magnetometer/ak8975.yaml     | 70 +++++++++++++++++++
 drivers/iio/magnetometer/ak8975.c             | 22 +++++-
 3 files changed, 90 insertions(+), 32 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml

-- 
2.17.1

[PATCH v2 1/3] dt-bindings: iio: magnetometer: ak8975: convert txt format to yaml

[Jonathan Albrieux]

Converts documentation from txt format to yaml

Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
 .../bindings/iio/magnetometer/ak8975.txt      | 30 ---------
 .../bindings/iio/magnetometer/ak8975.yaml     | 66 +++++++++++++++++++
 2 files changed, 66 insertions(+), 30 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
deleted file mode 100644
index aa67ceb0d4e0..000000000000
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.txt
+++ /dev/null
@@ -1,30 +0,0 @@
-* AsahiKASEI AK8975 magnetometer sensor
-
-Required properties:
-
-  - compatible : should be "asahi-kasei,ak8975"
-  - reg : the I2C address of the magnetometer
-
-Optional properties:
-
-  - gpios : should be device tree identifier of the magnetometer DRDY pin
-  - vdd-supply: an optional regulator that needs to be on to provide VDD
-  - mount-matrix: an optional 3x3 mounting rotation matrix
-
-Example:
-
-ak8975@c {
-        compatible = "asahi-kasei,ak8975";
-        reg = <0x0c>;
-        gpios = <&gpj0 7 0>;
-        vdd-supply = <&ldo_3v3_gnss>;
-        mount-matrix = "-0.984807753012208",  /* x0 */
-                       "0",                   /* y0 */
-                       "-0.173648177666930",  /* z0 */
-                       "0",                   /* x1 */
-                       "-1",                  /* y1 */
-                       "0",                   /* z1 */
-                       "-0.173648177666930",  /* x2 */
-                       "0",                   /* y2 */
-                       "0.984807753012208";   /* z2 */
-};
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
new file mode 100644
index 000000000000..86e3efa693a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/ak8975.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AsahiKASEI AK8975 magnetometer sensor
+
+maintainers:
+  - can't find a mantainer, author is Laxman Dewangan <ldewangan@nvidia.com>
+
+properties:
+  compatible:
+    enum:
+      - "asahi-kasei,ak8975"
+      - "ak8975"
+      - "asahi-kasei,ak8963"
+      - "ak8963"
+      - "asahi-kasei,ak09911"
+      - "ak09911"
+      - "asahi-kasei,ak09912"
+      - "ak09912"
+
+  reg:
+    maxItems: 1
+    description: the I2C address of the magnetometer
+
+  gpios:
+    description: should be device tree identifier of the magnetometer DRDY pin
+
+  vdd-supply:
+    maxItems: 1
+    description: |
+      an optional regulator that needs to be on to provide VDD power to
+      the sensor.
+
+  mount-matrix:
+    description: an optional 3x3 mounting rotation matrix
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c@78b7000 {
+        reg = <0x78b6000 0x600>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ak8975@c {
+            compatible = "asahi-kasei,ak8975";
+            reg = <0x0c>;
+            gpios = <&gpj0 7 0>;
+            vdd-supply = <&ldo_3v3_gnss>;
+            mount-matrix = "-0.984807753012208",  /* x0 */
+                           "0",                   /* y0 */
+                           "-0.173648177666930",  /* z0 */
+                           "0",                   /* x1 */
+                           "-1",                  /* y1 */
+                           "0",                   /* z1 */
+                           "-0.173648177666930",  /* x2 */
+                           "0",                   /* y2 */
+                           "0.984807753012208";   /* z2 */
+        };
+    };
-- 
2.17.1

[PATCH v2 2/3] dt-bindings: iio: magnetometer: ak8975: add gpio reset support

[Jonathan Albrieux]

Add reset-gpio support.

Without reset's deassertion during ak8975_power_on, driver's probe fails on
ak8975_who_i_am while checking for device identity for AK09911 chip

AK09911 has an active low reset gpio to handle register's reset.
AK09911 datasheed says that, if not used, reset pin should be connected
to VID. This patch emulates this situation

Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
 .../devicetree/bindings/iio/magnetometer/ak8975.yaml          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
index 86e3efa693a8..a82c0ff5d098 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ak8975.yaml
@@ -37,6 +37,9 @@ properties:
   mount-matrix:
     description: an optional 3x3 mounting rotation matrix
 
+  reset-gpio:
+    description: an optional pin needed for AK09911 to set the reset state
+
 required:
   - compatible
   - reg
@@ -53,6 +56,7 @@ examples:
             reg = <0x0c>;
             gpios = <&gpj0 7 0>;
             vdd-supply = <&ldo_3v3_gnss>;
+            reset-gpio = <&msmgpio 111 1>;
             mount-matrix = "-0.984807753012208",  /* x0 */
                            "0",                   /* y0 */
                            "-0.173648177666930",  /* z0 */
-- 
2.17.1

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

[Jonathan Albrieux]

According to AK09911 datasheet, if reset gpio is provided then 
deassert reset on ak8975_power_on and assert reset on ak8975_power_off.

Without reset's deassertion during ak8975_power_on, driver's probe fails on
ak8975_who_i_am while checking for device identity for AK09911 chip

AK09911 has an active low reset gpio to handle register's reset.
AK09911 datasheed says that, if not used, reset pin should be connected
to VID. This patch emulates this situation

Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
---
 drivers/iio/magnetometer/ak8975.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 3c881541ae72..84dea520db02 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -358,6 +358,7 @@ struct ak8975_data {
 	u8			asa[3];
 	long			raw_to_gauss[3];
 	struct gpio_desc	*eoc_gpiod;
+	struct gpio_desc	*reset_gpiod;
 	int			eoc_irq;
 	wait_queue_head_t	data_ready_queue;
 	unsigned long		flags;
@@ -384,10 +385,13 @@ static int ak8975_power_on(const struct ak8975_data *data)
 			 "Failed to enable specified Vid supply\n");
 		return ret;
 	}
+
+	gpiod_set_value_cansleep(data->reset_gpiod, 0);
+
 	/*
-	 * According to the datasheet the power supply rise time i 200us
+	 * According to the datasheet the power supply rise time is 200us
 	 * and the minimum wait time before mode setting is 100us, in
-	 * total 300 us. Add some margin and say minimum 500us here.
+	 * total 300us. Add some margin and say minimum 500us here.
 	 */
 	usleep_range(500, 1000);
 	return 0;
@@ -396,6 +400,8 @@ static int ak8975_power_on(const struct ak8975_data *data)
 /* Disable attached power regulator if any. */
 static void ak8975_power_off(const struct ak8975_data *data)
 {
+	gpiod_set_value_cansleep(data->reset_gpiod, 1);
+
 	regulator_disable(data->vid);
 	regulator_disable(data->vdd);
 }
@@ -839,6 +845,7 @@ static int ak8975_probe(struct i2c_client *client,
 	struct ak8975_data *data;
 	struct iio_dev *indio_dev;
 	struct gpio_desc *eoc_gpiod;
+	struct gpio_desc *reset_gpiod;
 	const void *match;
 	unsigned int i;
 	int err;
@@ -856,6 +863,16 @@ static int ak8975_probe(struct i2c_client *client,
 	if (eoc_gpiod)
 		gpiod_set_consumer_name(eoc_gpiod, "ak_8975");
 
+	/*
+	 * According to AK09911 datasheet, if reset gpio is provided then
+	 * deassert reset on ak8975_power_on and assert reset on
+	 * ak8975_power_off.
+	 */
+	reset_gpiod = devm_gpiod_get_optional(&client->dev,
+					      "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpiod))
+		return PTR_ERR(reset_gpiod);
+
 	/* Register with IIO */
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (indio_dev == NULL)
@@ -866,6 +883,7 @@ static int ak8975_probe(struct i2c_client *client,
 
 	data->client = client;
 	data->eoc_gpiod = eoc_gpiod;
+	data->reset_gpiod = reset_gpiod;
 	data->eoc_irq = 0;
 
 	err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
-- 
2.17.1

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

[Andy Shevchenko]

On Tue, May 19, 2020 at 08:57:43AM +0200, Jonathan Albrieux wrote:

Thanks for an update, my comments below.

> According to AK09911 datasheet, if reset gpio is provided then 

Trailing whitespace.

> deassert reset on ak8975_power_on and assert reset on ak8975_power_off.
> 
> Without reset's deassertion during ak8975_power_on, driver's probe fails on
> ak8975_who_i_am while checking for device identity for AK09911 chip
> 
> AK09911 has an active low reset gpio to handle register's reset.
> AK09911 datasheed says that, if not used, reset pin should be connected
> to VID. This patch emulates this situation

Please, put periods at the end of the phrases.

>  	/*
> -	 * According to the datasheet the power supply rise time i 200us
> +	 * According to the datasheet the power supply rise time is 200us
>  	 * and the minimum wait time before mode setting is 100us, in
> -	 * total 300 us. Add some margin and say minimum 500us here.
> +	 * total 300us. Add some margin and say minimum 500us here.
>  	 */

This is not related change (perhaps, you may do in separate patch), but I'm not
against it. Up to maintainer.

> +	/*
> +	 * According to AK09911 datasheet, if reset gpio is provided then
> +	 * deassert reset on ak8975_power_on and assert reset on
> +	 * ak8975_power_off.

We try to refer to the code accordingly to the kernel doc recommendations (even
for non-kdoc comments). So, here, 'function' as 'function()' (without quotes)
and so on.

> +	 */
> +	reset_gpiod = devm_gpiod_get_optional(&client->dev,
> +					      "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpiod))
> +		return PTR_ERR(reset_gpiod);
> +
>  	/* Register with IIO */
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (indio_dev == NULL)
> @@ -866,6 +883,7 @@ static int ak8975_probe(struct i2c_client *client,
>  
>  	data->client = client;
>  	data->eoc_gpiod = eoc_gpiod;
> +	data->reset_gpiod = reset_gpiod;
>  	data->eoc_irq = 0;
>  
>  	err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Albrieux]

On Tue, May 19, 2020 at 12:21:27PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 08:57:43AM +0200, Jonathan Albrieux wrote:
> 
> Thanks for an update, my comments below.
> 

Thanks to you for taking time to review the code!

> > According to AK09911 datasheet, if reset gpio is provided then 
> 
> Trailing whitespace.
> 
> > deassert reset on ak8975_power_on and assert reset on ak8975_power_off.
> > 
> > Without reset's deassertion during ak8975_power_on, driver's probe fails on
> > ak8975_who_i_am while checking for device identity for AK09911 chip
> > 
> > AK09911 has an active low reset gpio to handle register's reset.
> > AK09911 datasheed says that, if not used, reset pin should be connected
> > to VID. This patch emulates this situation
> 
> Please, put periods at the end of the phrases.
> 
> >  	/*
> > -	 * According to the datasheet the power supply rise time i 200us
> > +	 * According to the datasheet the power supply rise time is 200us
> >  	 * and the minimum wait time before mode setting is 100us, in
> > -	 * total 300 us. Add some margin and say minimum 500us here.
> > +	 * total 300us. Add some margin and say minimum 500us here.
> >  	 */
> 
> This is not related change (perhaps, you may do in separate patch), but I'm not
> against it. Up to maintainer.
> 

Ok I will separate the patch

> > +	/*
> > +	 * According to AK09911 datasheet, if reset gpio is provided then
> > +	 * deassert reset on ak8975_power_on and assert reset on
> > +	 * ak8975_power_off.
> 
> We try to refer to the code accordingly to the kernel doc recommendations (even
> for non-kdoc comments). So, here, 'function' as 'function()' (without quotes)
> and so on.
> 

Will fix this as like as the other changes you pointed out about periods and
trailing spaces as soon as possible

> > +	 */
> > +	reset_gpiod = devm_gpiod_get_optional(&client->dev,
> > +					      "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpiod))
> > +		return PTR_ERR(reset_gpiod);
> > +
> >  	/* Register with IIO */
> >  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >  	if (indio_dev == NULL)
> > @@ -866,6 +883,7 @@ static int ak8975_probe(struct i2c_client *client,
> >  
> >  	data->client = client;
> >  	data->eoc_gpiod = eoc_gpiod;
> > +	data->reset_gpiod = reset_gpiod;
> >  	data->eoc_irq = 0;
> >  
> >  	err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
> > -- 
> > 2.17.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Again, thank you,
Best regards,
Jonathan Albrieux

Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[Andy Shevchenko]

On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
> Convert documentation from txt format to yaml. Add documentation about
> reset-gpio. 

Trailing white space (though it doesn't matter here, in cover letter).

> Deassert reset on ak8975_power_on, assert reset on ak8975_power_off.
> 
> Without reset's deassertion during ak8975_power_on, driver's probe fails
> on ak8975_who_i_am while checking for device identity for AK09911 chip
> 
> AK09911 has an active low reset gpio to handle register's reset.
> AK09911 datasheed says that, if not used, reset pin should be connected
> to VID. This patch emulates this situation

I dunno if it's your first submission to Linux kernel project or other OSS,
but here you missed a changelog. Rule of thumb is to provide a summary of
the changes done in the history of the evolution of a patch series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[Jonathan Albrieux]

On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
> > Convert documentation from txt format to yaml. Add documentation about
> > reset-gpio. 
> 
> Trailing white space (though it doesn't matter here, in cover letter).
> 

Ok will fix it!

> > Deassert reset on ak8975_power_on, assert reset on ak8975_power_off.
> > 
> > Without reset's deassertion during ak8975_power_on, driver's probe fails
> > on ak8975_who_i_am while checking for device identity for AK09911 chip
> > 
> > AK09911 has an active low reset gpio to handle register's reset.
> > AK09911 datasheed says that, if not used, reset pin should be connected
> > to VID. This patch emulates this situation
> 
> I dunno if it's your first submission to Linux kernel project or other OSS,
> but here you missed a changelog. Rule of thumb is to provide a summary of
> the changes done in the history of the evolution of a patch series.
> 

Oh thank you and sorry for not having included it. 

Does the changelog needs to be added to all patch files or just on the ones
subject of the changes?

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Jonathan Albrieux

Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[Andy Shevchenko]

On Tue, May 19, 2020 at 11:48:35AM +0200, Jonathan Albrieux wrote:
> On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> > On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:

...

> > I dunno if it's your first submission to Linux kernel project or other OSS,
> > but here you missed a changelog. Rule of thumb is to provide a summary of
> > the changes done in the history of the evolution of a patch series.
> > 
> 
> Oh thank you and sorry for not having included it. 
> 
> Does the changelog needs to be added to all patch files or just on the ones
> subject of the changes?

Up to you and maintainer of the corresponding subsystem.

My common sense tells me that
1) if there is a cover letter, just put a joined changelog there
2) otherwise, put changelog in each patch.

I saw in practice all possible variants, i.e.
a) cover letter w/o changelog + changelog per patch;
b) cover letter w/ changelog + changelog per patch;
c) cover letter w/ changelog.

I think any of it is fine in general.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] iio: magnetometer: ak8975: Add gpio reset support

[Jonathan Albrieux]

On Tue, May 19, 2020 at 01:08:15PM +0300, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 11:48:35AM +0200, Jonathan Albrieux wrote:
> > On Tue, May 19, 2020 at 12:22:12PM +0300, Andy Shevchenko wrote:
> > > On Tue, May 19, 2020 at 08:57:40AM +0200, Jonathan Albrieux wrote:
> 
> ...
> 
> > > I dunno if it's your first submission to Linux kernel project or other OSS,
> > > but here you missed a changelog. Rule of thumb is to provide a summary of
> > > the changes done in the history of the evolution of a patch series.
> > > 
> > 
> > Oh thank you and sorry for not having included it. 
> > 
> > Does the changelog needs to be added to all patch files or just on the ones
> > subject of the changes?
> 
> Up to you and maintainer of the corresponding subsystem.
> 
> My common sense tells me that
> 1) if there is a cover letter, just put a joined changelog there
> 2) otherwise, put changelog in each patch.
> 
> I saw in practice all possible variants, i.e.
> a) cover letter w/o changelog + changelog per patch;
> b) cover letter w/ changelog + changelog per patch;
> c) cover letter w/ changelog.
> 
> I think any of it is fine in general.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thank you for the tips, will work on it!

Best regards,
Jonathan Albrieux