Re: [RFT][PATCH 2/2] gpio: bcm-kona: Implement get_direction callback

[Axel Lin <axel.lin@xxxxxxxxxx>]

2015-04-11 0:13 GMT+08:00 Ray Jui <rjui@xxxxxxxxxxxx>:
> Hi Axel,
>
> On 4/9/2015 6:30 PM, Axel Lin wrote:
>> Implement gpio_chip's get_direction() callback, that lets other drivers get
>> particular GPIOs direction using gpiod_get_direction().
>>
>> Signed-off-by: Axel Lin <axel.lin@xxxxxxxxxx>
>> ---
>>  drivers/gpio/gpio-bcm-kona.c | 24 ++++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
>> index 632352d..3649ed5 100644
>> --- a/drivers/gpio/gpio-bcm-kona.c
>> +++ b/drivers/gpio/gpio-bcm-kona.c
>> @@ -129,6 +129,14 @@ static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
>>       return 0;
>>  }
>>
>> +static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +     struct bcm_kona_gpio *kona_gpio = to_kona_gpio(chip);
>> +     void __iomem *reg_base = kona_gpio->reg_base;
>> +
>> +     return readl(reg_base + GPIO_CONTROL(gpio)) & GPIO_GPCTR0_IOTR_MASK;
>
> You are making assumption of actual values of GPIOF_DIR_IN and
> GPIOF_DIR_OUT. I think it would be better to return GPIOF_DIR_IN or
> GPIOF_DIR_OUT based on the register readling.

Yes, it's because GPIO_GPCTR0_IOTR_CMD_INPUT == GPIOF_DIR_IN and
GPIO_GPCTR0_IOTR_CMD_0UTPUT == GPIOF_DIR_OUT.

I can change it to below if you prefer, but current patch is shorter
and logically exactly the same.

if (readl(reg_base + GPIO_CONTROL(gpio)) & GPIO_GPCTR0_IOTR_MASK)
        return GPIOF_DIR_IN;
else
        return GPIOF_DIR_OUT;

>
> In addition, with your change, when bcm_kona_gpio_get_dir is called
> within bcm_kona_gpio_set and bcm_kona_gpio_get, it's protected by the
> kona_gpio->lock spinlock. But no lock protection while being invoked
> through the upper layer get_direction callback.

It is because bcm_kona_gpio_get_dir is a single register read.
In bcm_kona_gpio_set/bcm_kona_gpio_get, the lock is to ensure
read/write to both the DIR and LEVEL registers so we won't set/get
wrong level value in wrong DIR status.

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