[BUGFIX][PATCH] gpiolib: add missing initialisation of bgc->dir

[BUGFIX][PATCH] gpiolib: add missing initialisation of bgc->dir

[Lothar Waßmann]

bgpio_init() does not initialise the shadow variable for the GPIO_GDIR
register. Thus the first 'set_direction' call will clear the whole
GPIO_GDIR register of the affected GPIO bank.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

[PATCH] add missing intialisation of bgc->dir

[Lothar Waßmann]

---
 drivers/gpio/basic_mmio_gpio.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 8152e9f..e69a453 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
 		return ret;
 
 	bgc->data = bgc->read_reg(bgc->reg_dat);
+	bgc->dir = bgc->read_reg(bgc->reg_dir);
 
 	return ret;
 }
-- 
1.5.6.5

Re: [PATCH] add missing intialisation of bgc->dir

[Grant Likely]

On Fri, Jul 01, 2011 at 02:08:51PM +0200, Lothar Waßmann wrote:
> ---
>  drivers/gpio/basic_mmio_gpio.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 8152e9f..e69a453 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
>  		return ret;
>  
>  	bgc->data = bgc->read_reg(bgc->reg_dat);
> +	bgc->dir = bgc->read_reg(bgc->reg_dir);

Hi Lothar.

You'll need to double check your email setup.  The patch got emailed
separately from the description.

I don't know if this is a valid thing to do or not.  Not every GPIO
controller supports reading back the initial state from the direction
register.  I suspect it is better to leave initialization of the
direction to the gpio controller driver itself.

g.

Re: [PATCH] add missing intialisation of bgc->dir

[Lothar Waßmann]

Hi,

Grant Likely writes:
> On Fri, Jul 01, 2011 at 02:08:51PM +0200, Lothar Waßmann wrote:
> > ---
> >  drivers/gpio/basic_mmio_gpio.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> > index 8152e9f..e69a453 100644
> > --- a/drivers/gpio/basic_mmio_gpio.c
> > +++ b/drivers/gpio/basic_mmio_gpio.c
> > @@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> >  		return ret;
> >  
> >  	bgc->data = bgc->read_reg(bgc->reg_dat);
> > +	bgc->dir = bgc->read_reg(bgc->reg_dir);
> 
> Hi Lothar.
> 
> You'll need to double check your email setup.  The patch got emailed
> separately from the description.
> 
git-send-email did that.

> I don't know if this is a valid thing to do or not.  Not every GPIO
> controller supports reading back the initial state from the direction
> register.  I suspect it is better to leave initialization of the
> direction to the gpio controller driver itself.
> 
That would mean, that external software would have to have access to
the bgpio internal data structures which is a Bad Thing(TM) IMO. Or we
need a function that allows to explicitly initialize the internal
shadow registers.



Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] add missing intialisation of bgc->dir

[Grant Likely]

On Mon, Jul 04, 2011 at 11:37:57AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Grant Likely writes:
> > On Fri, Jul 01, 2011 at 02:08:51PM +0200, Lothar Waßmann wrote:
> > > ---
> > >  drivers/gpio/basic_mmio_gpio.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> > > index 8152e9f..e69a453 100644
> > > --- a/drivers/gpio/basic_mmio_gpio.c
> > > +++ b/drivers/gpio/basic_mmio_gpio.c
> > > @@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> > >  		return ret;
> > >  
> > >  	bgc->data = bgc->read_reg(bgc->reg_dat);
> > > +	bgc->dir = bgc->read_reg(bgc->reg_dir);
> > 
> > Hi Lothar.
> > 
> > You'll need to double check your email setup.  The patch got emailed
> > separately from the description.
> > 
> git-send-email did that.

It appears that you asked it to send a cover letter, which you usually
don't want when sending a single patch.  The patch description should
be part of the git commit itself.

> 
> > I don't know if this is a valid thing to do or not.  Not every GPIO
> > controller supports reading back the initial state from the direction
> > register.  I suspect it is better to leave initialization of the
> > direction to the gpio controller driver itself.
> > 
> That would mean, that external software would have to have access to
> the bgpio internal data structures which is a Bad Thing(TM) IMO. Or we
> need a function that allows to explicitly initialize the internal
> shadow registers.

What device are you using this for.  Are you using the basic gpio
platform_driver, or do you have a driver using the library part of the
basic gpio code?

g.

Re: [PATCH] add missing intialisation of bgc->dir

[Lothar Waßmann]

Hi,

