Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan]

Hello Stephen,

On 7/15/2012 10:53 PM, Stephen Warren wrote:

On 07/11/2012 02:37 PM, Vikram Narayanan wrote:

Driver for BCM2835 SoC. This gives the basic functionality of
setting/clearing the output.



diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h 
b/arch/arm/include/asm/arch-bcm2835/gpio.h



+#define BCM2835_GPIO_BASE  0x7E20
+#define BCM2835_NUM_GPIOS  53


For consistency, that might be better as BCM2835_GPIO_COUNT, but not a
big deal.


diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile



  COBJS-$(CONFIG_DA8XX_GPIO)+= da8xx_gpio.o
  COBJS-$(CONFIG_ALTERA_PIO)+= altera_pio.o
  COBJS-$(CONFIG_MPC83XX_GPIO)  += mpc83xx_gpio.o
+COBJS-$(CONFIG_BCM2835_GPIO)   += gpio_bcm2835.o


It looks like the name bcm2835_gpio.c would be more consistent with
existing drivers, but not a big deal.


diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c




Linux kernel follows this naming, to be exact, it should've been 
gpio-bcm2835.c. Having a thought in mind that one day the namings would 
be made consistent with the kernel. That is the reason for this naming, 
but isn't a big deal to change it.



+inline int gpio_is_valid(unsigned gpio)
+{
+   return (gpio  BCM2835_NUM_GPIOS) ? 0 : 1;


Presumably gpio==0 is a valid GPIO, so that should be= not. It'd be
simpler to write it as:

return gpio  BCM2835_NUM_GPIOS;


+int gpio_request(unsigned gpio, const char *label)
+{
+   return (gpio_is_valid(gpio)) ? 1 : 0;


Why not just return gpio_is_valid_(gpio) directly?


+int gpio_direction_input(unsigned gpio)



+   val = readl(reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
+   val= ~(BCM2835_GPIO_FSEL_MASK  BCM2835_GPIO_FSEL_SHIFT(gpio));


Even if BCM2835_GPIO_OUTPUT==0, it seems better to | it in here for
documentation purposes, so add:

val |= (BCM2835_GPIO_INPUT  BCM2835_GPIO_FSEL_SHIFT(gpio));

Otherwise, there's not much point creating the #define BCM2835_GPIO_INPUT.


+int gpio_direction_output(unsigned gpio, int value)
+{
+   struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
+   unsigned val;
+
+   val = readl(reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
+   val= ~(BCM2835_GPIO_FSEL_MASK  BCM2835_GPIO_FSEL_SHIFT(gpio));
+   val |= (BCM2835_GPIO_OUTPUT  BCM2835_GPIO_FSEL_SHIFT(gpio));
+   writel(val, reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);


This (setting the direction) should happen after the following to set
the value:


+   if (value)
+   gpio_set_value(gpio, value);


That way, when the GPIO is set to output, the correct value will
immediately be driven onto the GPIO, so a glitch may be avoided.


+int gpio_get_value(unsigned gpio)



+   return (val  BCM2835_GPIO_COMMON_MASK(gpio))  0x1;




Agree for all the above. Will get reflected in the v3.


Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?


If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then 
it shall be COMMON_SHIFT.


But it doesn't do any shifting like the FSEL_SHIFT, rather it does only 
masking of bits. So, it makes more sense for me to name it as MASK and 
not SHIFT.


~Vikram
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Stephen Warren]

On 07/31/2012 09:46 AM, Vikram Narayanan wrote:
 On 7/15/2012 10:53 PM, Stephen Warren wrote:
 On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
 Driver for BCM2835 SoC. This gives the basic functionality of
 setting/clearing the output.

 diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h
 b/arch/arm/include/asm/arch-bcm2835/gpio.h

One more comment on the patch subject; it probably should be gpio:
bcm2835: not bcm: since (a) it's in the GPIO directory and (b) the
GPIO module is specifically for a BCM2835, and probably doesn't apply to
any/all Broadcom devices.

 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile

   COBJS-$(CONFIG_DA8XX_GPIO)+= da8xx_gpio.o
   COBJS-$(CONFIG_ALTERA_PIO)+= altera_pio.o
   COBJS-$(CONFIG_MPC83XX_GPIO)+= mpc83xx_gpio.o
 +COBJS-$(CONFIG_BCM2835_GPIO)+= gpio_bcm2835.o

 It looks like the name bcm2835_gpio.c would be more consistent with
 existing drivers, but not a big deal.

 diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c
 
 Linux kernel follows this naming, to be exact, it should've been
 gpio-bcm2835.c. Having a thought in mind that one day the namings would
 be made consistent with the kernel. That is the reason for this naming,
 but isn't a big deal to change it.

Hmmm. It seems better to be internally consistent with U-Boot rather
than keeping (onyl part of) U-Boot consistent with the kernel...

 Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
 
 If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then
 it shall be COMMON_SHIFT.
 
 But it doesn't do any shifting like the FSEL_SHIFT, rather it does only
 masking of bits. So, it makes more sense for me to name it as MASK and
 not SHIFT.

The full quote you're replying to was:

 +int gpio_get_value(unsigned gpio)
 
 +return (val  BCM2835_GPIO_COMMON_MASK(gpio))  0x1;
 
 Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?

... so that macro is being used as a shift not as a mask.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan]

On 7/31/2012 9:22 PM, Stephen Warren wrote:

On 07/31/2012 09:46 AM, Vikram Narayanan wrote:

On 7/15/2012 10:53 PM, Stephen Warren wrote:

On 07/11/2012 02:37 PM, Vikram Narayanan wrote:

Driver for BCM2835 SoC. This gives the basic functionality of
setting/clearing the output.



diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h
b/arch/arm/include/asm/arch-bcm2835/gpio.h


One more comment on the patch subject; it probably should be gpio:
bcm2835: not bcm: since (a) it's in the GPIO directory and (b) the
GPIO module is specifically for a BCM2835, and probably doesn't apply to
any/all Broadcom devices.



Linux kernel follows this naming, to be exact, it should've been
gpio-bcm2835.c. Having a thought in mind that one day the namings would
be made consistent with the kernel. That is the reason for this naming,
but isn't a big deal to change it.


Hmmm. It seems better to be internally consistent with U-Boot rather
than keeping (onyl part of) U-Boot consistent with the kernel...


Yes.




Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?


If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then
it shall be COMMON_SHIFT.

But it doesn't do any shifting like the FSEL_SHIFT, rather it does only
masking of bits. So, it makes more sense for me to name it as MASK and
not SHIFT.


The full quote you're replying to was:


+int gpio_get_value(unsigned gpio)



+   return (val  BCM2835_GPIO_COMMON_MASK(gpio))  0x1;


Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?


... so that macro is being used as a shift not as a mask.


Naming isn't really a problem for me. If you want it to be SHIFT, I'd go 
with it.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Stephen Warren]

On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
 Driver for BCM2835 SoC. This gives the basic functionality of
 setting/clearing the output.

 diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h 
 b/arch/arm/include/asm/arch-bcm2835/gpio.h

 +#define BCM2835_GPIO_BASE0x7E20
 +#define BCM2835_NUM_GPIOS53

For consistency, that might be better as BCM2835_GPIO_COUNT, but not a
big deal.

 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile

  COBJS-$(CONFIG_DA8XX_GPIO)   += da8xx_gpio.o
  COBJS-$(CONFIG_ALTERA_PIO)   += altera_pio.o
  COBJS-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o
 +COBJS-$(CONFIG_BCM2835_GPIO) += gpio_bcm2835.o

It looks like the name bcm2835_gpio.c would be more consistent with
existing drivers, but not a big deal.

 diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c

 +inline int gpio_is_valid(unsigned gpio)
 +{
 + return (gpio  BCM2835_NUM_GPIOS) ? 0 : 1;

Presumably gpio==0 is a valid GPIO, so that should be = not . It'd be
simpler to write it as:

return gpio  BCM2835_NUM_GPIOS;

 +int gpio_request(unsigned gpio, const char *label)
 +{
 + return (gpio_is_valid(gpio)) ? 1 : 0;

Why not just return gpio_is_valid_(gpio) directly?

 +int gpio_direction_input(unsigned gpio)

 + val = readl(reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
 + val = ~(BCM2835_GPIO_FSEL_MASK  BCM2835_GPIO_FSEL_SHIFT(gpio));

Even if BCM2835_GPIO_OUTPUT==0, it seems better to | it in here for
documentation purposes, so add:

val |= (BCM2835_GPIO_INPUT  BCM2835_GPIO_FSEL_SHIFT(gpio));

Otherwise, there's not much point creating the #define BCM2835_GPIO_INPUT.

 +int gpio_direction_output(unsigned gpio, int value)
 +{
 + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
 + unsigned val;
 +
 + val = readl(reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
 + val = ~(BCM2835_GPIO_FSEL_MASK  BCM2835_GPIO_FSEL_SHIFT(gpio));
 + val |= (BCM2835_GPIO_OUTPUT  BCM2835_GPIO_FSEL_SHIFT(gpio));
 + writel(val, reg-gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);

This (setting the direction) should happen after the following to set
the value:

 + if (value)
 + gpio_set_value(gpio, value);

That way, when the GPIO is set to output, the correct value will
immediately be driven onto the GPIO, so a glitch may be avoided.

 +int gpio_get_value(unsigned gpio)

 + return (val  BCM2835_GPIO_COMMON_MASK(gpio))  0x1;

Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?

 +int gpio_set_value(unsigned gpio, int value)
 +{
 + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
 + u32 *output_reg = value ? reg-gpset : reg-gpclr;
 +
 + writel(1  BCM2835_GPIO_COMMON_MASK(gpio),
 + output_reg[BCM2835_GPIO_COMMON_BANK(gpio)]);

Same comment here.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot