Re: [PATCH v3] Add bt8xxgpio driver

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

Hi,

On Thu, Jul 10, 2008 at 04:02:58PM -0300, Mauro Carvalho Chehab wrote:
> 
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Sun, Jul 13, 2008 at 12:15:11PM +0200, Michael Buesch wrote:
> On Sunday 13 July 2008 02:42:48 Domenico Andreoli wrote:
> > On Thu, Jul 10, 2008 at 04:02:58PM -0300, Mauro Carvalho Chehab wrote:
> > > 
> > > 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.
> 
> 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.

> 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

Index: v4l-dvb.git/drivers/media/video/bt8xx/Kconfig
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/Kconfig	2008-07-13 15:52:08.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/Kconfig	2008-07-13 17:11:02.000000000 +0200
@@ -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
Index: v4l-dvb.git/drivers/media/video/bt8xx/Makefile
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/Makefile	2008-07-13 15:52:08.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/Makefile	2008-07-13 17:11:27.000000000 +0200
@@ -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
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-cards.c
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv-cards.c	2008-07-13 15:52:08.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-cards.c	2008-07-13 16:53:59.000000000 +0200
@@ -2162,6 +2162,7 @@
 		.gpiomask       = 0xdf,
 		.muxsel         = { 2 },
 		.pll            = PLL_28,
