Message ID | 1448803057-8572-1-git-send-email-albeu@free.fr |
---|---|
State | Deferred |
Headers | show |
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
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-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)