| Message ID | 1452890030-23316-2-git-send-email-timo.teras@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Timo, 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". > > 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... Thanks. > pdata->inverted_a = flags & OF_GPIO_ACTIVE_LOW; > > - pdata->gpio_b = of_get_gpio_flags(np, 1, &flags); > + ret = of_get_gpio_flags(np, 1, &flags); > + if (ret < 0) > + return ERR_PTR(ret); > + pdata->gpio_b = ret; > pdata->inverted_b = flags & OF_GPIO_ACTIVE_LOW; > > pdata->relative_axis = > of_property_read_bool(np, "rotary-encoder,relative-axis"); > pdata->rollover = of_property_read_bool(np, "rotary-encoder,rollover"); > > - error = of_property_read_u32(np, "rotary-encoder,steps-per-period", > - &pdata->steps_per_period); > - if (error) { > + ret = of_property_read_u32(np, "rotary-encoder,steps-per-period", > + &pdata->steps_per_period); > + if (ret) { > /* > * The 'half-period' property has been deprecated, you must use > * 'steps-per-period' and set an appropriate value, but we still > -- > 2.7.0 > > -- > 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
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
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; 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; pdata->inverted_a = flags & OF_GPIO_ACTIVE_LOW; - pdata->gpio_b = of_get_gpio_flags(np, 1, &flags); + ret = of_get_gpio_flags(np, 1, &flags); + if (ret < 0) + return ERR_PTR(ret); + pdata->gpio_b = ret; pdata->inverted_b = flags & OF_GPIO_ACTIVE_LOW; pdata->relative_axis = of_property_read_bool(np, "rotary-encoder,relative-axis"); pdata->rollover = of_property_read_bool(np, "rotary-encoder,rollover"); - error = of_property_read_u32(np, "rotary-encoder,steps-per-period", - &pdata->steps_per_period); - if (error) { + ret = of_property_read_u32(np, "rotary-encoder,steps-per-period", + &pdata->steps_per_period); + if (ret) { /* * The 'half-period' property has been deprecated, you must use * 'steps-per-period' and set an appropriate value, but we still
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(-)