| Message ID | 20190117124015.19104-1-axel.lin@ingics.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] gpio: altera-a10sr: Set proper output level for direction_output | expand |
czw., 17 sty 2019 o 13:40 Axel Lin <axel.lin@ingics.com> napisał(a): > > The altr_a10sr_gpio_direction_output should set proper output level > based on the value argument. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> Please add a Fixes: and Cc stable tags here. Take a look at recent commits from linux-stable for reference. Bart > --- > v2: Based on Bartosz's comment to split the patch. > 1/2 is bug fix > 2/2 is coding style fix and fix checkpatch warning > drivers/gpio/gpio-altera-a10sr.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-altera-a10sr.c b/drivers/gpio/gpio-altera-a10sr.c > index 6b11f1314248..7f9e0304b510 100644 > --- a/drivers/gpio/gpio-altera-a10sr.c > +++ b/drivers/gpio/gpio-altera-a10sr.c > @@ -66,8 +66,10 @@ static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, > static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, > unsigned int nr, int value) > { > - if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) > + if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) { > + altr_a10sr_gpio_set(gc, nr, value); > return 0; > + } > return -EINVAL; > } > > -- > 2.17.1 >
Hi Axel, On 1/17/19 6:40 AM, Axel Lin wrote: > The altr_a10sr_gpio_direction_output should set proper output level > based on the value argument. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > v2: Based on Bartosz's comment to split the patch. > 1/2 is bug fix > 2/2 is coding style fix and fix checkpatch warning > drivers/gpio/gpio-altera-a10sr.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-altera-a10sr.c b/drivers/gpio/gpio-altera-a10sr.c > index 6b11f1314248..7f9e0304b510 100644 > --- a/drivers/gpio/gpio-altera-a10sr.c > +++ b/drivers/gpio/gpio-altera-a10sr.c > @@ -66,8 +66,10 @@ static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, > static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, > unsigned int nr, int value) > { > - if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) > + if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) { > + altr_a10sr_gpio_set(gc, nr, value); > return 0; > + } > return -EINVAL; > } > > Sorry, this patch is not valid. I should add a comment above these functions to explain better. These are actually GPI and GPO on a custom expansion chip. The directions are hard coded as INPUT or OUTPUT. If the range is valid, it returns 0 and -EINVAL if not. For GPO, the only valid values are from 4 to 7. For GPI, reading 4 to 7 will tell you what is being written on GPO. There is another GPI bank starting at 8 and extending to 15. I do agree that altr_a10sr_gpio_direction_input() should be better constrained from 4 to 15. Thanks for pointing that out. Thor
Axel Lin <axel.lin@ingics.com> 於 2019年1月19日 週六 上午7:44寫道： > > > > Thor Thayer <thor.thayer@linux.intel.com> 於 2019年1月19日 週六 上午2:50寫道： >> >> Hi Axel, >> >> On 1/17/19 6:40 AM, Axel Lin wrote: >> > The altr_a10sr_gpio_direction_output should set proper output level >> > based on the value argument. >> > >> > Signed-off-by: Axel Lin <axel.lin@ingics.com> >> > --- >> > v2: Based on Bartosz's comment to split the patch. >> > 1/2 is bug fix >> > 2/2 is coding style fix and fix checkpatch warning >> > drivers/gpio/gpio-altera-a10sr.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpio/gpio-altera-a10sr.c b/drivers/gpio/gpio-altera-a10sr.c >> > index 6b11f1314248..7f9e0304b510 100644 >> > --- a/drivers/gpio/gpio-altera-a10sr.c >> > +++ b/drivers/gpio/gpio-altera-a10sr.c >> > @@ -66,8 +66,10 @@ static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, >> > static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, >> > unsigned int nr, int value) >> > { >> > - if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) >> > + if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) { >> > + altr_a10sr_gpio_set(gc, nr, value); >> > return 0; >> > + } >> > return -EINVAL; >> > } >> > >> > >> >> Sorry, this patch is not valid. I should add a comment above these >> functions to explain better. These are actually GPI and GPO on a custom >> expansion chip. The directions are hard coded as INPUT or OUTPUT. If the >> range is valid, it returns 0 and -EINVAL if not. >> >> For GPO, the only valid values are from 4 to 7. > > Hi Thor, > It seems you misunderstand the point. > from 4 to 7 seems to be the nr argument. > The value argument is for gpio output level HIGH or LOW. > This patch does not change the nr argument checking, what it does is > calling altr_a10sr_gpio_set(gc, nr, value); to ensure the output value is set > to the setting of altr_a10sr_gpio_direction_output(). Hi Thor, Any update regarding my comment above? Thanks, Axel
Hi Axel, On 1/18/19 5:44 PM, Axel Lin wrote: > Thor Thayer <thor.thayer@linux.intel.com> 於 2019年1月19日 週六 上午2:50寫道： > >> Hi Axel, >> >> On 1/17/19 6:40 AM, Axel Lin wrote: >>> The altr_a10sr_gpio_direction_output should set proper output level >>> based on the value argument. >>> >>> Signed-off-by: Axel Lin <axel.lin@ingics.com> >>> --- >>> v2: Based on Bartosz's comment to split the patch. >>> 1/2 is bug fix >>> 2/2 is coding style fix and fix checkpatch warning >>> drivers/gpio/gpio-altera-a10sr.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpio/gpio-altera-a10sr.c >> b/drivers/gpio/gpio-altera-a10sr.c >>> index 6b11f1314248..7f9e0304b510 100644 >>> --- a/drivers/gpio/gpio-altera-a10sr.c >>> +++ b/drivers/gpio/gpio-altera-a10sr.c >>> @@ -66,8 +66,10 @@ static int altr_a10sr_gpio_direction_input(struct >> gpio_chip *gc, >>> static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, >>> unsigned int nr, int value) >>> { >>> - if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - >> ALTR_A10SR_LED_VALID_SHIFT)) >>> + if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - >> ALTR_A10SR_LED_VALID_SHIFT)) { >>> + altr_a10sr_gpio_set(gc, nr, value); >>> return 0; >>> + } >>> return -EINVAL; >>> } >>> >>> >> >> Sorry, this patch is not valid. I should add a comment above these >> functions to explain better. These are actually GPI and GPO on a custom >> expansion chip. The directions are hard coded as INPUT or OUTPUT. If the >> range is valid, it returns 0 and -EINVAL if not. >> >> For GPO, the only valid values are from 4 to 7. >> > Hi Thor, > It seems you misunderstand the point. > from 4 to 7 seems to be the nr argument. > The value argument is for gpio output level HIGH or LOW. > This patch does not change the nr argument checking, what it does is > calling altr_a10sr_gpio_set(gc, nr, value); to ensure the output value is > set > to the setting of altr_a10sr_gpio_direction_output(). > > Regards, > Axel > Yes, I see. This patch looks good and I successfully tested on our platform. Thanks, Tested by: Thor Thayer <thor.thayer@linux.intel.com> Reviewed by: Thor Thayer <thor.thayer@linux.intel.com> > For GPI, reading 4 to 7 will tell you what is being written on GPO. >> There is another GPI bank starting at 8 and extending to 15. >> >> I do agree that altr_a10sr_gpio_direction_input() should be better >> constrained from 4 to 15. >> >> Thanks for pointing that out. >> >> Thor >> >
diff --git a/drivers/gpio/gpio-altera-a10sr.c b/drivers/gpio/gpio-altera-a10sr.c index 6b11f1314248..7f9e0304b510 100644 --- a/drivers/gpio/gpio-altera-a10sr.c +++ b/drivers/gpio/gpio-altera-a10sr.c @@ -66,8 +66,10 @@ static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, unsigned int nr, int value) { - if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) + if (nr <= (ALTR_A10SR_OUT_VALID_RANGE_HI - ALTR_A10SR_LED_VALID_SHIFT)) { + altr_a10sr_gpio_set(gc, nr, value); return 0; + } return -EINVAL; }
The altr_a10sr_gpio_direction_output should set proper output level based on the value argument. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- v2: Based on Bartosz's comment to split the patch. 1/2 is bug fix 2/2 is coding style fix and fix checkpatch warning drivers/gpio/gpio-altera-a10sr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)