[PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Frank Schäfer]

Patch 1 removes the unneeded and broken gpio register caching code.
Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
and patch 3 finally adds a new helper function for gpio ports with separate
registers for read and write access.


Frank Schäfer (3):
  em28xx: give up GPIO register tracking/caching
  em28xx: add register defines for em25xx/em276x/7x/8x GPIO registers
  em28xx: add helper function for handling the GPIO registers of newer
    devices

 drivers/media/usb/em28xx/em28xx-cards.c |   12 --------
 drivers/media/usb/em28xx/em28xx-core.c  |   50 ++++++++++++-------------------
 drivers/media/usb/em28xx/em28xx-reg.h   |    8 +++++
 drivers/media/usb/em28xx/em28xx.h       |   11 ++-----
 4 Dateien geändert, 30 Zeilen hinzugefügt(+), 51 Zeilen entfernt(-)

-- 
1.7.10.4

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

[Frank Schäfer]

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(-)

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;
-- 
1.7.10.4

[PATCH 2/3] em28xx: add register defines for em25xx/em276x/7x/8x GPIO registers

[Frank Schäfer]

em25xx/em276x/7x/8x provides 3 GPIO register sets,
each of them consisting of separate read and a write registers.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-reg.h |    8 ++++++++
 1 Datei geändert, 8 Zeilen hinzugefügt(+)

diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
index 622871d..ebc5663 100644
--- a/drivers/media/usb/em28xx/em28xx-reg.h
+++ b/drivers/media/usb/em28xx/em28xx-reg.h
@@ -195,6 +195,14 @@
 #define EM2874_R5F_TS_ENABLE    0x5f
 #define EM2874_R80_GPIO         0x80
 
+/* em25xx, em276x/7x/8x GPIO registers */
+#define EM25XX_R80_GPIO_P0_W    0x80
+#define EM25XX_R81_GPIO_P1_W    0x81
+#define EM25XX_R83_GPIO_P3_W    0x83
+#define EM25XX_R84_GPIO_P0_R    0x84
+#define EM25XX_R85_GPIO_P1_R    0x85
+#define EM25XX_R87_GPIO_P3_R    0x87
+
 /* em2874 IR config register (0x50) */
 #define EM2874_IR_NEC           0x00
 #define EM2874_IR_NEC_NO_PARITY 0x01
-- 
1.7.10.4

[PATCH 3/3] em28xx: add helper function for handling the GPIO registers of newer devices

[Frank Schäfer]

The current code provides a helper function em28xx_write_reg_bits() that reads
the current state/value of a GPIO register, modifies only the bits specified
with the value and bitmask parmaters and writes the new value back to the
register.

Newer devices (em25xx, em276x/7x/8x) are using separate registers for reading
and changing the states of the GPIO ports/lines, for which this helper function
cannot be used.

Introduce a new function em28xx_write_regs_bits() that uses two register
parameters reg_r (register for reading the current value) and
reg_w (register for writing the new value).
Make em28xx_write_reg_bits() a wrapper function calling this new function with
the same value for both registers.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |   26 +++++++++++++++++++-------
 drivers/media/usb/em28xx/em28xx.h      |    5 +++--
 2 Dateien geändert, 22 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index fc157af..293dc31 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -205,23 +205,35 @@ int em28xx_write_reg(struct em28xx *dev, u16 reg, u8 val)
 EXPORT_SYMBOL_GPL(em28xx_write_reg);
 
 /*
- * em28xx_write_reg_bits()
- * sets only some bits (specified by bitmask) of a register, by first reading
- * the actual value
+ * em28xx_write_regs_bits()
+ * reads value from a register, modifies only the bits specified with the value
+ * and bitmask parameters and writes the new value two a second register.
  */
