diff mbox

[1/3] em28xx: give up GPIO register tracking/caching

Message ID 1365846521-3127-2-git-send-email-fschaefer.oss@googlemail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Frank Schaefer April 13, 2013, 9:48 a.m. UTC 
  The GPIO register tracking/caching code is partially broken, because newer
devices provide more than one GPIO register and some of them are even using
separate registers for read and write access.
Making it work would be too complicated.
It is also used nowhere and doesn't make sense in cases where input lines are
connected to buttons etc.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
 drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
 drivers/media/usb/em28xx/em28xx.h       |    6 ------
 3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)

Comments

Mauro Carvalho Chehab April 13, 2013, 2:41 p.m. UTC | #1 
Em Sat, 13 Apr 2013 11:48:39 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> The GPIO register tracking/caching code is partially broken, because newer
> devices provide more than one GPIO register and some of them are even using
> separate registers for read and write access.
> Making it work would be too complicated.
> It is also used nowhere and doesn't make sense in cases where input lines are
> connected to buttons etc.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)

...


> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>  	int oldval;
>  	u8 newval;
>  
> -	/* Uses cache for gpo/gpio registers */
> -	if (reg == dev->reg_gpo_num)
> -		oldval = dev->reg_gpo;
> -	else if (reg == dev->reg_gpio_num)
> -		oldval = dev->reg_gpio;
> -	else
> -		oldval = em28xx_read_reg(dev, reg);
> -
> +	oldval = em28xx_read_reg(dev, reg);
>  	if (oldval < 0)
>  		return oldval;


That's plain wrong, as it will break GPIO input.

With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
code works for output ports.

However, an input port requires an specific value (either 1 or 0 depending
on the GPIO circuitry). If the wrong value is written there, the input port
will stop working.

So, you can't simply read a value from a GPIO input and write it. You need
to shadow the GPIO write values instead.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Schaefer April 13, 2013, 3:33 p.m. UTC | #2 
Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Apr 2013 11:48:39 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> The GPIO register tracking/caching code is partially broken, because newer
>> devices provide more than one GPIO register and some of them are even using
>> separate registers for read and write access.
>> Making it work would be too complicated.
>> It is also used nowhere and doesn't make sense in cases where input lines are
>> connected to buttons etc.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
> ...
>
>
>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>>  	int oldval;
>>  	u8 newval;
>>  
>> -	/* Uses cache for gpo/gpio registers */
>> -	if (reg == dev->reg_gpo_num)
>> -		oldval = dev->reg_gpo;
>> -	else if (reg == dev->reg_gpio_num)
>> -		oldval = dev->reg_gpio;
>> -	else
>> -		oldval = em28xx_read_reg(dev, reg);
>> -
>> +	oldval = em28xx_read_reg(dev, reg);
>>  	if (oldval < 0)
>>  		return oldval;
> That's plain wrong, as it will break GPIO input.
>
> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
> code works for output ports.
>
> However, an input port requires an specific value (either 1 or 0 depending
> on the GPIO circuitry). If the wrong value is written there, the input port
> will stop working.
>
> So, you can't simply read a value from a GPIO input and write it. You need
> to shadow the GPIO write values instead.

I don't understand what you mean.
Why can I not read the value of a GPIO input and write it ?

Regards,
Frank

> Regards,
> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab April 13, 2013, 5:04 p.m. UTC | #3 
Em Sat, 13 Apr 2013 17:33:28 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
> > Em Sat, 13 Apr 2013 11:48:39 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> The GPIO register tracking/caching code is partially broken, because newer
> >> devices provide more than one GPIO register and some of them are even using
> >> separate registers for read and write access.
> >> Making it work would be too complicated.
> >> It is also used nowhere and doesn't make sense in cases where input lines are
> >> connected to buttons etc.
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
> >>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
> >>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
> >>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
> > ...
> >
> >
> >> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
> >>  	int oldval;
> >>  	u8 newval;
> >>  
> >> -	/* Uses cache for gpo/gpio registers */
> >> -	if (reg == dev->reg_gpo_num)
> >> -		oldval = dev->reg_gpo;
> >> -	else if (reg == dev->reg_gpio_num)
> >> -		oldval = dev->reg_gpio;
> >> -	else
> >> -		oldval = em28xx_read_reg(dev, reg);
> >> -
> >> +	oldval = em28xx_read_reg(dev, reg);
> >>  	if (oldval < 0)
> >>  		return oldval;
> > That's plain wrong, as it will break GPIO input.
> >
> > With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
> > code works for output ports.
> >
> > However, an input port requires an specific value (either 1 or 0 depending
> > on the GPIO circuitry). If the wrong value is written there, the input port
> > will stop working.
> >
> > So, you can't simply read a value from a GPIO input and write it. You need
> > to shadow the GPIO write values instead.
> 
> I don't understand what you mean.
> Why can I not read the value of a GPIO input and write it ?

Because, depending on the value you write, it can transform the input into an
output port.

If you don't understand why, I suggest you to take a look on how the GPIO
circuits are implemented. A very quick explanation could be find here:
	http://www.mcc-us.com/Open-collectorFAQ.htm

A more detailed one could be find here:

	http://www.coactionos.com/embedded-design/28-using-pull-ups-and-pull-downs.html


So, looking at the picture at http://www.coactionos.com/images/pullup.png and
assuming that a 0 means that the MOSFET gate is open, 1 means it is closed, 
for a pull-up GPIO input pin to work, driver needs to write "1" on it, so that
it will have VCC there.

This way, when MOSFEG goes to 1, the GPIO will be short-ciruited with GND, and
the driver will read a 0.

Note, however, that, if the driver writes a 0 to GPIO, no matter if the MOSFET
is opened or closed, it will read 0 every time.

