[RFT][PATCH 1/2] gpio: bcm-kona: Return error if requesting an already taken gpio

[RFT][PATCH 1/2] gpio: bcm-kona: Return error if requesting an already taken gpio

[Axel Lin]

If a gpio is already taken, .gpio_request should return error.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/gpio/gpio-bcm-kona.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
index b164ce8..632352d 100644
--- a/drivers/gpio/gpio-bcm-kona.c
+++ b/drivers/gpio/gpio-bcm-kona.c
@@ -106,7 +106,7 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
 	spin_unlock_irqrestore(&kona_gpio->lock, flags);
 }
 
-static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
+static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
 					unsigned gpio)
 {
 	u32 val;
@@ -116,10 +116,17 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
 	spin_lock_irqsave(&kona_gpio->lock, flags);
 
 	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
+	if (!(val & BIT(gpio))) {
+		spin_unlock_irqrestore(&kona_gpio->lock, flags);
+		return -EBUSY;
+	}
+
 	val &= ~BIT(gpio);
 	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
 
 	spin_unlock_irqrestore(&kona_gpio->lock, flags);
+
+	return 0;
 }
 
 static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
@@ -185,8 +192,7 @@ static int bcm_kona_gpio_request(struct gpio_chip *chip, unsigned gpio)
 {
 	struct bcm_kona_gpio *kona_gpio = to_kona_gpio(chip);
 
-	bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
-	return 0;
+	return bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
 }
 
 static void bcm_kona_gpio_free(struct gpio_chip *chip, unsigned gpio)
-- 
1.9.1

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

[Axel Lin]

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@ingics.com>
---
 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;
+}
+
 static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
 {
 	struct bcm_kona_gpio *kona_gpio;
@@ -142,12 +150,8 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
 	reg_base = kona_gpio->reg_base;
 	spin_lock_irqsave(&kona_gpio->lock, flags);
 
-	/* determine the GPIO pin direction */
-	val = readl(reg_base + GPIO_CONTROL(gpio));
-	val &= GPIO_GPCTR0_IOTR_MASK;
-
 	/* this function only applies to output pin */
-	if (GPIO_GPCTR0_IOTR_CMD_INPUT == val)
+	if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN)
 		goto out;
 
 	reg_offset = value ? GPIO_OUT_SET(bank_id) : GPIO_OUT_CLEAR(bank_id);
@@ -173,13 +177,12 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio)
 	reg_base = kona_gpio->reg_base;
 	spin_lock_irqsave(&kona_gpio->lock, flags);
 
-	/* determine the GPIO pin direction */
-	val = readl(reg_base + GPIO_CONTROL(gpio));
-	val &= GPIO_GPCTR0_IOTR_MASK;
+	if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN)
+		reg_offset = GPIO_IN_STATUS(bank_id);
+	else
+		reg_offset = GPIO_OUT_STATUS(bank_id);
 
 	/* read the GPIO bank status */
-	reg_offset = (GPIO_GPCTR0_IOTR_CMD_INPUT == val) ?
-	    GPIO_IN_STATUS(bank_id) : GPIO_OUT_STATUS(bank_id);
 	val = readl(reg_base + reg_offset);
 
 	spin_unlock_irqrestore(&kona_gpio->lock, flags);