-int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
-				 u8 bitmask)
+int em28xx_write_regs_bits(struct em28xx *dev, u16 reg_r, u16 reg_w, u8 val,
+			   u8 bitmask)
 {
 	int oldval;
 	u8 newval;
 
-	oldval = em28xx_read_reg(dev, reg);
+	oldval = em28xx_read_reg(dev, reg_r);
 	if (oldval < 0)
 		return oldval;
 
 	newval = (((u8) oldval) & ~bitmask) | (val & bitmask);
 
-	return em28xx_write_regs(dev, reg, &newval, 1);
+	return em28xx_write_regs(dev, reg_w, &newval, 1);
+}
+EXPORT_SYMBOL_GPL(em28xx_write_regs_bits);
+
+/*
+ * em28xx_write_reg_bits()
+ * modifies only the bits specified with the value and bitmask parameters of
+ * a register
+ */
+int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
+				 u8 bitmask)
+{
+	return em28xx_write_regs_bits(dev, reg, reg, val, bitmask);
 }
 EXPORT_SYMBOL_GPL(em28xx_write_reg_bits);
 
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index e070de0..a817c3d 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -667,8 +667,9 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 reg, char *buf,
 			  int len);
 int em28xx_write_regs(struct em28xx *dev, u16 reg, char *buf, int len);
 int em28xx_write_reg(struct em28xx *dev, u16 reg, u8 val);
-int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val,
-				 u8 bitmask);
+int em28xx_write_regs_bits(struct em28xx *dev, u16 reg_r, u16 reg_w,
+			   u8 val, u8 bitmask);
+int em28xx_write_reg_bits(struct em28xx *dev, u16 reg, u8 val, u8 bitmask);
 
 int em28xx_read_ac97(struct em28xx *dev, u8 reg);
 int em28xx_write_ac97(struct em28xx *dev, u8 reg, u16 val);
-- 
1.7.10.4

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Antti Palosaari]

On 04/13/2013 12:48 PM, Frank Schäfer wrote:
> Patch 1 removes the unneeded and broken gpio register caching code.
> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
> and patch 3 finally adds a new helper function for gpio ports with separate
> registers for read and write access.


I have nothing to say directly about those patches - they looked good at 
the quick check. But I wonder if you have any idea if it is possible to 
use some existing Kernel GPIO functionality in order to provide standard 
interface (interface like I2C). I did some work last summer in order to 
use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA 
control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled 
by default and due to that there is macros to disable LNA when GPIOLIB 
is not compiled.
I noticed recently there is some ongoing development for Kernel GPIO. I 
haven't looked yet if it makes use of GPIO interface more common...

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Mauro Carvalho Chehab]

Em Sat, 13 Apr 2013 16:15:39 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
> > Patch 1 removes the unneeded and broken gpio register caching code.
> > Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
> > and patch 3 finally adds a new helper function for gpio ports with separate
> > registers for read and write access.
> 
> 
> I have nothing to say directly about those patches - they looked good at 
> the quick check. But I wonder if you have any idea if it is possible to 
> use some existing Kernel GPIO functionality in order to provide standard 
> interface (interface like I2C). I did some work last summer in order to 
> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA 
> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled 
> by default and due to that there is macros to disable LNA when GPIOLIB 
> is not compiled.
> I noticed recently there is some ongoing development for Kernel GPIO. I 
> haven't looked yet if it makes use of GPIO interface more common...

I have conflicting opinions myself weather we should use gpiolib or not.

I don't mind with the fact that GPIOLIB is disabled by default. If all
media drivers start depending on it, distros will enable it to keep
media support on it.

I never took the time to take a look on what methods gpiolib provides.
Maybe it will bring some benefits. I dunno.

Just looking at the existing drivers (almost all has some sort of GPIO
config), GPIO is just a single register bitmask read/write. Most drivers
need already bitmask read/write operations. So, in principle, I can't
foresee any code simplification by using a library.

Also, from a very pragmatic view, changing (almost) all existing drivers
to use gpiolib is a big effort.

However, for that to happen, one question should be answered: what
benefits would be obtained by using gpiolib?

Regards,
Mauro

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Antti Palosaari]