Just the opposite logic applies for the pull-down logic.
Frank Schaefer April 13, 2013, 5:46 p.m. UTC | #4 
Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Apr 2013 17:33:28 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
>>> Em Sat, 13 Apr 2013 11:48:39 +0200
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> The GPIO register tracking/caching code is partially broken, because newer
>>>> devices provide more than one GPIO register and some of them are even using
>>>> separate registers for read and write access.
>>>> Making it work would be too complicated.
>>>> It is also used nowhere and doesn't make sense in cases where input lines are
>>>> connected to buttons etc.
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> ---
>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
>>> ...
>>>
>>>
>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>>>>  	int oldval;
>>>>  	u8 newval;
>>>>  
>>>> -	/* Uses cache for gpo/gpio registers */
>>>> -	if (reg == dev->reg_gpo_num)
>>>> -		oldval = dev->reg_gpo;
>>>> -	else if (reg == dev->reg_gpio_num)
>>>> -		oldval = dev->reg_gpio;
>>>> -	else
>>>> -		oldval = em28xx_read_reg(dev, reg);
>>>> -
>>>> +	oldval = em28xx_read_reg(dev, reg);
>>>>  	if (oldval < 0)
>>>>  		return oldval;
>>> That's plain wrong, as it will break GPIO input.
>>>
>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
>>> code works for output ports.
>>>
>>> However, an input port requires an specific value (either 1 or 0 depending
>>> on the GPIO circuitry). If the wrong value is written there, the input port
>>> will stop working.
>>>
>>> So, you can't simply read a value from a GPIO input and write it. You need
>>> to shadow the GPIO write values instead.
>> I don't understand what you mean.
>> Why can I not read the value of a GPIO input and write it ?
> Because, depending on the value you write, it can transform the input into an
> output port.

I don't get it.
We always write to the GPIO register. That's why these functions are
called em28xx_write_* ;)
Whether the write operation is sane or not (e.g. because it modifies the
bit corresponding to an input line) is not subject of these functions.


Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab April 13, 2013, 6:08 p.m. UTC | #5 
Em Sat, 13 Apr 2013 19:46:20 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
> > Em Sat, 13 Apr 2013 17:33:28 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
> >>> Em Sat, 13 Apr 2013 11:48:39 +0200
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> The GPIO register tracking/caching code is partially broken, because newer
> >>>> devices provide more than one GPIO register and some of them are even using
> >>>> separate registers for read and write access.
> >>>> Making it work would be too complicated.
> >>>> It is also used nowhere and doesn't make sense in cases where input lines are
> >>>> connected to buttons etc.
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
> >>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
> >>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
> >>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
> >>> ...
> >>>
> >>>
> >>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
> >>>>  	int oldval;
> >>>>  	u8 newval;
> >>>>  
> >>>> -	/* Uses cache for gpo/gpio registers */
> >>>> -	if (reg == dev->reg_gpo_num)
> >>>> -		oldval = dev->reg_gpo;
> >>>> -	else if (reg == dev->reg_gpio_num)
> >>>> -		oldval = dev->reg_gpio;
> >>>> -	else
> >>>> -		oldval = em28xx_read_reg(dev, reg);
> >>>> -
> >>>> +	oldval = em28xx_read_reg(dev, reg);
> >>>>  	if (oldval < 0)
> >>>>  		return oldval;
> >>> That's plain wrong, as it will break GPIO input.
> >>>
> >>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
> >>> code works for output ports.
> >>>
> >>> However, an input port requires an specific value (either 1 or 0 depending
> >>> on the GPIO circuitry). If the wrong value is written there, the input port
> >>> will stop working.
> >>>
> >>> So, you can't simply read a value from a GPIO input and write it. You need
> >>> to shadow the GPIO write values instead.
> >> I don't understand what you mean.
> >> Why can I not read the value of a GPIO input and write it ?
> > Because, depending on the value you write, it can transform the input into an
> > output port.
> 
> I don't get it.
> We always write to the GPIO register. That's why these functions are
> called em28xx_write_* ;)
> Whether the write operation is sane or not (e.g. because it modifies the
> bit corresponding to an input line) is not subject of these functions.

Writing is sane: GPIO input lines requires writing as well, in order to 
set it to either pull-up or pull-down mode (not sure if em28xx supports
both ways).

So, the driver needs to know if it will write there a 0 or 1, and this is part
of its GPIO configuration.

Let's assume that, on a certain device, you need to write "1" to enable that
input.

A read I/O to that port can return either 0 or 1. 

Giving an hypothetical example, please assume this code:

static int write_gpio_bits(u32 out, u32 mask)
{
	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
	write_gpio_ports(gpio);
}


...
	/* Use bit 1 as input GPIO */
	write_gpio_bits(1, 1);

	/* send a reset via bit 2 GPIO */
	write_gpio_bits(2, 2);
	write_gpio_bits(0, 2);
	write_gpio_bits(2, 2);

If, at the time the above code runs, the input bit 1 is at "0" state,
the subsequent calls will disable the input.

If, instead, only the write operations are cached like:

static int write_gpio_bits(u32 out, u32 mask)
{
	static u32 shadow_cache;

	shadow_cache = (shadow_cache & ~mask) | (out & mask);
	write_gpio_ports(gpio);
}

there's no such risk, as it will keep using "1" for the input bit.