Grant Likely writes:
> On Mon, Jul 04, 2011 at 11:37:57AM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > Grant Likely writes:
> > > On Fri, Jul 01, 2011 at 02:08:51PM +0200, Lothar Waßmann wrote:
> > > > ---
> > > >  drivers/gpio/basic_mmio_gpio.c |    1 +
> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> > > > index 8152e9f..e69a453 100644
> > > > --- a/drivers/gpio/basic_mmio_gpio.c
> > > > +++ b/drivers/gpio/basic_mmio_gpio.c
> > > > @@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> > > >  		return ret;
> > > >  
> > > >  	bgc->data = bgc->read_reg(bgc->reg_dat);
> > > > +	bgc->dir = bgc->read_reg(bgc->reg_dir);
> > > 
> > > Hi Lothar.
> > > 
> > > You'll need to double check your email setup.  The patch got emailed
> > > separately from the description.
> > > 
> > git-send-email did that.
> 
> It appears that you asked it to send a cover letter, which you usually
> don't want when sending a single patch.  The patch description should
> be part of the git commit itself.
>
Yes, I noticed that already.

> > > I don't know if this is a valid thing to do or not.  Not every GPIO
> > > controller supports reading back the initial state from the direction
> > > register.  I suspect it is better to leave initialization of the
> > > direction to the gpio controller driver itself.
> > > 
> > That would mean, that external software would have to have access to
> > the bgpio internal data structures which is a Bad Thing(TM) IMO. Or we
> > need a function that allows to explicitly initialize the internal
> > shadow registers.
> 
> What device are you using this for.  Are you using the basic gpio
> platform_driver, or do you have a driver using the library part of the
> basic gpio code?
> 
I'm using it via drivers/gpio/gpio-mxc.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] add missing intialisation of bgc->dir

[Grant Likely]

On Mon, Jul 4, 2011 at 9:51 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> Grant Likely writes:
>> On Mon, Jul 04, 2011 at 11:37:57AM +0200, Lothar Waßmann wrote:
>> > Hi,
>> >
>> > Grant Likely writes:
>> > > On Fri, Jul 01, 2011 at 02:08:51PM +0200, Lothar Waßmann wrote:
>> > > > ---
>> > > >  drivers/gpio/basic_mmio_gpio.c |    1 +
>> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
>> > > > index 8152e9f..e69a453 100644
>> > > > --- a/drivers/gpio/basic_mmio_gpio.c
>> > > > +++ b/drivers/gpio/basic_mmio_gpio.c
>> > > > @@ -399,6 +399,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
>> > > >                 return ret;
>> > > >
>> > > >         bgc->data = bgc->read_reg(bgc->reg_dat);
>> > > > +       bgc->dir = bgc->read_reg(bgc->reg_dir);
>> > >
>> > > Hi Lothar.
>> > >
>> > > You'll need to double check your email setup.  The patch got emailed
>> > > separately from the description.
>> > >
>> > git-send-email did that.
>>
>> It appears that you asked it to send a cover letter, which you usually
>> don't want when sending a single patch.  The patch description should
>> be part of the git commit itself.
>>
> Yes, I noticed that already.
>
>> > > I don't know if this is a valid thing to do or not.  Not every GPIO
>> > > controller supports reading back the initial state from the direction
>> > > register.  I suspect it is better to leave initialization of the
>> > > direction to the gpio controller driver itself.
>> > >
>> > That would mean, that external software would have to have access to
>> > the bgpio internal data structures which is a Bad Thing(TM) IMO. Or we
>> > need a function that allows to explicitly initialize the internal
>> > shadow registers.
>>
>> What device are you using this for.  Are you using the basic gpio
>> platform_driver, or do you have a driver using the library part of the
>> basic gpio code?
>>
> I'm using it via drivers/gpio/gpio-mxc.

You can safely do the "bgc->dir = bgc->read_reg(bgc->reg_dir);" in
gpio-mxc after bgpio_init returns.

g.

Re: [PATCH] add missing intialisation of bgc->dir

[Lothar Waßmann]

Hi,

Grant Likely writes:
> On Mon, Jul 4, 2011 at 9:51 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > Hi,
> >
> >> > > I don't know if this is a valid thing to do or not.  Not every GPIO
> >> > > controller supports reading back the initial state from the direction
> >> > > register.  I suspect it is better to leave initialization of the
> >> > > direction to the gpio controller driver itself.
> >> > >
> >> > That would mean, that external software would have to have access to
> >> > the bgpio internal data structures which is a Bad Thing(TM) IMO. Or we
> >> > need a function that allows to explicitly initialize the internal
> >> > shadow registers.
> >>
> >> What device are you using this for.  Are you using the basic gpio
> >> platform_driver, or do you have a driver using the library part of the
> >> basic gpio code?
> >>
> > I'm using it via drivers/gpio/gpio-mxc.
> 
> You can safely do the "bgc->dir = bgc->read_reg(bgc->reg_dir);" in
> gpio-mxc after bgpio_init returns.
> 
I know I can do that, but IMO it's a horrible way of programming, to
let a higher SW layer freely mess with the data structures of another
SW layer. There should be accessor functions to do that!


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________