On 04/13/2013 05:25 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 13 Apr 2013 16:15:39 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
>>> Patch 1 removes the unneeded and broken gpio register caching code.
>>> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
>>> and patch 3 finally adds a new helper function for gpio ports with separate
>>> registers for read and write access.
>>
>>
>> I have nothing to say directly about those patches - they looked good at
>> the quick check. But I wonder if you have any idea if it is possible to
>> use some existing Kernel GPIO functionality in order to provide standard
>> interface (interface like I2C). I did some work last summer in order to
>> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA
>> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled
>> by default and due to that there is macros to disable LNA when GPIOLIB
>> is not compiled.
>> I noticed recently there is some ongoing development for Kernel GPIO. I
>> haven't looked yet if it makes use of GPIO interface more common...
>
> I have conflicting opinions myself weather we should use gpiolib or not.
>
> I don't mind with the fact that GPIOLIB is disabled by default. If all
> media drivers start depending on it, distros will enable it to keep
> media support on it.
>
> I never took the time to take a look on what methods gpiolib provides.
> Maybe it will bring some benefits. I dunno.

Compare to benefits of I2C bus. It offers standard interface. Also it 
offers userspace debug interface - like I2C also does.

> Just looking at the existing drivers (almost all has some sort of GPIO
> config), GPIO is just a single register bitmask read/write. Most drivers
> need already bitmask read/write operations. So, in principle, I can't
> foresee any code simplification by using a library.

Use of lib interface is not very practical inside of module, however it 
could be used. Again, as compared to I2C there is some bridge drivers 
which do some I2C access using I2C interface, even bridge could do it 
directly (as it offers I2C adapter). I think it is most common to do it 
directly to simplify things.

> Also, from a very pragmatic view, changing (almost) all existing drivers
> to use gpiolib is a big effort.

It is not needed to implement for all driver as one go.

> However, for that to happen, one question should be answered: what
> benefits would be obtained by using gpiolib?

Obtain GPIO access between modules using standard interface and offer 
handy debug interface to switch GPIOs from userspace.

You could ask why we use Kernel I2C library as we could do it directly 
:) Or clock framework. Or SPI, is there SPI bus modeled yet?

regards
Antti

-- 
http://palosaari.fi/

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

[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.

Regards,
Mauro

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Frank Schäfer]

Am 13.04.2013 16:25, schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Apr 2013 16:15:39 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
>>> Patch 1 removes the unneeded and broken gpio register caching code.
>>> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
>>> and patch 3 finally adds a new helper function for gpio ports with separate
>>> registers for read and write access.
>>
>> I have nothing to say directly about those patches - they looked good at 
>> the quick check. But I wonder if you have any idea if it is possible to 
>> use some existing Kernel GPIO functionality in order to provide standard 
>> interface (interface like I2C). I did some work last summer in order to 
>> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA 
>> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled 
>> by default and due to that there is macros to disable LNA when GPIOLIB 
>> is not compiled.
>> I noticed recently there is some ongoing development for Kernel GPIO. I 
>> haven't looked yet if it makes use of GPIO interface more common...
> I have conflicting opinions myself weather we should use gpiolib or not.
>
> I don't mind with the fact that GPIOLIB is disabled by default. If all
> media drivers start depending on it, distros will enable it to keep
> media support on it.

Right.

> I never took the time to take a look on what methods gpiolib provides.
> Maybe it will bring some benefits. I dunno.
>
> Just looking at the existing drivers (almost all has some sort of GPIO
> config), GPIO is just a single register bitmask read/write. Most drivers
> need already bitmask read/write operations. So, in principle, I can't
> foresee any code simplification by using a library.
...
> However, for that to happen, one question should be answered: what
> benefits would be obtained by using gpiolib?
>

I've checked the documentation about the gpio and led frameworks a few
weeks ago to find out if it makes sense to use them for the
gpio/buttons/led stuff of the VAD Laplace webcam.
AFAICS, there are no benfits as long as you are dealing with these
things internally. It just increases the code size and adds an
additional dependency in this case.
Of course, the situation is different when there is an interaction with
other modules or userspace. In that case using gpiolib could make sense.
I don't know which case applies to the LAN stuff, but for the
buttons/leds it's the first case.

Regards,
Frank

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

[Frank Schäfer]

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

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Devin Heitmueller]

