Re: [PATCH 2/3] Input: rotary_encoder - fix device tree error handling

[Timo Teras <timo.teras@iki.fi>]

On Sat, 16 Jan 2016 11:02:43 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Fri, Jan 15, 2016 at 10:33:49PM +0200, Timo Teräs wrote:
> > of_get_gpio_flags() can fail, so pass through the error code
> > properly. Most important use case is when it returns EPROBE_DEFER
> > so that the probe gets called again later.
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > ---
> >  drivers/input/misc/rotary_encoder.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/input/misc/rotary_encoder.c
> > b/drivers/input/misc/rotary_encoder.c index 0f23d32..156f40f 100644
> > --- a/drivers/input/misc/rotary_encoder.c
> > +++ b/drivers/input/misc/rotary_encoder.c
> > @@ -206,7 +206,7 @@ static struct rotary_encoder_platform_data
> > *rotary_encoder_parse_dt(struct devic struct device_node *np =
> > dev->of_node; struct rotary_encoder_platform_data *pdata;
> >  	enum of_gpio_flags flags;
> > -	int error;
> > +	int ret;  
> 
> I strongly prefer variables holding error codes and not being returned
> to caller in success path to be called "error".

Uh. Ok. My preference was on 'ret' or similar as the return value is
used on success path for something useful. But error be it.

> >  	if (!of_id || !np)
> >  		return NULL;
> > @@ -219,19 +219,25 @@ static struct rotary_encoder_platform_data
> > *rotary_encoder_parse_dt(struct devic of_property_read_u32(np,
> > "rotary-encoder,steps", &pdata->steps); of_property_read_u32(np,
> > "linux,axis", &pdata->axis); 
> > -	pdata->gpio_a = of_get_gpio_flags(np, 0, &flags);
> > +	ret = of_get_gpio_flags(np, 0, &flags);
> > +	if (ret < 0)
> > +		return ERR_PTR(ret);
> > +	pdata->gpio_a = ret;  
> 
> Instead of doing this maybe we could switch to gpiod interface that
> handles polarity internally and does not depend on OF? The only issue
> is that we have a few board files in tree that use raw gpio numbers so
> we'll need to convert them...

This is kinda two unrelated things... of course using gpiod_get_array()
would simplify the error path handling and other parts. And as side
effect probably fix the issue at hand too.

The only place where the platform data is used that I found is:
arch/arm/mach-pxa/raumfeld.c

So it should be relatively constrained change.

Could you perhaps also push what you already committed from me (dunno
what you decided with patch #3). If I do gpiod_* conversion, it'll
conflict with the last patch.

Thanks,
Timo
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html