| Message ID | 1406618686-22385-5-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 29, 2014 at 9:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote: > The pca953x has a negated reset input. This patch adds a DT binding for > the reset gpio and resets the chip when it is probed. This will reset > the device and leave the gpio in the correct state so reset is not > triggered. > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> Why on earth should this be in the GPIO driver? The driver should be in drivers/reset/reset-gpio.c and you should provide a separate driver for it. As it happens, Houcheng Lin has already proposed such a driver: http://marc.info/?l=linux-kernel&m=140309916607115&w=2 Please coordinate with Houcheng and use his driver for what you want to achieve. Yours, Linus Walleij
Am Freitag, den 08.08.2014, 15:14 +0200 schrieb Linus Walleij: > On Tue, Jul 29, 2014 at 9:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote: > > > The pca953x has a negated reset input. This patch adds a DT binding for > > the reset gpio and resets the chip when it is probed. This will reset > > the device and leave the gpio in the correct state so reset is not > > triggered. > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Why on earth should this be in the GPIO driver? > > The driver should be in drivers/reset/reset-gpio.c and you > should provide a separate driver for it. I still think we should keep using the reset-gpios binding for simple cases like this; I see no reason to add a separate device to the device tree for a single GPIO. > As it happens, Houcheng Lin has already proposed such a > driver: > http://marc.info/?l=linux-kernel&m=140309916607115&w=2 That is a different issue, as there the device does not appear on the bus until the reset is released. Here the I2C device will be probed from the device tree, so the reset can be released or triggered from the probe function. regards Philipp
On Fri, Aug 8, 2014 at 4:11 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Freitag, den 08.08.2014, 15:14 +0200 schrieb Linus Walleij: >> On Tue, Jul 29, 2014 at 9:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote: >> >> > The pca953x has a negated reset input. This patch adds a DT binding for >> > the reset gpio and resets the chip when it is probed. This will reset >> > the device and leave the gpio in the correct state so reset is not >> > triggered. >> > >> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> >> >> Why on earth should this be in the GPIO driver? >> >> The driver should be in drivers/reset/reset-gpio.c and you >> should provide a separate driver for it. > > I still think we should keep using the reset-gpios binding for simple > cases like this; I see no reason to add a separate device to the device > tree for a single GPIO. In any case you have to propose something generic that happens in drivers/gpio/gpiolib.c at the end of the gpiochip_add() function, because this will likely appear in many other systems. The binding should also be generic in Documentation/devicetree/bindings/gpio/gpio.txt not for just this driver. >> As it happens, Houcheng Lin has already proposed such a >> driver: >> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 > > That is a different issue, as there the device does not appear on the > bus until the reset is released. You're just looking at the patch description. Look at what the driver does. I wrote this reply to the patch: http://marc.info/?l=linux-kernel&m=140480593524472&w=2 With a delay of zero, the reset will be released immediately, by the use of a helper OF node and this driver from the reset subsystem. It's nice, generic code that solves a generic problem of deasserting GPIO lines for some reset. Yours, Linus Walleij
Hi Linus, Am Montag, den 11.08.2014, 10:43 +0200 schrieb Linus Walleij: > On Fri, Aug 8, 2014 at 4:11 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Am Freitag, den 08.08.2014, 15:14 +0200 schrieb Linus Walleij: > >> On Tue, Jul 29, 2014 at 9:24 AM, Markus Pargmann <mpa@pengutronix.de> wrote: > >> > >> > The pca953x has a negated reset input. This patch adds a DT binding for > >> > the reset gpio and resets the chip when it is probed. This will reset > >> > the device and leave the gpio in the correct state so reset is not > >> > triggered. > >> > > >> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > >> > >> Why on earth should this be in the GPIO driver? > >> > >> The driver should be in drivers/reset/reset-gpio.c and you > >> should provide a separate driver for it. > > > > I still think we should keep using the reset-gpios binding for simple > > cases like this; I see no reason to add a separate device to the device > > tree for a single GPIO. > > In any case you have to propose something generic that > happens in drivers/gpio/gpiolib.c at the end of the > gpiochip_add() function, because this will likely appear in > many other systems. In general, I'd like to keep drivers in control over how and when the reset is asserted; mainly because there are variations of reset, powerdown/enable, and bootstrap pins that have to be taken into account. On some devices the powerdown needs to be deasserted before the reset can work, on some devices it might be the other way around, or the powerdown pin may cause a reset itself, or there are bootstrap pins that need to be configured a certain way while the reset is issued (but are used for something else while the device is active). Others occasionally need to reset the chip while in operation. If you expect none of those issues for GPIO chips, I don't argue against doing this for the drivers in gpiochip_add, if it is documented that this function might reset the chip. This would work with a separate gpio reset provider driver by just calling device_reset(chip->dev) at the end of gpiochip_add. With my previous suggestion, as Maxime points out, I'd still need to find a way to provide the duration of the reset pulse. > The binding should also be generic in > Documentation/devicetree/bindings/gpio/gpio.txt > not for just this driver. Ideally it should be the same as all other devices with an external reset pin. > >> As it happens, Houcheng Lin has already proposed such a > >> driver: > >> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 > > > > That is a different issue, as there the device does not appear on the > > bus until the reset is released. > > You're just looking at the patch description. Look at what the > driver does. > > I wrote this reply to the patch: > http://marc.info/?l=linux-kernel&m=140480593524472&w=2 > > With a delay of zero, the reset will be released > immediately, by the use of a helper OF node and this driver > from the reset subsystem. It's nice, generic code that solves > a generic problem of deasserting GPIO lines for > some reset. Yes, it does what you want. But its purpose is different, it's a bit indirect for the use case at hand, and we'll still find cases where this won't work (interaction with other pins). As to whether the sub-node might conflict with existing bindings, I don't know. This is something that should be taken to devicetree discussion list. regards Philipp
On Thu, Aug 14, 2014 at 11:21 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Montag, den 11.08.2014, 10:43 +0200 schrieb Linus Walleij: >> On Fri, Aug 8, 2014 at 4:11 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > Am Freitag, den 08.08.2014, 15:14 +0200 schrieb Linus Walleij: >> >> As it happens, Houcheng Lin has already proposed such a >> >> driver: >> >> http://marc.info/?l=linux-kernel&m=140309916607115&w=2 >> > >> > That is a different issue, as there the device does not appear on the >> > bus until the reset is released. >> >> You're just looking at the patch description. Look at what the >> driver does. >> >> I wrote this reply to the patch: >> http://marc.info/?l=linux-kernel&m=140480593524472&w=2 >> >> With a delay of zero, the reset will be released >> immediately, by the use of a helper OF node and this driver >> from the reset subsystem. It's nice, generic code that solves >> a generic problem of deasserting GPIO lines for >> some reset. > > Yes, it does what you want. But its purpose is different, it's a bit > indirect for the use case at hand, I think a generic driver is always best, define what you mean by "indirect" in this context. > and we'll still find cases where this > won't work (interaction with other pins). But that is not what you're trying to solve right now. > As to whether the sub-node > might conflict with existing bindings, I don't know. This is something > that should be taken to devicetree discussion list. Um? I don't get this. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt index eb65157d47f6..57e31414f74d 100644 --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt @@ -27,6 +27,10 @@ Required properties: ti,tca6424 exar,xra1202 +Optional properties: + - reset-gpios: phandle with arguments identifying the reset gpio. See + Documentation/devicetree/bindings/gpio/gpio.txt for more information + Example: @@ -37,4 +41,5 @@ Example: pinctrl-0 = <&pinctrl_pca9505>; interrupt-parent = <&gpio3>; interrupts = <23 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&gpio3 24 GPIO_ACTIVE_LOW>; }; diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index df5eb6e6be1e..053d8b4702e6 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -11,6 +11,7 @@ * the Free Software Foundation; version 2 of the License. */ +#include <linux/delay.h> #include <linux/module.h> #include <linux/init.h> #include <linux/gpio.h> @@ -660,8 +661,25 @@ static int pca953x_probe(struct i2c_client *client, invert = pdata->invert; chip->names = pdata->names; } else { + struct gpio_desc *reset; + chip->gpio_start = -1; irq_base = 0; + + reset = devm_gpiod_get(&client->dev, "reset"); + if (IS_ERR(reset)) { + if (PTR_ERR(reset) == -EPROBE_DEFER) + return -EPROBE_DEFER; + else + dev_info(&client->dev, "Did not find/get a gpio for reset (%ld)\n", + PTR_ERR(reset)); + } else { + /* Reset the chip if the reset is wired */ + gpiod_direction_output(reset, 0); + udelay(100); + gpiod_set_value(reset, 1); + udelay(100); + } } chip->client = client;
The pca953x has a negated reset input. This patch adds a DT binding for the reset gpio and resets the chip when it is probed. This will reset the device and leave the gpio in the correct state so reset is not triggered. Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- .../devicetree/bindings/gpio/gpio-pca953x.txt | 5 +++++ drivers/gpio/gpio-pca953x.c | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+)