gpio-ucb1400

gpio-ucb1400

[Jean Delvare]

Hi all,

In September 2009, a driver for the GPIO function of the UCB1400 chip
was added to the kernel tree. The probe function of this driver requires
ucbdata to be set. The only place where this happens is in function
ucb1400_gpio_set_data(). This function was never call, and still isn't.
So this is dead code for 3.5 years as far as the upstream kernel is
concerned.

To make things worse, this driver can't be built as a module, for no
good reason that I can see.

Marek, can you explain what was the point of submitting this driver that
nobody can use?

I would like either this driver to be fixed so that it can be used (and
that would IMHO start with dropping the ugly ucb1400_gpio_set_data hook
and global variable ucbdata), or this driver to be dropped from the
kernel tree. If the driver is kept, it should be adjusted so that it can
be built as a module.

If I overlooked something, please let me know.

Thanks,
-- 
Jean Delvare
Suse L3

Re: gpio-ucb1400

[Guenter Roeck]

On Fri, Mar 29, 2013 at 08:46:39PM +0100, Jean Delvare wrote:
> Hi all,
> 
> In September 2009, a driver for the GPIO function of the UCB1400 chip
> was added to the kernel tree. The probe function of this driver requires
> ucbdata to be set. The only place where this happens is in function
> ucb1400_gpio_set_data(). This function was never call, and still isn't.
> So this is dead code for 3.5 years as far as the upstream kernel is
> concerned.
> 
> To make things worse, this driver can't be built as a module, for no
> good reason that I can see.
> 
> Marek, can you explain what was the point of submitting this driver that
> nobody can use?
> 
> I would like either this driver to be fixed so that it can be used (and
> that would IMHO start with dropping the ugly ucb1400_gpio_set_data hook
> and global variable ucbdata), or this driver to be dropped from the
> kernel tree. If the driver is kept, it should be adjusted so that it can
> be built as a module.
> 
> If I overlooked something, please let me know.
> 
Interestingly, the author made an attempt to fix that with [1]. It looks like
the rest of that series was merged, but this patch wasn't, though I don't find
any information about the reason.

Guenter

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/028656.html

Re: gpio-ucb1400

[Marek Vasut]

Dear Guenter Roeck,

> On Fri, Mar 29, 2013 at 08:46:39PM +0100, Jean Delvare wrote:
> > Hi all,
> > 
> > In September 2009, a driver for the GPIO function of the UCB1400 chip
> > was added to the kernel tree. The probe function of this driver requires
> > ucbdata to be set. The only place where this happens is in function
> > ucb1400_gpio_set_data(). This function was never call, and still isn't.
> > So this is dead code for 3.5 years as far as the upstream kernel is
> > concerned.
> > 
> > To make things worse, this driver can't be built as a module, for no
> > good reason that I can see.
> > 
> > Marek, can you explain what was the point of submitting this driver that
> > nobody can use?
> > 
> > I would like either this driver to be fixed so that it can be used (and
> > that would IMHO start with dropping the ugly ucb1400_gpio_set_data hook
> > and global variable ucbdata), or this driver to be dropped from the
> > kernel tree. If the driver is kept, it should be adjusted so that it can
> > be built as a module.
> > 
> > If I overlooked something, please let me know.
> 
> Interestingly, the author made an attempt to fix that with [1]. It looks
> like the rest of that series was merged, but this patch wasn't, though I
> don't find any information about the reason.

It's been a while. Guenter, thanks for finding that link, but I suspect the 
patch is heavily obsolete by now.

Best regards,
Marek Vasut

Re: gpio-ucb1400

[Guenter Roeck]