On Sat, Apr 13, 2013 at 11:30 AM, Frank Schäfer
<fschaefer.oss@googlemail.com> wrote:
> I've checked the documentation about the gpio and led frameworks a few
> weeks ago to find out if it makes sense to use them for the
> gpio/buttons/led stuff of the VAD Laplace webcam.
> AFAICS, there are no benfits as long as you are dealing with these
> things internally. It just increases the code size and adds an
> additional dependency in this case.
> Of course, the situation is different when there is an interaction with
> other modules or userspace. In that case using gpiolib could make sense.
> I don't know which case applies to the LAN stuff, but for the
> buttons/leds it's the first case.

IMHO, it would be a bad idea to expose the actual GPIOs to userspace.
Improperly setting the GPIOs can cause damage to the board, and all of
the functionality that the GPIOs control are exposed through other
much better supported interfaces.  It's a nice debug feature for
driver developers who want to hack at the driver, but you really don't
want any situation where end users or applications are making direct
use of the GPIOs.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Antti Palosaari]

On 04/13/2013 06:34 PM, Devin Heitmueller wrote:
> On Sat, Apr 13, 2013 at 11:30 AM, Frank Schäfer
> <fschaefer.oss@googlemail.com> wrote:
>> I've checked the documentation about the gpio and led frameworks a few
>> weeks ago to find out if it makes sense to use them for the
>> gpio/buttons/led stuff of the VAD Laplace webcam.
>> AFAICS, there are no benfits as long as you are dealing with these
>> things internally. It just increases the code size and adds an
>> additional dependency in this case.
>> Of course, the situation is different when there is an interaction with
>> other modules or userspace. In that case using gpiolib could make sense.
>> I don't know which case applies to the LAN stuff, but for the
>> buttons/leds it's the first case.
>
> IMHO, it would be a bad idea to expose the actual GPIOs to userspace.
> Improperly setting the GPIOs can cause damage to the board, and all of
> the functionality that the GPIOs control are exposed through other
> much better supported interfaces.  It's a nice debug feature for
> driver developers who want to hack at the driver, but you really don't
> want any situation where end users or applications are making direct
> use of the GPIOs.

Existing userspace sysfs interface is clearly debug interface. You will 
need root privileges to mount it and IIRC it was not even compiled by 
default (needs Kconfig debug option?).

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Devin Heitmueller]

On Sat, Apr 13, 2013 at 12:21 PM, Antti Palosaari <crope@iki.fi> wrote:
> Existing userspace sysfs interface is clearly debug interface. You will need
> root privileges to mount it and IIRC it was not even compiled by default
> (needs Kconfig debug option?).

You would like to think that.  Tell Mauro then, since he wanted to
rely on sysfs to associate V4L2 video devices with ALSA devices
(rather than just adding a simple call to the V4L2 API).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

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

