Re: [U-Boot] [PATCH v2 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

On Thu, May 19, 2016 at 5:59 AM, Simon Glass sjg@chromium.org wrote:

Hi Mario,

On 10 May 2016 at 01:51, Mario Six mario.six@gdsys.cc wrote:

...

This patch implements the open-drain setting feature for the MPC85XX GPIO controller.

Signed-off-by: Mario Six mario.six@gdsys.cc

v3:

• Added missing commit message
• Fixed white space issues in function headers

---

drivers/gpio/Kconfig | 6 +++--- drivers/gpio/mpc85xx_gpio.c | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

But please see below.

...

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 068ee63..b250622 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -162,9 +162,9 @@ config MPC85XX_GPIO configurable to match the actual GPIO count of the SoC (e.g. the 32/32/23 banks of the P1022 SoC).

•     The standard functions of input/output mode, and output value setting
  

•     are supported; the open-drain capability of the controller is not
  

•     supported yet.
  

•     Aside from the standard functions of input/output mode, and output
  

•     value setting, the open-drain feature, which can configure individual
  

•     GPIOs to work as open-drain outputs, is supported.
  
      The driver has been tested on MPC85XX, but it is likely that other
      PowerQUICC III devices will work as well.
  

diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c index acf0414..dc6193c 100644 --- a/drivers/gpio/mpc85xx_gpio.c +++ b/drivers/gpio/mpc85xx_gpio.c @@ -73,6 +73,25 @@ static inline void mpc85xx_gpio_set_high(struct ccsr_gpio *base, setbits_be32(&base->gpdir, gpios); }

+static inline int mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base,

•                                         unsigned int mask)
  

+{

•   /* Read the requested values */
  

•   return in_be32(&base->gpodr) & mask;
  

+}

+static inline void mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base,

•                                         unsigned int gpios)
  

+{

•   setbits_be32(&base->gpodr, gpios);
  

Why gpios? This would normally be 'offset', indicating that it is the GPIO offset within the bank.

Also the code seems odd - don't you need to convert the value into a mask?

Ah, yes. Sorry, just noticed that I missed the actual mpc85xx_gpio_{set,get}_open_drain functions when merging the patches. You might notice that nothing is actually added to the dm_gpio_ops structure as well (that's what I get for trying to follow the original code style too closely). Will add those in v4, and the test/dm/gpio.c / sandbox additions as well.

...

+}

+static inline void mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base,

•                                          unsigned int gpios)
  

+{

•   clrbits_be32(&base->gpodr, gpios);
  

+}

static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio) { struct mpc85xx_gpio_data *data = dev_get_priv(dev); -- 2.7.0.GIT

Regards, Simon

Thanks for reviewing!

Best regards, Mario