See the difference?


Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Schaefer April 13, 2013, 6:19 p.m. UTC | #6 
Am 13.04.2013 19:46, schrieb Frank Schäfer:
> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
>> Em Sat, 13 Apr 2013 17:33:28 +0200
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
>>>> Em Sat, 13 Apr 2013 11:48:39 +0200
>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>
>>>>> The GPIO register tracking/caching code is partially broken, because newer
>>>>> devices provide more than one GPIO register and some of them are even using
>>>>> separate registers for read and write access.
>>>>> Making it work would be too complicated.
>>>>> It is also used nowhere and doesn't make sense in cases where input lines are
>>>>> connected to buttons etc.
>>>>>
>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>> ---
>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
>>>> ...
>>>>
>>>>
>>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>>>>>  	int oldval;
>>>>>  	u8 newval;
>>>>>  
>>>>> -	/* Uses cache for gpo/gpio registers */
>>>>> -	if (reg == dev->reg_gpo_num)
>>>>> -		oldval = dev->reg_gpo;
>>>>> -	else if (reg == dev->reg_gpio_num)
>>>>> -		oldval = dev->reg_gpio;
>>>>> -	else
>>>>> -		oldval = em28xx_read_reg(dev, reg);
>>>>> -
>>>>> +	oldval = em28xx_read_reg(dev, reg);
>>>>>  	if (oldval < 0)
>>>>>  		return oldval;
>>>> That's plain wrong, as it will break GPIO input.
>>>>
>>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
>>>> code works for output ports.
>>>>
>>>> However, an input port requires an specific value (either 1 or 0 depending
>>>> on the GPIO circuitry). If the wrong value is written there, the input port
>>>> will stop working.
>>>>
>>>> So, you can't simply read a value from a GPIO input and write it. You need
>>>> to shadow the GPIO write values instead.
>>> I don't understand what you mean.
>>> Why can I not read the value of a GPIO input and write it ?
>> Because, depending on the value you write, it can transform the input into an
>> output port.
> I don't get it.
> We always write to the GPIO register. That's why these functions are
> called em28xx_write_* ;)
> Whether the write operation is sane or not (e.g. because it modifies the
> bit corresponding to an input line) is not subject of these functions.

Hmm... that's actually not true for em28xx_write_regs().
The current/old code never writes the value to GPIO registers, it just
saves it to the device struct.
IMHO, this is plain wrong and yet antoher reason for applying this patch. ;)
It just didn't cause any trouble (hopefully) because for the GPIO
registers em28xx_write_reg_bits() is usually used instead (which works
correctly).

After checking the whole GPIO stuff again, I noticed a different
potential problem:
Register 0x04 seems to be a pure GPO register, so it is possible that
reading the current value from this register doesn't work.
The note in em28xx_write_regs() implies that noone has ever tested if it
works correctly.
Anyway, the current code reads register 0x04, too, to get the initial
value for caching. ;)

Regards,
Frank

>
>
> Frank
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Schaefer April 13, 2013, 6:41 p.m. UTC | #7 
Am 13.04.2013 20:19, schrieb Frank Schäfer:
> Am 13.04.2013 19:46, schrieb Frank Schäfer:
>> ...
>> We always write to the GPIO register. That's why these functions are
>> called em28xx_write_* ;)
>> Whether the write operation is sane or not (e.g. because it modifies the
>> bit corresponding to an input line) is not subject of these functions.
> Hmm... that's actually not true for em28xx_write_regs().
> The current/old code never writes the value to GPIO registers, it just
> saves it to the device struct.

Arghh... no ... please disregard this paragraph, I simply overlooked the
register write.
I have to stop for today, will try to get back to this tomorrow.

Regards,
Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Schaefer April 14, 2013, 8:35 p.m. UTC | #8 
Am 13.04.2013 20:08, schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Apr 2013 19:46:20 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
>>> Em Sat, 13 Apr 2013 17:33:28 +0200
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
>>>>> Em Sat, 13 Apr 2013 11:48:39 +0200
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>> The GPIO register tracking/caching code is partially broken, because newer
>>>>>> devices provide more than one GPIO register and some of them are even using
>>>>>> separate registers for read and write access.
>>>>>> Making it work would be too complicated.
>>>>>> It is also used nowhere and doesn't make sense in cases where input lines are
>>>>>> connected to buttons etc.
>>>>>>
>>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>>> ---
>>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>>>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>>>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>>>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
>>>>> ...
>>>>>
>>>>>
>>>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>>>>>>  	int oldval;
>>>>>>  	u8 newval;
>>>>>>  
>>>>>> -	/* Uses cache for gpo/gpio registers */
>>>>>> -	if (reg == dev->reg_gpo_num)
>>>>>> -		oldval = dev->reg_gpo;
>>>>>> -	else if (reg == dev->reg_gpio_num)
>>>>>> -		oldval = dev->reg_gpio;
>>>>>> -	else
>>>>>> -		oldval = em28xx_read_reg(dev, reg);
>>>>>> -
>>>>>> +	oldval = em28xx_read_reg(dev, reg);
>>>>>>  	if (oldval < 0)
>>>>>>  		return oldval;
>>>>> That's plain wrong, as it will break GPIO input.
>>>>>
>>>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
>>>>> code works for output ports.
>>>>>
>>>>> However, an input port requires an specific value (either 1 or 0 depending
>>>>> on the GPIO circuitry). If the wrong value is written there, the input port
>>>>> will stop working.
>>>>>
>>>>> So, you can't simply read a value from a GPIO input and write it. You need
>>>>> to shadow the GPIO write values instead.
>>>> I don't understand what you mean.
>>>> Why can I not read the value of a GPIO input and write it ?
>>> Because, depending on the value you write, it can transform the input into an
>>> output port.
>> I don't get it.
>> We always write to the GPIO register. That's why these functions are
>> called em28xx_write_* ;)
>> Whether the write operation is sane or not (e.g. because it modifies the
>> bit corresponding to an input line) is not subject of these functions.
> Writing is sane: GPIO input lines requires writing as well, in order to 
> set it to either pull-up or pull-down mode (not sure if em28xx supports
> both ways).
>
> So, the driver needs to know if it will write there a 0 or 1, and this is part
> of its GPIO configuration.
>
> Let's assume that, on a certain device, you need to write "1" to enable that
> input.
>
> A read I/O to that port can return either 0 or 1. 
>
> Giving an hypothetical example, please assume this code:
>
> static int write_gpio_bits(u32 out, u32 mask)
> {
> 	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
> 	write_gpio_ports(gpio);
> }
>
>
> ...
> 	/* Use bit 1 as input GPIO */
> 	write_gpio_bits(1, 1);
>
> 	/* send a reset via bit 2 GPIO */
> 	write_gpio_bits(2, 2);
> 	write_gpio_bits(0, 2);
> 	write_gpio_bits(2, 2);
>
> If, at the time the above code runs, the input bit 1 is at "0" state,
> the subsequent calls will disable the input.
>
> If, instead, only the write operations are cached like:
>
> static int write_gpio_bits(u32 out, u32 mask)
> {
> 	static u32 shadow_cache;
>
> 	shadow_cache = (shadow_cache & ~mask) | (out & mask);
> 	write_gpio_ports(gpio);
> }
>
> there's no such risk, as it will keep using "1" for the input bit.