+		.has_gpiolib    = 1,
 	},
 	[BTTV_BOARD_XGUARD] = {
 		.name           = "Grand X-Guard / Trust 814PCI",
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-driver.c
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv-driver.c	2008-07-13 15:52:08.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-driver.c	2008-07-13 15:52:15.000000000 +0200
@@ -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 */
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-gpiolib.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-gpiolib.c	2008-07-13 17:23:56.000000000 +0200
@@ -0,0 +1,153 @@
+/*
+ * Bt8xx based GPIOLIB driver
+ *
+ * Copyright (C) 2008 Domenico Andreoli <cavok@dandreoli.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+
+#include <asm/gpio.h>
+
+#include "bttvp.h"
+
+static struct bttv_gpiolib_card {
+	unsigned int has_gpiolib:1;
+	u16 ngpio;
+	int gpiobase;
+} bttv_gpiolib_cards[] = {
+	[BTTV_BOARD_IVC200] = {
+		.has_gpiolib    = 1,
+		.ngpio          = 24,
+		.gpiobase       = -1,
+	},
+};
+
+struct bttv_gpiolib_device {
+	struct bttv_sub_device *sub;
+	struct gpio_chip chip;
+};
+
+#define gpio_to_btv(gpio)   container_of(\
+                              container_of(gpio, struct bttv_gpiolib_device, chip)->sub->core, \
+                                struct bttv, c);
+
+static int bttv_gpiolib_direction_input(struct gpio_chip *gpio, unsigned nr)
+{
+	struct bttv *btv = gpio_to_btv(gpio);
+
+	// TODO
+
+	return 0;
+}
+
+static int bttv_gpiolib_get(struct gpio_chip *gpio, unsigned nr)
+{
+	struct bttv *btv = gpio_to_btv(gpio);
+
+	// TODO
+
+	return 0;
+}
+
+static int bttv_gpiolib_direction_output(struct gpio_chip *gpio, unsigned nr, int val)
+{
+	struct bttv *btv = gpio_to_btv(gpio);
+
+	// TODO
+
+	return 0;
+}
+
+static void bttv_gpiolib_set(struct gpio_chip *gpio, unsigned nr, int val)
+{
+	struct bttv *btv = gpio_to_btv(gpio);
+
+	// TODO
+}
+
+static int __devinit bttv_gpiolib_probe(struct bttv_sub_device *sub)
+{
+	int ret;
+	struct bttv_gpiolib_device *device;
+
+	if(!(device = kzalloc(sizeof(struct bttv_gpiolib_device), GFP_KERNEL)))
+		return -ENOMEM;
+
+	device->sub = sub;
+	device->chip.label = sub->dev.bus_id;
+	device->chip.owner = THIS_MODULE;
+
+	device->chip.ngpio = bttv_gpiolib_cards[sub->core->type].ngpio;
+	device->chip.base = bttv_gpiolib_cards[sub->core->type].gpiobase;
+
+	device->chip.direction_input = bttv_gpiolib_direction_input;
+	device->chip.direction_output = bttv_gpiolib_direction_output;
+	device->chip.get = bttv_gpiolib_get;
+	device->chip.set = bttv_gpiolib_set;
+
+	device->chip.dbg_show = NULL;
+	device->chip.can_sleep = 0;
+
+	ret = gpiochip_add(&device->chip);
+	if(ret) {
+		kfree(device);
+		return ret;
+	}
+
+	dev_set_drvdata(&sub->dev, device);
+	return 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);
+		schedule();
+	}
+
+	kfree(device);
+}
+
+static struct bttv_sub_driver driver = {
+	.drv = {
+		.name		= "bttv-gpiolib",
+	},
+	.probe		= bttv_gpiolib_probe,
+	.remove		= bttv_gpiolib_remove,
+};
+
+static int __init bttv_gpiolib_init(void)
+{
+	return bttv_sub_register(&driver, "gpiolib");
+}
+
+static void __exit bttv_gpiolib_exit(void)
+{
+	bttv_sub_unregister(&driver);
+}
+
+module_init(bttv_gpiolib_init);
+module_exit(bttv_gpiolib_exit);
+
+MODULE_DESCRIPTION("Bt8xx based GPIOLIB driver");
+MODULE_AUTHOR("Domenico Andreoli <cavok@dandreoli.com>");
+MODULE_LICENSE("GPL");
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv.h
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv.h	2008-07-13 15:52:08.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv.h	2008-07-13 16:53:42.000000000 +0200
@@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Sun, Jul 13, 2008 at 06:08:35PM +0200, Michael Buesch wrote:
> On Sunday 13 July 2008 17:43:33 Domenico Andreoli wrote:
> > 
> > Index: v4l-dvb.git/drivers/media/video/bt8xx/Makefile
> > ===================================================================
> > --- v4l-dvb.git.orig/drivers/media/video/bt8xx/Makefile	2008-07-13 15:52:08.000000000 +0200
> > +++ v4l-dvb.git/drivers/media/video/bt8xx/Makefile	2008-07-13 17:11:27.000000000 +0200
> > @@ -6,7 +6,11 @@
> >  		       bttv-risc.o bttv-vbi.o bttv-i2c.o bttv-gpio.o \
> >  		       bttv-input.o bttv-audio-hook.o

[...]

> > +
> > +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);
> > +		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 think gpiochip_remove should 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.

[...]

> >
> > Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv.h
> > ===================================================================
> > --- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv.h	2008-07-13 15:52:08.000000000 +0200
> > +++ v4l-dvb.git/drivers/media/video/bt8xx/bttv.h	2008-07-13 16:53:42.000000000 +0200
> > @@ -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".

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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Sun, Jul 13, 2008 at 01:00:34PM -0700, David Brownell wrote:
> On Sunday 13 July 2008, Domenico Andreoli wrote:
> > 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Sun, Jul 13, 2008 at 10:59:54PM -0700, David Brownell wrote:
> On Sunday 13 July 2008, Domenico Andreoli wrote:
> > 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.

Index: v4l-dvb.git/drivers/media/video/bt8xx/Kconfig
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/Kconfig	2008-07-14 06:33:54.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/Kconfig	2008-07-14 06:33:58.000000000 +0200
@@ -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
Index: v4l-dvb.git/drivers/media/video/bt8xx/Makefile
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/Makefile	2008-07-14 06:33:54.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/Makefile	2008-07-14 06:33:58.000000000 +0200
@@ -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
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-cards.c
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv-cards.c	2008-07-14 06:33:54.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-cards.c	2008-07-14 06:33:58.000000000 +0200
@@ -2162,6 +2162,7 @@
 		.gpiomask       = 0xdf,
 		.muxsel         = { 2 },
 		.pll            = PLL_28,
+		.has_gpiolib    = 1,
 	},
 	[BTTV_BOARD_XGUARD] = {
 		.name           = "Grand X-Guard / Trust 814PCI",
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-driver.c
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv-driver.c	2008-07-14 06:33:55.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-driver.c	2008-07-14 06:33:58.000000000 +0200
@@ -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 */
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv-gpiolib.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv-gpiolib.c	2008-07-14 08:32:01.000000000 +0200
@@ -0,0 +1,235 @@
+/*
+ * Bt8xx based GPIOLIB driver
+ *
+ * Copyright (C) 2008 Domenico Andreoli <cavok@dandreoli.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+
+#include <asm/gpio.h>
+
+#include "bttvp.h"
+
+struct bttv_gpiolib_device {
+	struct bttv_sub_device *sub;
+	struct gpio_chip chip;
+
+	u32 in_mask;
+	u32 out_mask;
+	u32 out;
+};
+
+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++;
+	}
+
+	return 1 << shift;
+}
+
+static void bttv_gpiolib_inout(struct bttv_core *core, u32 mask, u32 outbits, u32 outvals)
+{
+	struct bttv *btv = container_of(core, struct bttv, c);
+	unsigned long flags;
+	u32 data;
+
+	spin_lock_irqsave(&btv->gpio_lock, flags);
+
+	/* set direction */
+	data = btread(BT848_GPIO_OUT_EN);
+	data &= ~mask;
+	data |= mask & outbits;
+	btwrite(data, BT848_GPIO_OUT_EN);
+
+	mask &= outbits;
+	if(mask) {
+		/* set output values */
+		data = btread(BT848_GPIO_DATA);
+		data &= ~mask;
+		data |= mask & outvals;
+		btwrite(data, BT848_GPIO_DATA);
+	}
+
+	spin_unlock_irqrestore(&btv->gpio_lock, flags);
+}
+
+static int bttv_gpiolib_direction_input(struct gpio_chip *gpio, unsigned nr)
+{
+	struct bttv_gpiolib_device *dev = container_of(gpio, struct bttv_gpiolib_device, chip);
+	u32 mask = nr_to_mask(dev, nr);
+
+	if(!(mask & dev->in_mask))
+		return -EPERM;
+
+	bttv_gpiolib_inout(dev->sub->core, mask, 0, 0);
+	return 0;
+}
+
+static int bttv_gpiolib_direction_output(struct gpio_chip *gpio, unsigned nr, int val)
+{
+	struct bttv_gpiolib_device *dev = container_of(gpio, struct bttv_gpiolib_device, chip);
+	u32 mask = nr_to_mask(dev, nr);
+
+	if(!(mask & dev->out_mask))
+		return -EPERM;
+
+	bttv_gpiolib_inout(dev->sub->core, mask, mask, val ? mask : 0);
+	return 0;
+}
+
+static int bttv_gpiolib_get(struct gpio_chip *gpio, unsigned nr)
+{
+	u32 data;
+	unsigned long flags;
+	struct bttv_gpiolib_device *dev = container_of(gpio, struct bttv_gpiolib_device, chip);
+	struct bttv *btv = container_of(dev->sub->core, struct bttv, c);
+
+	spin_lock_irqsave(&btv->gpio_lock, flags);
+	data = btread(BT848_GPIO_DATA);
+	spin_unlock_irqrestore(&btv->gpio_lock, flags);
+
+	return !!(data & nr_to_mask(dev, nr));
+}
+
+static void bttv_gpiolib_set(struct gpio_chip *gpio, unsigned nr, int val)
+{
+	u32 data;
+	unsigned long flags;
+	struct bttv_gpiolib_device *dev = container_of(gpio, struct bttv_gpiolib_device, chip);
+	struct bttv *btv = container_of(dev->sub->core, struct bttv, c);
+	u32 mask = nr_to_mask(dev, nr);
+
+	spin_lock_irqsave(&btv->gpio_lock, flags);
+	data = btread(BT848_GPIO_DATA);
+	data &= ~mask;
+	data |= val ? mask : 0;
+	btwrite(data, BT848_GPIO_DATA);
+	spin_unlock_irqrestore(&btv->gpio_lock, flags);
+}
+
+static int bttv_gpiolib_setup(unsigned int type, struct bttv_gpiolib_device *dev)
+{
+	switch(type) {
+		case BTTV_BOARD_IVC200:
+			dev->in_mask  = 0x0f;
+			dev->out_mask = 0xff;
+			break;
+
+		default:
+			printk(KERN_ERR "card type %d: missing gpiolib configuration\n", type);
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int __devinit bttv_gpiolib_probe(struct bttv_sub_device *sub)
+{
+	struct bttv_gpiolib_device *dev;
+	u32 io_mask;
+	int ret;
+
+	dev = kzalloc(sizeof(struct bttv_gpiolib_device), GFP_KERNEL);
+	if(!dev)
+		return -ENOMEM;
+
+	ret = bttv_gpiolib_setup(sub->core->type, dev);
+	if(ret) {
+		kfree(dev);
+		return ret;
+	}
+
+	io_mask = dev->in_mask | dev->out_mask;
+	if(!io_mask) {
+		printk(KERN_ERR "card type %d: missing GPIOs configuration\n", sub->core->type);
+		kfree(dev);
+		return -EINVAL;
+	}
+
+	/* set direction and output values */
+	bttv_gpiolib_inout(sub->core, io_mask, dev->out_mask, dev->out);
+
+	while(io_mask) {
+		dev->chip.ngpio++;
+		io_mask >>= 1;
+	}
+
+	dev->sub = sub;
+	dev->chip.label = sub->dev.bus_id;
+	dev->chip.owner = THIS_MODULE;
+
+	dev->chip.base = -1;
+	dev->chip.direction_input = bttv_gpiolib_direction_input;
+	dev->chip.direction_output = bttv_gpiolib_direction_output;
+	dev->chip.get = bttv_gpiolib_get;
+	dev->chip.set = bttv_gpiolib_set;
+
+	ret = gpiochip_add(&dev->chip);
+	if(ret) {
+		printk(KERN_ERR "error adding gpio chip %s: %d\n", dev->chip.label, ret);
+		kfree(dev);
+		return ret;
+	}
+
+	dev_set_drvdata(&sub->dev, dev);
+	return 0;
+}
+
+static void __devexit bttv_gpiolib_remove(struct bttv_sub_device *sub)
+{
+	struct bttv_gpiolib_device *dev = dev_get_drvdata(&sub->dev);
+	int ret;
+
+	ret = gpiochip_remove(&dev->chip);
+	if(ret)
+		printk(KERN_ERR "error removing gpio chip %s: %d). leaking memory...\n", dev->chip.label, ret);
+	else
+		kfree(dev);
+}
+
+static struct bttv_sub_driver driver = {
+	.drv = {
+		.name		= "bttv-gpiolib",
+	},
+	.probe		= bttv_gpiolib_probe,
+	.remove		= bttv_gpiolib_remove,
+};
+
+static int __init bttv_gpiolib_init(void)
+{
+	return bttv_sub_register(&driver, "gpiolib");
+}
+
+static void __exit bttv_gpiolib_exit(void)
+{
+	bttv_sub_unregister(&driver);
+}
+
+module_init(bttv_gpiolib_init);
+module_exit(bttv_gpiolib_exit);
+
+MODULE_DESCRIPTION("Bt8xx based GPIOLIB driver");
+MODULE_AUTHOR("Domenico Andreoli <cavok@dandreoli.com>");
+MODULE_LICENSE("GPL");
Index: v4l-dvb.git/drivers/media/video/bt8xx/bttv.h
===================================================================
--- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv.h	2008-07-14 06:33:54.000000000 +0200
+++ v4l-dvb.git/drivers/media/video/bt8xx/bttv.h	2008-07-14 06:33:58.000000000 +0200
@@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Mon, Jul 14, 2008 at 03:58:29PM +0200, Michael Buesch wrote:
> On Monday 14 July 2008 09:27:33 Domenico Andreoli wrote:
> > +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++;
> > +	}
> > +
> > +	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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Mon, Jul 14, 2008 at 09:26:28AM -0700, David Brownell wrote:
> > 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_chip packages.
>  
> > 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Domenico Andreoli]

On Mon, Jul 14, 2008 at 07:51:39PM +0200, Michael Buesch wrote:
> On Monday 14 July 2008 17:25:50 Domenico Andreoli wrote:
> > On Mon, Jul 14, 2008 at 03:58:29PM +0200, Michael Buesch wrote:
> > > On Monday 14 July 2008 09:27:33 Domenico Andreoli wrote:
> > > > +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++;
> > > > +	}
> > > > +
> > > > +	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.
> 
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH v3] Add bt8xxgpio driver

[Trent Piepho]

On Sun, 13 Jul 2008, Domenico Andreoli wrote:
> On Sun, Jul 13, 2008 at 06:08:35PM +0200, Michael Buesch wrote:
> > > +
> > > +	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 think gpiochip_remove should 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.

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

> > > --- v4l-dvb.git.orig/drivers/media/video/bt8xx/bttv.h	2008-07-13 15:52:08.000000000 +0200
> > > +++ v4l-dvb.git/drivers/media/video/bt8xx/bttv.h	2008-07-13 16:53:42.000000000 +0200
> > > @@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list