@@ -316,6 +319,7 @@ static struct gpio_chip template_chip = {
 	.owner = THIS_MODULE,
 	.request = bcm_kona_gpio_request,
 	.free = bcm_kona_gpio_free,
+	.get_direction = bcm_kona_gpio_get_dir,
 	.direction_input = bcm_kona_gpio_direction_input,
 	.get = bcm_kona_gpio_get,
 	.direction_output = bcm_kona_gpio_direction_output,
-- 
1.9.1

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

[Ray Jui]

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@ingics.com>
> ---
>  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.

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.

Thanks,

Ray

> +}
> +
>  static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
>  {
>  	struct bcm_kona_gpio *kona_gpio;
> @@ -142,12 +150,8 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
>  	reg_base = kona_gpio->reg_base;
>  	spin_lock_irqsave(&kona_gpio->lock, flags);
>  
> -	/* determine the GPIO pin direction */
> -	val = readl(reg_base + GPIO_CONTROL(gpio));
> -	val &= GPIO_GPCTR0_IOTR_MASK;
> -
>  	/* this function only applies to output pin */
> -	if (GPIO_GPCTR0_IOTR_CMD_INPUT == val)
> +	if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN)
>  		goto out;
>  
>  	reg_offset = value ? GPIO_OUT_SET(bank_id) : GPIO_OUT_CLEAR(bank_id);
> @@ -173,13 +177,12 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio)
>  	reg_base = kona_gpio->reg_base;
>  	spin_lock_irqsave(&kona_gpio->lock, flags);
>  
> -	/* determine the GPIO pin direction */
> -	val = readl(reg_base + GPIO_CONTROL(gpio));
> -	val &= GPIO_GPCTR0_IOTR_MASK;
> +	if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN)
> +		reg_offset = GPIO_IN_STATUS(bank_id);
> +	else
> +		reg_offset = GPIO_OUT_STATUS(bank_id);
>  
>  	/* read the GPIO bank status */
> -	reg_offset = (GPIO_GPCTR0_IOTR_CMD_INPUT == val) ?
> -	    GPIO_IN_STATUS(bank_id) : GPIO_OUT_STATUS(bank_id);
>  	val = readl(reg_base + reg_offset);
>  
>  	spin_unlock_irqrestore(&kona_gpio->lock, flags);
> @@ -316,6 +319,7 @@ static struct gpio_chip template_chip = {
>  	.owner = THIS_MODULE,
>  	.request = bcm_kona_gpio_request,
>  	.free = bcm_kona_gpio_free,
> +	.get_direction = bcm_kona_gpio_get_dir,
>  	.direction_input = bcm_kona_gpio_direction_input,
>  	.get = bcm_kona_gpio_get,
>  	.direction_output = bcm_kona_gpio_direction_output,
> 

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

[Axel Lin]

2015-04-11 0:13 GMT+08:00 Ray Jui <rjui@broadcom.com>:
> 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@ingics.com>
>> ---
>>  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

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

[Ray Jui]

Hi Axel,

On 4/10/2015 4:13 PM, Axel Lin wrote:
> 2015-04-11 0:13 GMT+08:00 Ray Jui <rjui@broadcom.com>:
>> 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@ingics.com>
>>> ---
>>>  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;
> 

Or one line like:

    return readl(reg_base + GPIO_CONTROL(gpio)) & GPIO_GPCTR0_IOTR_MASK
? GPIOF_DIR_IN : 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.

Yes, you are right. The locking part is fine!

> 
> Regards,
> Axel
> 

Re: [RFT][PATCH 1/2] gpio: bcm-kona: Return error if requesting an already taken gpio

[Ray Jui]

Hi Axel,

On 4/9/2015 6:28 PM, Axel Lin wrote:
> If a gpio is already taken, .gpio_request should return error.

Isn't the protection logic already there at the higher layer in gpiolib?

Ray

> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  drivers/gpio/gpio-bcm-kona.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
> index b164ce8..632352d 100644
> --- a/drivers/gpio/gpio-bcm-kona.c
> +++ b/drivers/gpio/gpio-bcm-kona.c
> @@ -106,7 +106,7 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
>  	spin_unlock_irqrestore(&kona_gpio->lock, flags);
>  }
>  
> -static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
> +static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
>  					unsigned gpio)
>  {
>  	u32 val;
> @@ -116,10 +116,17 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
>  	spin_lock_irqsave(&kona_gpio->lock, flags);
>  
>  	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> +	if (!(val & BIT(gpio))) {
> +		spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +		return -EBUSY;
> +	}
> +
>  	val &= ~BIT(gpio);
>  	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
>  
>  	spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +
> +	return 0;
>  }
>  
>  static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> @@ -185,8 +192,7 @@ static int bcm_kona_gpio_request(struct gpio_chip *chip, unsigned gpio)
>  {
>  	struct bcm_kona_gpio *kona_gpio = to_kona_gpio(chip);
>  
> -	bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
> -	return 0;
> +	return bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
>  }
>  
>  static void bcm_kona_gpio_free(struct gpio_chip *chip, unsigned gpio)
> 

Re: [RFT][PATCH 1/2] gpio: bcm-kona: Return error if requesting an already taken gpio

[Axel Lin]

2015-04-11 0:05 GMT+08:00 Ray Jui <rjui@broadcom.com>:
> Hi Axel,
>
> On 4/9/2015 6:28 PM, Axel Lin wrote:
>> If a gpio is already taken, .gpio_request should return error.
>
> Isn't the protection logic already there at the higher layer in gpiolib?

OK, you are right.
Thanks for the review.
Axel