On Sat, Mar 30, 2013 at 08:20:44PM +0100, Marek Vasut wrote:
> Dear Guenter Roeck,
> 
> > On Fri, Mar 29, 2013 at 08:46:39PM +0100, Jean Delvare wrote:
> > > Hi all,
> > > 
> > > In September 2009, a driver for the GPIO function of the UCB1400 chip
> > > was added to the kernel tree. The probe function of this driver requires
> > > ucbdata to be set. The only place where this happens is in function
> > > ucb1400_gpio_set_data(). This function was never call, and still isn't.
> > > So this is dead code for 3.5 years as far as the upstream kernel is
> > > concerned.
> > > 
> > > To make things worse, this driver can't be built as a module, for no
> > > good reason that I can see.
> > > 
> > > Marek, can you explain what was the point of submitting this driver that
> > > nobody can use?
> > > 
> > > I would like either this driver to be fixed so that it can be used (and
> > > that would IMHO start with dropping the ugly ucb1400_gpio_set_data hook
> > > and global variable ucbdata), or this driver to be dropped from the
> > > kernel tree. If the driver is kept, it should be adjusted so that it can
> > > be built as a module.
> > > 
> > > If I overlooked something, please let me know.
> > 
> > Interestingly, the author made an attempt to fix that with [1]. It looks
> > like the rest of that series was merged, but this patch wasn't, though I
> > don't find any information about the reason.
> 
> It's been a while. Guenter, thanks for finding that link, but I suspect the 
> patch is heavily obsolete by now.
> 
Oh, it most definitely is, starting with the gpio driver name. Just wonder
why it was never applied, and why no one seems to have noticed or cared.

Jean is absolutely right - it should get fixed, or the driver should be dropped
if no one is using it anyway.

Guenter

Re: gpio-ucb1400

[Marek Vasut]

Dear Guenter Roeck,

> On Sat, Mar 30, 2013 at 08:20:44PM +0100, Marek Vasut wrote:
> > Dear Guenter Roeck,
> > 
> > > On Fri, Mar 29, 2013 at 08:46:39PM +0100, Jean Delvare wrote:
> > > > Hi all,
> > > > 
> > > > In September 2009, a driver for the GPIO function of the UCB1400 chip
> > > > was added to the kernel tree. The probe function of this driver
> > > > requires ucbdata to be set. The only place where this happens is in
> > > > function ucb1400_gpio_set_data(). This function was never call, and
> > > > still isn't. So this is dead code for 3.5 years as far as the
> > > > upstream kernel is concerned.
> > > > 
> > > > To make things worse, this driver can't be built as a module, for no
> > > > good reason that I can see.
> > > > 
> > > > Marek, can you explain what was the point of submitting this driver
> > > > that nobody can use?
> > > > 
> > > > I would like either this driver to be fixed so that it can be used
> > > > (and that would IMHO start with dropping the ugly
> > > > ucb1400_gpio_set_data hook and global variable ucbdata), or this
> > > > driver to be dropped from the kernel tree. If the driver is kept, it
> > > > should be adjusted so that it can be built as a module.
> > > > 
> > > > If I overlooked something, please let me know.
> > > 
> > > Interestingly, the author made an attempt to fix that with [1]. It
> > > looks like the rest of that series was merged, but this patch wasn't,
> > > though I don't find any information about the reason.
> > 
> > It's been a while. Guenter, thanks for finding that link, but I suspect
> > the patch is heavily obsolete by now.
> 
> Oh, it most definitely is, starting with the gpio driver name. Just wonder
> why it was never applied, and why no one seems to have noticed or cared.
> 
> Jean is absolutely right - it should get fixed, or the driver should be
> dropped if no one is using it anyway.

I think ARM/palmtc was using this.

Best regards,
Marek Vasut

Re: gpio-ucb1400

[Jean Delvare]

Le dimanche 31 mars 2013 à 19:19 +0200, Marek Vasut a écrit :
> Dear Guenter Roeck,
> > Jean is absolutely right - it should get fixed, or the driver should be
> > dropped if no one is using it anyway.
> 
> I think ARM/palmtc was using this.

