Re: [PATCH v2 1/2] gpio: altera-a10sr: Set proper output level for direction_output

[Axel Lin <axel.lin@xxxxxxxxxx>]

Axel Lin <axel.lin@xxxxxxxxxx> 於 2019年1月19日 週六 上午7:44寫道：
>
>
>
> Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx> 於 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@xxxxxxxxxx>
>> > ---
>> > 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