[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.

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.


-- 

Cheers,
Mauro

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

[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.


Frank

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

[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.

See the difference?


Cheers,
Mauro

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

[Frank Schäfer]

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
>

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

[Frank Schäfer]

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

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Mauro Carvalho Chehab]

Em Sat, 13 Apr 2013 17:37:26 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 04/13/2013 05:25 PM, Mauro Carvalho Chehab wrote:
> > Em Sat, 13 Apr 2013 16:15:39 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
> >>> Patch 1 removes the unneeded and broken gpio register caching code.
> >>> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
> >>> and patch 3 finally adds a new helper function for gpio ports with separate
> >>> registers for read and write access.
> >>
> >>
> >> I have nothing to say directly about those patches - they looked good at
> >> the quick check. But I wonder if you have any idea if it is possible to
> >> use some existing Kernel GPIO functionality in order to provide standard
> >> interface (interface like I2C). I did some work last summer in order to
> >> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA
> >> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled
> >> by default and due to that there is macros to disable LNA when GPIOLIB
> >> is not compiled.
> >> I noticed recently there is some ongoing development for Kernel GPIO. I
> >> haven't looked yet if it makes use of GPIO interface more common...
> >
> > I have conflicting opinions myself weather we should use gpiolib or not.
> >
> > I don't mind with the fact that GPIOLIB is disabled by default. If all
> > media drivers start depending on it, distros will enable it to keep
> > media support on it.
> >
> > I never took the time to take a look on what methods gpiolib provides.
> > Maybe it will bring some benefits. I dunno.
> 
> Compare to benefits of I2C bus. It offers standard interface. Also it 
> offers userspace debug interface - like I2C also does.

I2C benefit is that the same I2C driver can be used by several different
drivers. GPIO code, on the other hand, is on most cases[1] specific to a
given device.

[1] Ok, if you're using a GPIO pin to carry some protocol inside it, like
UART, RC, etc, then I can see a benefit on using a bus type of solution.

> > Just looking at the existing drivers (almost all has some sort of GPIO
> > config), GPIO is just a single register bitmask read/write. Most drivers
> > need already bitmask read/write operations. So, in principle, I can't
> > foresee any code simplification by using a library.
> 
> Use of lib interface is not very practical inside of module, however it 
> could be used. Again, as compared to I2C there is some bridge drivers 
> which do some I2C access using I2C interface, even bridge could do it 
> directly (as it offers I2C adapter). I think it is most common to do it 
> directly to simplify things.
> 
> > Also, from a very pragmatic view, changing (almost) all existing drivers
> > to use gpiolib is a big effort.
> 
> It is not needed to implement for all driver as one go.
> 
> > However, for that to happen, one question should be answered: what
> > benefits would be obtained by using gpiolib?
> 
> Obtain GPIO access between modules using standard interface and offer 
> handy debug interface to switch GPIOs from userspace.

It is known that enabling both analog and digital demods at the same time
can melt some devices. So, it is risky to allow userspace to touch
the GPIOs that enable such chips.

(ok, there are also other forms to melt such devices in userspace
 if the user has CAP_SYS_ADMIN)

> You could ask why we use Kernel I2C library as we could do it directly 
> :) Or clock framework. Or SPI, is there SPI bus modeled yet?

As I said, i2c allowed code re-usage. Probably, the clock framework and
SPI also can be used for that.

With regards to GPIO, at least currently, I can only see its usage
justified, in terms of code reuse, for remote controllers.

Cheers,
Mauro

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Antti Palosaari]

On 04/14/2013 04:32 AM, Mauro Carvalho Chehab wrote:
> Em Sat, 13 Apr 2013 17:37:26 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 04/13/2013 05:25 PM, Mauro Carvalho Chehab wrote:
>>> Em Sat, 13 Apr 2013 16:15:39 +0300
>>> Antti Palosaari <crope@iki.fi> escreveu:
>>>
>>>> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
>>>>> Patch 1 removes the unneeded and broken gpio register caching code.
>>>>> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
>>>>> and patch 3 finally adds a new helper function for gpio ports with separate
>>>>> registers for read and write access.
>>>>
>>>>
>>>> I have nothing to say directly about those patches - they looked good at
>>>> the quick check. But I wonder if you have any idea if it is possible to
>>>> use some existing Kernel GPIO functionality in order to provide standard
>>>> interface (interface like I2C). I did some work last summer in order to
>>>> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA
>>>> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled
>>>> by default and due to that there is macros to disable LNA when GPIOLIB
>>>> is not compiled.
>>>> I noticed recently there is some ongoing development for Kernel GPIO. I
>>>> haven't looked yet if it makes use of GPIO interface more common...
>>>
>>> I have conflicting opinions myself weather we should use gpiolib or not.
>>>
>>> I don't mind with the fact that GPIOLIB is disabled by default. If all
>>> media drivers start depending on it, distros will enable it to keep
>>> media support on it.
>>>
>>> I never took the time to take a look on what methods gpiolib provides.
>>> Maybe it will bring some benefits. I dunno.
>>
>> Compare to benefits of I2C bus. It offers standard interface. Also it
>> offers userspace debug interface - like I2C also does.
>
> I2C benefit is that the same I2C driver can be used by several different
> drivers. GPIO code, on the other hand, is on most cases[1] specific to a
> given device.

That is same for GPIO - it offers standard interface between modules for 
GPIO "bus".

I used it to control LNA, which is connected to demodulator (cxd2820r) 
GPIO. It is bridge which gets LNA API commands and GPIO is property of 
demod. Some interface is needed in order to deliver data between bridge 
and demod in that case.


