gpio: bcm-kona: Return error if requesting an already taken gpio
PATCH 1/2 gpio: bcm-kona: Return error if requesting an already taken gpio
If a gpio is already taken, .gpio_request should return error.
--- drivers/gpio/gpio-bcm-kona.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/gpio/gpio-bcm-kona.c +9 -3
@@ -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)
Signature · 1 line
1.9.1
PATCH 2/2 gpio: bcm-kona: Implement get_direction callback
Implement gpio_chip's get_direction() callback, that lets other drivers get particular GPIOs direction using gpiod_get_direction().
--- drivers/gpio/gpio-bcm-kona.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
drivers/gpio/gpio-bcm-kona.c +14 -10
@@ -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,
Signature · 1 line
1.9.1
Ray Jui
maintainerRe: [RFT][PATCH 2/2] gpio: bcm-kona: Implement get_direction callback
Hi Axel,
Quoted 13 lines · Axel Lin
Implement gpio_chip's
get_direction()callback, that lets other drivers get particular GPIOs direction usinggpiod_get_direction().Signed-off-by Axel Lin <[email protected]>--- drivers/gpio/gpio-bcm-kona.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)drivers/gpio/gpio-bcm-kona.c +0
-0
drivers/gpio/gpio-bcm-kona.c static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, · line 137
Quoted 9 lines
drivers/gpio/gpio-bcm-kona.c +6
-0@@ -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
Quoted 46 lines
drivers/gpio/gpio-bcm-kona.c +8
-10+} +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
2015-04-11 0:13 GMT+08:00 Ray Jui <[email protected]>:
Quoted 19 lines
Hi Axel,
Quoted 16 lines · Axel Lin
Implement gpio_chip's
get_direction()callback, that lets other drivers get particular GPIOs direction usinggpiod_get_direction().Signed-off-by Axel Lin <[email protected]>--- drivers/gpio/gpio-bcm-kona.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)drivers/gpio/gpio-bcm-kona.c +0
-0@@ -129,6 +129,14 @@ static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,return 0 ; }
Quoted 10 lines
drivers/gpio/gpio-bcm-kona.c static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, · line 137
Quoted 6 lines
drivers/gpio/gpio-bcm-kona.c +6
-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_INandGPIOF_DIR_OUT. I think it would be better to returnGPIOF_DIR_INorGPIOF_DIR_OUTbased 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;Quoted 5 lines
In addition, with your change, when
bcm_kona_gpio_get_diris called withinbcm_kona_gpio_setandbcm_kona_gpio_get, it's protected by thekona_gpio->lockspinlock. But no lock protection while being invoked through the upper layerget_directioncallback.
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
Ray Jui
maintainerRe: [RFT][PATCH 2/2] gpio: bcm-kona: Implement get_direction callback
Hi Axel,
Quoted 34 lines · Axel Lin
2015-04-11 0:13 GMT+08:00 Ray Jui <[email protected]>:
Quoted 19 lines
Hi Axel,
Quoted 16 lines · Axel Lin
Implement gpio_chip's
get_direction()callback, that lets other drivers get particular GPIOs direction usinggpiod_get_direction().Signed-off-by Axel Lin <[email protected]>--- drivers/gpio/gpio-bcm-kona.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)drivers/gpio/gpio-bcm-kona.c +0
-0@@ -129,6 +129,14 @@ static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,return 0 ; }Quoted 10 lines
drivers/gpio/gpio-bcm-kona.c static int bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, · line 137
Quoted 6 lines
drivers/gpio/gpio-bcm-kona.c +6
-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_INandGPIOF_DIR_OUT. I think it would be better to returnGPIOF_DIR_INorGPIOF_DIR_OUTbased on the register readling.Yes, it's because
GPIO_GPCTR0_IOTR_CMD_INPUT==GPIOF_DIR_INandGPIO_GPCTR0_IOTR_CMD_0UTPUT==GPIOF_DIR_OUT.
Quoted 8 lines
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;
Quoted 10 lines
Quoted 5 lines
In addition, with your change, when
bcm_kona_gpio_get_diris called withinbcm_kona_gpio_setandbcm_kona_gpio_get, it's protected by thekona_gpio->lockspinlock. But no lock protection while being invoked through the upper layerget_directioncallback.It is because
bcm_kona_gpio_get_diris 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!
Quoted 4 lines
Regards,
Axel
Ray Jui
maintainerHi Axel,
Quoted 1 line · Axel Lin
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
Quoted 48 lines
Signed-off-by Axel Lin <[email protected]>--- drivers/gpio/gpio-bcm-kona.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpio/gpio-bcm-kona.c +9
-3@@ -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)
2015-04-11 0:05 GMT+08:00 Ray Jui <[email protected]>:
Quoted 6 lines
Hi Axel,
Quoted 1 line · Axel Lin
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