diff mbox series

i2c: cbus-gpio: Switch to use GPIO descriptors

Message ID 20190205125204.10843-1-linus.walleij@linaro.org (mailing list archive)
State New, archived
Headers show
Series i2c: cbus-gpio: Switch to use GPIO descriptors | expand

Commit Message

Linus Walleij Feb. 5, 2019, 12:52 p.m. UTC
This augments the CBUS GPIO I2C driver to use GPIO
descriptors for clock, sel and data. We drop the platform
data that was only used for carrying GPIO numbers and
use machine descriptor tables instead.

Cc: linux-omap@vger.kernel.org
Cc: Tony Lindgren <tony@atomide.com>
Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This is only compile tested, I hope someone can test it
on the n770, it seems like Aaro is still using it!
Tony, if you're happy can you ACK this so Wolfram can
apply it?
---
 arch/arm/mach-omap1/board-nokia770.c        | 17 ++---
 drivers/i2c/busses/i2c-cbus-gpio.c          | 80 ++++++++-------------
 include/linux/platform_data/i2c-cbus-gpio.h | 27 -------
 3 files changed, 39 insertions(+), 85 deletions(-)
 delete mode 100644 include/linux/platform_data/i2c-cbus-gpio.h

Comments

Tony Lindgren Feb. 5, 2019, 4:52 p.m. UTC | #1
* Linus Walleij <linus.walleij@linaro.org> [190205 12:52]:
> This augments the CBUS GPIO I2C driver to use GPIO
> descriptors for clock, sel and data. We drop the platform
> data that was only used for carrying GPIO numbers and
> use machine descriptor tables instead.
> 
> Cc: linux-omap@vger.kernel.org
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is only compile tested, I hope someone can test it
> on the n770, it seems like Aaro is still using it!
> Tony, if you're happy can you ACK this so Wolfram can
> apply it?

Looks like I can no longer test on 770, I'm just getting
error "Timeout waiting for flashing commands" on my PC.
And the n800 in my rack no longer boots at all.

So best to wait for Aaro to test these on 770 and n8x0 as
otherwise these devices won't stay running because the
watchdog is always enabled and is on the i2c.

The changes look nice for me though :)

Regards,

Tony
Aaro Koskinen Feb. 5, 2019, 9:23 p.m. UTC | #2
Hi,

On Tue, Feb 05, 2019 at 01:52:04PM +0100, Linus Walleij wrote:
> This augments the CBUS GPIO I2C driver to use GPIO
> descriptors for clock, sel and data. We drop the platform
> data that was only used for carrying GPIO numbers and
> use machine descriptor tables instead.
> 
> Cc: linux-omap@vger.kernel.org
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is only compile tested, I hope someone can test it
> on the n770, it seems like Aaro is still using it!

Unfortunately it doesn't work, so the device shuts down...

> +static struct gpiod_lookup_table nokia770_cbus_gpio_table = {
> +        .table = {
> +                GPIO_LOOKUP("mpuio", 9, "clk", 0),
> +                GPIO_LOOKUP("mpuio", 10, "dat", 0),
> +                GPIO_LOOKUP("mpuio", 11, "sel", 0),
> +		{ },
> +	},
>  };

Should there be dev_id? I tried adding "i2c-cbus-gpio.2" but it still
didn't work...

Minor note:
> +	gpiod_set_consumer_name(chost->dat, "CBUS data");

Maybe use "CBUS dat" to be consistent with other pins.

A.
Aaro Koskinen Feb. 5, 2019, 9:31 p.m. UTC | #3
Hi,

On Tue, Feb 05, 2019 at 08:52:47AM -0800, Tony Lindgren wrote:
> Looks like I can no longer test on 770, I'm just getting
> error "Timeout waiting for flashing commands" on my PC.
> And the n800 in my rack no longer boots at all.

Is this still the same 0xFFFF issue with USB 3.0 that you had
earlier? With 0xFFFF you need to use latest git HEAD and go back to
libusb 0.1.

> So best to wait for Aaro to test these on 770 and n8x0 as
> otherwise these devices won't stay running because the
> watchdog is always enabled and is on the i2c.

Yeah, CBUS and Retu are critical. Should we add these drivers under the
OMAP1 in MAINTAINERS? It's much nicer to test patches beforehand than
spend time on bisecting...