> [1] Ok, if you're using a GPIO pin to carry some protocol inside it, like
> UART, RC, etc, then I can see a benefit on using a bus type of solution.
>
>>> Just looking at the existing drivers (almost all has some sort of GPIO
>>> config), GPIO is just a single register bitmask read/write. Most drivers
>>> need already bitmask read/write operations. So, in principle, I can't
>>> foresee any code simplification by using a library.
>>
>> Use of lib interface is not very practical inside of module, however it
>> could be used. Again, as compared to I2C there is some bridge drivers
>> which do some I2C access using I2C interface, even bridge could do it
>> directly (as it offers I2C adapter). I think it is most common to do it
>> directly to simplify things.
>>
>>> Also, from a very pragmatic view, changing (almost) all existing drivers
>>> to use gpiolib is a big effort.
>>
>> It is not needed to implement for all driver as one go.
>>
>>> However, for that to happen, one question should be answered: what
>>> benefits would be obtained by using gpiolib?
>>
>> Obtain GPIO access between modules using standard interface and offer
>> handy debug interface to switch GPIOs from userspace.
>
> It is known that enabling both analog and digital demods at the same time
> can melt some devices. So, it is risky to allow userspace to touch
> the GPIOs that enable such chips.
>
> (ok, there are also other forms to melt such devices in userspace
>   if the user has CAP_SYS_ADMIN)

Do you need eyeglasses? I said it is debug interface. It needs root 
privileges in order to setup and use.

I can say I could surely break more devices via I2C debug interface than 
GPIO debug interface in case both are implemented by every driver. Just 
sent garbage writes to well known eeprom addresses and kaboom. Your 
device is bricked. It is totally stupid to say you could brick your 
device using debug functionality - yes you can, but it is very unlikely 
someone does it as a mistake.


>> You could ask why we use Kernel I2C library as we could do it directly
>> :) Or clock framework. Or SPI, is there SPI bus modeled yet?
>
> As I said, i2c allowed code re-usage. Probably, the clock framework and
> SPI also can be used for that.
>
> With regards to GPIO, at least currently, I can only see its usage
> justified, in terms of code reuse, for remote controllers.

Maybe better to read Kernel GPIO documentation. There is few points 
mentioned why to use it and what are advantages.

regards
Antti

-- 
http://palosaari.fi/

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

[Frank Schäfer]

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

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