My point was, the upstream gpio-ucb1400 driver in its current form is
unusable (on top of being ugly.) I have no doubt that someone used that
driver successfully in some external kernel tree, but the code that was
merged upstream is incomplete and no good.

-- 
Jean Delvare
Suse L3

Re: gpio-ucb1400

[Mark Brown]

On Mon, Apr 01, 2013 at 01:06:43PM +0200, Jean Delvare wrote:

> My point was, the upstream gpio-ucb1400 driver in its current form is
> unusable (on top of being ugly.) I have no doubt that someone used that
> driver successfully in some external kernel tree, but the code that was
> merged upstream is incomplete and no good.

What the driver is doing is, unfortunately, the best practice for an
AC'97 connected device like this - we've got an uncomfortable mix of
an enumerable bus and platform data combined with a subsystem which has
never had enough love to work in a nice way with the kernel.  The idea
is that any boards which have the device will call the _set_data()
function in their board-specific code.

Sadly we're a little short on volunteers to clean up the AC'97
subsystem so this situation shows little sign of improving.

Re: gpio-ucb1400

[Jean Delvare]

Le lundi 01 avril 2013 à 16:32 +0100, Mark Brown a écrit :
> On Mon, Apr 01, 2013 at 01:06:43PM +0200, Jean Delvare wrote:
> 
> > My point was, the upstream gpio-ucb1400 driver in its current form is
> > unusable (on top of being ugly.) I have no doubt that someone used that
> > driver successfully in some external kernel tree, but the code that was
> > merged upstream is incomplete and no good.
> 
> What the driver is doing is, unfortunately, the best practice for an
> AC'97 connected device like this - we've got an uncomfortable mix of
> an enumerable bus and platform data combined with a subsystem which has
> never had enough love to work in a nice way with the kernel.  The idea
> is that any boards which have the device will call the _set_data()
> function in their board-specific code.

OK but where are these users? I can't see any in the upstream kernel.

> Sadly we're a little short on volunteers to clean up the AC'97
> subsystem so this situation shows little sign of improving.

Well Marek proposed a cleanup which looked good [1], several times
apparently, but it was never applied.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/028656.html

-- 
Jean Delvare
Suse L3

Re: gpio-ucb1400

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On Tue, Apr 02, 2013 at 09:22:37AM +0200, Jean Delvare wrote:
> Le lundi 01 avril 2013 à 16:32 +0100, Mark Brown a écrit :

> > What the driver is doing is, unfortunately, the best practice for an
> > AC'97 connected device like this - we've got an uncomfortable mix of
> > an enumerable bus and platform data combined with a subsystem which has
> > never had enough love to work in a nice way with the kernel.  The idea
> > is that any boards which have the device will call the _set_data()
> > function in their board-specific code.

> OK but where are these users? I can't see any in the upstream kernel.

They'll be out of tree I guess, it's fairly common (especially for the
hacky bits like this) for things to struggle to make it to mainline.

> > Sadly we're a little short on volunteers to clean up the AC'97
> > subsystem so this situation shows little sign of improving.

> Well Marek proposed a cleanup which looked good [1], several times
> apparently, but it was never applied.

That's not really doing much for the whole AC'97 problem although it
does help a bit with that particular driver providing the AC'97
controller driver it's used with has had support added for passing
platform data through.  Don't see any reason not to apply it though,
you're probably just looking at a busy/missing maintainer problem there.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: gpio-ucb1400

[Linus Walleij]

On Tue, Apr 2, 2013 at 9:22 AM, Jean Delvare <jdelvare@suse.de> wrote:

> Well Marek proposed a cleanup which looked good [1], several times
> apparently, but it was never applied.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/028656.html

Marek, could you rebase and repost this and set Samuel, me & Grant on
the To: line so we can ACK this and have it merged into Sam's tree then?

Yours,
Linus Walleij

[PATCH v2] UCB1400: Pass ucb1400-gpio data through ac97 bus

