diff mbox

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

Message ID 1462866662-18285-4-git-send-email-mario.six@gdsys.cc
State Superseded
Delegated to: York Sun
Headers show

Commit Message

Mario Six May 10, 2016, 7:51 a.m. UTC
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(-)

--
2.7.0.GIT

Comments

Simon Glass May 19, 2016, 3:59 a.m. UTC | #1
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
Mario Six May 19, 2016, 6:16 a.m. UTC | #2
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
diff mbox

Patch

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