[PATCH v3 1/2] mfd: arizona: Update reset pin to use GPIOD

Now GPIOD has support for both pdata systems and for non-standard DT
bindings the Arizona reset GPIO can be converted to use it.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v2:
 - Kept null check in arizona_enable_reset, although
   gpiod_set_value_cansleep does its own null check it will also issue
   a fat WARN we don't want if gpiolib is not built in.
 - Added a check for ENOSYS in arizona_of_get_core_pdata, just to
   silence the extra error that would be printed here if gpiolib is not
   built in.

Thanks,
Charles

 drivers/mfd/arizona-core.c        | 50 ++++++++++++++++++++++++++-------------
 include/linux/mfd/arizona/pdata.h |  3 ++-
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 77875250abe5..9558c4d9c8ca 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c

@@ -279,7 +279,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 static inline void arizona_enable_reset(struct arizona *arizona)
 {
 	if (arizona->pdata.reset)
-		gpio_set_value_cansleep(arizona->pdata.reset, 0);
+		gpiod_set_value_cansleep(arizona->pdata.reset, 0);
 }
 
 static void arizona_disable_reset(struct arizona *arizona)

@@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
 			break;
 		}
 
-		gpio_set_value_cansleep(arizona->pdata.reset, 1);
+		gpiod_set_value_cansleep(arizona->pdata.reset, 1);
 		usleep_range(1000, 5000);
 	}
 }

@@ -799,14 +799,26 @@ static int arizona_of_get_core_pdata(struct arizona *arizona)
 	struct arizona_pdata *pdata = &arizona->pdata;
 	int ret, i;
 
-	pdata->reset = of_get_named_gpio(arizona->dev->of_node, "wlf,reset", 0);
-	if (pdata->reset == -EPROBE_DEFER) {
-		return pdata->reset;
-	} else if (pdata->reset < 0) {
-		dev_err(arizona->dev, "Reset GPIO missing/malformed: %d\n",
-			pdata->reset);
+	pdata->reset = devm_gpiod_get_from_of_node(arizona->dev,
+						   arizona->dev->of_node,
+						   "wlf,reset", 0,
+						   GPIOD_OUT_LOW,
+						   "arizona /RESET");
+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {
+		case -ENOENT:
+		case -ENOSYS:
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
+				ret);
+			break;
+		}
 
-		pdata->reset = 0;
+		pdata->reset = NULL;
 	}
 
 	ret = of_property_read_u32_array(arizona->dev->of_node,

@@ -1050,14 +1062,20 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_early;
 	}
 
-	if (arizona->pdata.reset) {
+	if (!arizona->pdata.reset) {
 		/* Start out with /RESET low to put the chip into reset */
-		ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
-					    GPIOF_DIR_OUT | GPIOF_INIT_LOW,
-					    "arizona /RESET");
-		if (ret != 0) {
-			dev_err(dev, "Failed to request /RESET: %d\n", ret);
-			goto err_dcvdd;
+		arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
+						      GPIOD_OUT_LOW);
+		if (IS_ERR(arizona->pdata.reset)) {
+			ret = PTR_ERR(arizona->pdata.reset);
+			if (ret == -EPROBE_DEFER)
+				goto err_dcvdd;
+			else
+				dev_err(arizona->dev,
+					"Reset GPIO missing/malformed: %d\n",
+					ret);
+
+			arizona->pdata.reset = NULL;
 		}
 	}
 

diff --git a/include/linux/mfd/arizona/pdata.h b/include/linux/mfd/arizona/pdata.h
index f72dc53848d7..0013075d4cda 100644
--- a/include/linux/mfd/arizona/pdata.h
+++ b/include/linux/mfd/arizona/pdata.h

@@ -56,6 +56,7 @@
 #define ARIZONA_MAX_PDM_SPK 2
 
 struct regulator_init_data;
