All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pwm: Fix of_pwm_get() for consistent return values
@ 2015-11-29 13:17 Alban Bedel
  2016-01-04  8:10 ` Thierry Reding
  0 siblings, 1 reply; 3+ messages in thread
From: Alban Bedel @ 2015-11-29 13:17 UTC (permalink / raw)
  To: linux-pwm; +Cc: Alban Bedel, Thierry Reding, linux-kernel

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values
  2015-11-29 13:17 [PATCH] pwm: Fix of_pwm_get() for consistent return values Alban Bedel
@ 2016-01-04  8:10 ` Thierry Reding
  2016-01-04 13:51   ` Alban
  0 siblings, 1 reply; 3+ messages in thread
From: Thierry Reding @ 2016-01-04  8:10 UTC (permalink / raw)
  To: Alban Bedel; +Cc: linux-pwm, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values
  2016-01-04  8:10 ` Thierry Reding
@ 2016-01-04 13:51   ` Alban
  0 siblings, 0 replies; 3+ messages in thread
From: Alban @ 2016-01-04 13:51 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Aban Bedel, linux-pwm, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-01-04 13:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-29 13:17 [PATCH] pwm: Fix of_pwm_get() for consistent return values Alban Bedel
2016-01-04  8:10 ` Thierry Reding
2016-01-04 13:51   ` Alban

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.