[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. 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

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

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: [PATCH 0/3] em28xx: clean up end extend the GPIO port handling

[Mauro Carvalho Chehab]

Em Sun, 14 Apr 2013 22:32:34 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 04/14/2013 04:32 AM, Mauro Carvalho Chehab wrote:
> > Em Sat, 13 Apr 2013 17:37:26 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> On 04/13/2013 05:25 PM, Mauro Carvalho Chehab wrote:
> >>> Em Sat, 13 Apr 2013 16:15:39 +0300
> >>> Antti Palosaari <crope@iki.fi> escreveu:
> >>>
> >>>> On 04/13/2013 12:48 PM, Frank Schäfer wrote:
> >>>>> Patch 1 removes the unneeded and broken gpio register caching code.
> >>>>> Patch 2 adds the gpio register defintions for the em25xx/em276x/7x/8x
> >>>>> and patch 3 finally adds a new helper function for gpio ports with separate
> >>>>> registers for read and write access.
> >>>>
> >>>>
> >>>> I have nothing to say directly about those patches - they looked good at
> >>>> the quick check. But I wonder if you have any idea if it is possible to
> >>>> use some existing Kernel GPIO functionality in order to provide standard
> >>>> interface (interface like I2C). I did some work last summer in order to
> >>>> use GPIOLIB and it is used between em28xx-dvb and cxd2820r for LNA
> >>>> control. Anyhow, I was a little bit disappointed as GPIOLIB is disabled
> >>>> by default and due to that there is macros to disable LNA when GPIOLIB
> >>>> is not compiled.
> >>>> I noticed recently there is some ongoing development for Kernel GPIO. I
> >>>> haven't looked yet if it makes use of GPIO interface more common...
> >>>
> >>> I have conflicting opinions myself weather we should use gpiolib or not.
> >>>
> >>> I don't mind with the fact that GPIOLIB is disabled by default. If all
> >>> media drivers start depending on it, distros will enable it to keep
> >>> media support on it.
> >>>
> >>> I never took the time to take a look on what methods gpiolib provides.
> >>> Maybe it will bring some benefits. I dunno.
> >>
> >> Compare to benefits of I2C bus. It offers standard interface. Also it
> >> offers userspace debug interface - like I2C also does.
> >
> > I2C benefit is that the same I2C driver can be used by several different
> > drivers. GPIO code, on the other hand, is on most cases[1] specific to a
> > given device.
> 
> That is same for GPIO - it offers standard interface between modules for 
> GPIO "bus".
> 
> I used it to control LNA, which is connected to demodulator (cxd2820r) 
> GPIO. It is bridge which gets LNA API commands and GPIO is property of 
> demod. Some interface is needed in order to deliver data between bridge 
> and demod in that case.

LNA control is device specific. I fail to see code optimization with any
GPIO that it is used only as a simple switch.

Ok, if you're doing something more complex at a GPIO, like implementing an
UART protocol or I2C (cx231xx does I2C via GPIO), then I can see an advantage
on using a library, as the UART/I2C code can be written on a very generic
way, using the GPIOLIB to connect the generic code to the device specific
GPIO.

> > [1] Ok, if you're using a GPIO pin to carry some protocol inside it, like
> > UART, RC, etc, then I can see a benefit on using a bus type of solution.
> >
> >>> Just looking at the existing drivers (almost all has some sort of GPIO
> >>> config), GPIO is just a single register bitmask read/write. Most drivers
> >>> need already bitmask read/write operations. So, in principle, I can't
> >>> foresee any code simplification by using a library.
> >>
> >> Use of lib interface is not very practical inside of module, however it
> >> could be used. Again, as compared to I2C there is some bridge drivers
> >> which do some I2C access using I2C interface, even bridge could do it
> >> directly (as it offers I2C adapter). I think it is most common to do it
> >> directly to simplify things.
> >>
> >>> Also, from a very pragmatic view, changing (almost) all existing drivers
> >>> to use gpiolib is a big effort.
> >>
> >> It is not needed to implement for all driver as one go.
> >>
> >>> However, for that to happen, one question should be answered: what
> >>> benefits would be obtained by using gpiolib?
> >>
> >> Obtain GPIO access between modules using standard interface and offer
> >> handy debug interface to switch GPIOs from userspace.
> >
> > It is known that enabling both analog and digital demods at the same time
> > can melt some devices. So, it is risky to allow userspace to touch
> > the GPIOs that enable such chips.
> >
> > (ok, there are also other forms to melt such devices in userspace
> >   if the user has CAP_SYS_ADMIN)
> 
> Do you need eyeglasses? I said it is debug interface. It needs root 
> privileges in order to setup and use.
> 
> I can say I could surely break more devices via I2C debug interface than 
> GPIO debug interface in case both are implemented by every driver. Just 
> sent garbage writes to well known eeprom addresses and kaboom. Your 
> device is bricked. It is totally stupid to say you could brick your 
> device using debug functionality - yes you can, but it is very unlikely 
> someone does it as a mistake.

There are lots of reported cases of devices that got their
eeprom corrupted. We even wrote a tool to allow recovering such damaged
eeproms.

Yet, an eeprom data is something that can be recovered (if the dump of the
old eeprom can be obtained from someone else or from a previous dump).
A melted device can't be recovered at all.

> >> You could ask why we use Kernel I2C library as we could do it directly
> >> :) Or clock framework. Or SPI, is there SPI bus modeled yet?
> >
> > As I said, i2c allowed code re-usage. Probably, the clock framework and
> > SPI also can be used for that.
> >
> > With regards to GPIO, at least currently, I can only see its usage
> > justified, in terms of code reuse, for remote controllers.
> 
> Maybe better to read Kernel GPIO documentation. There is few points 
> mentioned why to use it and what are advantages.

Well, I'm a little pragmatic with it. Basically, if you think it is worth
using it, well, write a patch converting from the way we currently do
to gpiolib, and let's see. If the end result is better than before, I'm
OK with that.

Regards,
Mauro

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

[Frank Schäfer]

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

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

[Mauro Carvalho Chehab]

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
> 


-- 

Cheers,
Mauro

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

[Frank Schäfer]

...
>>>
>>>> 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