+struct gpio_desc;
 
 struct arizona_micbias {
 	int mV;                    /** Regulated voltage */

@@ -77,7 +78,7 @@ struct arizona_micd_range {
 };
 
 struct arizona_pdata {
-	int reset;      /** GPIO controlling /RESET, if any */
+	struct gpio_desc *reset;      /** GPIO controlling /RESET, if any */
 
 	/** Regulator configuration for MICVDD */
 	struct arizona_micsupp_pdata micvdd;

-- 
2.11.0

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

No changes since v2.

Thanks,
Charles

 Documentation/devicetree/bindings/mfd/arizona.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
index bdd017686ea5..a014afb07902 100644
--- a/Documentation/devicetree/bindings/mfd/arizona.txt
+++ b/Documentation/devicetree/bindings/mfd/arizona.txt

@@ -50,7 +50,7 @@ Required properties:
 
 Optional properties:
 
-  - wlf,reset : GPIO specifier for the GPIO controlling /RESET
+  - reset-gpios : GPIO specifier for the GPIO controlling /RESET
 
   - clocks: Should reference the clocks supplied on MCLK1 and MCLK2
   - clock-names: Should contains two strings:

@@ -70,6 +70,10 @@ Optional properties:
     Documentation/devicetree/bindings/regulator/regulator.txt
     (wm5102, wm5110, wm8280, wm8997, wm8998, wm1814)
 
+Deprecated properties:
+
+  - wlf,reset : GPIO specifier for the GPIO controlling /RESET
+
 Also see child specific device properties:
   Regulator - ../regulator/arizona-regulator.txt
   Extcon    - ../extcon/extcon-arizona.txt

-- 
2.11.0

Hi Charles,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Charles-Keepax/mfd-arizona-Update-reset-pin-to-use-GPIOD/20180221-052657
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-randconfig-h0-02210830 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/mfd/arizona-core.c: In function 'arizona_enable_reset':

quoted

drivers/mfd/arizona-core.c:282:3: error: implicit declaration of function 'gpiod_set_value_cansleep'; did you mean 'gpio_set_value_cansleep'? [-Werror=implicit-function-declaration]

      gpiod_set_value_cansleep(arizona->pdata.reset, 0);
      ^~~~~~~~~~~~~~~~~~~~~~~~
      gpio_set_value_cansleep
   drivers/mfd/arizona-core.c: In function 'arizona_of_get_core_pdata':

quoted

drivers/mfd/arizona-core.c:802:17: error: implicit declaration of function 'devm_gpiod_get_from_of_node'; did you mean 'devm_gpio_request_one'? [-Werror=implicit-function-declaration]

     pdata->reset = devm_gpiod_get_from_of_node(arizona->dev,
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
                    devm_gpio_request_one

quoted

drivers/mfd/arizona-core.c:805:10: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOF_INIT_LOW'?

             GPIOD_OUT_LOW,
             ^~~~~~~~~~~~~
             GPIOF_INIT_LOW
   drivers/mfd/arizona-core.c:805:10: note: each undeclared identifier is reported only once for each function it appears in
   drivers/mfd/arizona-core.c: In function 'arizona_dev_init':

quoted

drivers/mfd/arizona-core.c:1067:26: error: implicit declaration of function 'devm_gpiod_get'; did you mean 'devm_gpio_free'? [-Werror=implicit-function-declaration]

      arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
                             ^~~~~~~~~~~~~~
                             devm_gpio_free
   drivers/mfd/arizona-core.c:1068:13: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOF_INIT_LOW'?
                GPIOD_OUT_LOW);
                ^~~~~~~~~~~~~
                GPIOF_INIT_LOW
   cc1: some warnings being treated as errors

vim +282 drivers/mfd/arizona-core.c

   278	
   279	static inline void arizona_enable_reset(struct arizona *arizona)
   280	{
   281		if (arizona->pdata.reset)
 > 282			gpiod_set_value_cansleep(arizona->pdata.reset, 0);
   283	}
   284	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On Tue, 20 Feb 2018, Charles Keepax wrote:

I haven't seen a switch statement used in the error handling path
before.  There is probably good reason for that.

What is the 'default' case?  -EINVAL?

You don't need the else.

	arizona->pdata.reset =
		devm_gpiod_get(arizona->dev, "reset", GPIOD_OUT_LOW);

	ret = PTR_ERR(arizona->pdata.reset);
	if (ret == -EPROBE_DEFER)
		goto err_dcvdd;

	if (ret) {
		dev_err(arizona->dev,
			"Reset GPIO missing/malformed: %d\n", ret);
		arizona->pdata.reset = NULL;
	}

I know it doesn't have much to do with this patch, but it's probably
better to use a Kerneldoc header for this struct.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Mar 07, 2018 at 01:28:12PM +0000, Lee Jones wrote:

On Tue, 20 Feb 2018, Charles Keepax wrote:

quoted

+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {
+		case -ENOENT:
+		case -ENOSYS:
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
+				ret);
+			break;
+		}

I haven't seen a switch statement used in the error handling path
before.  There is probably good reason for that.

There are a fair few of them "grep -rI "switch (ret)" | wc -l" ==
205. Although to be fair that has rarely been an argument in
somethings favour.

What is the 'default' case?  -EINVAL?

I would guess most of the time yes, but I would rather not assume
that. I can redo this as if's if you prefer it that way? The if is
slightly less lines although I do think the switch is much
clearer as to intent.

if (ret == -EPROBE_DEFER) {
	return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
	dev_err(arizona->dev, ....);
}

quoted

-	if (arizona->pdata.reset) {
+	if (!arizona->pdata.reset) {
 		/* Start out with /RESET low to put the chip into reset */
-		ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
-					    GPIOF_DIR_OUT | GPIOF_INIT_LOW,
-					    "arizona /RESET");
-		if (ret != 0) {
-			dev_err(dev, "Failed to request /RESET: %d\n", ret);
-			goto err_dcvdd;
+		arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
+						      GPIOD_OUT_LOW);
+		if (IS_ERR(arizona->pdata.reset)) {
+			ret = PTR_ERR(arizona->pdata.reset);
+			if (ret == -EPROBE_DEFER)
+				goto err_dcvdd;
+			else
+				dev_err(arizona->dev,
+					"Reset GPIO missing/malformed: %d\n",
+					ret);

You don't need the else.

Happy to drop.

quoted

--- a/include/linux/mfd/arizona/pdata.h
+++ b/include/linux/mfd/arizona/pdata.h

@@ -56,6 +56,7 @@
 #define ARIZONA_MAX_PDM_SPK 2
 
 struct regulator_init_data;
+struct gpio_desc;
 
 struct arizona_micbias {
 	int mV;                    /** Regulated voltage */

@@ -77,7 +78,7 @@ struct arizona_micd_range {
 };
 
 struct arizona_pdata {
-	int reset;      /** GPIO controlling /RESET, if any */
+	struct gpio_desc *reset;      /** GPIO controlling /RESET, if any */
 
 	/** Regulator configuration for MICVDD */
 	struct arizona_micsupp_pdata micvdd;

I know it doesn't have much to do with this patch, but it's probably
better to use a Kerneldoc header for this struct.

Indeed I would agree that would be better, not sure what the
commenting style there is about. I should be able to find time to
make a patch for this soon, but seems unrelated to this series.

Thanks,
Charles

Hi Charles,

On Tue, Feb 20, 2018 at 1:35 PM, Charles Keepax
[off-list ref] wrote:

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 1);

as here you want to put the reset GPIO into its active state.

Assuming that in the dts this GPIO is defined as GPIO_ACTIVE_LOW, then
the command should put it to logic level 0 as done originally.

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 0);

as here you want to put the reset GPIO into its inactive state.

On Wed, Mar 07, 2018 at 11:24:10AM -0300, Fabio Estevam wrote:

quoted

 static inline void arizona_enable_reset(struct arizona *arizona)
 {
        if (arizona->pdata.reset)
-               gpio_set_value_cansleep(arizona->pdata.reset, 0);
+               gpiod_set_value_cansleep(arizona->pdata.reset, 0);

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 1);

as here you want to put the reset GPIO into its active state.

Assuming that in the dts this GPIO is defined as GPIO_ACTIVE_LOW, then
the command should put it to logic level 0 as done originally.

quoted

 static void arizona_disable_reset(struct arizona *arizona)

@@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
                        break;
                }

-               gpio_set_value_cansleep(arizona->pdata.reset, 1);
+               gpiod_set_value_cansleep(arizona->pdata.reset, 1);

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 0);

as here you want to put the reset GPIO into its inactive state.

Hmm... this raises a rather good point that I hadn't considered.
This driver is used in a lot of shipping devices and the old
style GPIO calls didn't take the activeness of the GPIO into
account just blindly setting the value you asked for. However the
new style calls do take this into account.

The trouble is I guess we don't know whether most users bothered
to set GPIO_ACTIVE_LOW or not. So it is very hard to say here if
we are about to breaking a lot of existing device trees here.

I guess probably the safest approach is to use
gpiod_set_raw_value_cansleep here?

Thanks,
Charles

On Tue, 20 Feb 2018, Charles Keepax wrote:

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

No changes since v2.

Thanks,
Charles

 Documentation/devicetree/bindings/mfd/arizona.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Acked-by: Lee Jones <redacted>

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Mar 7, 2018 at 11:55 AM, Charles Keepax
[off-list ref] wrote:

Hmm... this raises a rather good point that I hadn't considered.
This driver is used in a lot of shipping devices and the old
style GPIO calls didn't take the activeness of the GPIO into
account just blindly setting the value you asked for. However the
new style calls do take this into account.

The trouble is I guess we don't know whether most users bothered
to set GPIO_ACTIVE_LOW or not. So it is very hard to say here if
we are about to breaking a lot of existing device trees here.

I guess probably the safest approach is to use
gpiod_set_raw_value_cansleep here?

Yes, probably needed to avoid breakage of existing systems.

On Wed, 07 Mar 2018, Charles Keepax wrote:

On Wed, Mar 07, 2018 at 01:28:12PM +0000, Lee Jones wrote:

quoted

On Tue, 20 Feb 2018, Charles Keepax wrote:

quoted

+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {
+		case -ENOENT:
+		case -ENOSYS:
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
+				ret);
+			break;
+		}