Hmm... ok, now I understand what you mean.
Are you sure the Empia chips are really working this way ?
I checked the em25xx datasheet (excerpt) and it talks about separate
registers for GPIO configuration (unfortunately without explaining their
function in detail).
I going to do some tests with the Laplace webcam, so far it seems to be
working fine without this caching stuff.
But the reverse-engineering possibilities are quite limited, so someone
with a detailed datasheet should really look this up.

Regards,
Frank



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab April 15, 2013, 12:51 p.m. UTC | #9 
Em Sun, 14 Apr 2013 22:35:05 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 13.04.2013 20:08, schrieb Mauro Carvalho Chehab:
> > Em Sat, 13 Apr 2013 19:46:20 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
> >>> Em Sat, 13 Apr 2013 17:33:28 +0200
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
> >>>>> Em Sat, 13 Apr 2013 11:48:39 +0200
> >>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>>>
> >>>>>> The GPIO register tracking/caching code is partially broken, because newer
> >>>>>> devices provide more than one GPIO register and some of them are even using
> >>>>>> separate registers for read and write access.
> >>>>>> Making it work would be too complicated.
> >>>>>> It is also used nowhere and doesn't make sense in cases where input lines are
> >>>>>> connected to buttons etc.
> >>>>>>
> >>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>>>> ---
> >>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
> >>>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
> >>>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
> >>>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
> >>>>> ...
> >>>>>
> >>>>>
> >>>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
> >>>>>>  	int oldval;
> >>>>>>  	u8 newval;
> >>>>>>  
> >>>>>> -	/* Uses cache for gpo/gpio registers */
> >>>>>> -	if (reg == dev->reg_gpo_num)
> >>>>>> -		oldval = dev->reg_gpo;
> >>>>>> -	else if (reg == dev->reg_gpio_num)
> >>>>>> -		oldval = dev->reg_gpio;
> >>>>>> -	else
> >>>>>> -		oldval = em28xx_read_reg(dev, reg);
> >>>>>> -
> >>>>>> +	oldval = em28xx_read_reg(dev, reg);
> >>>>>>  	if (oldval < 0)
> >>>>>>  		return oldval;
> >>>>> That's plain wrong, as it will break GPIO input.
> >>>>>
> >>>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
> >>>>> code works for output ports.
> >>>>>
> >>>>> However, an input port requires an specific value (either 1 or 0 depending
> >>>>> on the GPIO circuitry). If the wrong value is written there, the input port
> >>>>> will stop working.
> >>>>>
> >>>>> So, you can't simply read a value from a GPIO input and write it. You need
> >>>>> to shadow the GPIO write values instead.
> >>>> I don't understand what you mean.
> >>>> Why can I not read the value of a GPIO input and write it ?
> >>> Because, depending on the value you write, it can transform the input into an
> >>> output port.
> >> I don't get it.
> >> We always write to the GPIO register. That's why these functions are
> >> called em28xx_write_* ;)
> >> Whether the write operation is sane or not (e.g. because it modifies the
> >> bit corresponding to an input line) is not subject of these functions.
> > Writing is sane: GPIO input lines requires writing as well, in order to 
> > set it to either pull-up or pull-down mode (not sure if em28xx supports
> > both ways).
> >
> > So, the driver needs to know if it will write there a 0 or 1, and this is part
> > of its GPIO configuration.
> >
> > Let's assume that, on a certain device, you need to write "1" to enable that
> > input.
> >
> > A read I/O to that port can return either 0 or 1. 
> >
> > Giving an hypothetical example, please assume this code:
> >
> > static int write_gpio_bits(u32 out, u32 mask)
> > {
> > 	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
> > 	write_gpio_ports(gpio);
> > }
> >
> >
> > ...
> > 	/* Use bit 1 as input GPIO */
> > 	write_gpio_bits(1, 1);
> >
> > 	/* send a reset via bit 2 GPIO */
> > 	write_gpio_bits(2, 2);
> > 	write_gpio_bits(0, 2);
> > 	write_gpio_bits(2, 2);
> >
> > If, at the time the above code runs, the input bit 1 is at "0" state,
> > the subsequent calls will disable the input.
> >
> > If, instead, only the write operations are cached like:
> >
> > static int write_gpio_bits(u32 out, u32 mask)
> > {
> > 	static u32 shadow_cache;
> >
> > 	shadow_cache = (shadow_cache & ~mask) | (out & mask);
> > 	write_gpio_ports(gpio);
> > }
> >
> > there's no such risk, as it will keep using "1" for the input bit.
> 
> Hmm... ok, now I understand what you mean.
> Are you sure the Empia chips are really working this way ?

Yes. It uses a pretty standard GPIO mechanism at register 0x08. I'm not
so sure about the "GPO" register 0x04, but using a shadow for it as
well won't hurt, and will reduce a little bit the USB bus traffic.

> I checked the em25xx datasheet (excerpt) and it talks about separate
> registers for GPIO configuration (unfortunately without explaining their
> function in detail).

Interesting. There are several old designs (bttv, saa7134,...) that uses
a separate register for defining the input and the output pins.

> I going to do some tests with the Laplace webcam, so far it seems to be
> working fine without this caching stuff.
> But the reverse-engineering possibilities are quite limited, so someone
> with a detailed datasheet should really look this up.

Well, that will affect only devices with input pins connected.
If you test on a hardware without it, you won't notice any difference
at all.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antti Palosaari April 15, 2013, 2:11 p.m. UTC | #10 
On 04/15/2013 03:51 PM, Mauro Carvalho Chehab wrote:
> Em Sun, 14 Apr 2013 22:35:05 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