[Marek Vasut]

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-kernel <linux-kernel@vger.kernel.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/gpio/gpio-ucb1400.c |   19 ++++++-------------
 drivers/mfd/ucb1400_core.c  |    5 +++++
 include/linux/ucb1400.h     |   18 ++++++------------
 3 files changed, 17 insertions(+), 25 deletions(-)

v2: Rebase patch from:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/028656.html

NOTE: I didn't even compile-test this, but the fix was plenty straightforward.

diff --git a/drivers/gpio/gpio-ucb1400.c b/drivers/gpio/gpio-ucb1400.c
index 26405ef..6d0feb2 100644
--- a/drivers/gpio/gpio-ucb1400.c
+++ b/drivers/gpio/gpio-ucb1400.c
@@ -12,8 +12,6 @@
 #include <linux/module.h>
 #include <linux/ucb1400.h>
 
-struct ucb1400_gpio_data *ucbdata;
-
 static int ucb1400_gpio_dir_in(struct gpio_chip *gc, unsigned off)
 {
 	struct ucb1400_gpio *gpio;
@@ -50,7 +48,7 @@ static int ucb1400_gpio_probe(struct platform_device *dev)
 	struct ucb1400_gpio *ucb = dev->dev.platform_data;
 	int err = 0;
 
-	if (!(ucbdata && ucbdata->gpio_offset)) {
+	if (!(ucb && ucb->gpio_offset)) {
 		err = -EINVAL;
 		goto err;
 	}
@@ -58,7 +56,7 @@ static int ucb1400_gpio_probe(struct platform_device *dev)
 	platform_set_drvdata(dev, ucb);
 
 	ucb->gc.label = "ucb1400_gpio";
-	ucb->gc.base = ucbdata->gpio_offset;
+	ucb->gc.base = ucb->gpio_offset;
 	ucb->gc.ngpio = 10;
 	ucb->gc.owner = THIS_MODULE;
 
@@ -72,8 +70,8 @@ static int ucb1400_gpio_probe(struct platform_device *dev)
 	if (err)
 		goto err;
 
-	if (ucbdata && ucbdata->gpio_setup)
-		err = ucbdata->gpio_setup(&dev->dev, ucb->gc.ngpio);
+	if (ucb && ucb->gpio_setup)
+		err = ucb->gpio_setup(&dev->dev, ucb->gc.ngpio);
 
 err:
 	return err;
@@ -85,8 +83,8 @@ static int ucb1400_gpio_remove(struct platform_device *dev)
 	int err = 0;
 	struct ucb1400_gpio *ucb = platform_get_drvdata(dev);
 
-	if (ucbdata && ucbdata->gpio_teardown) {
-		err = ucbdata->gpio_teardown(&dev->dev, ucb->gc.ngpio);
+	if (ucb && ucb->gpio_teardown) {
+		err = ucb->gpio_teardown(&dev->dev, ucb->gc.ngpio);
 		if (err)
 			return err;
 	}
@@ -103,11 +101,6 @@ static struct platform_driver ucb1400_gpio_driver = {
 	},
 };
 
-void __init ucb1400_gpio_set_data(struct ucb1400_gpio_data *data)
-{
-	ucbdata = data;
-}
-
 module_platform_driver(ucb1400_gpio_driver);
 
 MODULE_DESCRIPTION("Philips UCB1400 GPIO driver");
diff --git a/drivers/mfd/ucb1400_core.c b/drivers/mfd/ucb1400_core.c
index daf6952..e9031fa 100644
--- a/drivers/mfd/ucb1400_core.c
+++ b/drivers/mfd/ucb1400_core.c
@@ -75,6 +75,11 @@ static int ucb1400_core_probe(struct device *dev)
 
 	/* GPIO */
 	ucb_gpio.ac97 = ac97;
+	if (pdata) {
+		ucb_gpio.gpio_setup = pdata->gpio_setup;
+		ucb_gpio.gpio_teardown = pdata->gpio_teardown;
+		ucb_gpio.gpio_offset = pdata->gpio_offset;
+	}
 	ucb->ucb1400_gpio = platform_device_alloc("ucb1400_gpio", -1);
 	if (!ucb->ucb1400_gpio) {
 		err = -ENOMEM;
diff --git a/include/linux/ucb1400.h b/include/linux/ucb1400.h
index d21b33c..2e9ee4d 100644
--- a/include/linux/ucb1400.h
+++ b/include/linux/ucb1400.h
@@ -83,15 +83,12 @@
 #define UCB_ID			0x7e
 #define UCB_ID_1400             0x4304
 
-struct ucb1400_gpio_data {
-	int gpio_offset;
-	int (*gpio_setup)(struct device *dev, int ngpio);
-	int (*gpio_teardown)(struct device *dev, int ngpio);
-};
-
 struct ucb1400_gpio {
 	struct gpio_chip	gc;
 	struct snd_ac97		*ac97;
+	int			gpio_offset;
+	int			(*gpio_setup)(struct device *dev, int ngpio);
+	int			(*gpio_teardown)(struct device *dev, int ngpio);
 };
 
 struct ucb1400_ts {
@@ -110,6 +107,9 @@ struct ucb1400 {
 
 struct ucb1400_pdata {
 	int	irq;
+	int	gpio_offset;
+	int	(*gpio_setup)(struct device *dev, int ngpio);
+	int	(*gpio_teardown)(struct device *dev, int ngpio);
 };
 
 static inline u16 ucb1400_reg_read(struct snd_ac97 *ac97, u16 reg)
@@ -162,10 +162,4 @@ static inline void ucb1400_adc_disable(struct snd_ac97 *ac97)
 unsigned int ucb1400_adc_read(struct snd_ac97 *ac97, u16 adc_channel,
 			      int adcsync);
 
-#ifdef CONFIG_GPIO_UCB1400
-void __init ucb1400_gpio_set_data(struct ucb1400_gpio_data *data);
-#else
-static inline void ucb1400_gpio_set_data(struct ucb1400_gpio_data *data) {}
-#endif
-
 #endif
-- 
1.7.10.4

Re: [PATCH v2] UCB1400: Pass ucb1400-gpio data through ac97 bus

[Marek Vasut]

Dear Marek Vasut,

> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/gpio/gpio-ucb1400.c |   19 ++++++-------------
>  drivers/mfd/ucb1400_core.c  |    5 +++++
>  include/linux/ucb1400.h     |   18 ++++++------------
>  3 files changed, 17 insertions(+), 25 deletions(-)
> 
> v2: Rebase patch from:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-October/028656.h
> tml
> 
> NOTE: I didn't even compile-test this, but the fix was plenty
> straightforward.

But damn, this code is ugly. I'm retrospectively-ashamed.

Best regards,
Marek Vasut

Re: [PATCH v2] UCB1400: Pass ucb1400-gpio data through ac97 bus

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Sun, Apr 14, 2013 at 08:35:48PM +0200, Marek Vasut wrote:
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

it may be a bit ugly but it's still an improvement :)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] UCB1400: Pass ucb1400-gpio data through ac97 bus

[Linus Walleij]

On Sun, Apr 14, 2013 at 8:35 PM, Marek Vasut <marex@denx.de> wrote:

> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Marek Vasut <marex@denx.de>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Sam, please pick it up if you're also OK with this.

Yours,
Linus Walleij

Re: [PATCH v2] UCB1400: Pass ucb1400-gpio data through ac97 bus

[Samuel Ortiz]

On Sun, Apr 14, 2013 at 08:35:48PM +0200, Marek Vasut wrote:
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/gpio/gpio-ucb1400.c |   19 ++++++-------------
>  drivers/mfd/ucb1400_core.c  |    5 +++++
>  include/linux/ucb1400.h     |   18 ++++++------------
>  3 files changed, 17 insertions(+), 25 deletions(-)
Applied, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/