A.
Tony Lindgren Feb. 5, 2019, 10:20 p.m. UTC | #4
* Aaro Koskinen <aaro.koskinen@iki.fi> [190205 21:31]:
> Hi,
> 
> On Tue, Feb 05, 2019 at 08:52:47AM -0800, Tony Lindgren wrote:
> > Looks like I can no longer test on 770, I'm just getting
> > error "Timeout waiting for flashing commands" on my PC.
> > And the n800 in my rack no longer boots at all.
> 
> Is this still the same 0xFFFF issue with USB 3.0 that you had
> earlier? With 0xFFFF you need to use latest git HEAD and go back to
> libusb 0.1.

OK thanks I'll check.

> > So best to wait for Aaro to test these on 770 and n8x0 as
> > otherwise these devices won't stay running because the
> > watchdog is always enabled and is on the i2c.
> 
> Yeah, CBUS and Retu are critical. Should we add these drivers under the
> OMAP1 in MAINTAINERS? It's much nicer to test patches beforehand than
> spend time on bisecting...

Good idea.

Regards,

Tony
Aaro Koskinen Feb. 5, 2019, 10:38 p.m. UTC | #5
Hi,

On Tue, Feb 05, 2019 at 11:23:23PM +0200, Aaro Koskinen wrote:
> On Tue, Feb 05, 2019 at 01:52:04PM +0100, Linus Walleij wrote:
> > This augments the CBUS GPIO I2C driver to use GPIO
> > descriptors for clock, sel and data. We drop the platform
> > data that was only used for carrying GPIO numbers and
> > use machine descriptor tables instead.
> > 
> > Cc: linux-omap@vger.kernel.org
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > This is only compile tested, I hope someone can test it
> > on the n770, it seems like Aaro is still using it!
> 
> Unfortunately it doesn't work, so the device shuts down...

The patch seems to work fine on N810 (device tree boot). So the 770
issue must be related to lookup table setup somehow..

A.
Linus Walleij Feb. 6, 2019, 7:06 a.m. UTC | #6
On Tue, Feb 5, 2019 at 10:23 PM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> On Tue, Feb 05, 2019 at 01:52:04PM +0100, Linus Walleij wrote:

> Unfortunately it doesn't work, so the device shuts down...
>
> > +static struct gpiod_lookup_table nokia770_cbus_gpio_table = {
> > +        .table = {
> > +                GPIO_LOOKUP("mpuio", 9, "clk", 0),
> > +                GPIO_LOOKUP("mpuio", 10, "dat", 0),
> > +                GPIO_LOOKUP("mpuio", 11, "sel", 0),
> > +             { },
> > +     },
> >  };
>
> Should there be dev_id? I tried adding "i2c-cbus-gpio.2" but it still
> didn't work...

Yeah definately. That is one of the bugs, so I added it.

I also notice that it looks up by index so I'll fix that for a v2
you can test.

> Minor note:
> > +     gpiod_set_consumer_name(chost->dat, "CBUS data");
>
> Maybe use "CBUS dat" to be consistent with other pins.

OK just copied what was already in there. I'll fix.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/arch/arm/mach-omap1/board-nokia770.c b/arch/arm/mach-omap1/board-nokia770.c
index eb41db78cd47..d32b8ed6b3f1 100644
--- a/arch/arm/mach-omap1/board-nokia770.c
+++ b/arch/arm/mach-omap1/board-nokia770.c
@@ -10,6 +10,7 @@ 
 #include <linux/clkdev.h>
 #include <linux/irq.h>
 #include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
@@ -25,7 +26,6 @@ 
 #include <linux/platform_data/keypad-omap.h>
 #include <linux/platform_data/lcd-mipid.h>
 #include <linux/platform_data/gpio-omap.h>
-#include <linux/platform_data/i2c-cbus-gpio.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -217,18 +217,18 @@  static inline void nokia770_mmc_init(void)
 #endif
 
 #if IS_ENABLED(CONFIG_I2C_CBUS_GPIO)