>> I checked the em25xx datasheet (excerpt) and it talks about separate
>> registers for GPIO configuration (unfortunately without explaining their
>> function in detail).
>
> Interesting. There are several old designs (bttv, saa7134,...) that uses
> a separate register for defining the input and the output pins.

That's pretty usual way, likely most common, having separate GPIO 
configuration and GPIO value registers. If you has a port of GPIO values 
in one register, and you has configured those as 0-3 INPUT and 4-7 
OUTPUT, then writing to that register does not make any changes to bits 
that are mapped as IN (are just discarded as don't care).

In case a bit I/O (which is not not supported by Kernel) writing to 
input register could enable internal pull-up or pull-down resistor :) 
IIRC Atmel micro-controllers has such option.

regards
Antti
Frank Schaefer April 15, 2013, 4:26 p.m. UTC | #11 
Am 15.04.2013 14:51, schrieb Mauro Carvalho Chehab:
> Em Sun, 14 Apr 2013 22:35:05 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 13.04.2013 20:08, schrieb Mauro Carvalho Chehab:
>>> Em Sat, 13 Apr 2013 19:46:20 +0200
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
>>>>> Em Sat, 13 Apr 2013 17:33:28 +0200
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
>>>>>>> Em Sat, 13 Apr 2013 11:48:39 +0200
>>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>>>
>>>>>>>> The GPIO register tracking/caching code is partially broken, because newer
>>>>>>>> devices provide more than one GPIO register and some of them are even using
>>>>>>>> separate registers for read and write access.
>>>>>>>> Making it work would be too complicated.
>>>>>>>> It is also used nowhere and doesn't make sense in cases where input lines are
>>>>>>>> connected to buttons etc.
>>>>>>>>
>>>>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>>>>> ---
>>>>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
>>>>>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
>>>>>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
>>>>>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
>>>>>>> ...
>>>>>>>
>>>>>>>
>>>>>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
>>>>>>>>  	int oldval;
>>>>>>>>  	u8 newval;
>>>>>>>>  
>>>>>>>> -	/* Uses cache for gpo/gpio registers */
>>>>>>>> -	if (reg == dev->reg_gpo_num)
>>>>>>>> -		oldval = dev->reg_gpo;
>>>>>>>> -	else if (reg == dev->reg_gpio_num)
>>>>>>>> -		oldval = dev->reg_gpio;
>>>>>>>> -	else
>>>>>>>> -		oldval = em28xx_read_reg(dev, reg);
>>>>>>>> -
>>>>>>>> +	oldval = em28xx_read_reg(dev, reg);
>>>>>>>>  	if (oldval < 0)
>>>>>>>>  		return oldval;
>>>>>>> That's plain wrong, as it will break GPIO input.
>>>>>>>
>>>>>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
>>>>>>> code works for output ports.
>>>>>>>
>>>>>>> However, an input port requires an specific value (either 1 or 0 depending
>>>>>>> on the GPIO circuitry). If the wrong value is written there, the input port
>>>>>>> will stop working.
>>>>>>>
>>>>>>> So, you can't simply read a value from a GPIO input and write it. You need
>>>>>>> to shadow the GPIO write values instead.
>>>>>> I don't understand what you mean.
>>>>>> Why can I not read the value of a GPIO input and write it ?
>>>>> Because, depending on the value you write, it can transform the input into an
>>>>> output port.
>>>> I don't get it.
>>>> We always write to the GPIO register. That's why these functions are
>>>> called em28xx_write_* ;)
>>>> Whether the write operation is sane or not (e.g. because it modifies the
>>>> bit corresponding to an input line) is not subject of these functions.
>>> Writing is sane: GPIO input lines requires writing as well, in order to 
>>> set it to either pull-up or pull-down mode (not sure if em28xx supports
>>> both ways).
>>>
>>> So, the driver needs to know if it will write there a 0 or 1, and this is part
>>> of its GPIO configuration.
>>>
>>> Let's assume that, on a certain device, you need to write "1" to enable that
>>> input.
>>>
>>> A read I/O to that port can return either 0 or 1. 
>>>
>>> Giving an hypothetical example, please assume this code:
>>>
>>> static int write_gpio_bits(u32 out, u32 mask)
>>> {
>>> 	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
>>> 	write_gpio_ports(gpio);
>>> }
>>>
>>>
>>> ...
>>> 	/* Use bit 1 as input GPIO */
>>> 	write_gpio_bits(1, 1);
>>>
>>> 	/* send a reset via bit 2 GPIO */
>>> 	write_gpio_bits(2, 2);
>>> 	write_gpio_bits(0, 2);
>>> 	write_gpio_bits(2, 2);
>>>
>>> If, at the time the above code runs, the input bit 1 is at "0" state,
>>> the subsequent calls will disable the input.
>>>
>>> If, instead, only the write operations are cached like:
>>>
>>> static int write_gpio_bits(u32 out, u32 mask)
>>> {
>>> 	static u32 shadow_cache;
>>>
>>> 	shadow_cache = (shadow_cache & ~mask) | (out & mask);
>>> 	write_gpio_ports(gpio);
>>> }
>>>
>>> there's no such risk, as it will keep using "1" for the input bit.
>> Hmm... ok, now I understand what you mean.
>> Are you sure the Empia chips are really working this way ?
> Yes. It uses a pretty standard GPIO mechanism at register 0x08.

Ok, will try to find out how those 0x80...0x87 GPIO registers are working.

> I'm not so sure about the "GPO" register 0x04,

Well, we don't need caching for output lines, just for input lines.

> but using a shadow for it as
> well won't hurt, and will reduce a little bit the USB bus traffic.