I haven't seen a switch statement used in the error handling path
before.  There is probably good reason for that.

There are a fair few of them "grep -rI "switch (ret)" | wc -l" ==
205. Although to be fair that has rarely been an argument in
somethings favour.

I didn't say "it has never been done", just that I hadn't seen it.

Besides, 205 uses is a very small amount in kernel code:

`git grep "if (ret" | wc -l` == 74710

quoted

What is the 'default' case?  -EINVAL?

I would guess most of the time yes, but I would rather not assume
that. I can redo this as if's if you prefer it that way? The if is
slightly less lines although I do think the switch is much
clearer as to intent.

if (ret == -EPROBE_DEFER) {
	return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
	dev_err(arizona->dev, ....);
}

I don't know enough about the API to see why -ENOENT and -ENOSYS do
not deserve error messages.

What do the other users of the API do?

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Wed, Mar 07, 2018 at 04:25:37PM +0000, Lee Jones wrote:

On Wed, 07 Mar 2018, Charles Keepax wrote:

quoted

On Wed, Mar 07, 2018 at 01:28:12PM +0000, Lee Jones wrote:

quoted

On Tue, 20 Feb 2018, Charles Keepax wrote:

I would guess most of the time yes, but I would rather not assume
that. I can redo this as if's if you prefer it that way? The if is
slightly less lines although I do think the switch is much
clearer as to intent.

if (ret == -EPROBE_DEFER) {
	return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
	dev_err(arizona->dev, ....);
}

I don't know enough about the API to see why -ENOENT and -ENOSYS do
not deserve error messages.

ENOENT means the property was not found, and ENOSYS means that
there is no GPIOLIB built into the kernel.

In hindsight really I think this did deserve a comment. Basically
what is going on here, is we are supporting both the old and the
new binding, this is the code that looks for the old binding. If
we don't find anything then we don't want to print a message now
because we will print a message when we later check for the newer
binding. However if the binding is present but somehow invalid
then we would like to print the error. The newer binding read
later won't detect if the older binding is present and corrupt.

What do the other users of the API do?

Alas this looks like one of the first users.

Thanks,
Charles