Add bt8xxgpio driver
Subsystems (3)
PATCH v3 1/1 Add bt8xxgpio driver
This adds the bt8xxgpio driver.
The purpose of the bt8xxgpio driver is to export all of the 24 GPIO pins available on Brooktree 8xx chips to the kernel GPIO infrastructure.
This makes it possible to use a physically modified BT8xx card as cheap digital GPIO card.
---
Please queue this patch for 2.6.27
checkpatch warnings were cleaned up since the last version. defines were added for PM functions, as requested.
drivers/gpio/Kconfig +18 -0
@@ -92,6 +92,24 @@ config GPIO_PCF857X This driver provides an in-kernel interface to those GPIOs using platform-neutral GPIO calls. +comment "PCI GPIO expanders:" + +config GPIO_BT8XX + tristate "BT8XX GPIO abuser" + depends on PCI && VIDEO_BT848=n + help + The BT8xx frame grabber chip has 24 GPIO pins than can be abused + as a cheap PCI GPIO card. + + This chip can be found on Miro, Hauppauge and STB TV-cards. + + The card needs to be physically altered for using it as a + GPIO card. For more information on how to build a GPIO card + from a BT8xx TV card, see the documentation file at + Documentation/bt8xxgpio.txt + + If unsure, say N. + comment "SPI GPIO expanders:" config GPIO_MCP23S08
drivers/gpio/Makefile +1 -0
@@ -7,3 +7,4 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o obj-$(CONFIG_GPIO_PCA953X) += pca953x.o obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o +obj-$(CONFIG_GPIO_BT8XX) += bt8xxgpio.o
drivers/gpio/bt8xxgpio.c new file +348 -0
348 lines not on this page · raw message
MAINTAINERS +6 -0
@@ -1035,6 +1035,12 @@ M: [email protected] L: [email protected] S: Supported +BT8XXGPIO DRIVER +P: Michael Buesch +M: [email protected] +W: http://bu3sch.de/btgpio.php +S: Maintained + BTTV VIDEO4LINUX DRIVER P: Mauro Carvalho Chehab M: [email protected]
Documentation/bt8xxgpio.txt new file +67 -0
67 lines not on this page · raw message
Signature · 1 line
Greetings Michael.
<Moved from the bottom> +A·generic·digital·24-port·PCI·GPIO·card·can·be·built·out·of·an·ordinary +Brooktree·bt848,·bt849,·bt878·or·bt879·based·analog·TV·tuner·card.·The +Brooktree·chip·is·used·in·old·analog·Hauppauge·WinTV·PCI·cards.·You·can·easily +find·them·used·for·low·prices·on·the·net.
Thanks for that!
Quoted 5 lines · Michael Buesch
Index:
linux-next/drivers/gpio/bt8xxgpio.c
===================================================================drivers/gpio/bt8xxgpio.c new file +0
-0@@ -0,0 +1,348 @@
[...]
drivers/gpio/bt8xxgpio.c line 86
Quoted 7 lines
drivers/gpio/bt8xxgpio.c new file +7
-0+static int bt8xxgpio_gpio_direction_input (struct gpio_chip *gpio,unsigned nr) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags; +u32 outen, data; + +spin_lock_irqsave (&bg->lock, flags);
Why all those irq variants? I can't see interrupts anywhere. May gpio call this from irq?
drivers/gpio/bt8xxgpio.c line 94
Quoted 8 lines
drivers/gpio/bt8xxgpio.c new file +8
-0+ + data =bgread (BT848_GPIO_DATA ); + data &= ~(1 << nr); +bgwrite (data,BT848_GPIO_DATA ); + + outen =bgread (BT848_GPIO_OUT_EN ); + outen &= ~(1 << nr); +bgwrite (outen,BT848_GPIO_OUT_EN );
some flushing of posted values here?
Quoted 4 lines
drivers/gpio/bt8xxgpio.c new file +4
-0+spin_unlock_irqrestore (&bg->lock, flags); + +return 0 ; +}
[...]
Quoted 9 lines
drivers/gpio/bt8xxgpio.c new file +9
-0+static int bt8xxgpio_gpio_direction_output (struct gpio_chip *gpio, +unsigned nr,int val) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags; +u32 outen, data; + +spin_lock_irqsave (&bg->lock, flags); +
drivers/gpio/bt8xxgpio.c line 132
Quoted 10 lines
drivers/gpio/bt8xxgpio.c new file +10
-0+ outen =bgread (BT848_GPIO_OUT_EN ); + outen |= (1 << nr); +bgwrite (outen,BT848_GPIO_OUT_EN ); + + data =bgread (BT848_GPIO_DATA ); +if (val) + data |= (1 << nr); +else + data &= ~(1 << nr); +bgwrite (data,BT848_GPIO_DATA );
and here
Quoted 11 lines
drivers/gpio/bt8xxgpio.c new file +11
-0+ +spin_unlock_irqrestore (&bg->lock, flags); + +return 0 ; +} + +static void bt8xxgpio_gpio_set (struct gpio_chip *gpio, +unsigned nr,int val) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags;
drivers/gpio/bt8xxgpio.c line 153
Quoted 10 lines
drivers/gpio/bt8xxgpio.c new file +10
-0+u32 data; + +spin_lock_irqsave (&bg->lock, flags); + + data =bgread (BT848_GPIO_DATA ); +if (val) + data |= (1 << nr); +else + data &= ~(1 << nr); +bgwrite (data,BT848_GPIO_DATA );
and here and further in the code
Quoted 2 lines
drivers/gpio/bt8xxgpio.c new file +2
-0+spin_unlock_irqrestore (&bg->lock, flags); +}
[...]
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+static int bt8xxgpio_probe (struct pci_dev *dev, +const struct pci_device_id *pci_id) +{ +struct bt8xxgpio *bg; +int err; +
drivers/gpio/bt8xxgpio.c line 189
Quoted 10 lines
drivers/gpio/bt8xxgpio.c new file +10
-0+ bg =kzalloc (sizeof (*bg),GFP_KERNEL ); +if (!bg) +return -ENOMEM ; + + bg->pdev = dev; +spin_lock_init (&bg->lock); + + err =pci_enable_device (dev); +if (err) { +printk (KERN_ERR "bt8xxgpio: Can't enable device.\n" );
dev_err() et al. all over here.
drivers/gpio/bt8xxgpio.c line 194
Quoted 5 lines
drivers/gpio/bt8xxgpio.c new file +5
-0+ goto err_freebg; + } +if (!request_mem_region (pci_resource_start (dev,0 ), +pci_resource_len (dev,0 ), +"bt8xxgpio" )) {
pci_request_region();?
drivers/gpio/bt8xxgpio.c line 197
Quoted 3 lines
drivers/gpio/bt8xxgpio.c new file +3
-0+printk (KERN_WARNING +"bt8xxgpio: Can't request iomem (0x%llx).\n" , + (unsigned long long )pci_resource_start (dev,0 ));
You don't need to print the value out, we can see it in lspci.
Quoted 12 lines
drivers/gpio/bt8xxgpio.c new file +12
-0+ err = -EBUSY ; + goto err_disable; + } +pci_set_master (dev); +pci_set_drvdata (dev, bg); + + bg->mmio =ioremap (pci_resource_start (dev,0 ),0x1000 ); +if (!bg->mmio) { +printk (KERN_ERR "bt8xxgpio: ioremap() failed\n" ); + err = -EIO ; + goto err_release_mem; + }
[...]
drivers/gpio/bt8xxgpio.c line 292
Quoted 7 lines
drivers/gpio/bt8xxgpio.c new file +7
-0+static int bt8xxgpio_resume (struct pci_dev *pdev) +{ +struct bt8xxgpio *bg =pci_get_drvdata (pdev); +unsigned long flags; +int err; + +pci_set_power_state (pdev,0 );
No need to set that state again (PCI layer cares).
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+ err =pci_enable_device (pdev); +if (err) +return err; +pci_restore_state (pdev); + +spin_lock_irqsave (&bg->lock, flags);
[...]
Quoted 1 line
+static int
bt8xxgpio_init(void)
__init
drivers/gpio/bt8xxgpio.c line 340
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+{ +return pci_register_driver (&bt8xxgpio_pci_driver); +} +module_init (bt8xxgpio_init) + +static void bt8xxgpio_exit(void)
__exit
Quoted 4 lines
drivers/gpio/bt8xxgpio.c new file +4
-0+{ +pci_unregister_driver (&bt8xxgpio_pci_driver); +} +module_exit (bt8xxgpio_exit)
Quoted 10 lines · On Thursday 10 July 2008 20:15:38 Jiri Slaby wrote:
drivers/gpio/bt8xxgpio.c line 86
Quoted 7 lines
drivers/gpio/bt8xxgpio.c new file +7
-0+static int bt8xxgpio_gpio_direction_input (struct gpio_chip *gpio,unsigned nr) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags; +u32 outen, data; + +spin_lock_irqsave (&bg->lock, flags);Why all those irq variants? I can't see interrupts anywhere. May gpio call this from irq?
Yes. Call context is undefined.
Quoted 21 lines
drivers/gpio/bt8xxgpio.c line 94
Quoted 8 lines
drivers/gpio/bt8xxgpio.c new file +8
-0+ + data =bgread (BT848_GPIO_DATA ); + data &= ~(1 << nr); +bgwrite (data,BT848_GPIO_DATA ); + + outen =bgread (BT848_GPIO_OUT_EN ); + outen &= ~(1 << nr); +bgwrite (outen,BT848_GPIO_OUT_EN );some flushing of posted values here?
Quoted 9 lines
drivers/gpio/bt8xxgpio.c new file +9
-0+static void bt8xxgpio_gpio_set (struct gpio_chip *gpio, +unsigned nr,int val) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags; +u32 data; + +spin_lock_irqsave (&bg->lock, flags); +
Quoted 8 lines
drivers/gpio/bt8xxgpio.c line 132
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+ data =bgread (BT848_GPIO_DATA ); +if (val) + data |= (1 << nr); +else + data &= ~(1 << nr); +bgwrite (data,BT848_GPIO_DATA );and here and further in the code
Well, I'm not sure. This is pretty damn hotpath code. It can be used for bitbanging busdata, for example. I do actually consider optimizing that bgread() in the function above away, by caching it in memory. (First need to do measurements on whether it improves stuff).
So well. What about flushing. In theory it might be needed on some hardware. But unless somebody can show me some hardware, where this actually breaks without flushing, I won't consider adding flushes for performance reasons.
However, an mmiowb() might be added here right before the spinlock to make the locking safe.
Quoted 22 lines
Quoted 2 lines
(quoted diff) +2
-0+ spin_unlock_irqrestore(&bg->lock, flags); +}[...]
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+static int bt8xxgpio_probe (struct pci_dev *dev, +const struct pci_device_id *pci_id) +{ +struct bt8xxgpio *bg; +int err; +drivers/gpio/bt8xxgpio.c line 189
Quoted 10 lines
drivers/gpio/bt8xxgpio.c new file +10
-0+ bg =kzalloc (sizeof (*bg),GFP_KERNEL ); +if (!bg) +return -ENOMEM ; + + bg->pdev = dev; +spin_lock_init (&bg->lock); + + err =pci_enable_device (dev); +if (err) { +printk (KERN_ERR "bt8xxgpio: Can't enable device.\n" );
dev_err()et al. all over here.
Quoted 7 lines
drivers/gpio/bt8xxgpio.c line 194
Quoted 5 lines
drivers/gpio/bt8xxgpio.c new file +5
-0+ goto err_freebg; + } +if (!request_mem_region (pci_resource_start (dev,0 ), +pci_resource_len (dev,0 ), +"bt8xxgpio" )) {
pci_request_region();?
Hell, I shouldn't have derived this from the bttv driver. Almost all style comments origin from that. ;)
Quoted 5 lines
drivers/gpio/bt8xxgpio.c line 197
Quoted 3 lines
drivers/gpio/bt8xxgpio.c new file +3
-0+printk (KERN_WARNING +"bt8xxgpio: Can't request iomem (0x%llx).\n" , + (unsigned long long )pci_resource_start (dev,0 ));You don't need to print the value out, we can see it in lspci.
Also from bttv...
Signature · 1 line
Greetings Michael.
Quoted 4 lines · Jiri Slaby
Quoted 3 lines
drivers/gpio/bt8xxgpio.c new file +0
-0@@ -0,0 +1,348 @@[...]
Quoted 10 lines
drivers/gpio/bt8xxgpio.c line 86
Quoted 7 lines
drivers/gpio/bt8xxgpio.c new file +7
-0+static int bt8xxgpio_gpio_direction_input (struct gpio_chip *gpio,unsigned nr) +{ + struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio); +unsigned long flags; +u32 outen, data; + +spin_lock_irqsave (&bg->lock, flags);Why all those irq variants? I can't see interrupts anywhere. May gpio call this from irq?
Not that routine (see Documentation/gpio.txt where that's specified) ... but other using the same lock.
When setting GPIO direction, spin_lock_irq() style calls are appropriate (but this isn't wrong).
The gpio_{get,set}_value() accessors may be called from IRQ context, so they need to save/restor the IRQ flags.
Quoted 1 line
some flushing of posted values here?
See Documentation/gpio.txt:
+ Note that these operations include I/O barriers on platforms
+ which need to use them; drivers don't need to add them explicitly.
That's the key thing: drivers using I/O calls should not need to insert bus or platform specific calls to make sure the calls take effect.
Also:
drivers/gpio/bt8xxgpio.c line 111
Quoted 1 line
drivers/gpio/bt8xxgpio.c new file +1
-0+return !!(val & (1 << nr));
GPIO values are zero/nonzero, not zero/one. So the "!!" can be removed.
- Dave
Quoted 8 lines · On Thursday 10 July 2008 22:02:12 David Brownell wrote:
See
Documentation/gpio.txt:+ Note that these operations include I/O barriers on platforms
+ which need to use them; drivers don't need to add them explicitly.That's the key thing: drivers using I/O calls should not need to insert bus or platform specific calls to make sure the calls take effect.
Well, as I said, I am OK with an I/O-barrier, which is mmiowb(). But flushing, by reading the register back, is not acceptable for me in this case, unless someone can show me hardware that needs this. And even then I'd implement the flushing only conditionally for that hardware.
Signature · 1 line
Greetings Michael.
Hi Michael,
Quoted 19 lines · On Thu, 10 Jul 2008 19:14:09 +0200 Michael Buesch <[email protected]> wrote:
drivers/gpio/Kconfig +18
-0+comment "PCI GPIO expanders:" + +config GPIO_BT8XX + tristate "BT8XX GPIO abuser" + depends on PCI && VIDEO_BT848=n + help + The BT8xx frame grabber chip has 24 GPIO pins than can be abused + as a cheap PCI GPIO card. + + This chip can be found on Miro, Hauppauge and STB TV-cards. + + The card needs to be physically altered for using it as a + GPIO card. For more information on how to build a GPIO card + from a BT8xx TV card, see the documentation file at + Documentation/bt8xxgpio.txt + + If unsure, say N. + comment "SPI GPIO expanders:"
...
drivers/gpio/bt8xxgpio.c line 321
Quoted 6 lines
drivers/gpio/bt8xxgpio.c new file +6
-0+static struct pci_device_id bt8xxgpio_pci_tbl[] = { + {PCI_DEVICE (PCI_VENDOR_ID_BROOKTREE ,PCI_DEVICE_ID_BT848 ) }, + {PCI_DEVICE (PCI_VENDOR_ID_BROOKTREE ,PCI_DEVICE_ID_BT849 ) }, + {PCI_DEVICE (PCI_VENDOR_ID_BROOKTREE ,PCI_DEVICE_ID_BT878 ) }, + {PCI_DEVICE (PCI_VENDOR_ID_BROOKTREE ,PCI_DEVICE_ID_BT879 ) }, + {0 , },
Even being an old design, there are still several new cheap boards based on those devices (for example, I received those days a new Encore board with a bttv chip). So, a driver for electronics hackers for using this chip as a generic io driver shouldn't cause regressions at the real video streaming driver, that are used by a large number of people.
However, the way you've defined the pci table, and your Kconfig allows someone to compile both the real driver and the gpio only driver, and they are currently incompatible.
So, this will cause a regression.
One alternative would be to make the gpio driver dependent of !VIDEO_BT848.
However, a much better alternative would be if you can rework on it to be a module that adds this functionality to the original driver, allowing to have both video control and gpio control, since, on a few cases like surveillance systems, the gpio's may be used for other things, like controlling security sensors, or switching a video commutter.
Btw, if you are thinking on using it for your electronics hacking, one very interesting feature would be to use the bttv 8-bit high speed ADC as a generic ADC. On its normal operation, it samples from video inputs at 27 MHz (maybe you can sample on even higher frequencies, by properly configuring the sampling divider).
Cheers,
Mauro
Quoted 2 lines · On Thursday 10 July 2008 21:02:58 Mauro Carvalho Chehab wrote:
and your
Kconfigallows someone
to compile both the real driver and the gpio only driver
No it doesn't.
Quoted 1 line
So, this will cause a regression.
No it isn't.
Quoted 1 line
One alternative would be to make the gpio driver dependent of !VIDEO_BT848.
+config GPIO_BT8XX
+ tristate "BT8XX GPIO abuser"
+ depends on PCI && VIDEO_BT848=n
Quoted 4 lines
Btw, if you are thinking on using it for your electronics hacking, one very interesting feature would be to use the bttv 8-bit high speed ADC as a generic ADC. On its normal operation, it samples from video inputs at 27 MHz (maybe you can sample on even higher frequencies, by properly configuring the sampling divider).
Cool, nice to know. But I'm currently only interested in digital IO.
Signature · 1 line
Greetings Michael.
drivers/gpio/Kconfig config GPIO_PCF857X · line 99
Quoted 5 lines · On Thu, 10 Jul 2008 21:12:09 +0200 Michael Buesch <[email protected]> wrote:
Quoted 1 line
One alternative would be to make the gpio driver dependent of !VIDEO_BT848.
drivers/gpio/Kconfig +3
-0+config GPIO_BT8XX + tristate "BT8XX GPIO abuser" + depends on PCI && VIDEO_BT848=n
Seems fine to me.
Cheers,
Mauro
Quoted 10 lines · On Thursday 10 July 2008 21:33:54 Mauro Carvalho Chehab wrote:
drivers/gpio/Kconfig config GPIO_PCF857X · line 99
Quoted 5 lines · On Thu, 10 Jul 2008 21:12:09 +0200 Michael Buesch <[email protected]> wrote:
Quoted 1 line
One alternative would be to make the gpio driver dependent of !VIDEO_BT848.
drivers/gpio/Kconfig +3
-0+config GPIO_BT8XX + tristate "BT8XX GPIO abuser" + depends on PCI && VIDEO_BT848=nSeems fine to me.
This is what's currently in the code. So I'm not sure what you are complaining about. I'm confused.
Signature · 1 line
Greetings Michael.
Hi,
Quoted 6 lines · Mauro Carvalho Chehab
However, a much better alternative would be if you can rework on it to be a module that adds this functionality to the original driver, allowing to have both video control and gpio control, since, on a few cases like surveillance systems, the gpio's may be used for other things, like controlling security sensors, or switching a video commutter.
I need this. Is anybody working on it? If nobody is, I step forward.
What about a tiny bttv sub-driver in its own tiny module?
The sub-driver would be configurable card-by-card, so that those using gpio ports for their remote controls would keep them hidden to gpiolib.
How instead could all these gpios communicate to user-space?
cheers,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Michael Buesch
Quoted 8 lines · On Sunday 13 July 2008 02:42:48 Domenico Andreoli wrote:
Quoted 6 lines · Mauro Carvalho Chehab
However, a much better alternative would be if you can rework on it to be a module that adds this functionality to the original driver, allowing to have both video control and gpio control, since, on a few cases like surveillance systems, the gpio's may be used for other things, like controlling security sensors, or switching a video commutter.
Quoted 4 lines
Quoted 1 line
I need this. Is anybody working on it? If nobody is, I step forward.
Feel free to do so. I didn't want to go down the road to change bttv, as I don't need the bttv functionality (the card doesn't work as TV card anymore).
Here is a first incomplete patch I would like to discuss.
Quoted 7 lines
So if you do that, please make sure that:
- The driver can be loaded on cards that don't work as TV cards anymore. I removed the tuner and other stuff from the card. I also detached all GPIO pins. So the original bttv will get confused when trying to initialize it. So there must be a way to load the bttv driver in "gpio-only" mode then.
- In the gpio-only mode it must be possible to access _all_ GPIO pins, including those that were originally used by the card itself.
Something respecting these conditions clearly conflicts with bttv and looks like your bt8xxgpio driver. Indeed I do not see the need of dropping it.
Currently my patch requires the bttv functionality, since it is done to work toghether with bttv. Surely one must be able to use bttv only, without gpiolib.
The definition of bttv_gpiolib_card[] in bttv-gpiolib.c stinks, it's aim is to easily allow people to add their favourite card gpio configuration, but I am pretty sure its initialization is completely wrong. Please give me some suggestion.
cheers,
Domenico
This patch adds the gpiolib bttv sub-driver.
Every bttv card may define a set of available gpio ports which are registered to gpiolib and then made available through kernel's generic gpio interface.
--
State of the patch: unusable. RFC
drivers/media/video/bt8xx/Kconfig +6 -0
@@ -21,6 +21,12 @@ To compile this driver as a module, choose M here: the module will be called bttv. +config VIDEO_BT848_GPIO + tristate "GPIO support for bt848 cards" + depends on VIDEO_BT848 && GPIOLIB + ---help--- + This enables GPIOLIB bindings on BT848 boards. + config VIDEO_BT848_DVB bool "DVB/ATSC Support for bt878 based TV cards" depends on VIDEO_BT848 && DVB_CORE
drivers/media/video/bt8xx/Makefile +4 -0
@@ -6,7 +6,11 @@ bttv-risc.o bttv-vbi.o bttv-i2c.o bttv-gpio.o \ bttv-input.o bttv-audio-hook.o +#ifeq ($(CONFIG_VIDEO_BT848_GPIO),y) +#endif + obj-$(CONFIG_VIDEO_BT848) += bttv.o +obj-$(CONFIG_VIDEO_BT848_GPIO) += bttv-gpiolib.o EXTRA_CFLAGS += -Idrivers/media/video EXTRA_CFLAGS += -Idrivers/media/common/tuners
drivers/media/video/bt8xx/bttv-cards.c +1 -0
@@ -2162,6 +2162,7 @@ .gpiomask = 0xdf, .muxsel = { 2 }, .pll = PLL_28, + .has_gpiolib = 1, }, [BTTV_BOARD_XGUARD] = { .name = "Grand X-Guard / Trust 814PCI",
drivers/media/video/bt8xx/bttv-driver.c +4 -0
@@ -4455,6 +4455,10 @@request_modules (btv); } +/* add gpiolib subdevice */ +if (bttv_tvcards[btv->c.type].has_gpiolib) +bttv_sub_add_device (&btv->c,"gpiolib" ); +bttv_input_init (btv);/* everything is fine */
drivers/media/video/bt8xx/bttv-gpiolib.c new file +153 -0
153 lines not on this page · raw message
drivers/media/video/bt8xx/bttv.h +2 -0
@@ -249,6 +249,8 @@void (*audio_mode_gpio)(struct bttv *btv,struct v4l2_tuner *tuner,int set);void (*muxsel_hook)(struct bttv *btv,unsigned int input); + + unsigned int has_gpiolib:1; };extern struct tvcard bttv_tvcards[];
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Michael Buesch
Quoted 8 lines · On Sunday 13 July 2008 17:43:33 Domenico Andreoli wrote:
Index: v4l-dvb.git/drivers/media/video/bt8xx/Makefile
===================================================================drivers/media/video/bt8xx/Makefile +0
-0@@ -6,7 +6,11 @@ bttv-risc.o bttv-vbi.o bttv-i2c.o bttv-gpio.o \ bttv-input.o bttv-audio-hook.o
[...]
Quoted 8 lines
Quoted 8 lines
drivers/media/video/bt8xx/Makefile +8
-0+ +static void __devexit bttv_gpiolib_remove(struct bttv_sub_device *sub) +{ + int ret; + struct bttv_gpiolib_device *device = dev_get_drvdata(&sub->dev); + + while((ret = gpiochip_remove(&device->chip)) != 0) { + printk(KERN_INFO "error unregistering chip %s: %d\n", device->chip.label, ret);
Quoted 8 lines
drivers/media/video/bt8xx/Makefile
Quoted 2 lines
drivers/media/video/bt8xx/Makefile +2
-0+ schedule(); + }This loop is dangerous.
Simply try to unregister it and exit with an error message if it failed. Looping is of no use.
In fact, I thinkgpiochip_removeshould return void. Drivers cannot do anything if it failed, anyway.
Here the module is going to free device structure. Ignoring the error looks even more dangerous to me. I need to see if scheduling while unloading a module is allowed.
[...]
Quoted 8 lines
Quoted 8 lines
Index:
v4l-dvb.git/drivers/media/video/bt8xx/bttv.h
===================================================================drivers/media/video/bt8xx/bttv.h +0
-0@@ -249,6 +249,8 @@void (*audio_mode_gpio)(struct bttv *btv,struct v4l2_tuner *tuner,int set);
Quoted 6 lines
drivers/media/video/bt8xx/bttv.h line 253
Quoted 3 lines
drivers/media/video/bt8xx/bttv.h +2
-0void (*muxsel_hook)(struct bttv *btv,unsigned int input); + + unsigned int has_gpiolib:1;:1 style bitfields generate ugly code and they make no sense most of the time. better use "bool".
This is copied stuff from bt8xxgpio and dvb-bt8xx ;)
Thanks,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 12 lines · Domenico Andreoli
Quoted 11 lines · Michael Buesch
drivers/media/video/bt8xx/bttv-gpiolib.c line 125
Quoted 5 lines
drivers/media/video/bt8xx/bttv-gpiolib.c new file +5
-0+ +while ((ret =gpiochip_remove (&device->chip)) !=0 ) { +printk (KERN_INFO "error unregistering chip %s: %d\n" , device->chip.label, ret); +schedule (); + }This loop is dangerous.
Simply try to unregister it and exit with an error message if it failed. Looping is of no use.
In fact, I thinkgpiochip_removeshould return void. Drivers cannot do anything if it failed, anyway.
gpiolib will inc the use count of the bttvgpio module when a gpio is requested, so the module shouldn't unload while a gpio is requested. The only reason gpiochip_remove() should fail is if a gpio is still requested. So it shouldn't happen that gpiochip_remove will fail when the module is unloaded. Unless the module unload is forced, but in that case the right thing to do is unload and let the gpiochip_remove() fail instead of hanging.
There is an issue if the bt8x8 device is hot-unplugged while it's in use. I doubt the bttv driver handles this very well.
Quoted 3 lines
Here the module is going to free device structure. Ignoring the error looks even more dangerous to me. I need to see if scheduling while unloading a module is allowed.
I'm pretty sure it is allowed. The problem is that unless something un-requests the gpio in use, the module unload will hang in the kernel for ever. If someone does rmmod -f, don't turn it into a un-killable rmmod -w. If they wanted rmmod -w, they would have done it.
Quoted 11 lines
Quoted 11 lines
drivers/media/video/bt8xx/bttv.h line 253
Quoted 8 lines
drivers/media/video/bt8xx/bttv.h +2
-0@@ -249,6 +249,8 @@void (*audio_mode_gpio)(struct bttv *btv,struct v4l2_tuner *tuner,int set);void (*muxsel_hook)(struct bttv *btv,unsigned int input); + + unsigned int has_gpiolib:1;:1 style bitfields generate ugly code and they make no sense most of the time. better use "bool".
David is wrong here. The bttv driver has a huge card database and it shouldn't be bloated. These fields are typically used only once when the card loads are the code generated is perfectly ok. But, you must put the new field with the other bit fields to avoid increasing the size of the structure. You should measure the module size with objdump before and after and see how much bloat you added.
If someone mods their card to use the gpio lines the driver still won't drive it unless they change it in the card database. It would be nicer if there was a module option to export the gpios. That could also be useful for figuring out how the gpios are used for things like sound routing, mute, radio mode, and so on. I've been using the v4l2 debug register interface for this, but the sysfs would be another way.
BTW David, V4L2 has an ioctl() that allows one to set the bttv gpios even if the driver is using them, and it's never caused any complaints. I fail see what is different about modifying the same gpios through sysfs that makes it so dangerous it can't be allowed.
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · David Brownell
Quoted 7 lines · Domenico Andreoli
Something respecting these conditions clearly conflicts with bttv and looks like your bt8xxgpio driver. Indeed I do not see the need of dropping it.
Currently my patch requires the bttv functionality, since it is done to work toghether with bttv. Surely one must be able to use bttv only, without gpiolib.
Just an idea ... I tend to agree that Michael's GPIO-only scenario is atypical for these chips.
I am facing another atipical scenario (I am writing the initialization of bttv gpiolib sub-driver).
Say that a given card has 4 relays attached to its GPIOs. Only the card's driver may know about these relays and only the driver should be able to set those GPIO directions. Moreover, driver should prevent direction change for them, which are intended for output only.
While I figure out how to prevent user direction tampering, how about making gpiolib know which are the initial directions?
cheers,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · David Brownell
Quoted 7 lines · Domenico Andreoli
Say that a given card has 4 relays attached to its GPIOs. Only the card's driver may know about these relays and only the driver should be able to set those GPIO directions. Moreover, driver should prevent direction change for them, which are intended for output only.
While I figure out how to prevent user direction tampering, how about making gpiolib know which are the initial directions?
I don't quite follow. If you export the GPIOs to userspace using the (pending, to-appear-in-2.6.27) sysfs interface, you can use
I already tried it, very nifty. I used it to show my driver is working, I could hear the relays and see the tester.. yuk!
Anyway I was too free to play with GPIOs, I would like to be able to set limits at driver level. i.e. relay GPIOs should be output only and no one, being another driver or user-space, should be able to change their direction.
While I am able to enforce direction policy, I am not able to setup the initial direction. Indeed it is set by gpiolib basing on the presence of chip.direction_input.
cheers,
Domenico
This patch adds the gpiolib bttv sub-driver.
Every bttv card may define a set of available gpio ports which are registered to gpiolib and then made available through kernel's generic gpio interface.
--
State of the patch: working. RFC.
drivers/media/video/bt8xx/Kconfig +6 -0
@@ -21,6 +21,12 @@ To compile this driver as a module, choose M here: the module will be called bttv. +config VIDEO_BT848_GPIO + tristate "GPIO support for bt848 cards" + depends on VIDEO_BT848 && GPIOLIB + ---help--- + This enables GPIOLIB bindings on BT848 boards. + config VIDEO_BT848_DVB bool "DVB/ATSC Support for bt878 based TV cards" depends on VIDEO_BT848 && DVB_CORE
drivers/media/video/bt8xx/Makefile +1 -0
@@ -7,6 +7,7 @@ bttv-input.o bttv-audio-hook.o obj-$(CONFIG_VIDEO_BT848) += bttv.o +obj-$(CONFIG_VIDEO_BT848_GPIO) += bttv-gpiolib.o EXTRA_CFLAGS += -Idrivers/media/video EXTRA_CFLAGS += -Idrivers/media/common/tuners
drivers/media/video/bt8xx/bttv-cards.c +1 -0
@@ -2162,6 +2162,7 @@ .gpiomask = 0xdf, .muxsel = { 2 }, .pll = PLL_28, + .has_gpiolib = 1, }, [BTTV_BOARD_XGUARD] = { .name = "Grand X-Guard / Trust 814PCI",
drivers/media/video/bt8xx/bttv-driver.c +6 -0
@@ -4455,6 +4455,12 @@request_modules (btv); } +#ifdef CONFIG_VIDEO_BT848_GPIO_MODULE +/* add gpiolib subdevice */ +if (bttv_tvcards[btv->c.type].has_gpiolib) +bttv_sub_add_device (&btv->c,"gpiolib" ); +#endif +bttv_input_init (btv);/* everything is fine */
drivers/media/video/bt8xx/bttv-gpiolib.c new file +235 -0
235 lines not on this page · raw message
drivers/media/video/bt8xx/bttv.h +2 -0
@@ -249,6 +249,8 @@void (*audio_mode_gpio)(struct bttv *btv,struct v4l2_tuner *tuner,int set);void (*muxsel_hook)(struct bttv *btv,unsigned int input); + +bool has_gpiolib; };extern struct tvcard bttv_tvcards[];
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 10 lines · Michael Buesch
Quoted 9 lines · On Monday 14 July 2008 09:27:33 Domenico Andreoli wrote:
(quoted diff) +9
-0+static u32 nr_to_mask(struct bttv_gpiolib_device *dev, unsigned nr) +{ + u32 io_mask = dev->in_mask | dev->out_mask; + int shift = 0; + + while(io_mask && nr) { + nr -= io_mask & 1; + io_mask >>= 1; + shift++;
Quoted 8 lines
drivers/gpio/bt8xxgpio.c line 112
Quoted 4 lines
drivers/gpio/bt8xxgpio.c new file +4
-0+ } + +return 1 << shift; +}This loop is really really weird.
What the hell are you doing here?
You ususally convert GPIO numbers to masks by doing (1 << nr), only.
gpiolib does not allow holes in the number space of gpios. once you set chip.ngpio, you get a contiguous slice.
should the board have some of its gpio connected to something private, they are not to be exported to gpiolib and to the user.
indeed once, as a user, I know to have a board which has n inputs and m output and z in/out, that's all, I do not want to know how many GPIOs actually are on the board and how are connected.
nr_to_mask() hides those holes to the user, it maps a pin iff it is available for gpiolib fiddling.
thanks,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · David Brownell
Quoted 2 lines
gpiolib does not allow holes in the number space of gpios. once you set chip.ngpio, you get a contiguous slice.
Right ... the hardware has N gpios, that's what a
gpio_chippackages.
Quoted 7 lines
Quoted 2 lines
should the board have some of its gpio connected to something private, they are not to be exported to gpiolib and to the user.
If some of those are connected to something "private", the driver that's using them should just request those GPIOs. That will keep them from being requested by anything else. Don't try to create a second mechanism duplicating the request/free allocation scheme.
hmm.. I had completely missed the point of your previous message. so there is no space for hybrids like mine, gpiolib is an all-or-nothing solution.
why a tvcard driver should use generic gpio to wrap its own registers which already knows very well? it does not make sense to me. as I intended it, to export available gpios (= not used for its inner workings) to a generic layer, it really gives me some advantages. i.e. I could user sysfs to immediately test things.
I am not criticizing the gpiolib design, it's only different from what I initially imagined and does not completely fit my needs. probably I should give up with gpiolib and use V4L2's controls, surely losing any IRQ ability.
all I want from the tvcard's driver is "here is the video device, the dvb device, the xyz device and there are also some GPIOs for your pleasure". this does not sound like "there are 8 GPIOs but you can't use 3, 4 and 5". as a user, I do no care of GPIOs I cannot use.
I do not see gpiolib fit this at all without any glue.
thanks,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 31 lines · Michael Buesch
Quoted 30 lines · On Monday 14 July 2008 17:25:50 Domenico Andreoli wrote:
Quoted 10 lines · Michael Buesch
Quoted 9 lines · On Monday 14 July 2008 09:27:33 Domenico Andreoli wrote:
(quoted diff) +9
-0+static u32 nr_to_mask(struct bttv_gpiolib_device *dev, unsigned nr) +{ + u32 io_mask = dev->in_mask | dev->out_mask; + int shift = 0; + + while(io_mask && nr) { + nr -= io_mask & 1; + io_mask >>= 1; + shift++;Quoted 8 lines
drivers/gpio/bt8xxgpio.c line 112
Quoted 4 lines
drivers/gpio/bt8xxgpio.c new file +4
-0+ } + +return 1 << shift; +}This loop is really really weird.
What the hell are you doing here?
You ususally convert GPIO numbers to masks by doing (1 << nr), only.gpiolib does not allow holes in the number space of gpios. once you set chip.ngpio, you get a contiguous slice.
should the board have some of its gpio connected to something private, they are not to be exported to gpiolib and to the user.
indeed once, as a user, I know to have a board which has n inputs and m output and z in/out, that's all, I do not want to know how many GPIOs actually are on the board and how are connected.
Quoted 5 lines
Quoted 2 lines
nr_to_mask()hides those holes to the user, it maps a pin iff it is available for gpiolib fiddling.Ok, I see. However, I'd suggest to implement this with a lookup table rather than this weird loop.
yes, of course. it was never meant to be definitive, all the patch was a prototype. thanks for the support.
cheers,
Domenico
-----[ Domenico Andreoli, aka cavok
--[ http://www.dandreoli.com/gpgkey.asc
---[ 3A0F 2F80 F79C 678A 8936 4FEE 0677 9033 A20E BC50
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list