Sure, but the problem is that caching is getting complicated with the
newer devices.
The em2765 in the VAD Laplace webcam for example uses registers
0x80/0x84, 0x81/0x85, 0x83/0x87 and also at least register 0x08 for
GPIO. I don't not about about reg 0x04.
And its seems some bits of reg 0x0C are used for GPIO, too (current
snapshot button support uses bit 6).
Have fun. :(

>> I checked the em25xx datasheet (excerpt) and it talks about separate
>> registers for GPIO configuration (unfortunately without explaining their
>> function in detail).
> Interesting. There are several old designs (bttv, saa7134,...) that uses
> a separate register for defining the input and the output pins.

IMHO separate registers are the better design.

>
>> I going to do some tests with the Laplace webcam, so far it seems to be
>> working fine without this caching stuff.
>> But the reverse-engineering possibilities are quite limited, so someone
>> with a detailed datasheet should really look this up.
> Well, that will affect only devices with input pins connected.
> If you test on a hardware without it, you won't notice any difference
> at all.

The Laplace webcam has three buttons assigned to regs 0x80/0x84 and
0x81/0x85.
They are inverted (0=pressed, 1=unpressed), which could be the reason
why I didn't notice any problems without caching.
I don't have any other devices with buttons for testing.

Regards,
Frank

> Cheers,
> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab April 15, 2013, 11:01 p.m. UTC | #12 
Em Mon, 15 Apr 2013 18:26:56 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 15.04.2013 14:51, schrieb Mauro Carvalho Chehab:
> > Em Sun, 14 Apr 2013 22:35:05 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 13.04.2013 20:08, schrieb Mauro Carvalho Chehab:
> >>> Em Sat, 13 Apr 2013 19:46:20 +0200
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> Am 13.04.2013 19:04, schrieb Mauro Carvalho Chehab:
> >>>>> Em Sat, 13 Apr 2013 17:33:28 +0200
> >>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>>>
> >>>>>> Am 13.04.2013 16:41, schrieb Mauro Carvalho Chehab:
> >>>>>>> Em Sat, 13 Apr 2013 11:48:39 +0200
> >>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>>>>>
> >>>>>>>> The GPIO register tracking/caching code is partially broken, because newer
> >>>>>>>> devices provide more than one GPIO register and some of them are even using
> >>>>>>>> separate registers for read and write access.
> >>>>>>>> Making it work would be too complicated.
> >>>>>>>> It is also used nowhere and doesn't make sense in cases where input lines are
> >>>>>>>> connected to buttons etc.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/media/usb/em28xx/em28xx-cards.c |   12 ------------
> >>>>>>>>  drivers/media/usb/em28xx/em28xx-core.c  |   27 ++-------------------------
> >>>>>>>>  drivers/media/usb/em28xx/em28xx.h       |    6 ------
> >>>>>>>>  3 Dateien geändert, 2 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)
> >>>>>>> ...
> >>>>>>>
> >>>>>>>
> >>>>>>>> @@ -231,14 +215,7 @@ int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
> >>>>>>>>  	int oldval;
> >>>>>>>>  	u8 newval;
> >>>>>>>>  
> >>>>>>>> -	/* Uses cache for gpo/gpio registers */
> >>>>>>>> -	if (reg == dev->reg_gpo_num)
> >>>>>>>> -		oldval = dev->reg_gpo;
> >>>>>>>> -	else if (reg == dev->reg_gpio_num)
> >>>>>>>> -		oldval = dev->reg_gpio;
> >>>>>>>> -	else
> >>>>>>>> -		oldval = em28xx_read_reg(dev, reg);
> >>>>>>>> -
> >>>>>>>> +	oldval = em28xx_read_reg(dev, reg);
> >>>>>>>>  	if (oldval < 0)
> >>>>>>>>  		return oldval;
> >>>>>>> That's plain wrong, as it will break GPIO input.
> >>>>>>>
> >>>>>>> With GPIO, you can write either 0 or 1 to a GPIO output port. So, your
> >>>>>>> code works for output ports.
> >>>>>>>
> >>>>>>> However, an input port requires an specific value (either 1 or 0 depending
> >>>>>>> on the GPIO circuitry). If the wrong value is written there, the input port
> >>>>>>> will stop working.
> >>>>>>>
> >>>>>>> So, you can't simply read a value from a GPIO input and write it. You need
> >>>>>>> to shadow the GPIO write values instead.
> >>>>>> I don't understand what you mean.
> >>>>>> Why can I not read the value of a GPIO input and write it ?
> >>>>> Because, depending on the value you write, it can transform the input into an
> >>>>> output port.
> >>>> I don't get it.
> >>>> We always write to the GPIO register. That's why these functions are
> >>>> called em28xx_write_* ;)
> >>>> Whether the write operation is sane or not (e.g. because it modifies the
> >>>> bit corresponding to an input line) is not subject of these functions.
> >>> Writing is sane: GPIO input lines requires writing as well, in order to 
> >>> set it to either pull-up or pull-down mode (not sure if em28xx supports
> >>> both ways).
> >>>
> >>> So, the driver needs to know if it will write there a 0 or 1, and this is part
> >>> of its GPIO configuration.
> >>>
> >>> Let's assume that, on a certain device, you need to write "1" to enable that
> >>> input.
> >>>
> >>> A read I/O to that port can return either 0 or 1. 
> >>>
> >>> Giving an hypothetical example, please assume this code:
> >>>
> >>> static int write_gpio_bits(u32 out, u32 mask)
> >>> {
> >>> 	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
> >>> 	write_gpio_ports(gpio);
> >>> }
> >>>
> >>>
> >>> ...
> >>> 	/* Use bit 1 as input GPIO */
> >>> 	write_gpio_bits(1, 1);
> >>>
> >>> 	/* send a reset via bit 2 GPIO */
> >>> 	write_gpio_bits(2, 2);
> >>> 	write_gpio_bits(0, 2);
> >>> 	write_gpio_bits(2, 2);
> >>>
> >>> If, at the time the above code runs, the input bit 1 is at "0" state,
> >>> the subsequent calls will disable the input.
> >>>
> >>> If, instead, only the write operations are cached like:
> >>>
> >>> static int write_gpio_bits(u32 out, u32 mask)
> >>> {
> >>> 	static u32 shadow_cache;
> >>>
> >>> 	shadow_cache = (shadow_cache & ~mask) | (out & mask);
> >>> 	write_gpio_ports(gpio);
> >>> }
> >>>
> >>> there's no such risk, as it will keep using "1" for the input bit.
> >> Hmm... ok, now I understand what you mean.
> >> Are you sure the Empia chips are really working this way ?
> > Yes. It uses a pretty standard GPIO mechanism at register 0x08.
> 
> Ok, will try to find out how those 0x80...0x87 GPIO registers are working.

