On Thu, May 19, 2016 at 5:59 AM, Simon Glass <s...@chromium.org> wrote: > Hi Mario, > > On 10 May 2016 at 01:51, Mario Six <mario....@gdsys.cc> wrote: >> This patch implements the open-drain setting feature for the MPC85XX >> GPIO controller. >> >> Signed-off-by: Mario Six <mario....@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 <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot