Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300538 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values Thierry Reding <thierry.reding@gmail.com> - 2016-01-04 09:20 +0100
    Re: [PATCH] pwm: Fix of_pwm_get() for consistent return values Alban <albeu@free.fr> - 2016-01-04 15:00 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1300756

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web