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

Simon Glass
2016-05-19T03:59:48Z

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?

> +}
> +
> +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