-static struct i2c_cbus_platform_data nokia770_cbus_data = {
-	.clk_gpio = OMAP_MPUIO(9),
-	.dat_gpio = OMAP_MPUIO(10),
-	.sel_gpio = OMAP_MPUIO(11),
+static struct gpiod_lookup_table nokia770_cbus_gpio_table = {
+        .table = {
+                GPIO_LOOKUP("mpuio", 9, "clk", 0),
+                GPIO_LOOKUP("mpuio", 10, "dat", 0),
+                GPIO_LOOKUP("mpuio", 11, "sel", 0),
+		{ },
+	},
 };
 
 static struct platform_device nokia770_cbus_device = {
 	.name   = "i2c-cbus-gpio",
 	.id     = 2,
-	.dev    = {
-		.platform_data = &nokia770_cbus_data,
-	},
 };
 
 static struct i2c_board_info nokia770_i2c_board_info_2[] __initdata = {
@@ -257,6 +257,7 @@  static void __init nokia770_cbus_init(void)
 	nokia770_i2c_board_info_2[1].irq = gpio_to_irq(tahvo_irq_gpio);
 	i2c_register_board_info(2, nokia770_i2c_board_info_2,
 				ARRAY_SIZE(nokia770_i2c_board_info_2));
+	gpiod_add_lookup_table(&nokia770_cbus_gpio_table);
 	platform_device_register(&nokia770_cbus_device);
 }
 #else /* CONFIG_I2C_CBUS_GPIO */
diff --git a/drivers/i2c/busses/i2c-cbus-gpio.c b/drivers/i2c/busses/i2c-cbus-gpio.c
index b4f91e48948a..3c18d11df87a 100644
--- a/drivers/i2c/busses/i2c-cbus-gpio.c
+++ b/drivers/i2c/busses/i2c-cbus-gpio.c
@@ -18,16 +18,14 @@ 
 
 #include <linux/io.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/platform_data/i2c-cbus-gpio.h>
 
 /*
  * Bit counts are derived from Nokia implementation. These should be checked
@@ -39,9 +37,9 @@ 
 struct cbus_host {
 	spinlock_t	lock;		/* host lock */
 	struct device	*dev;
-	int		clk_gpio;
-	int		dat_gpio;
-	int		sel_gpio;
+	struct gpio_desc *clk;
+	struct gpio_desc *dat;
+	struct gpio_desc *sel;
 };
 
 /**
@@ -51,9 +49,9 @@  struct cbus_host {
  */
 static void cbus_send_bit(struct cbus_host *host, unsigned bit)
 {
-	gpio_set_value(host->dat_gpio, bit ? 1 : 0);
-	gpio_set_value(host->clk_gpio, 1);
-	gpio_set_value(host->clk_gpio, 0);
+	gpiod_set_value(host->dat, bit ? 1 : 0);
+	gpiod_set_value(host->clk, 1);
+	gpiod_set_value(host->clk, 0);
 }
 
 /**
@@ -78,9 +76,9 @@  static int cbus_receive_bit(struct cbus_host *host)
 {
 	int ret;
 
-	gpio_set_value(host->clk_gpio, 1);
-	ret = gpio_get_value(host->dat_gpio);
-	gpio_set_value(host->clk_gpio, 0);
+	gpiod_set_value(host->clk, 1);
+	ret = gpiod_get_value(host->dat);
+	gpiod_set_value(host->clk, 0);
 	return ret;
 }
 
@@ -123,10 +121,10 @@  static int cbus_transfer(struct cbus_host *host, char rw, unsigned dev,
 	spin_lock_irqsave(&host->lock, flags);
 
 	/* Reset state and start of transfer, SEL stays down during transfer */
-	gpio_set_value(host->sel_gpio, 0);
+	gpiod_set_value(host->sel, 0);
 
 	/* Set the DAT pin to output */
-	gpio_direction_output(host->dat_gpio, 1);
+	gpiod_direction_output(host->dat, 1);
 
 	/* Send the device address */
 	cbus_send_data(host, dev, CBUS_ADDR_BITS);