Ok.
> 
> > I'm not so sure about the "GPO" register 0x04,
> 
> Well, we don't need caching for output lines, just for input lines.

You understood me wrong: I mean that I'm not sure if register 0x04 is
only for output pins, or if then can also be used for input.

In doubt, better to cache.

> > but using a shadow for it as
> > well won't hurt, and will reduce a little bit the USB bus traffic.
> 
> Sure, but the problem is that caching is getting complicated with the
> newer devices.
> The em2765 in the VAD Laplace webcam for example uses registers
> 0x80/0x84, 0x81/0x85, 0x83/0x87 and also at least register 0x08 for
> GPIO. I don't not about about reg 0x04.
> And its seems some bits of reg 0x0C are used for GPIO, too (current
> snapshot button support uses bit 6).
> Have fun. :(

If GPIOLIB solves it on a clean way, then we have a reason to move it.
Otherwise, we'll need to cache all those registers, and reg 0x0C GPIO
bits.

> >> I checked the em25xx datasheet (excerpt) and it talks about separate
> >> registers for GPIO configuration (unfortunately without explaining their
> >> function in detail).
> > Interesting. There are several old designs (bttv, saa7134,...) that uses
> > a separate register for defining the input and the output pins.
> 
> IMHO separate registers are the better design.

It is a safer design, but it is harder to reverse engineer them.
See the wiki page that explains it on bt848:
	http://linuxtv.org/wiki/index.php/GPIO_pins
Even experienced people sometimes do bad GPIO settings.

Using just one register is a way easier.

> >> I going to do some tests with the Laplace webcam, so far it seems to be
> >> working fine without this caching stuff.
> >> But the reverse-engineering possibilities are quite limited, so someone
> >> with a detailed datasheet should really look this up.
> > Well, that will affect only devices with input pins connected.
> > If you test on a hardware without it, you won't notice any difference
> > at all.
> 
> The Laplace webcam has three buttons assigned to regs 0x80/0x84 and
> 0x81/0x85.
> They are inverted (0=pressed, 1=unpressed), which could be the reason
> why I didn't notice any problems without caching.

It seems it uses a pull-up resistor on open-drain mode. That's the most
common way to do GPIO.

> I don't have any other devices with buttons for testing.
> 
> Regards,
> Frank
> 
> > Cheers,
> > Mauro
>
Frank Schaefer April 23, 2013, 4:58 p.m. UTC | #13 
...
>>>
>>>> Am 13.04.2013 20:08, schrieb Mauro Carvalho Chehab:
>>>>> Writing is sane: GPIO input lines requires writing as well, in order to 
>>>>> set it to either pull-up or pull-down mode (not sure if em28xx supports
>>>>> both ways).
>>>>>
>>>>> So, the driver needs to know if it will write there a 0 or 1, and this is part
>>>>> of its GPIO configuration.
>>>>>
>>>>> Let's assume that, on a certain device, you need to write "1" to enable that
>>>>> input.
>>>>>
>>>>> A read I/O to that port can return either 0 or 1. 
>>>>>
>>>>> Giving an hypothetical example, please assume this code:
>>>>>
>>>>> static int write_gpio_bits(u32 out, u32 mask)
>>>>> {
>>>>> 	u32 gpio = (read_gpio_ports() & ~mask) | (out & mask);
>>>>> 	write_gpio_ports(gpio);
>>>>> }
>>>>>
>>>>>
>>>>> ...
>>>>> 	/* Use bit 1 as input GPIO */
>>>>> 	write_gpio_bits(1, 1);
>>>>>
>>>>> 	/* send a reset via bit 2 GPIO */
>>>>> 	write_gpio_bits(2, 2);
>>>>> 	write_gpio_bits(0, 2);
>>>>> 	write_gpio_bits(2, 2);
>>>>>
>>>>> If, at the time the above code runs, the input bit 1 is at "0" state,
>>>>> the subsequent calls will disable the input.
>>>>>
>>>>> If, instead, only the write operations are cached like:
>>>>>
>>>>> static int write_gpio_bits(u32 out, u32 mask)
>>>>> {
>>>>> 	static u32 shadow_cache;
>>>>>
>>>>> 	shadow_cache = (shadow_cache & ~mask) | (out & mask);
>>>>> 	write_gpio_ports(gpio);
>>>>> }
>>>>>
>>>>> there's no such risk, as it will keep using "1" for the input bit.
>>>> Hmm... ok, now I understand what you mean.
>>>> Are you sure the Empia chips are really working this way ?
>>> Yes. It uses a pretty standard GPIO mechanism at register 0x08.
>> Ok, will try to find out how those 0x80...0x87 GPIO registers are working.
> Ok.

Ok, I've made some tests and it seems you are right.
Registers 0x80...0x87 are working the same way.

Will have to think about all this for a while with a clear head before
coming up with a proper solution.

>>> I'm not so sure about the "GPO" register 0x04,
>> Well, we don't need caching for output lines, just for input lines.
> You understood me wrong: I mean that I'm not sure if register 0x04 is
> only for output pins, or if then can also be used for input.

Well, it's named GPO in contrast to reg 0x08 GPIO.
What the hell can we rely on ?

> In doubt, better to cache.
>
>>> but using a shadow for it as
>>> well won't hurt, and will reduce a little bit the USB bus traffic.
>> Sure, but the problem is that caching is getting complicated with the
>> newer devices.
>> The em2765 in the VAD Laplace webcam for example uses registers
>> 0x80/0x84, 0x81/0x85, 0x83/0x87 and also at least register 0x08 for
>> GPIO. I don't not about about reg 0x04.
>> And its seems some bits of reg 0x0C are used for GPIO, too (current
>> snapshot button support uses bit 6).
>> Have fun. :(
> If GPIOLIB solves it on a clean way, then we have a reason to move it.
> Otherwise, we'll need to cache all those registers, and reg 0x0C GPIO
> bits.

And what are the GPIO bits of register 0x0C_USBSUSP ? It seems to be a
mixed register.
Bit 5 is likely GPIO (used for snapshot button), I'm not sure about bit
4 (enabled/disabled on capturing start/stop).

What I hate most about register caching at the moment is, that we are
trying to do a complex thing without even knowing exactly which
registers/bits need to be handled (also depends on the chip variant). :(
It's a mess.

>>>> I checked the em25xx datasheet (excerpt) and it talks about separate
>>>> registers for GPIO configuration (unfortunately without explaining their
>>>> function in detail).
>>> Interesting. There are several old designs (bttv, saa7134,...) that uses
>>> a separate register for defining the input and the output pins.
>> IMHO separate registers are the better design.
> It is a safer design, but it is harder to reverse engineer them.
> See the wiki page that explains it on bt848:
> 	http://linuxtv.org/wiki/index.php/GPIO_pins
> Even experienced people sometimes do bad GPIO settings.

Maybe.

> Using just one register is a way easier.

I think this case proves the opposite. ;)

Regards,
Frank
>
>>>> I going to do some tests with the Laplace webcam, so far it seems to be
>>>> working fine without this caching stuff.
>>>> But the reverse-engineering possibilities are quite limited, so someone
>>>> with a detailed datasheet should really look this up.
>>> Well, that will affect only devices with input pins connected.
>>> If you test on a hardware without it, you won't notice any difference
>>> at all.
>> The Laplace webcam has three buttons assigned to regs 0x80/0x84 and
>> 0x81/0x85.
>> They are inverted (0=pressed, 1=unpressed), which could be the reason
>> why I didn't notice any problems without caching.
> It seems it uses a pull-up resistor on open-drain mode. That's the most
> common way to do GPIO.
>
>> I don't have any other devices with buttons for testing.
>>
>> Regards,
>> Frank
>>
>>> Cheers,
>>> Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index bec604f..e328159 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -2880,10 +2880,6 @@  static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 
 	em28xx_set_model(dev);
 
-	/* Set the default GPO/GPIO for legacy devices */
-	dev->reg_gpo_num = EM2880_R04_GPO;
-	dev->reg_gpio_num = EM28XX_R08_GPIO;
-
 	dev->wait_after_write = 5;
 
 	/* Based on the Chip ID, set the device configuration */
@@ -2930,13 +2926,11 @@  static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 			break;
 		case CHIP_ID_EM2874:
 			chip_name = "em2874";
-			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
 			dev->eeprom_addrwidth_16bit = 1;
 			break;
 		case CHIP_ID_EM28174:
 			chip_name = "em28174";
-			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
 			dev->eeprom_addrwidth_16bit = 1;
 			break;
@@ -2946,7 +2940,6 @@  static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 			break;
 		case CHIP_ID_EM2884:
 			chip_name = "em2884";
-			dev->reg_gpio_num = EM2874_R80_GPIO;
 			dev->wait_after_write = 0;
 			dev->eeprom_addrwidth_16bit = 1;
 			break;
@@ -2975,11 +2968,6 @@  static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 		return 0;
 	}
 
-	/* Prepopulate cached GPO register content */
-	retval = em28xx_read_reg(dev, dev->reg_gpo_num);
-	if (retval >= 0)
-		dev->reg_gpo = retval;
-
 	em28xx_pre_card_setup(dev);
 
 	if (!dev->board.is_em2800) {
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index a802128..fc157af 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -193,23 +193,7 @@  int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 reg, char *buf,
 
 int em28xx_write_regs(struct em28xx *dev, u16 reg, char *buf, int len)
 {
-	int rc;
-
-	rc = em28xx_write_regs_req(dev, USB_REQ_GET_STATUS, reg, buf, len);
-
-	/* Stores GPO/GPIO values at the cache, if changed
-	   Only write values should be stored, since input on a GPIO
-	   register will return the input bits.
-	   Not sure what happens on reading GPO register.
-	 */
-	if (rc >= 0) {
-		if (reg == dev->reg_gpo_num)
-			dev->reg_gpo = buf[0];
-		else if (reg == dev->reg_gpio_num)
-			dev->reg_gpio = buf[0];
-	}
-
-	return rc;
+	return em28xx_write_regs_req(dev, USB_REQ_GET_STATUS, reg, buf, len);
 }
 EXPORT_SYMBOL_GPL(em28xx_write_regs);
 
@@ -231,14 +215,7 @@  int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
 	int oldval;
 	u8 newval;
 
-	/* Uses cache for gpo/gpio registers */
-	if (reg == dev->reg_gpo_num)
-		oldval = dev->reg_gpo;
-	else if (reg == dev->reg_gpio_num)
-		oldval = dev->reg_gpio;
-	else
-		oldval = em28xx_read_reg(dev, reg);
-
+	oldval = em28xx_read_reg(dev, reg);
 	if (oldval < 0)
 		return oldval;
 
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index a9323b6..e070de0 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -636,12 +636,6 @@  struct em28xx {
 
 	enum em28xx_mode mode;
 
-	/* register numbers for GPO/GPIO registers */
-	u16 reg_gpo_num, reg_gpio_num;
-
-	/* Caches GPO and GPIO registers */
-	unsigned char	reg_gpo, reg_gpio;
-
 	/* Snapshot button */
 	char snapshot_button_path[30];	/* path of the input dev */
 	struct input_dev *sbutton_input_dev;