[PATCH] pwm: Fix of_pwm_get() for consistent return values

[PATCH] pwm: Fix of_pwm_get() for consistent return values

[Alban Bedel]

When of_pwm_get() is called without connection ID it returns
-ENOENT when the 'pwms' property doesn't exists or is an empty entry.
However when a connection ID is given and the 'pwm-names' property
doesn't exists or doesn't contains the requested name it returns
-ENODATA or -EINVAL.

To get a consistent return value with both code paths we must return
-ENOENT when of_property_match_string() fails.

Signed-off-by: Alban Bedel <albeu@free.fr>
---
 drivers/pwm/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d24ca5f..3b4dcb6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -578,7 +578,7 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 	if (con_id) {
 		index = of_property_match_string(np, "pwm-names", con_id);
 		if (index < 0)
-			return ERR_PTR(index);
+			return ERR_PTR(-ENOENT);
 	}
 
 	err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", index,
-- 
2.0.0

Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values

[Thierry Reding]

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

On Sun, Nov 29, 2015 at 02:17:37PM +0100, Alban Bedel wrote:
> When of_pwm_get() is called without connection ID it returns
> -ENOENT when the 'pwms' property doesn't exists or is an empty entry.
> However when a connection ID is given and the 'pwm-names' property
> doesn't exists or doesn't contains the requested name it returns
> -ENODATA or -EINVAL.
> 
> To get a consistent return value with both code paths we must return
> -ENOENT when of_property_match_string() fails.

I'm not sure I understand the need for a consistent return value here.
These are all different error conditions and the only reasonable thing
to do here is propagate the error code from the of_*() parsing routine
that is being called.

Thierry

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

Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values

[Alban]

On Mon, 4 Jan 2016 09:10:28 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Sun, Nov 29, 2015 at 02:17:37PM +0100, Alban Bedel wrote:
> > When of_pwm_get() is called without connection ID it returns
> > -ENOENT when the 'pwms' property doesn't exists or is an empty
> > entry. However when a connection ID is given and the 'pwm-names'
> > property doesn't exists or doesn't contains the requested name it
> > returns -ENODATA or -EINVAL.
> > 
> > To get a consistent return value with both code paths we must return
> > -ENOENT when of_property_match_string() fails.
> 
> I'm not sure I understand the need for a consistent return value here.
> These are all different error conditions and the only reasonable thing
> to do here is propagate the error code from the of_*() parsing routine
> that is being called.

The problem is with optional named PWM, they will not return a clean
-ENOENT like with no name. It mean you need a different error handling
with and without name, that's error prone and unexpected. For example
that mean:

	err = pwm_get(dev, NULL);
	if (IS_ERR(err) && PTR_ERR(err) != -ENOENT)
		return PTR_ERR(err);

need to move to:

	err = pwm_get(dev, "CLK");
	if (IS_ERR(err) && PTR_ERR(err) != -ENODATA &&
			   PTR_ERR(err) != -EINVAL)
		return err;

To be somewhat equivalent, but not really. Beside the inconsistence I
also find it problematic because of the -EINVAL. It is returned because
of_pwm_get() itself pass a broken argument to
of_parse_phandle_with_args(), not the caller of of_pwm_get().

TBH I first found and fixed this error in the reset controller
framework, I later checked all users of of_property_match_string() for
similar error and found the PWM and clock subsystem to be lacking. It
sure is not essential as there is probably few, or no users of optional
PWM. However as code often get copy-pasted I think it is still
important to have a correct pattern everywhere.

Alban