@@ -141,12 +139,12 @@  static int cbus_transfer(struct cbus_host *host, char rw, unsigned dev,
 		cbus_send_data(host, data, 16);
 		ret = 0;
 	} else {
-		ret = gpio_direction_input(host->dat_gpio);
+		ret = gpiod_direction_input(host->dat);
 		if (ret) {
 			dev_dbg(host->dev, "failed setting direction\n");
 			goto out;
 		}
-		gpio_set_value(host->clk_gpio, 1);
+		gpiod_set_value(host->clk, 1);
 
 		ret = cbus_receive_word(host);
 		if (ret < 0) {
@@ -156,9 +154,9 @@  static int cbus_transfer(struct cbus_host *host, char rw, unsigned dev,
 	}
 
 	/* Indicate end of transfer, SEL goes up until next transfer */
-	gpio_set_value(host->sel_gpio, 1);
-	gpio_set_value(host->clk_gpio, 1);
-	gpio_set_value(host->clk_gpio, 0);
+	gpiod_set_value(host->sel, 1);
+	gpiod_set_value(host->clk, 1);
+	gpiod_set_value(host->clk, 0);
 
 out:
 	spin_unlock_irqrestore(&host->lock, flags);
@@ -214,7 +212,6 @@  static int cbus_i2c_probe(struct platform_device *pdev)
 {
 	struct i2c_adapter *adapter;
 	struct cbus_host *chost;
-	int ret;
 
 	adapter = devm_kzalloc(&pdev->dev, sizeof(struct i2c_adapter),
 			       GFP_KERNEL);
@@ -225,22 +222,20 @@  static int cbus_i2c_probe(struct platform_device *pdev)
 	if (!chost)
 		return -ENOMEM;
 
-	if (pdev->dev.of_node) {
-		struct device_node *dnode = pdev->dev.of_node;
-		if (of_gpio_count(dnode) != 3)
-			return -ENODEV;
-		chost->clk_gpio = of_get_gpio(dnode, 0);
-		chost->dat_gpio = of_get_gpio(dnode, 1);
-		chost->sel_gpio = of_get_gpio(dnode, 2);
-	} else if (dev_get_platdata(&pdev->dev)) {
-		struct i2c_cbus_platform_data *pdata =
-			dev_get_platdata(&pdev->dev);
-		chost->clk_gpio = pdata->clk_gpio;
-		chost->dat_gpio = pdata->dat_gpio;
-		chost->sel_gpio = pdata->sel_gpio;
-	} else {
+	if (gpiod_count(&pdev->dev, NULL) != 3)
 		return -ENODEV;
-	}
+	chost->clk = devm_gpiod_get_index(&pdev->dev, NULL, 0, GPIOD_OUT_LOW);
+	if (IS_ERR(chost->clk))
+		return PTR_ERR(chost->clk);
+	chost->dat = devm_gpiod_get_index(&pdev->dev, NULL, 1, GPIOD_IN);
+	if (IS_ERR(chost->dat))
+		return PTR_ERR(chost->dat);
+	chost->sel = devm_gpiod_get_index(&pdev->dev, NULL, 2, GPIOD_OUT_HIGH);
+	if (IS_ERR(chost->sel))
+		return PTR_ERR(chost->sel);
+	gpiod_set_consumer_name(chost->clk, "CBUS clk");
+	gpiod_set_consumer_name(chost->dat, "CBUS data");
+	gpiod_set_consumer_name(chost->sel, "CBUS sel");
 
 	adapter->owner		= THIS_MODULE;
 	adapter->class		= I2C_CLASS_HWMON;
@@ -254,21 +249,6 @@  static int cbus_i2c_probe(struct platform_device *pdev)
 	spin_lock_init(&chost->lock);
 	chost->dev = &pdev->dev;
 
-	ret = devm_gpio_request_one(&pdev->dev, chost->clk_gpio,
-				    GPIOF_OUT_INIT_LOW, "CBUS clk");
-	if (ret)
-		return ret;
-
-	ret = devm_gpio_request_one(&pdev->dev, chost->dat_gpio, GPIOF_IN,
-				    "CBUS data");
-	if (ret)
-		return ret;
-
-	ret = devm_gpio_request_one(&pdev->dev, chost->sel_gpio,
-				    GPIOF_OUT_INIT_HIGH, "CBUS sel");
-	if (ret)
-		return ret;
-
 	i2c_set_adapdata(adapter, chost);
 	platform_set_drvdata(pdev, adapter);
 
diff --git a/include/linux/platform_data/i2c-cbus-gpio.h b/include/linux/platform_data/i2c-cbus-gpio.h
deleted file mode 100644
index 6faa992a9502..000000000000
--- a/include/linux/platform_data/i2c-cbus-gpio.h
+++ /dev/null
@@ -1,27 +0,0 @@ 
-/*
- * i2c-cbus-gpio.h - CBUS I2C platform_data definition
- *
- * Copyright (C) 2004-2009 Nokia Corporation
- *
- * Written by Felipe Balbi and Aaro Koskinen.
- *
- * This file is subject to the terms and conditions of the GNU General
- * Public License. See the file "COPYING" in the main directory of this
- * archive for more details.
- *
- * 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.
- */
-
-#ifndef __INCLUDE_LINUX_I2C_CBUS_GPIO_H
-#define __INCLUDE_LINUX_I2C_CBUS_GPIO_H
-
-struct i2c_cbus_platform_data {
-	int dat_gpio;
-	int clk_gpio;
-	int sel_gpio;
-};
-
-#endif /* __INCLUDE_LINUX_I2C_CBUS_GPIO_H */