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

[Alban <albeu@free.fr>]

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