[PATCH] watchdog: tegra: Stop watchdog first if restarting
If we need to restart the watchdog due to someone changing the timeout
interval, stop the watchdog before restarting it. Otherwise, the new
timeout doesn't seem to take.
Signed-off-by: Andrew Chew
---
drivers/watchdog/tegra_wdt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
index 7f97cdd..9ec5760 100644
--- a/drivers/watchdog/tegra_wdt.c
+++ b/drivers/watchdog/tegra_wdt.c
@@ -140,8 +140,10 @@ static int tegra_wdt_set_timeout(struct watchdog_device
*wdd,
{
wdd->timeout = timeout;
- if (watchdog_active(wdd))
+ if (watchdog_active(wdd)) {
+ tegra_wdt_stop(wdd);
return tegra_wdt_start(wdd);
+ }
return 0;
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] watchdog: tegra: Stop watchdog first if restarting
If we need to restart the watchdog due to someone changing the timeout
interval, stop the watchdog before restarting it. Otherwise, the new
timeout doesn't seem to take.
Signed-off-by: Andrew Chew <ac...@nvidia.com>
---
drivers/watchdog/tegra_wdt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
index 7f97cdd..9ec5760 100644
--- a/drivers/watchdog/tegra_wdt.c
+++ b/drivers/watchdog/tegra_wdt.c
@@ -140,8 +140,10 @@ static int tegra_wdt_set_timeout(struct watchdog_device
*wdd,
{
wdd->timeout = timeout;
- if (watchdog_active(wdd))
+ if (watchdog_active(wdd)) {
+ tegra_wdt_stop(wdd);
return tegra_wdt_start(wdd);
+ }
return 0;
}
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v7 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew Reviewed-by: Guenter Roeck Tested-by: Stephen Warren Reviewed-by: Stephen Warren --- Changes from v6: Removed kref stuff. We can't think of why it's needed here. Changes from v5: Changed license to GPL v2 per Stephen Warren's guidance. Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck 's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, "echo 0 > /dev/watchdog0". The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 302 + 4 files changed, 319 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..750e2a2 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,302 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT
[PATCH v7 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com Reviewed-by: Guenter Roeck li...@roeck-us.net Tested-by: Stephen Warren swar...@nvidia.com Reviewed-by: Stephen Warren swar...@nvidia.com --- Changes from v6: Removed kref stuff. We can't think of why it's needed here. Changes from v5: Changed license to GPL v2 per Stephen Warren's guidance. Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck li...@roeck-us.net's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, echo 0 /dev/watchdog0. The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 302 + 4 files changed, 319 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..750e2a2 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,302 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/watchdog.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT
[PATCH v6 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew Reviewed-by: Guenter Roeck Tested-by: Stephen Warren Reviewed-by: Stephen Warren --- Changes from v5: Changed license to GPL v2 per Stephen Warren's guidance. Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck 's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, "echo 0 > /dev/watchdog0". The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 327 + 4 files changed, 344 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..0df1d14 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,327 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures
RE: [PATCH v5 1/1] watchdog: Add tegra watchdog
> > +static void tegra_wdt_unref(struct watchdog_device *wdd) {
> > + struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> > +
> > + kref_put(>kref, tegra_wdt_release_resources); }
>
> I forget why these were needed; they seem to do nothing.
The reason I did the whole kref thing was by following the guidance
in Documentation/watchdog/watchdog-kernel-api.txt, which says
that if the watchdog_device struct is dynamically allocated, then
one needs this.
> > +MODULE_LICENSE("GPL");
>
> That should be "GPL v2" according to the license header in the file.
Done. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v5 1/1] watchdog: Add tegra watchdog
+static void tegra_wdt_unref(struct watchdog_device *wdd) {
+ struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+
+ kref_put(wdt-kref, tegra_wdt_release_resources); }
I forget why these were needed; they seem to do nothing.
The reason I did the whole kref thing was by following the guidance
in Documentation/watchdog/watchdog-kernel-api.txt, which says
that if the watchdog_device struct is dynamically allocated, then
one needs this.
+MODULE_LICENSE(GPL);
That should be GPL v2 according to the license header in the file.
Done. Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v6 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com Reviewed-by: Guenter Roeck li...@roeck-us.net Tested-by: Stephen Warren swar...@nvidia.com Reviewed-by: Stephen Warren swar...@nvidia.com --- Changes from v5: Changed license to GPL v2 per Stephen Warren's guidance. Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck li...@roeck-us.net's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, echo 0 /dev/watchdog0. The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 327 + 4 files changed, 344 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..0df1d14 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,327 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/watchdog.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can
RE: [PATCH v5 1/1] watchdog: Add tegra watchdog
> On 02/06/2014 05:54 PM, Andrew Chew wrote: > > Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 > > and later). This driver will configure one watchdog timer that will > > reset the system in the case of a watchdog timeout. > > > > This driver binds to the nvidia,tegra30-timer device node and gets its > > register base from there. > > > > Signed-off-by: Andrew Chew > > Reviewed-by: Guenter Roeck Thanks, Guenter. Pinging other potential reviewers. Any other comments for me, guys? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v5 1/1] watchdog: Add tegra watchdog
On 02/06/2014 05:54 PM, Andrew Chew wrote: Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com Reviewed-by: Guenter Roeck li...@roeck-us.net Thanks, Guenter. Pinging other potential reviewers. Any other comments for me, guys? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 1/1] watchdog: Add tegra watchdog
> On 02/06/2014 02:05 PM, Andrew Chew wrote:
> > Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30
> > and later). This driver will configure one watchdog timer that will
> > reset the system in the case of a watchdog timeout.
> >
> > This driver binds to the nvidia,tegra30-timer device node and gets its
> > register base from there.
> >
> > Signed-off-by: Andrew Chew
> > ---
>
> Hi Andrew,
>
> > +
> > + /* This is the timer base. */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(>dev, "incorrect resources\n");
> > + return -ENODEV;
> > + }
> > +
>
> I thought I mentioned that before - dem_iomap_resource() creates the very
> same error message and returns an error if NULL is passed as res argument
> to it. So the above error message (and error check) is really redundant.
Sorry about that. I guess I misunderstood. I'll send a new one out shortly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew --- Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck 's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, "echo 0 > /dev/watchdog0". The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 327 + 4 files changed, 344 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..4c39aad --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,327 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 + +/* + * Register base of the time
[PATCH v4 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew --- Changes from v3: Applied Cuenter Roeck 's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, "echo 0 > /dev/watchdog0". The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 332 + 4 files changed, 349 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..1e7bdea --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,332 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 + +/* + * Register base of the timer that's selected for pairing with the watchdog. + * This driver arbitrarily uses timer 5, which is currently unused by + * other drivers (in particular,
[PATCH v4 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com --- Changes from v3: Applied Cuenter Roeck li...@roeck-us.net's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, echo 0 /dev/watchdog0. The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 332 + 4 files changed, 349 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..1e7bdea --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,332 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/watchdog.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 + +/* + * Register base of the timer that's selected for pairing with the watchdog. + * This driver
RE: [PATCH v4 1/1] watchdog: Add tegra watchdog
On 02/06/2014 02:05 PM, Andrew Chew wrote:
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30
and later). This driver will configure one watchdog timer that will
reset the system in the case of a watchdog timeout.
This driver binds to the nvidia,tegra30-timer device node and gets its
register base from there.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Hi Andrew,
+
+ /* This is the timer base. */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(pdev-dev, incorrect resources\n);
+ return -ENODEV;
+ }
+
I thought I mentioned that before - dem_iomap_resource() creates the very
same error message and returns an error if NULL is passed as res argument
to it. So the above error message (and error check) is really redundant.
Sorry about that. I guess I misunderstood. I'll send a new one out shortly.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com --- Changes from v4: Skip the error check from the platform_get_resources() call, because devm_ioremap_resource() correctly handles the case when res is NULL. Changes from v3: Applied Cuenter Roeck li...@roeck-us.net's comments. In particular, since tegra_wdt_start() and tegra_wdt_stop() always return success, skip error checking for these calls. Also, removed some unnecessarily verbose logging around which watchdog and timer ID is being used. Fixed typo regarding the ref and unref callback setup. Removed miscdev stuff. Cuenter was right in that the watchdog doesn't need to be stopped prior to changing the timeout. It is sufficient to just reprogram the watchdog with the new timeout. Tested on T124. To do a simple test, echo 0 /dev/watchdog0. The target will reset when two minutes (the default heartbeat interval) have expired since the last write to /dev/watchdog0. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 327 + 4 files changed, 344 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..4c39aad --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,327 @@ +/* + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/watchdog.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE
[PATCH v3 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew --- Changes from V2: Applied all of Stephen Warren's comments. Modified suspend callback by only stopping the watchdog timer if it was currently active. Added some logging during suspend/resume. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 371 + 4 files changed, 388 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..1314475 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,371 @@ +/* + * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 + +/* + * Register base of the timer that's selected for pairing with the watchdog. + * This driver arbitrarily uses timer 5, which is currently unused by + * other drivers (in particular, the Tegra clocksource driver). If this + * needs to change, take care that the new timer is not used by the + * clocksource driver. + */ +#define WDT_TIMER_BASE 0x60 +#define WDT_TIMER_ID 5 + +/* WDT registers */ +#define WDT_CFG0x0 +#define WDT_CFG_PERIOD_SHIFT 4 +#define WDT_CFG_PERIOD_MASK0xff +#define WDT_CFG_INT_EN (1 << 12) +#define WDT_CFG_PMC2CAR_RST_EN (1 << 15) +#define WDT_STS
RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
> > +/* Tegra 20 timers */ > > +#define TEGRA20_TIMER1_BASE0x0 > > +#define TEGRA20_TIMER2_BASE0x8 > > +#define TEGRA20_TIMER3_BASE0x50 > > +#define TEGRA20_TIMER4_BASE0x58 > > + > > +/* Tegra 30 timers */ > > +#define TEGRA30_TIMER1_BASETEGRA20_TIMER1_BASE > > +#define TEGRA30_TIMER2_BASETEGRA20_TIMER2_BASE > > +#define TEGRA30_TIMER3_BASETEGRA20_TIMER3_BASE > > +#define TEGRA30_TIMER4_BASETEGRA20_TIMER4_BASE > > +#define TEGRA30_TIMER5_BASE0x60 > > +#define TEGRA30_TIMER6_BASE0x68 > > +#define TEGRA30_TIMER7_BASE0x70 > > +#define TEGRA30_TIMER8_BASE0x78 > > +#define TEGRA30_TIMER9_BASE0x80 > > +#define TEGRA30_TIMER0_BASE0x88 > > Why put the SoC name in the define names? Why not just have > TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch, > right?) and have the driver know that 1..4 are valid on Tegra20, and > 1..10 are valid on later chips. > > I guess if the defines are moved into a header file, adding a TEGRA_ prefix > does make sense. > > But I wonder if it wouldn't be simpler for the Tegra WDT driver to just call a > function on the Tegra clocksource driver to find out which timer > ID(s) to avoid using? Even simpler would be to just put a comment in the > WDT driver saying that timer 5 was chosen arbitrarily, but if it's changed > make > sure not to conflict with the clocksource driver (and an equivalent change to > the clocksource driver). Alright, I think I'll just go with putting a comment in the WDT driver then, so that We don't need to add this new header file. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
> On 02/05/2014 01:06 PM, Andrew Chew wrote: > >> On 02/03/2014 05:17 PM, Andrew Chew wrote: > >>> There are some differences between tegra20's timer registers and > >>> tegra30's (and later). For example, tegra30 has more timers. In > >>> addition, watchdogs are not present in tegra20. > >>> > >>> Add this compatibility string in order to be able to distinguish > >>> whether the additional timers and watchdogs are there or not. > >> > >>> diff --git a/drivers/clocksource/tegra20_timer.c > >>> b/drivers/clocksource/tegra20_timer.c > >> > >>> CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", > >>> tegra20_init_timer); > >>> +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer", > >>> +tegra20_init_timer); > >> > >> Thinking about this more, nothing in this driver actually cares about > >> Tegra20 vs. Tegra30+, since the timer that's used is present in all chips. > >> Hence, this patch isn't needed. > > > > Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver > > can match against it? It would be weird to have the tegra WDT driver > > bind against nvidia,tegra20-timer when the tegra WDT driver as it is > > won't work at all on tegra20. > > The DT files need to contain all of: > > * The specific SoC (this is already present) > * nvidia,tegra30-timer for SoCs >= Tegra30 (this is missing) > * nvidia,tegra20-timer for all SoCs (this is already present) > > However, since all DTs will contain nvidia,tegra20-timer, since all HW is > backwards-compatible IIUC, any code that only cares about the parts that > have existed since Tegra20 only need match against the Tegra20 compatible > value. Okay, I think I get what you're saying. So I'll drop this patch. The tegra WDT driver will still match against nvidia,tegra30-timer, and device trees for SoCs that expect to use this WDT driver will have to have nvidia,tegra30-timer. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
> On 02/03/2014 05:17 PM, Andrew Chew wrote: > > There are some differences between tegra20's timer registers and > > tegra30's (and later). For example, tegra30 has more timers. In > > addition, watchdogs are not present in tegra20. > > > > Add this compatibility string in order to be able to distinguish > > whether the additional timers and watchdogs are there or not. > > > diff --git a/drivers/clocksource/tegra20_timer.c > > b/drivers/clocksource/tegra20_timer.c > > > CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", > > tegra20_init_timer); > > +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer", > > +tegra20_init_timer); > > Thinking about this more, nothing in this driver actually cares about > Tegra20 vs. Tegra30+, since the timer that's used is present in all chips. > Hence, this patch isn't needed. Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver can match against it? It would be weird to have the tegra WDT driver bind against nvidia,tegra20-timer when the tegra WDT driver as it is won't work at all on tegra20. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
On 02/03/2014 05:17 PM, Andrew Chew wrote: There are some differences between tegra20's timer registers and tegra30's (and later). For example, tegra30 has more timers. In addition, watchdogs are not present in tegra20. Add this compatibility string in order to be able to distinguish whether the additional timers and watchdogs are there or not. diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c CLOCKSOURCE_OF_DECLARE(tegra20_timer, nvidia,tegra20-timer, tegra20_init_timer); +CLOCKSOURCE_OF_DECLARE(tegra30_timer, nvidia,tegra30-timer, +tegra20_init_timer); Thinking about this more, nothing in this driver actually cares about Tegra20 vs. Tegra30+, since the timer that's used is present in all chips. Hence, this patch isn't needed. Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver can match against it? It would be weird to have the tegra WDT driver bind against nvidia,tegra20-timer when the tegra WDT driver as it is won't work at all on tegra20. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
On 02/05/2014 01:06 PM, Andrew Chew wrote: On 02/03/2014 05:17 PM, Andrew Chew wrote: There are some differences between tegra20's timer registers and tegra30's (and later). For example, tegra30 has more timers. In addition, watchdogs are not present in tegra20. Add this compatibility string in order to be able to distinguish whether the additional timers and watchdogs are there or not. diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c CLOCKSOURCE_OF_DECLARE(tegra20_timer, nvidia,tegra20-timer, tegra20_init_timer); +CLOCKSOURCE_OF_DECLARE(tegra30_timer, nvidia,tegra30-timer, +tegra20_init_timer); Thinking about this more, nothing in this driver actually cares about Tegra20 vs. Tegra30+, since the timer that's used is present in all chips. Hence, this patch isn't needed. Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver can match against it? It would be weird to have the tegra WDT driver bind against nvidia,tegra20-timer when the tegra WDT driver as it is won't work at all on tegra20. The DT files need to contain all of: * The specific SoC (this is already present) * nvidia,tegra30-timer for SoCs = Tegra30 (this is missing) * nvidia,tegra20-timer for all SoCs (this is already present) However, since all DTs will contain nvidia,tegra20-timer, since all HW is backwards-compatible IIUC, any code that only cares about the parts that have existed since Tegra20 only need match against the Tegra20 compatible value. Okay, I think I get what you're saying. So I'll drop this patch. The tegra WDT driver will still match against nvidia,tegra30-timer, and device trees for SoCs that expect to use this WDT driver will have to have nvidia,tegra30-timer. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
+/* Tegra 20 timers */ +#define TEGRA20_TIMER1_BASE0x0 +#define TEGRA20_TIMER2_BASE0x8 +#define TEGRA20_TIMER3_BASE0x50 +#define TEGRA20_TIMER4_BASE0x58 + +/* Tegra 30 timers */ +#define TEGRA30_TIMER1_BASETEGRA20_TIMER1_BASE +#define TEGRA30_TIMER2_BASETEGRA20_TIMER2_BASE +#define TEGRA30_TIMER3_BASETEGRA20_TIMER3_BASE +#define TEGRA30_TIMER4_BASETEGRA20_TIMER4_BASE +#define TEGRA30_TIMER5_BASE0x60 +#define TEGRA30_TIMER6_BASE0x68 +#define TEGRA30_TIMER7_BASE0x70 +#define TEGRA30_TIMER8_BASE0x78 +#define TEGRA30_TIMER9_BASE0x80 +#define TEGRA30_TIMER0_BASE0x88 Why put the SoC name in the define names? Why not just have TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch, right?) and have the driver know that 1..4 are valid on Tegra20, and 1..10 are valid on later chips. I guess if the defines are moved into a header file, adding a TEGRA_ prefix does make sense. But I wonder if it wouldn't be simpler for the Tegra WDT driver to just call a function on the Tegra clocksource driver to find out which timer ID(s) to avoid using? Even simpler would be to just put a comment in the WDT driver saying that timer 5 was chosen arbitrarily, but if it's changed make sure not to conflict with the clocksource driver (and an equivalent change to the clocksource driver). Alright, I think I'll just go with putting a comment in the WDT driver then, so that We don't need to add this new header file. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 1/1] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (Tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com --- Changes from V2: Applied all of Stephen Warren's comments. Modified suspend callback by only stopping the watchdog timer if it was currently active. Added some logging during suspend/resume. Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 371 + 4 files changed, 388 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..3aae2da 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA || COMPILE_TEST + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..1314475 --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,371 @@ +/* + * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/miscdevice.h +#include linux/watchdog.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 + +/* + * Register base of the timer that's selected for pairing with the watchdog. + * This driver arbitrarily uses timer 5, which is currently unused by + * other drivers (in particular, the Tegra clocksource driver). If this + * needs to change, take care that the new timer is not used by the + * clocksource driver. + */ +#define WDT_TIMER_BASE 0x60 +#define WDT_TIMER_ID 5 + +/* WDT registers */ +#define WDT_CFG0x0 +#define WDT_CFG_PERIOD_SHIFT 4 +#define WDT_CFG_PERIOD_MASK0xff +#define WDT_CFG_INT_EN
RE: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
> > > On 01/31/2014 10:29 PM, Andrew Chew wrote: > > > > There are some differences between tegra20's timer registers and > > > > tegra30's (and later). For one thing, the watchdogs don't seem to > > > > be present in tegra20. > > > > > > "don't seem", so it is an assumption ? > > > > No, this is not an assumption. It has been verified by other NVIDIA > > engineers since I proposed this change. > > So why is your changelog saying "don't seem to be" ? I updated the commit message (see V2 of this patch). I hope the new commit message satisfies the concerns: "There are some differences between tegra20's timer registers and tegra30's (and later). For example, tegra30 has more timers. In addition, watchdogs are not present in tegra20. Add this compatibility string in order to be able to distinguish whether the additional timers and watchdogs are there or not." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
> From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] > Sent: Monday, February 03, 2014 11:55 PM > To: Andrew Chew; t...@linutronix.de; swar...@wwwdotorg.org; > thierry.red...@gmail.com; r...@landley.net; grant.lik...@linaro.org; > robh...@kernel.org; abres...@chromium.org; dgr...@chromium.org; > kati...@chromium.org > Cc: linux-kernel@vger.kernel.org; linux-te...@vger.kernel.org; linux- > watch...@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header > file > > On 02/04/2014 01:17 AM, Andrew Chew wrote: > > Added timers that are present in tegra30 and later, that are NOT in tegra20. > > > > Also, some of these timer bases are needed in the tegra watchdog > > driver, so separate them out into a header file that both the > > clocksource driver and the watchdog driver can share them. > > > > Signed-off-by: Andrew Chew > > When reading the patch 3/3, I don't see any define reused from this header > except TEGRA30_TIMER_WDT_BASE which is only used for the watchdog. > May be I missed something but I don't see any definition shared and thus I > don't see the point of creating this header file. I guess the point is to make other potential drivers that make use of the timer registers aware that this particular timer (timer 5) is provisioned for the watchdog driver. Right now, you're right, there really isn't much that's shared...the only thing really is the base address of timer 5 (and its associated ID)...the timer bases are kind of all over the place, so it's not straightforward to calculate the ID from the base address. But I believe the thought at the time (from Stephen Warren's suggestion) is to have a central place where this stuff is defined so that it's more clear what timers are provisioned for what purposes. It just happens that today, the only users of the timers, other than clocksource, is watchdog, as far as I can tell. > > +++ b/include/clocksource/tegra_timer.h > > @@ -0,0 +1,43 @@ > > +/* > > + * Copyright (C) 2010 Google, Inc. > > + * > > + * Author: > > + * Colin Cross The contents of this new header file are largely just a cut and paste of the corresponding snippet in tegra20-timer.c, so I thought I'd carry over the license verbatim. I didn't author this really...I just moved it around. What should I do instead?
RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] Sent: Monday, February 03, 2014 11:55 PM To: Andrew Chew; t...@linutronix.de; swar...@wwwdotorg.org; thierry.red...@gmail.com; r...@landley.net; grant.lik...@linaro.org; robh...@kernel.org; abres...@chromium.org; dgr...@chromium.org; kati...@chromium.org Cc: linux-kernel@vger.kernel.org; linux-te...@vger.kernel.org; linux- watch...@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file On 02/04/2014 01:17 AM, Andrew Chew wrote: Added timers that are present in tegra30 and later, that are NOT in tegra20. Also, some of these timer bases are needed in the tegra watchdog driver, so separate them out into a header file that both the clocksource driver and the watchdog driver can share them. Signed-off-by: Andrew Chew ac...@nvidia.com When reading the patch 3/3, I don't see any define reused from this header except TEGRA30_TIMER_WDT_BASE which is only used for the watchdog. May be I missed something but I don't see any definition shared and thus I don't see the point of creating this header file. I guess the point is to make other potential drivers that make use of the timer registers aware that this particular timer (timer 5) is provisioned for the watchdog driver. Right now, you're right, there really isn't much that's shared...the only thing really is the base address of timer 5 (and its associated ID)...the timer bases are kind of all over the place, so it's not straightforward to calculate the ID from the base address. But I believe the thought at the time (from Stephen Warren's suggestion) is to have a central place where this stuff is defined so that it's more clear what timers are provisioned for what purposes. It just happens that today, the only users of the timers, other than clocksource, is watchdog, as far as I can tell. +++ b/include/clocksource/tegra_timer.h @@ -0,0 +1,43 @@ +/* + * Copyright (C) 2010 Google, Inc. + * + * Author: + * Colin Cross ccr...@google.com The contents of this new header file are largely just a cut and paste of the corresponding snippet in tegra20-timer.c, so I thought I'd carry over the license verbatim. I didn't author this really...I just moved it around. What should I do instead?
RE: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
On 01/31/2014 10:29 PM, Andrew Chew wrote: There are some differences between tegra20's timer registers and tegra30's (and later). For one thing, the watchdogs don't seem to be present in tegra20. don't seem, so it is an assumption ? No, this is not an assumption. It has been verified by other NVIDIA engineers since I proposed this change. So why is your changelog saying don't seem to be ? I updated the commit message (see V2 of this patch). I hope the new commit message satisfies the concerns: There are some differences between tegra20's timer registers and tegra30's (and later). For example, tegra30 has more timers. In addition, watchdogs are not present in tegra20. Add this compatibility string in order to be able to distinguish whether the additional timers and watchdogs are there or not. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/3] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew --- Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 372 + 4 files changed, 389 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..2852447 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate "Tegra watchdog" + depends on ARCH_TEGRA + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..eebe7fc --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,372 @@ +/* + * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 +#define WDT_TIMER_BASE TEGRA30_TIMER_WDT_BASE +#define WDT_TIMER_ID TEGRA30_TIMER_WDT_ID + +/* WDT registers */ +#define WDT_CFG0x0 +#define WDT_CFG_PERIOD_SHIFT 4 +#define WDT_CFG_PERIOD_MASK0xff +#define WDT_CFG_INT_EN (1 << 12) +#define WDT_CFG_PMC2CAR_RST_EN (1 << 15) +#define WDT_STS0x4 +#define WDT_STS_COUNT_SHIFT4 +#define WDT_STS_COUNT_MASK 0xff +#define WDT_STS_EXP_SHIFT 12 +#define WDT_STS_EXP_MASK 0x3 +#define WDT_CMD0x8 +#define WDT_CMD_START_COUNTER (1 << 0) +#define WDT_CMD_DISABLE_COUNTER(1 << 1) +#define WDT_UNLOCK (0xc) +#define WDT_UNLOCK_PATTERN (0xc45a << 0) + +/* Timer registers
[PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
Added timers that are present in tegra30 and later, that are NOT in tegra20.
Also, some of these timer bases are needed in the tegra watchdog driver, so
separate them out into a header file that both the clocksource driver and
the watchdog driver can share them.
Signed-off-by: Andrew Chew
---
drivers/clocksource/tegra20_timer.c | 15 ++---
include/clocksource/tegra_timer.h | 43 +
2 files changed, 49 insertions(+), 9 deletions(-)
create mode 100644 include/clocksource/tegra_timer.h
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index 73cfa56..2c49643 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -28,6 +28,8 @@
#include
#include
+#include
+
#include
#include
@@ -39,11 +41,6 @@
#define TIMERUS_USEC_CFG 0x14
#define TIMERUS_CNTR_FREEZE 0x4c
-#define TIMER1_BASE 0x0
-#define TIMER2_BASE 0x8
-#define TIMER3_BASE 0x50
-#define TIMER4_BASE 0x58
-
#define TIMER_PTV 0x0
#define TIMER_PCR 0x4
@@ -64,7 +61,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
u32 reg;
reg = 0x8000 | ((cycles > 1) ? (cycles-1) : 0);
- timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+ timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
return 0;
}
@@ -74,12 +71,12 @@ static void tegra_timer_set_mode(enum clock_event_mode mode,
{
u32 reg;
- timer_writel(0, TIMER3_BASE + TIMER_PTV);
+ timer_writel(0, TEGRA20_TIMER3_BASE + TIMER_PTV);
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
reg = 0xC000 | ((100/HZ)-1);
- timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+ timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
break;
case CLOCK_EVT_MODE_ONESHOT:
break;
@@ -142,7 +139,7 @@ static void tegra_read_persistent_clock(struct timespec *ts)
static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
{
struct clock_event_device *evt = (struct clock_event_device *)dev_id;
- timer_writel(1<<30, TIMER3_BASE + TIMER_PCR);
+ timer_writel(1<<30, TEGRA20_TIMER3_BASE + TIMER_PCR);
evt->event_handler(evt);
return IRQ_HANDLED;
}
diff --git a/include/clocksource/tegra_timer.h
b/include/clocksource/tegra_timer.h
new file mode 100644
index 000..ea0bc8b
--- /dev/null
+++ b/include/clocksource/tegra_timer.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2010 Google, Inc.
+ *
+ * Author:
+ * Colin Cross
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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 __CLOCKSOURCE_TEGRA_TIMER_H
+#define __CLOCKSOURCE_TEGRA_TIMER_H
+
+/* Tegra 20 timers */
+#define TEGRA20_TIMER1_BASE0x0
+#define TEGRA20_TIMER2_BASE0x8
+#define TEGRA20_TIMER3_BASE0x50
+#define TEGRA20_TIMER4_BASE0x58
+
+/* Tegra 30 timers */
+#define TEGRA30_TIMER1_BASETEGRA20_TIMER1_BASE
+#define TEGRA30_TIMER2_BASETEGRA20_TIMER2_BASE
+#define TEGRA30_TIMER3_BASETEGRA20_TIMER3_BASE
+#define TEGRA30_TIMER4_BASETEGRA20_TIMER4_BASE
+#define TEGRA30_TIMER5_BASE0x60
+#define TEGRA30_TIMER6_BASE0x68
+#define TEGRA30_TIMER7_BASE0x70
+#define TEGRA30_TIMER8_BASE0x78
+#define TEGRA30_TIMER9_BASE0x80
+#define TEGRA30_TIMER0_BASE0x88
+
+/* Used by the tegra watchdog timer */
+#define TEGRA30_TIMER_WDT_BASE TEGRA30_TIMER5_BASE
+#define TEGRA30_TIMER_WDT_ID 5
+
+#endif /* __CLOCKSOURCE_TEGRA_TIMER_H */
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
There are some differences between tegra20's timer registers and tegra30's
(and later). For example, tegra30 has more timers. In addition, watchdogs
are not present in tegra20.
Add this compatibility string in order to be able to distinguish
whether the additional timers and watchdogs are there or not.
Signed-off-by: Andrew Chew
Acked-by: Stephen Warren
---
drivers/clocksource/tegra20_timer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index d1869f0..73cfa56 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -218,6 +218,7 @@ static void __init tegra20_init_timer(struct device_node
*np)
0x1, 0x1fff);
}
CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer",
tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer",
tegra20_init_timer);
static void __init tegra20_init_rtc(struct device_node *np)
{
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/3] tegra30 watchdog support
This patch series ultimately adds watchdog support for tegra30 and later chips. The existing tegra clocksource driver (drivers/clocksource/tegra20_timer.c) sadly does not distinguish between tegra20 and tegra30 (and later), which it should have done since the contents of the timer register base have changed significantly. In particular, tegra30 (and later) has more timers, and also hardware watchdog registers. The first patch adds nvidia,tegra30-timer to the list of compatibilty strings for the tegra timer device tree node, so that we can distinguish between tegra20 and tegra30 (and later). The second patch separates out some macros that are interesting to other drivers (in particular, the tegra watchdog driver), and also adds the the missing timers that are present in tegra30 and later. The third patch adds the actual watchdog driver. This driver configures a single watchdog (watchdog 0), pairs it with timer 5 (defined as TEGRA30_TIMER_WDT_* in the shared header file from the previous patch), and sets it up so that upon timer expiration, will cause the target system to reset. I've decided to encapsulate all related changes into one patch series, since I did not modify any device tree bindings and therefore don't need to review dt changes separately. This way, everything can be seen within its complete context. Andrew Chew (3): clocksource: tegra: Add nvidia,tegra30-timer compat clocksource: tegra: Define timer bases in header file watchdog: Add tegra watchdog Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/clocksource/tegra20_timer.c| 16 +- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 372 + include/clocksource/tegra_timer.h | 43 +++ 6 files changed, 439 insertions(+), 9 deletions(-) create mode 100644 drivers/watchdog/tegra_wdt.c create mode 100644 include/clocksource/tegra_timer.h -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
> On 02/03/2014 02:16 PM, Andrew Chew wrote: > >> On 02/03/2014 11:59 AM, Andrew Chew wrote: > >>>> On Fri, Jan 31, 2014 at 09:46:51PM +, Andrew Chew wrote: > >>>>> This optional property can be used to specify which timers are to > >>>>> be used for hardware watchdog timeouts (via a tegra wdt driver). > >>>> > >>>> Is there any reason that a particular timer should be used? > >>> > >>> I worry about colliding with other timer allocations, and wanted to > >>> be flexible in this regard. > >> > >> Are the other timer allocations represented in DT, or simply made by > >> or hard- coded in the driver? If the former, this property seems like > >> a good equivalent of any existing allocations. If the latter, can't > >> the driver just allocate or hard- code the allocation in the same way as > >> any > existing allocations? > > > > From what I've seen, timer allocations are just hard-coded into whatever > driver. > > I didn't think this was a particularly good idea, since when writing > > other drivers that for some reason need a timer, the author has to be > > aware of allocations made in other, barely related drivers. > > I'm not sure that they would; why wouldn't the timer driver register the > various timers with standard Linux APIs which the clients talk to, thus > avoiding the clients having any knowledge at all of which channels are used > for what. > > If you're talking about the watchdog driver, then can't we just create a > shared header file that the clocksource and watchdog drivers both include, > which defines the timer ID allocations? Sure, let's go with that. In that case, this patch isn't needed, and should be dropped. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
> On 02/03/2014 11:59 AM, Andrew Chew wrote: > >> On Fri, Jan 31, 2014 at 09:46:51PM +0000, Andrew Chew wrote: > >>> This optional property can be used to specify which timers are to be > >>> used for hardware watchdog timeouts (via a tegra wdt driver). > >> > >> Is there any reason that a particular timer should be used? > > > > I worry about colliding with other timer allocations, and wanted to be > > flexible in this regard. > > Are the other timer allocations represented in DT, or simply made by or hard- > coded in the driver? If the former, this property seems like a good equivalent > of any existing allocations. If the latter, can't the driver just allocate or > hard- > code the allocation in the same way as any existing allocations? >From what I've seen, timer allocations are just hard-coded into whatever >driver. I didn't think this was a particularly good idea, since when writing other drivers that for some reason need a timer, the author has to be aware of allocations made in other, barely related drivers. In addition, what seems like an arbitrary allocation in one scenario, I anticipate may not be completely arbitrary in a different scenario, so I thought it would be better to freeze the device driver code, and allow for flexibility at the device tree level. But I'll do whatever others think is right. I can make my watchdog driver just take an arbitrary (to me right now) timer and instantiate one watchdog for it. If I'm to do that, then this device node property isn't necessary, and we can drop this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
> On Fri, Jan 31, 2014 at 09:46:51PM +0000, Andrew Chew wrote: > > This optional property can be used to specify which timers are to be > > used for hardware watchdog timeouts (via a tegra wdt driver). > > Is there any reason that a particular timer should be used? I worry about colliding with other timer allocations, and wanted to be flexible in this regard. > This shouldn't even mention the driver, as the binding should describe the > HW, not how it's used by Linux at the moment. > > > > > Signed-off-by: Andrew Chew > > --- > > Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt | 8 > > > > 1 file changed, 8 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt > > b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt > > index b5082a1..e87fa70 100644 > > --- a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt > > +++ b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt > > @@ -13,6 +13,13 @@ Required properties: > > - clocks : Must contain one entry, for the module clock. > >See ../clocks/clock-bindings.txt for details. > > > > +Optional properties: > > + > > +- nvidia,wdt-timer-id: A list of timer IDs to be used for watchdogs. > > +Watchdog 0 will be assigned to the first timer listed, watchdog 1 will > > +be assigned to the second timer listed, etc. up to the number of > watchdogs > > +available. > > This sounds like a description of what software should do. Is there any > reason this order is important? The order in regards to which watchdog (watchdog 0, watchdog 1, etc) is paired with which timer is unimportant for purposes of the watchdog driver that I will follow up with. I can leave those details out of the bindings description if that resolves your concern. > Also, it feels odd for the proerty name to be singular given it's a list... You're right. Given what it is, it really should be nvidia,wdt-timer-ids. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
> From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] > Sent: Monday, February 03, 2014 8:40 AM > To: Andrew Chew; t...@linutronix.de; swar...@wwwdotorg.org; > thierry.red...@gmail.com; abres...@chromium.org; dgr...@chromium.org; > kati...@chromium.org > Cc: linux-kernel@vger.kernel.org; linux-te...@vger.kernel.org > Subject: Re: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat > > On 01/31/2014 10:29 PM, Andrew Chew wrote: > > There are some differences between tegra20's timer registers and > > tegra30's (and later). For one thing, the watchdogs don't seem to be > > present in tegra20. > > "don't seem", so it is an assumption ? No, this is not an assumption. It has been verified by other NVIDIA engineers since I proposed this change. > > Add this compatibility string in order to be able to distinguish > > whether the watchdogs are there or not. > > Sorry but I don't get the connection between declaring the tegra30_timer > and the log. Can you elaborate please ? I don't know what you mean by "the log". Was that a typo? Anyway, I have a watchdog driver that I intend to follow up with, that binds with tegra30-timer. I don't want this driver to be able to bind with tegra20-timer, because the driver won't actually work on tegra20. Does that answer your question?
RE: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] Sent: Monday, February 03, 2014 8:40 AM To: Andrew Chew; t...@linutronix.de; swar...@wwwdotorg.org; thierry.red...@gmail.com; abres...@chromium.org; dgr...@chromium.org; kati...@chromium.org Cc: linux-kernel@vger.kernel.org; linux-te...@vger.kernel.org Subject: Re: [PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat On 01/31/2014 10:29 PM, Andrew Chew wrote: There are some differences between tegra20's timer registers and tegra30's (and later). For one thing, the watchdogs don't seem to be present in tegra20. don't seem, so it is an assumption ? No, this is not an assumption. It has been verified by other NVIDIA engineers since I proposed this change. Add this compatibility string in order to be able to distinguish whether the watchdogs are there or not. Sorry but I don't get the connection between declaring the tegra30_timer and the log. Can you elaborate please ? I don't know what you mean by the log. Was that a typo? Anyway, I have a watchdog driver that I intend to follow up with, that binds with tegra30-timer. I don't want this driver to be able to bind with tegra20-timer, because the driver won't actually work on tegra20. Does that answer your question?
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
On Fri, Jan 31, 2014 at 09:46:51PM +, Andrew Chew wrote: This optional property can be used to specify which timers are to be used for hardware watchdog timeouts (via a tegra wdt driver). Is there any reason that a particular timer should be used? I worry about colliding with other timer allocations, and wanted to be flexible in this regard. This shouldn't even mention the driver, as the binding should describe the HW, not how it's used by Linux at the moment. Signed-off-by: Andrew Chew ac...@nvidia.com --- Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt | 8 1 file changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt index b5082a1..e87fa70 100644 --- a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt +++ b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt @@ -13,6 +13,13 @@ Required properties: - clocks : Must contain one entry, for the module clock. See ../clocks/clock-bindings.txt for details. +Optional properties: + +- nvidia,wdt-timer-id: A list of timer IDs to be used for watchdogs. +Watchdog 0 will be assigned to the first timer listed, watchdog 1 will +be assigned to the second timer listed, etc. up to the number of watchdogs +available. This sounds like a description of what software should do. Is there any reason this order is important? The order in regards to which watchdog (watchdog 0, watchdog 1, etc) is paired with which timer is unimportant for purposes of the watchdog driver that I will follow up with. I can leave those details out of the bindings description if that resolves your concern. Also, it feels odd for the proerty name to be singular given it's a list... You're right. Given what it is, it really should be nvidia,wdt-timer-ids. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
On 02/03/2014 11:59 AM, Andrew Chew wrote: On Fri, Jan 31, 2014 at 09:46:51PM +, Andrew Chew wrote: This optional property can be used to specify which timers are to be used for hardware watchdog timeouts (via a tegra wdt driver). Is there any reason that a particular timer should be used? I worry about colliding with other timer allocations, and wanted to be flexible in this regard. Are the other timer allocations represented in DT, or simply made by or hard- coded in the driver? If the former, this property seems like a good equivalent of any existing allocations. If the latter, can't the driver just allocate or hard- code the allocation in the same way as any existing allocations? From what I've seen, timer allocations are just hard-coded into whatever driver. I didn't think this was a particularly good idea, since when writing other drivers that for some reason need a timer, the author has to be aware of allocations made in other, barely related drivers. In addition, what seems like an arbitrary allocation in one scenario, I anticipate may not be completely arbitrary in a different scenario, so I thought it would be better to freeze the device driver code, and allow for flexibility at the device tree level. But I'll do whatever others think is right. I can make my watchdog driver just take an arbitrary (to me right now) timer and instantiate one watchdog for it. If I'm to do that, then this device node property isn't necessary, and we can drop this patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
On 02/03/2014 02:16 PM, Andrew Chew wrote: On 02/03/2014 11:59 AM, Andrew Chew wrote: On Fri, Jan 31, 2014 at 09:46:51PM +, Andrew Chew wrote: This optional property can be used to specify which timers are to be used for hardware watchdog timeouts (via a tegra wdt driver). Is there any reason that a particular timer should be used? I worry about colliding with other timer allocations, and wanted to be flexible in this regard. Are the other timer allocations represented in DT, or simply made by or hard- coded in the driver? If the former, this property seems like a good equivalent of any existing allocations. If the latter, can't the driver just allocate or hard- code the allocation in the same way as any existing allocations? From what I've seen, timer allocations are just hard-coded into whatever driver. I didn't think this was a particularly good idea, since when writing other drivers that for some reason need a timer, the author has to be aware of allocations made in other, barely related drivers. I'm not sure that they would; why wouldn't the timer driver register the various timers with standard Linux APIs which the clients talk to, thus avoiding the clients having any knowledge at all of which channels are used for what. If you're talking about the watchdog driver, then can't we just create a shared header file that the clocksource and watchdog drivers both include, which defines the timer ID allocations? Sure, let's go with that. In that case, this patch isn't needed, and should be dropped. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/3] tegra30 watchdog support
This patch series ultimately adds watchdog support for tegra30 and later chips. The existing tegra clocksource driver (drivers/clocksource/tegra20_timer.c) sadly does not distinguish between tegra20 and tegra30 (and later), which it should have done since the contents of the timer register base have changed significantly. In particular, tegra30 (and later) has more timers, and also hardware watchdog registers. The first patch adds nvidia,tegra30-timer to the list of compatibilty strings for the tegra timer device tree node, so that we can distinguish between tegra20 and tegra30 (and later). The second patch separates out some macros that are interesting to other drivers (in particular, the tegra watchdog driver), and also adds the the missing timers that are present in tegra30 and later. The third patch adds the actual watchdog driver. This driver configures a single watchdog (watchdog 0), pairs it with timer 5 (defined as TEGRA30_TIMER_WDT_* in the shared header file from the previous patch), and sets it up so that upon timer expiration, will cause the target system to reset. I've decided to encapsulate all related changes into one patch series, since I did not modify any device tree bindings and therefore don't need to review dt changes separately. This way, everything can be seen within its complete context. Andrew Chew (3): clocksource: tegra: Add nvidia,tegra30-timer compat clocksource: tegra: Define timer bases in header file watchdog: Add tegra watchdog Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/clocksource/tegra20_timer.c| 16 +- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 372 + include/clocksource/tegra_timer.h | 43 +++ 6 files changed, 439 insertions(+), 9 deletions(-) create mode 100644 drivers/watchdog/tegra_wdt.c create mode 100644 include/clocksource/tegra_timer.h -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/3] clocksource: tegra: Define timer bases in header file
Added timers that are present in tegra30 and later, that are NOT in tegra20.
Also, some of these timer bases are needed in the tegra watchdog driver, so
separate them out into a header file that both the clocksource driver and
the watchdog driver can share them.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
drivers/clocksource/tegra20_timer.c | 15 ++---
include/clocksource/tegra_timer.h | 43 +
2 files changed, 49 insertions(+), 9 deletions(-)
create mode 100644 include/clocksource/tegra_timer.h
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index 73cfa56..2c49643 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -28,6 +28,8 @@
#include linux/of_irq.h
#include linux/sched_clock.h
+#include clocksource/tegra_timer.h
+
#include asm/mach/time.h
#include asm/smp_twd.h
@@ -39,11 +41,6 @@
#define TIMERUS_USEC_CFG 0x14
#define TIMERUS_CNTR_FREEZE 0x4c
-#define TIMER1_BASE 0x0
-#define TIMER2_BASE 0x8
-#define TIMER3_BASE 0x50
-#define TIMER4_BASE 0x58
-
#define TIMER_PTV 0x0
#define TIMER_PCR 0x4
@@ -64,7 +61,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
u32 reg;
reg = 0x8000 | ((cycles 1) ? (cycles-1) : 0);
- timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+ timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
return 0;
}
@@ -74,12 +71,12 @@ static void tegra_timer_set_mode(enum clock_event_mode mode,
{
u32 reg;
- timer_writel(0, TIMER3_BASE + TIMER_PTV);
+ timer_writel(0, TEGRA20_TIMER3_BASE + TIMER_PTV);
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
reg = 0xC000 | ((100/HZ)-1);
- timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+ timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
break;
case CLOCK_EVT_MODE_ONESHOT:
break;
@@ -142,7 +139,7 @@ static void tegra_read_persistent_clock(struct timespec *ts)
static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
{
struct clock_event_device *evt = (struct clock_event_device *)dev_id;
- timer_writel(130, TIMER3_BASE + TIMER_PCR);
+ timer_writel(130, TEGRA20_TIMER3_BASE + TIMER_PCR);
evt-event_handler(evt);
return IRQ_HANDLED;
}
diff --git a/include/clocksource/tegra_timer.h
b/include/clocksource/tegra_timer.h
new file mode 100644
index 000..ea0bc8b
--- /dev/null
+++ b/include/clocksource/tegra_timer.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2010 Google, Inc.
+ *
+ * Author:
+ * Colin Cross ccr...@google.com
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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 __CLOCKSOURCE_TEGRA_TIMER_H
+#define __CLOCKSOURCE_TEGRA_TIMER_H
+
+/* Tegra 20 timers */
+#define TEGRA20_TIMER1_BASE0x0
+#define TEGRA20_TIMER2_BASE0x8
+#define TEGRA20_TIMER3_BASE0x50
+#define TEGRA20_TIMER4_BASE0x58
+
+/* Tegra 30 timers */
+#define TEGRA30_TIMER1_BASETEGRA20_TIMER1_BASE
+#define TEGRA30_TIMER2_BASETEGRA20_TIMER2_BASE
+#define TEGRA30_TIMER3_BASETEGRA20_TIMER3_BASE
+#define TEGRA30_TIMER4_BASETEGRA20_TIMER4_BASE
+#define TEGRA30_TIMER5_BASE0x60
+#define TEGRA30_TIMER6_BASE0x68
+#define TEGRA30_TIMER7_BASE0x70
+#define TEGRA30_TIMER8_BASE0x78
+#define TEGRA30_TIMER9_BASE0x80
+#define TEGRA30_TIMER0_BASE0x88
+
+/* Used by the tegra watchdog timer */
+#define TEGRA30_TIMER_WDT_BASE TEGRA30_TIMER5_BASE
+#define TEGRA30_TIMER_WDT_ID 5
+
+#endif /* __CLOCKSOURCE_TEGRA_TIMER_H */
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/3] watchdog: Add tegra watchdog
Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (tegra30 and later). This driver will configure one watchdog timer that will reset the system in the case of a watchdog timeout. This driver binds to the nvidia,tegra30-timer device node and gets its register base from there. Signed-off-by: Andrew Chew ac...@nvidia.com --- Documentation/watchdog/watchdog-parameters.txt | 5 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/tegra_wdt.c | 372 + 4 files changed, 389 insertions(+) create mode 100644 drivers/watchdog/tegra_wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index f9492fe..b39f355 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot stmp3xxx_wdt: heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19 - +tegra_wdt: +heartbeat: Watchdog heartbeats in seconds. (default = 120) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +- ts72xx_wdt: timeout: Watchdog timeout in seconds. (1 = timeout = 8, default=8) nowayout: Disable watchdog shutdown on close diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 4c4c566..2852447 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config TEGRA_WATCHDOG + tristate Tegra watchdog + depends on ARCH_TEGRA + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer + embedded in NVIDIA Tegra SoCs. + + To compile this driver as a module, choose M here: the + module will be called tegra_wdt. + # AVR32 Architecture config AT32AP700X_WDT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 985a66c..1b5f3d5 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o +obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o # AVR32 Architecture obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c new file mode 100644 index 000..eebe7fc --- /dev/null +++ b/drivers/watchdog/tegra_wdt.c @@ -0,0 +1,372 @@ +/* + * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/interrupt.h +#include linux/io.h +#include linux/of.h +#include linux/platform_device.h +#include linux/miscdevice.h +#include linux/watchdog.h + +#include clocksource/tegra_timer.h + +/* minimum and maximum watchdog trigger timeout, in seconds */ +#define MIN_WDT_TIMEOUT1 +#define MAX_WDT_TIMEOUT255 + +/* + * Base of the WDT registers, from the timer base address. There are + * actually 5 watchdogs that can be configured (by pairing with an available + * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4. + * This driver only configures the first watchdog (WDT ID 0). + */ +#define WDT_BASE 0x100 +#define WDT_ID 0 +#define WDT_TIMER_BASE TEGRA30_TIMER_WDT_BASE +#define WDT_TIMER_ID TEGRA30_TIMER_WDT_ID + +/* WDT registers */ +#define WDT_CFG0x0 +#define WDT_CFG_PERIOD_SHIFT 4 +#define WDT_CFG_PERIOD_MASK0xff +#define WDT_CFG_INT_EN (1 12) +#define WDT_CFG_PMC2CAR_RST_EN (1 15) +#define WDT_STS0x4 +#define WDT_STS_COUNT_SHIFT4 +#define WDT_STS_COUNT_MASK 0xff +#define WDT_STS_EXP_SHIFT 12 +#define WDT_STS_EXP_MASK 0x3 +#define WDT_CMD0x8 +#define WDT_CMD_START_COUNTER (1 0) +#define WDT_CMD_DISABLE_COUNTER(1 1) +#define
[PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat
There are some differences between tegra20's timer registers and tegra30's
(and later). For example, tegra30 has more timers. In addition, watchdogs
are not present in tegra20.
Add this compatibility string in order to be able to distinguish
whether the additional timers and watchdogs are there or not.
Signed-off-by: Andrew Chew ac...@nvidia.com
Acked-by: Stephen Warren swar...@nvidia.com
---
drivers/clocksource/tegra20_timer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index d1869f0..73cfa56 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -218,6 +218,7 @@ static void __init tegra20_init_timer(struct device_node
*np)
0x1, 0x1fff);
}
CLOCKSOURCE_OF_DECLARE(tegra20_timer, nvidia,tegra20-timer,
tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra30_timer, nvidia,tegra30-timer,
tegra20_init_timer);
static void __init tegra20_init_rtc(struct device_node *np)
{
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
This optional property can be used to specify which timers are to be used
for hardware watchdog timeouts (via a tegra wdt driver).
Signed-off-by: Andrew Chew
---
Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
index b5082a1..e87fa70 100644
--- a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
+++ b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
@@ -13,6 +13,13 @@ Required properties:
- clocks : Must contain one entry, for the module clock.
See ../clocks/clock-bindings.txt for details.
+Optional properties:
+
+- nvidia,wdt-timer-id: A list of timer IDs to be used for watchdogs.
+Watchdog 0 will be assigned to the first timer listed, watchdog 1 will
+be assigned to the second timer listed, etc. up to the number of watchdogs
+available.
+
timer {
compatible = "nvidia,tegra30-timer", "nvidia,tegra20-timer";
reg = <0x60005000 0x400>;
@@ -23,4 +30,5 @@ timer {
0 121 0x04
0 122 0x04>;
clocks = <_car 214>;
+ nvidia,wdt-timer-id = <7 8>;
};
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
There are some differences between tegra20's timer registers and tegra30's
(and later). For one thing, the watchdogs don't seem to be present in
tegra20. Add this compatibility string in order to be able to distinguish
whether the watchdogs are there or not.
Signed-off-by: Andrew Chew
---
drivers/clocksource/tegra20_timer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index d1869f0..73cfa56 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -218,6 +218,7 @@ static void __init tegra20_init_timer(struct device_node
*np)
0x1, 0x1fff);
}
CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer",
tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer",
tegra20_init_timer);
static void __init tegra20_init_rtc(struct device_node *np)
{
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] clocksource: tegra: Add nvidia,tegra30-timer compat
There are some differences between tegra20's timer registers and tegra30's
(and later). For one thing, the watchdogs don't seem to be present in
tegra20. Add this compatibility string in order to be able to distinguish
whether the watchdogs are there or not.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
drivers/clocksource/tegra20_timer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/tegra20_timer.c
b/drivers/clocksource/tegra20_timer.c
index d1869f0..73cfa56 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -218,6 +218,7 @@ static void __init tegra20_init_timer(struct device_node
*np)
0x1, 0x1fff);
}
CLOCKSOURCE_OF_DECLARE(tegra20_timer, nvidia,tegra20-timer,
tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra30_timer, nvidia,tegra30-timer,
tegra20_init_timer);
static void __init tegra20_init_rtc(struct device_node *np)
{
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1] ARM: tegra: add nvidia,wdt-timer-id optional property
This optional property can be used to specify which timers are to be used
for hardware watchdog timeouts (via a tegra wdt driver).
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
index b5082a1..e87fa70 100644
--- a/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
+++ b/Documentation/devicetree/bindings/timer/nvidia,tegra30-timer.txt
@@ -13,6 +13,13 @@ Required properties:
- clocks : Must contain one entry, for the module clock.
See ../clocks/clock-bindings.txt for details.
+Optional properties:
+
+- nvidia,wdt-timer-id: A list of timer IDs to be used for watchdogs.
+Watchdog 0 will be assigned to the first timer listed, watchdog 1 will
+be assigned to the second timer listed, etc. up to the number of watchdogs
+available.
+
timer {
compatible = nvidia,tegra30-timer, nvidia,tegra20-timer;
reg = 0x60005000 0x400;
@@ -23,4 +30,5 @@ timer {
0 121 0x04
0 122 0x04;
clocks = tegra_car 214;
+ nvidia,wdt-timer-id = 7 8;
};
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
> On 07/27/2013 03:55 AM, Laxman Dewangan wrote: > > On Saturday 27 July 2013 03:42 AM, Andrew Chew wrote: > >> I wrote: > >>> Andrew wrote: > >>>> [adding a third pinmux configuration property to Palmas's DT] > >>> > >>> How does this interact with the pinctrl driver that Laxman just sent > >>> for Palmas? > >>> > >>> https://lkml.org/lkml/2013/7/26/141 > >>> [PATCH 0/2] pinctrl: palmas: add pincontrol driver > .. > >> Abandoning this patch. > ... > > once we will have the pincontrol driver then mux pads are become > redundant. > > OK. The driver should probably operate like this then: > > * During probe(), parse the ti,mux-pad* parameters, if present, and apply > them. This is needed to maintain compatibility with old DTs that may contain > these properties. > > * At the end of probe(), register the pinctrl driver. If standard pinctrl > properties are present in DT, these will then be applied. These may override > the values set by any ti,mux-pad* properties if they were present. > > Also, we should remove, or mark deprecated, the ti,mux-pad* properties in > the binding document when adding pinctrl support. Sounds reasonable to me. The fate of my patch hasn't really been discussed, though. Can we apply it, to make the ti,mux-pad* stuff complete? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
On 07/27/2013 03:55 AM, Laxman Dewangan wrote: On Saturday 27 July 2013 03:42 AM, Andrew Chew wrote: I wrote: Andrew wrote: [adding a third pinmux configuration property to Palmas's DT] How does this interact with the pinctrl driver that Laxman just sent for Palmas? https://lkml.org/lkml/2013/7/26/141 [PATCH 0/2] pinctrl: palmas: add pincontrol driver .. Abandoning this patch. ... once we will have the pincontrol driver then mux pads are become redundant. OK. The driver should probably operate like this then: * During probe(), parse the ti,mux-pad* parameters, if present, and apply them. This is needed to maintain compatibility with old DTs that may contain these properties. * At the end of probe(), register the pinctrl driver. If standard pinctrl properties are present in DT, these will then be applied. These may override the values set by any ti,mux-pad* properties if they were present. Also, we should remove, or mark deprecated, the ti,mux-pad* properties in the binding document when adding pinctrl support. Sounds reasonable to me. The fate of my patch hasn't really been discussed, though. Can we apply it, to make the ti,mux-pad* stuff complete? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
> How does this interact with the pinctrl driver that Laxman just > sent for Palmas? > > https://lkml.org/lkml/2013/7/26/141 > [PATCH 0/2] pinctrl: palmas: add pincontrol driver > >>> > >>> Thanks for pointing this out. Given this: > >>> > >>> +Optional properties: > >>> +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 > mode. > >>> +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 > mode. > >>> > >>> I think his work already encompasses what my patch is supposed to do. > >>> > >>> Abandoning this patch. > >> > >> OK, that's simple! > >> > >> Are the existing ti,mux-pad1/ti,mux-pad2 properties already in the > >> binding redundant with Laxman's pinctrl driver? > > > > In linux-next (where I based my work), yes, those two properties > > already exist, and as far as I understand it, are redundant with Laxman's > pinctrl driver. > > I expect those properties will go away with Laxman's pinctrl driver. > > Except those properties have been there for many kernel revisions and are > an ABI and hence can't be removed, although I noticed that they got > renamed recently, and of course we aren't technically being strict about this > quite yet... > > Re: the complete pinctrl driver: is anything outside the Palmas going to need > to reprogram the Palmas pinctrl HW at run-time? Are the functions that can > be routed to the pins just static configuration for PMIC features, or might > other generic (non-Palmas) drivers use those pins for something? If not, > perhaps it's be simpler to just add your ti,mux-pad3 property and be done. I can imagine other projects wanting to do runtime muxing on those pins. These pins can serve as GPIOs, or can be programmed for special functions. For my particular scenario, I just need to statically set that particular mux register (the power-on default value is not suitable for what we want to do, namely to use the GPIO_6 pin as an actual GPIO pin). If the existing ti,mux-pad1 and ti,mux-pad2 properties are to stay, in the spirit of not changing the existing ABI, then sure, we can make a case for adding the missing one (ti,mux-pad3) for completeness. In this case, if the palmas PMIC's pin configuration can be statically set at start of day, one won't even need to instantiate the palmas pinctrl driver at all, and with the addition of ti,mux-pad3, the pin configuration control will actually be complete. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
> >> How does this interact with the pinctrl driver that Laxman just sent > >> for Palmas? > >> > >> https://lkml.org/lkml/2013/7/26/141 > >> [PATCH 0/2] pinctrl: palmas: add pincontrol driver > > > > Thanks for pointing this out. Given this: > > > > +Optional properties: > > +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 mode. > > +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 mode. > > > > I think his work already encompasses what my patch is supposed to do. > > > > Abandoning this patch. > > OK, that's simple! > > Are the existing ti,mux-pad1/ti,mux-pad2 properties already in the binding > redundant with Laxman's pinctrl driver? In linux-next (where I based my work), yes, those two properties already exist, and as far as I understand it, are redundant with Laxman's pinctrl driver. I expect those properties will go away with Laxman's pinctrl driver. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
> On 07/26/2013 02:41 PM, Andrew Chew wrote: > > There exists a PRIMARY_SECONDARY_PAD3 in the same register base as > > PRIMARY_SECONDARY_PAD2, which controls the function of certain pins. > > Add a property for this setting. > > How does this interact with the pinctrl driver that Laxman just sent for > Palmas? > > https://lkml.org/lkml/2013/7/26/141 > [PATCH 0/2] pinctrl: palmas: add pincontrol driver Thanks for pointing this out. Given this: +Optional properties: +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 mode. +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 mode. I think his work already encompasses what my patch is supposed to do. Abandoning this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mfd: palmas: Add DVFS mux setting
There exists a PRIMARY_SECONDARY_PAD3 in the same register base as
PRIMARY_SECONDARY_PAD2, which controls the function of certain pins. Add
a property for this setting.
Signed-off-by: Andrew Chew
---
Documentation/devicetree/bindings/mfd/palmas.txt | 3 ++-
drivers/mfd/palmas.c | 29 ++--
include/linux/mfd/palmas.h | 12 +-
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
b/Documentation/devicetree/bindings/mfd/palmas.txt
index 892537d..a718db5 100644
--- a/Documentation/devicetree/bindings/mfd/palmas.txt
+++ b/Documentation/devicetree/bindings/mfd/palmas.txt
@@ -24,7 +24,7 @@ and also the generic series names
- interrupt-parent : The parent interrupt controller.
Optional properties:
- ti,mux-padX : set the pad register X (1-2) to the correct muxing for the
+ ti,mux-padX : set the pad register X (1-3) to the correct muxing for the
hardware, if not set will use muxing in OTP.
Example:
@@ -38,6 +38,7 @@ palmas {
ti,mux-pad1 = <0>;
ti,mux-pad2 = <0>;
+ ti,mux-pad3 = <0>;
#address-cells = <1>;
#size-cells = <0>;
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index e4d1c70..b07b706 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -219,6 +219,12 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
pdata->pad2 = prop;
}
+ ret = of_property_read_u32(node, "ti,mux-pad3", );
+ if (!ret) {
+ pdata->mux_from_pdata = 1;
+ pdata->pad3 = prop;
+ }
+
/* The default for this register is all masked */
ret = of_property_read_u32(node, "ti,power-ctrl", );
if (!ret)
@@ -404,9 +410,28 @@ no_irq:
if (!(reg & PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_7_MASK))
palmas->gpio_muxed |= PALMAS_GPIO_7_MUXED;
- dev_info(palmas->dev, "Muxing GPIO %x, PWM %x, LED %x\n",
+ addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE,
+ PALMAS_PRIMARY_SECONDARY_PAD3);
+
+ if (pdata->mux_from_pdata) {
+ reg = pdata->pad3;
+ ret = regmap_write(palmas->regmap[slave], addr, reg);
+ if (ret)
+ goto err_irq;
+ } else {
+ ret = regmap_read(palmas->regmap[slave], addr, );
+ if (ret)
+ goto err_irq;
+ }
+
+ if (reg & PALMAS_PRIMARY_SECONDARY_PAD3_DVFS1)
+ palmas->dvfs_muxed |= PALMAS_DVFS1_MUXED;
+ if (reg & PALMAS_PRIMARY_SECONDARY_PAD3_DVFS2)
+ palmas->dvfs_muxed |= PALMAS_DVFS2_MUXED;
+
+ dev_info(palmas->dev, "Muxing GPIO %x, PWM %x, LED %x, DVFS %x\n",
palmas->gpio_muxed, palmas->pwm_muxed,
- palmas->led_muxed);
+ palmas->led_muxed, palmas->dvfs_muxed);
reg = pdata->power_ctrl;
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 1a8dd7a..e479107 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -84,6 +84,7 @@ struct palmas {
u8 gpio_muxed;
u8 led_muxed;
u8 pwm_muxed;
+ u8 dvfs_muxed;
};
struct palmas_gpadc_platform_data {
@@ -257,7 +258,7 @@ struct palmas_platform_data {
* then the two value to load into the registers if true
*/
int mux_from_pdata;
- u8 pad1, pad2;
+ u8 pad1, pad2, pad3;
struct palmas_pmic_platform_data *pmic_pdata;
struct palmas_gpadc_platform_data *gpadc_pdata;
@@ -436,6 +437,9 @@ enum usb_irq_events {
#define PALMAS_PWM1_MUXED (1 << 0)
#define PALMAS_PWM2_MUXED (1 << 1)
+#define PALMAS_DVFS1_MUXED (1 << 0)
+#define PALMAS_DVFS2_MUXED (1 << 1)
+
/* helper macro to get correct slave number */
#define PALMAS_BASE_TO_SLAVE(x)((x >> 8) - 1)
#define PALMAS_BASE_TO_REG(x, y) ((x & 0xff) + y)
@@ -1833,6 +1837,12 @@ enum usb_irq_events {
#define PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_4 0x01
#define PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_4_SHIFT 0
+/* Bit definitions for PRIMARY_SECONDARY_PAD3 */
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS20x02
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS2_SHIFT 1
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS10x01
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS1_SHIFT 0
+
/* Bit definitions for I2C_SPI */
#define PALMAS_I2C_SPI_I2C2OTP_EN 0x80
#define PALMAS_I2C_SPI_I2C2OTP_EN_SHIFT
[PATCH V2] gpio: palmas: Fix misreported GPIO out value
It seems that the value read back from the PALMAS_GPIO_DATA_IN register
isn't valid if the GPIO direction is out. When that's the case, we can
read back the PALMAS_GPIO_DATA_OUT register to get the proper output value.
Change-Id: Iaf877e538cfdb37a6759c45ec3c6e4ee31078709
Signed-off-by: Andrew Chew
---
V2: Fixed a warning from using test_bit with an int instead of long. Keeping
the int and just masking that bit in the raw now.
drivers/gpio/gpio-palmas.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-palmas.c b/drivers/gpio/gpio-palmas.c
index e3a4e56..723825d 100644
--- a/drivers/gpio/gpio-palmas.c
+++ b/drivers/gpio/gpio-palmas.c
@@ -43,9 +43,22 @@ static int palmas_gpio_get(struct gpio_chip *gc, unsigned
offset)
unsigned int val;
int ret;
- ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_IN, );
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_DIR, );
if (ret < 0) {
- dev_err(gc->dev, "GPIO_DATA_IN read failed, err = %d\n", ret);
+ dev_err(gc->dev, "GPIO_DATA_DIR read failed, err = %d\n", ret);
+ return ret;
+ }
+
+ if (val & (1 << offset)) {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_OUT, );
+ } else {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_IN, );
+ }
+ if (ret < 0) {
+ dev_err(gc->dev, "GPIO_DATA_IN/OUT read failed, err = %d\n",
+ ret);
return ret;
}
return !!(val & BIT(offset));
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio: palmas: Fix misreported GPIO out value
It seems that the value read back from the PALMAS_GPIO_DATA_IN register
isn't valid if the GPIO direction is out. When that's the case, we can
read back the PALMAS_GPIO_DATA_OUT register to get the proper output value.
Change-Id: Iaf877e538cfdb37a6759c45ec3c6e4ee31078709
Signed-off-by: Andrew Chew
---
drivers/gpio/gpio-palmas.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-palmas.c b/drivers/gpio/gpio-palmas.c
index e3a4e56..7606a85 100644
--- a/drivers/gpio/gpio-palmas.c
+++ b/drivers/gpio/gpio-palmas.c
@@ -43,9 +43,22 @@ static int palmas_gpio_get(struct gpio_chip *gc, unsigned
offset)
unsigned int val;
int ret;
- ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_IN, );
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_DIR, );
if (ret < 0) {
- dev_err(gc->dev, "GPIO_DATA_IN read failed, err = %d\n", ret);
+ dev_err(gc->dev, "GPIO_DATA_DIR read failed, err = %d\n", ret);
+ return ret;
+ }
+
+ if (test_bit(offset, )) {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_OUT, );
+ } else {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_IN, );
+ }
+ if (ret < 0) {
+ dev_err(gc->dev, "GPIO_DATA_IN/OUT read failed, err = %d\n",
+ ret);
return ret;
}
return !!(val & BIT(offset));
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpio: palmas: Fix misreported GPIO out value
It seems that the value read back from the PALMAS_GPIO_DATA_IN register
isn't valid if the GPIO direction is out. When that's the case, we can
read back the PALMAS_GPIO_DATA_OUT register to get the proper output value.
Change-Id: Iaf877e538cfdb37a6759c45ec3c6e4ee31078709
Signed-off-by: Andrew Chew ac...@nvidia.com
---
drivers/gpio/gpio-palmas.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-palmas.c b/drivers/gpio/gpio-palmas.c
index e3a4e56..7606a85 100644
--- a/drivers/gpio/gpio-palmas.c
+++ b/drivers/gpio/gpio-palmas.c
@@ -43,9 +43,22 @@ static int palmas_gpio_get(struct gpio_chip *gc, unsigned
offset)
unsigned int val;
int ret;
- ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_IN, val);
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_DIR, val);
if (ret 0) {
- dev_err(gc-dev, GPIO_DATA_IN read failed, err = %d\n, ret);
+ dev_err(gc-dev, GPIO_DATA_DIR read failed, err = %d\n, ret);
+ return ret;
+ }
+
+ if (test_bit(offset, val)) {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_OUT, val);
+ } else {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_IN, val);
+ }
+ if (ret 0) {
+ dev_err(gc-dev, GPIO_DATA_IN/OUT read failed, err = %d\n,
+ ret);
return ret;
}
return !!(val BIT(offset));
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2] gpio: palmas: Fix misreported GPIO out value
It seems that the value read back from the PALMAS_GPIO_DATA_IN register
isn't valid if the GPIO direction is out. When that's the case, we can
read back the PALMAS_GPIO_DATA_OUT register to get the proper output value.
Change-Id: Iaf877e538cfdb37a6759c45ec3c6e4ee31078709
Signed-off-by: Andrew Chew ac...@nvidia.com
---
V2: Fixed a warning from using test_bit with an int instead of long. Keeping
the int and just masking that bit in the raw now.
drivers/gpio/gpio-palmas.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-palmas.c b/drivers/gpio/gpio-palmas.c
index e3a4e56..723825d 100644
--- a/drivers/gpio/gpio-palmas.c
+++ b/drivers/gpio/gpio-palmas.c
@@ -43,9 +43,22 @@ static int palmas_gpio_get(struct gpio_chip *gc, unsigned
offset)
unsigned int val;
int ret;
- ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_IN, val);
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_DIR, val);
if (ret 0) {
- dev_err(gc-dev, GPIO_DATA_IN read failed, err = %d\n, ret);
+ dev_err(gc-dev, GPIO_DATA_DIR read failed, err = %d\n, ret);
+ return ret;
+ }
+
+ if (val (1 offset)) {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_OUT, val);
+ } else {
+ ret = palmas_read(palmas, PALMAS_GPIO_BASE,
+ PALMAS_GPIO_DATA_IN, val);
+ }
+ if (ret 0) {
+ dev_err(gc-dev, GPIO_DATA_IN/OUT read failed, err = %d\n,
+ ret);
return ret;
}
return !!(val BIT(offset));
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mfd: palmas: Add DVFS mux setting
There exists a PRIMARY_SECONDARY_PAD3 in the same register base as
PRIMARY_SECONDARY_PAD2, which controls the function of certain pins. Add
a property for this setting.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Documentation/devicetree/bindings/mfd/palmas.txt | 3 ++-
drivers/mfd/palmas.c | 29 ++--
include/linux/mfd/palmas.h | 12 +-
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
b/Documentation/devicetree/bindings/mfd/palmas.txt
index 892537d..a718db5 100644
--- a/Documentation/devicetree/bindings/mfd/palmas.txt
+++ b/Documentation/devicetree/bindings/mfd/palmas.txt
@@ -24,7 +24,7 @@ and also the generic series names
- interrupt-parent : The parent interrupt controller.
Optional properties:
- ti,mux-padX : set the pad register X (1-2) to the correct muxing for the
+ ti,mux-padX : set the pad register X (1-3) to the correct muxing for the
hardware, if not set will use muxing in OTP.
Example:
@@ -38,6 +38,7 @@ palmas {
ti,mux-pad1 = 0;
ti,mux-pad2 = 0;
+ ti,mux-pad3 = 0;
#address-cells = 1;
#size-cells = 0;
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index e4d1c70..b07b706 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -219,6 +219,12 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
pdata-pad2 = prop;
}
+ ret = of_property_read_u32(node, ti,mux-pad3, prop);
+ if (!ret) {
+ pdata-mux_from_pdata = 1;
+ pdata-pad3 = prop;
+ }
+
/* The default for this register is all masked */
ret = of_property_read_u32(node, ti,power-ctrl, prop);
if (!ret)
@@ -404,9 +410,28 @@ no_irq:
if (!(reg PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_7_MASK))
palmas-gpio_muxed |= PALMAS_GPIO_7_MUXED;
- dev_info(palmas-dev, Muxing GPIO %x, PWM %x, LED %x\n,
+ addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE,
+ PALMAS_PRIMARY_SECONDARY_PAD3);
+
+ if (pdata-mux_from_pdata) {
+ reg = pdata-pad3;
+ ret = regmap_write(palmas-regmap[slave], addr, reg);
+ if (ret)
+ goto err_irq;
+ } else {
+ ret = regmap_read(palmas-regmap[slave], addr, reg);
+ if (ret)
+ goto err_irq;
+ }
+
+ if (reg PALMAS_PRIMARY_SECONDARY_PAD3_DVFS1)
+ palmas-dvfs_muxed |= PALMAS_DVFS1_MUXED;
+ if (reg PALMAS_PRIMARY_SECONDARY_PAD3_DVFS2)
+ palmas-dvfs_muxed |= PALMAS_DVFS2_MUXED;
+
+ dev_info(palmas-dev, Muxing GPIO %x, PWM %x, LED %x, DVFS %x\n,
palmas-gpio_muxed, palmas-pwm_muxed,
- palmas-led_muxed);
+ palmas-led_muxed, palmas-dvfs_muxed);
reg = pdata-power_ctrl;
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 1a8dd7a..e479107 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -84,6 +84,7 @@ struct palmas {
u8 gpio_muxed;
u8 led_muxed;
u8 pwm_muxed;
+ u8 dvfs_muxed;
};
struct palmas_gpadc_platform_data {
@@ -257,7 +258,7 @@ struct palmas_platform_data {
* then the two value to load into the registers if true
*/
int mux_from_pdata;
- u8 pad1, pad2;
+ u8 pad1, pad2, pad3;
struct palmas_pmic_platform_data *pmic_pdata;
struct palmas_gpadc_platform_data *gpadc_pdata;
@@ -436,6 +437,9 @@ enum usb_irq_events {
#define PALMAS_PWM1_MUXED (1 0)
#define PALMAS_PWM2_MUXED (1 1)
+#define PALMAS_DVFS1_MUXED (1 0)
+#define PALMAS_DVFS2_MUXED (1 1)
+
/* helper macro to get correct slave number */
#define PALMAS_BASE_TO_SLAVE(x)((x 8) - 1)
#define PALMAS_BASE_TO_REG(x, y) ((x 0xff) + y)
@@ -1833,6 +1837,12 @@ enum usb_irq_events {
#define PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_4 0x01
#define PALMAS_PRIMARY_SECONDARY_PAD2_GPIO_4_SHIFT 0
+/* Bit definitions for PRIMARY_SECONDARY_PAD3 */
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS20x02
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS2_SHIFT 1
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS10x01
+#define PALMAS_PRIMARY_SECONDARY_PAD3_DVFS1_SHIFT 0
+
/* Bit definitions for I2C_SPI */
#define PALMAS_I2C_SPI_I2C2OTP_EN 0x80
#define PALMAS_I2C_SPI_I2C2OTP_EN_SHIFT7
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info
RE: [PATCH] mfd: palmas: Add DVFS mux setting
On 07/26/2013 02:41 PM, Andrew Chew wrote: There exists a PRIMARY_SECONDARY_PAD3 in the same register base as PRIMARY_SECONDARY_PAD2, which controls the function of certain pins. Add a property for this setting. How does this interact with the pinctrl driver that Laxman just sent for Palmas? https://lkml.org/lkml/2013/7/26/141 [PATCH 0/2] pinctrl: palmas: add pincontrol driver Thanks for pointing this out. Given this: +Optional properties: +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 mode. +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 mode. I think his work already encompasses what my patch is supposed to do. Abandoning this patch. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
How does this interact with the pinctrl driver that Laxman just sent for Palmas? https://lkml.org/lkml/2013/7/26/141 [PATCH 0/2] pinctrl: palmas: add pincontrol driver Thanks for pointing this out. Given this: +Optional properties: +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 mode. +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 mode. I think his work already encompasses what my patch is supposed to do. Abandoning this patch. OK, that's simple! Are the existing ti,mux-pad1/ti,mux-pad2 properties already in the binding redundant with Laxman's pinctrl driver? In linux-next (where I based my work), yes, those two properties already exist, and as far as I understand it, are redundant with Laxman's pinctrl driver. I expect those properties will go away with Laxman's pinctrl driver. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] mfd: palmas: Add DVFS mux setting
How does this interact with the pinctrl driver that Laxman just sent for Palmas? https://lkml.org/lkml/2013/7/26/141 [PATCH 0/2] pinctrl: palmas: add pincontrol driver Thanks for pointing this out. Given this: +Optional properties: +- ti,palams-enable-dvfs1: Enable DVFS1. Configure pins for DVFS1 mode. +- ti,palams-enable-dvfs2: Enable DVFS2. Configure pins for DVFS2 mode. I think his work already encompasses what my patch is supposed to do. Abandoning this patch. OK, that's simple! Are the existing ti,mux-pad1/ti,mux-pad2 properties already in the binding redundant with Laxman's pinctrl driver? In linux-next (where I based my work), yes, those two properties already exist, and as far as I understand it, are redundant with Laxman's pinctrl driver. I expect those properties will go away with Laxman's pinctrl driver. Except those properties have been there for many kernel revisions and are an ABI and hence can't be removed, although I noticed that they got renamed recently, and of course we aren't technically being strict about this quite yet... Re: the complete pinctrl driver: is anything outside the Palmas going to need to reprogram the Palmas pinctrl HW at run-time? Are the functions that can be routed to the pins just static configuration for PMIC features, or might other generic (non-Palmas) drivers use those pins for something? If not, perhaps it's be simpler to just add your ti,mux-pad3 property and be done. I can imagine other projects wanting to do runtime muxing on those pins. These pins can serve as GPIOs, or can be programmed for special functions. For my particular scenario, I just need to statically set that particular mux register (the power-on default value is not suitable for what we want to do, namely to use the GPIO_6 pin as an actual GPIO pin). If the existing ti,mux-pad1 and ti,mux-pad2 properties are to stay, in the spirit of not changing the existing ABI, then sure, we can make a case for adding the missing one (ti,mux-pad3) for completeness. In this case, if the palmas PMIC's pin configuration can be statically set at start of day, one won't even need to instantiate the palmas pinctrl driver at all, and with the addition of ti,mux-pad3, the pin configuration control will actually be complete. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] tps65090-charger: Fix AC detect
The VACG interrupt was not being enabled. Thus, interrupts were never
generated when AC status changes. In addition, interrupts were never
cleared after taking and processing the interrupt.
Added the register offset for the INTR_MASK register, since this is needed
to unmask the VACG interrupt.
Enabled the VACG interrupt in tps65090_config_charger().
Cleared interrupts after processing, in tps65090_charger_isr().
Also removed unused variable "enable" in tps65090_enable_charging(),
and fixed a typo in one of the dev_err() prints.
Signed-off-by: Andrew Chew
---
drivers/power/tps65090-charger.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index 9fbca31..a09f8c4 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -27,6 +27,7 @@
#include
#define TPS65090_REG_INTR_STS 0x00
+#define TPS65090_REG_INTR_MASK 0x02
#define TPS65090_REG_CG_CTRL0 0x04
#define TPS65090_REG_CG_CTRL1 0x05
#define TPS65090_REG_CG_CTRL2 0x06
@@ -67,8 +68,7 @@ static int tps65090_low_chrg_current(struct tps65090_charger
*charger)
return 0;
}
-static int tps65090_enable_charging(struct tps65090_charger *charger,
- uint8_t enable)
+static int tps65090_enable_charging(struct tps65090_charger *charger)
{
int ret;
uint8_t ctrl0 = 0;
@@ -84,7 +84,7 @@ static int tps65090_enable_charging(struct tps65090_charger
*charger,
ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL0,
(ctrl0 | TPS65090_CHARGER_ENABLE));
if (ret < 0) {
- dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ dev_err(charger->dev, "%s(): error writing in register 0x%x\n",
__func__, TPS65090_REG_CG_CTRL0);
return ret;
}
@@ -93,6 +93,7 @@ static int tps65090_enable_charging(struct tps65090_charger
*charger,
static int tps65090_config_charger(struct tps65090_charger *charger)
{
+ uint8_t intrmask = 0;
int ret;
if (charger->pdata->enable_low_current_chrg) {
@@ -104,6 +105,23 @@ static int tps65090_config_charger(struct tps65090_charger
*charger)
}
}
+ /* Enable the VACG interrupt for AC power detect */
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_INTR_MASK,
+ );
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_INTR_MASK);
+ return ret;
+ }
+
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_INTR_MASK,
+(intrmask | TPS65090_VACG));
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error writing in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+
return 0;
}
@@ -146,7 +164,7 @@ static irqreturn_t tps65090_charger_isr(int irq, void
*dev_id)
}
if (intrsts & TPS65090_VACG) {
- ret = tps65090_enable_charging(charger, 1);
+ ret = tps65090_enable_charging(charger);
if (ret < 0)
return IRQ_HANDLED;
charger->ac_online = 1;
@@ -154,6 +172,13 @@ static irqreturn_t tps65090_charger_isr(int irq, void
*dev_id)
charger->ac_online = 0;
}
+ /* Clear interrupts. */
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_INTR_STS, 0x00);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): Error in writing reg 0x%x\n",
+ __func__, TPS65090_REG_INTR_STS);
+ }
+
if (charger->prev_ac_online != charger->ac_online)
power_supply_changed(>ac);
@@ -270,7 +295,7 @@ static int tps65090_charger_probe(struct platform_device
*pdev)
}
if (status1 != 0) {
- ret = tps65090_enable_charging(cdata, 1);
+ ret = tps65090_enable_charging(cdata);
if (ret < 0) {
dev_err(cdata->dev, "error enabling charger\n");
goto fail_free_irq;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] tps65090-charger: Fix AC detect
The VACG interrupt was not being enabled. Thus, interrupts were never
generated when AC status changes. In addition, interrupts were never
cleared after taking and processing the interrupt.
Added the register offset for the INTR_MASK register, since this is needed
to unmask the VACG interrupt.
Enabled the VACG interrupt in tps65090_config_charger().
Cleared interrupts after processing, in tps65090_charger_isr().
Also removed unused variable enable in tps65090_enable_charging(),
and fixed a typo in one of the dev_err() prints.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
drivers/power/tps65090-charger.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index 9fbca31..a09f8c4 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -27,6 +27,7 @@
#include linux/mfd/tps65090.h
#define TPS65090_REG_INTR_STS 0x00
+#define TPS65090_REG_INTR_MASK 0x02
#define TPS65090_REG_CG_CTRL0 0x04
#define TPS65090_REG_CG_CTRL1 0x05
#define TPS65090_REG_CG_CTRL2 0x06
@@ -67,8 +68,7 @@ static int tps65090_low_chrg_current(struct tps65090_charger
*charger)
return 0;
}
-static int tps65090_enable_charging(struct tps65090_charger *charger,
- uint8_t enable)
+static int tps65090_enable_charging(struct tps65090_charger *charger)
{
int ret;
uint8_t ctrl0 = 0;
@@ -84,7 +84,7 @@ static int tps65090_enable_charging(struct tps65090_charger
*charger,
ret = tps65090_write(charger-dev-parent, TPS65090_REG_CG_CTRL0,
(ctrl0 | TPS65090_CHARGER_ENABLE));
if (ret 0) {
- dev_err(charger-dev, %s(): error reading in register 0x%x\n,
+ dev_err(charger-dev, %s(): error writing in register 0x%x\n,
__func__, TPS65090_REG_CG_CTRL0);
return ret;
}
@@ -93,6 +93,7 @@ static int tps65090_enable_charging(struct tps65090_charger
*charger,
static int tps65090_config_charger(struct tps65090_charger *charger)
{
+ uint8_t intrmask = 0;
int ret;
if (charger-pdata-enable_low_current_chrg) {
@@ -104,6 +105,23 @@ static int tps65090_config_charger(struct tps65090_charger
*charger)
}
}
+ /* Enable the VACG interrupt for AC power detect */
+ ret = tps65090_read(charger-dev-parent, TPS65090_REG_INTR_MASK,
+ intrmask);
+ if (ret 0) {
+ dev_err(charger-dev, %s(): error reading in register 0x%x\n,
+ __func__, TPS65090_REG_INTR_MASK);
+ return ret;
+ }
+
+ ret = tps65090_write(charger-dev-parent, TPS65090_REG_INTR_MASK,
+(intrmask | TPS65090_VACG));
+ if (ret 0) {
+ dev_err(charger-dev, %s(): error writing in register 0x%x\n,
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+
return 0;
}
@@ -146,7 +164,7 @@ static irqreturn_t tps65090_charger_isr(int irq, void
*dev_id)
}
if (intrsts TPS65090_VACG) {
- ret = tps65090_enable_charging(charger, 1);
+ ret = tps65090_enable_charging(charger);
if (ret 0)
return IRQ_HANDLED;
charger-ac_online = 1;
@@ -154,6 +172,13 @@ static irqreturn_t tps65090_charger_isr(int irq, void
*dev_id)
charger-ac_online = 0;
}
+ /* Clear interrupts. */
+ ret = tps65090_write(charger-dev-parent, TPS65090_REG_INTR_STS, 0x00);
+ if (ret 0) {
+ dev_err(charger-dev, %s(): Error in writing reg 0x%x\n,
+ __func__, TPS65090_REG_INTR_STS);
+ }
+
if (charger-prev_ac_online != charger-ac_online)
power_supply_changed(charger-ac);
@@ -270,7 +295,7 @@ static int tps65090_charger_probe(struct platform_device
*pdev)
}
if (status1 != 0) {
- ret = tps65090_enable_charging(cdata, 1);
+ ret = tps65090_enable_charging(cdata);
if (ret 0) {
dev_err(cdata-dev, error enabling charger\n);
goto fail_free_irq;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1 v6] pwm_bl: Add mandatory backlight enable regulator
Many backlights need to be explicitly enabled. Typically, this is done
with a GPIO. For flexibility, we generalize the enable mechanism to a
regulator.
If an enable regulator is not needed, then a dummy regulator can be given
to the backlight driver. If a GPIO is used to enable the backlight,
then a fixed regulator can be instantiated to control the GPIO.
The backlight enable regulator can be specified in the device tree node
for the backlight, or can be done with legacy board setup code in the
usual way.
Signed-off-by: Andrew Chew
Reviewed-by: Alexandre Courbot
---
Removed "pb->enable_supply = NULL;" per Alex's suggestion, since probe fails
in this case anyway, and on next probe (if probe is deferred) will assign
to pb->enable_supply before accessing it.
Updated the commit message to provide a description of the purpose of the
new mandatory regulator, and tweaked verbiage to not be device tree
specific.
I intend to follow up with patches to fix all existing machines that use the
pwm_bl driver.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 55
2 files changed, 59 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the "brightness-levels" property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = "regulator-fixed";
+regulator-name = "bl-en-supply";
+regulator-boot-on;
+gpio = <_gpio>;
+enable-active-high;
+ };
+
backlight {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ enable-supply = <_en>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f403b9d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already enabled. */
+ if (pb->enabled)
+ return;
+
+ pwm_enable(pb->pwm);
+
+ if (regulator_enable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+
+ pb->enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb->enabled)
+ return;
+
+ if (regulator_disable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+
+ pwm_disable(pb->pwm);
+
+ pb->enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
[PATCH 1/1 v6] pwm_bl: Add mandatory backlight enable regulator
Many backlights need to be explicitly enabled. Typically, this is done
with a GPIO. For flexibility, we generalize the enable mechanism to a
regulator.
If an enable regulator is not needed, then a dummy regulator can be given
to the backlight driver. If a GPIO is used to enable the backlight,
then a fixed regulator can be instantiated to control the GPIO.
The backlight enable regulator can be specified in the device tree node
for the backlight, or can be done with legacy board setup code in the
usual way.
Signed-off-by: Andrew Chew ac...@nvidia.com
Reviewed-by: Alexandre Courbot acour...@nvidia.com
---
Removed pb-enable_supply = NULL; per Alex's suggestion, since probe fails
in this case anyway, and on next probe (if probe is deferred) will assign
to pb-enable_supply before accessing it.
Updated the commit message to provide a description of the purpose of the
new mandatory regulator, and tweaked verbiage to not be device tree
specific.
I intend to follow up with patches to fix all existing machines that use the
pwm_bl driver.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 55
2 files changed, 59 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the brightness-levels property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = regulator-fixed;
+regulator-name = bl-en-supply;
+regulator-boot-on;
+gpio = some_gpio;
+enable-active-high;
+ };
+
backlight {
compatible = pwm-backlight;
pwms = pwm 0 500;
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 6;
+ enable-supply = bl_en;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f403b9d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already enabled. */
+ if (pb-enabled)
+ return;
+
+ pwm_enable(pb-pwm);
+
+ if (regulator_enable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+
+ pb-enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb-enabled)
+ return;
+
+ if (regulator_disable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+
+ pwm_disable(pb-pwm);
+
+ pb-enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle
[PATCH 1/1 v5] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
Renamed en_supply to enable_supply to match the corresponding device tree
property.
Removed unnecessary check for pb->enable_supply validity. This supply
is mandatory, and probe will fail if it is not provided.
Renamed the tracking bool from en_supply_enabled to enabled so that it's
more generic. Encapsulated pwm_enable() and pwm_disable() calls into the
enabled check so that we never unnecessarily turn on or off the pwm if
it's already been turned on or off.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 56
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the "brightness-levels" property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = "regulator-fixed";
+regulator-name = "bl-en-supply";
+regulator-boot-on;
+gpio = <_gpio>;
+enable-active-high;
+ };
+
backlight {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ enable-supply = <_en>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c517d4a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already enabled. */
+ if (pb->enabled)
+ return;
+
+ pwm_enable(pb->pwm);
+
+ if (regulator_enable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+
+ pb->enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb->enabled)
+ return;
+
+ if (regulator_disable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+
+ pwm_disable(pb->pwm);
+
+ pb->enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_enable(bl);
}
if (pb->notify_after)
@@ -145,12 +180,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
RE: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
> From: Thierry Reding [mailto:thierry.red...@avionic-design.de] > Sent: Thursday, March 07, 2013 3:27 AM > To: Alex Courbot > Cc: Andrew Chew; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable > regulator > > * PGP Signed by an unknown key > > On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote: > > On 03/07/2013 04:11 PM, Thierry Reding wrote: > > >>+ boolen_supply_enabled; > > > > > >This boolean can be dropped. As discussed in a previous thread, the > > >pwm-backlight driver shouldn't need to know about any other uses of > > >the regulator. > > > > Sorry for being obstinate - but I'm still not convinced we can get rid > > of it. I checked the regulator code, and as you mentioned in the > > previous version, calls to regulator_enable() and > > regulator_disable() *must* be balanced in this driver. > > > > Without this variable we would call regulator_enable() every time > > pwm_backlight_enable() is called (and same thing when disabling). > > Now imagine the driver is asked to set the following intensities: 5, > > 12, then 0. You would have two calls to regulator_enable() but only > > one to regulator_disable(), which would result in the enable GPIO > > remaining active even though it would be shut down. Or I missed > > something obvious. > > > > The regulator must be enabled/disabled on transitions from/to 0, and > > AFAICT there is no way for this driver to detect them. > > Yes, that's true, but I don't think it should be solved for just this one > regulator. Instead if we need to track the enable state we might as well track > it for *any* resource so that the PWM isn't enabled/disabled twice either. That makes sense, but I'm confused due to previous comments. The most obvious way to do this seems to be to have a bool track the enable state. Do you still want me to do away with this bool? I can satisfy your very last comment by keeping the bool (renaming it to something more generic) and encapsulating the pwm_enable()/pwm_disable() call within. I'll send out v5 today to show what I mean. > > >This effectively makes the regulator mandatory, so the board files > > >that use pwm-backlight need to be updated or otherwise will break. > > > > Yes. Btw, should such changes go into the same patch? This seems > > difficult to split without breaking things at some point. > > I expect that if the changes are split up then the board-setup code changes > need to be done prior to the driver change. Using the lookup tables should > make this easy because they aren't tied to the platform data and can be > added independently. The patches should probably go through the same > subsystem tree to take care of the dependencies. > > Keeping everything in one patch would work too, but it's certainly more > chaotic. Am I supposed to handle those patches? I'm concerned that I don't have hardware to test properly, but I can give it a shot if it's my responsibility. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
From: Thierry Reding [mailto:thierry.red...@avionic-design.de] Sent: Thursday, March 07, 2013 3:27 AM To: Alex Courbot Cc: Andrew Chew; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator * PGP Signed by an unknown key On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote: On 03/07/2013 04:11 PM, Thierry Reding wrote: + boolen_supply_enabled; This boolean can be dropped. As discussed in a previous thread, the pwm-backlight driver shouldn't need to know about any other uses of the regulator. Sorry for being obstinate - but I'm still not convinced we can get rid of it. I checked the regulator code, and as you mentioned in the previous version, calls to regulator_enable() and regulator_disable() *must* be balanced in this driver. Without this variable we would call regulator_enable() every time pwm_backlight_enable() is called (and same thing when disabling). Now imagine the driver is asked to set the following intensities: 5, 12, then 0. You would have two calls to regulator_enable() but only one to regulator_disable(), which would result in the enable GPIO remaining active even though it would be shut down. Or I missed something obvious. The regulator must be enabled/disabled on transitions from/to 0, and AFAICT there is no way for this driver to detect them. Yes, that's true, but I don't think it should be solved for just this one regulator. Instead if we need to track the enable state we might as well track it for *any* resource so that the PWM isn't enabled/disabled twice either. That makes sense, but I'm confused due to previous comments. The most obvious way to do this seems to be to have a bool track the enable state. Do you still want me to do away with this bool? I can satisfy your very last comment by keeping the bool (renaming it to something more generic) and encapsulating the pwm_enable()/pwm_disable() call within. I'll send out v5 today to show what I mean. This effectively makes the regulator mandatory, so the board files that use pwm-backlight need to be updated or otherwise will break. Yes. Btw, should such changes go into the same patch? This seems difficult to split without breaking things at some point. I expect that if the changes are split up then the board-setup code changes need to be done prior to the driver change. Using the lookup tables should make this easy because they aren't tied to the platform data and can be added independently. The patches should probably go through the same subsystem tree to take care of the dependencies. Keeping everything in one patch would work too, but it's certainly more chaotic. Am I supposed to handle those patches? I'm concerned that I don't have hardware to test properly, but I can give it a shot if it's my responsibility. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1 v5] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Renamed en_supply to enable_supply to match the corresponding device tree
property.
Removed unnecessary check for pb-enable_supply validity. This supply
is mandatory, and probe will fail if it is not provided.
Renamed the tracking bool from en_supply_enabled to enabled so that it's
more generic. Encapsulated pwm_enable() and pwm_disable() calls into the
enabled check so that we never unnecessarily turn on or off the pwm if
it's already been turned on or off.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 56
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the brightness-levels property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = regulator-fixed;
+regulator-name = bl-en-supply;
+regulator-boot-on;
+gpio = some_gpio;
+enable-active-high;
+ };
+
backlight {
compatible = pwm-backlight;
pwms = pwm 0 500;
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 6;
+ enable-supply = bl_en;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c517d4a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already enabled. */
+ if (pb-enabled)
+ return;
+
+ pwm_enable(pb-pwm);
+
+ if (regulator_enable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+
+ pb-enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb-enabled)
+ return;
+
+ if (regulator_disable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+
+ pwm_disable(pb-pwm);
+
+ pb-enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
- pwm_enable(pb-pwm);
+ pwm_backlight_enable(bl);
}
if (pb-notify_after)
@@ -145,12 +180,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data-max_brightness--;
}
- /*
-* TODO: Most users of this driver use a number of GPIOs to control
RE: [PATCH] clk: tegra: provide dummy cpu car ops
> From: linux-tegra-ow...@vger.kernel.org [mailto:linux-tegra- > ow...@vger.kernel.org] On Behalf Of Stephen Warren > Sent: Wednesday, March 06, 2013 3:43 PM > To: Andrew Chew > Cc: Peter De Schrijver; linux-te...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; Stephen Warren; Prashant Gaikwad; Mike > Turquette; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] clk: tegra: provide dummy cpu car ops > > On 03/06/2013 04:20 PM, Andrew Chew wrote: > >> Subject: [PATCH] clk: tegra: provide dummy cpu car ops > >> > >> tegra_boot_secondary() relies on some of the car ops. This means > >> having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. > >> Providing a dummy struct avoids this and makes adding Tegra114 clock > >> support in a bisectable way a lot easier. > >> > >> -- > >> > >> Stephen, > >> > >> Should this be a separate patch or should I make this part of new > >> release of the Tegra114 clock series? > > I'm not sure if I answered this. Peter, I intend to apply this patch to a > branch > right before the CCF, so there's no explicit need to include it in the series, > although if you do, that's fine. > > >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index > > >> /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops > >> *tegra_cpu_car_ops; > > > > Sorry for bringing this up so late... > > Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? > > > >> +static struct tegra_cpu_car_ops *dummy_car_ops; struct > >> +tegra_cpu_car_ops *tegra_cpu_car_ops = _car_ops; > > No, the value is used as a pointer in include/linux/clk/tegra.h, e.g.: > > tegra_cpu_car_ops->wait_for_reset(cpu); Yeah, I get that tegra_cpu_car_ops is a pointer to an ops table. It seems to me that what's happening above is that tegra_cpu_car_ops is getting assigned a pointer to a pointer that's supposed to point to an instance of struct tegra_cpu_car_ops (but it really points to NULL as far as I can tell). In any case, dummy_car_ops never actually gets instantiated. I assume the intention is for dummy_car_ops to be an instance of struct tegra_cpu_car_ops, but with all of its members zero'd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] clk: tegra: provide dummy cpu car ops
> Subject: [PATCH] clk: tegra: provide dummy cpu car ops > > tegra_boot_secondary() relies on some of the car ops. This means having an > uninitialized tegra_cpu_car_ops will lead to an early boot panic. > Providing a dummy struct avoids this and makes adding Tegra114 clock > support in a bisectable way a lot easier. > > -- > > Stephen, > > Should this be a separate patch or should I make this part of new release of > the Tegra114 clock series? > > Signed-off-by: Peter De Schrijver > --- > drivers/clk/tegra/clk.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index > a603b9a..f6c141f 100644 > --- a/drivers/clk/tegra/clk.c > +++ b/drivers/clk/tegra/clk.c > @@ -22,7 +22,8 @@ > #include "clk.h" > > /* Global data of Tegra CPU CAR ops */ > -struct tegra_cpu_car_ops *tegra_cpu_car_ops; Sorry for bringing this up so late... Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? > +static struct tegra_cpu_car_ops *dummy_car_ops; struct > +tegra_cpu_car_ops *tegra_cpu_car_ops = _car_ops; > > void __init tegra_init_dup_clks(struct tegra_clk_duplicate *dup_list, > struct clk *clks[], int clk_max) > -- > 1.7.7.rc0.72.g4b5ea.dirty > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the > body of a message to majord...@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
Changed the name of the property from en-supply to enable-supply.
Made enable-supply a mandatory property. Changed the example in the
bindings documentation accordingly.
Moved devm_regulator_get() to the probe function. Regulator is now
requested unconditionally.
Changed IS_ERR_OR_NULL to IS_ERR per Alex's recommendation.
.../bindings/video/backlight/pwm-backlight.txt | 14 ++
drivers/video/backlight/pwm_bl.c | 52
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the "brightness-levels" property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = "regulator-fixed";
+regulator-name = "bl-en-supply";
+regulator-boot-on;
+gpio = <_gpio>;
+enable-active-high;
+ };
+
backlight {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ enable-supply = <_en>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..ff98cdd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ pwm_enable(pb->pwm);
+
+ if (pb->en_supply && !pb->en_supply_enabled) {
+ if (regulator_enable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+ else
+ pb->en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ if (pb->en_supply && pb->en_supply_enabled) {
+ if (regulator_disable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+ else
+ pb->en_supply_enabled = false;
+ }
+
+ pwm_disable(pb->pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -52,7 +83,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_enable(bl);
}
if (pb->notify_after)
@@ -145,12 +176,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
- /*
-* TODO: Most users of this driver use a number of
[PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Changed the name of the property from en-supply to enable-supply.
Made enable-supply a mandatory property. Changed the example in the
bindings documentation accordingly.
Moved devm_regulator_get() to the probe function. Regulator is now
requested unconditionally.
Changed IS_ERR_OR_NULL to IS_ERR per Alex's recommendation.
.../bindings/video/backlight/pwm-backlight.txt | 14 ++
drivers/video/backlight/pwm_bl.c | 52
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the brightness-levels property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = regulator-fixed;
+regulator-name = bl-en-supply;
+regulator-boot-on;
+gpio = some_gpio;
+enable-active-high;
+ };
+
backlight {
compatible = pwm-backlight;
pwms = pwm 0 500;
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 6;
+ enable-supply = bl_en;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..ff98cdd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ pwm_enable(pb-pwm);
+
+ if (pb-en_supply !pb-en_supply_enabled) {
+ if (regulator_enable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+ else
+ pb-en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ if (pb-en_supply pb-en_supply_enabled) {
+ if (regulator_disable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+ else
+ pb-en_supply_enabled = false;
+ }
+
+ pwm_disable(pb-pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -52,7 +83,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
- pwm_enable(pb-pwm);
+ pwm_backlight_enable(bl);
}
if (pb-notify_after)
@@ -145,12 +176,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data-max_brightness--;
}
- /*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added
RE: [PATCH] clk: tegra: provide dummy cpu car ops
Subject: [PATCH] clk: tegra: provide dummy cpu car ops tegra_boot_secondary() relies on some of the car ops. This means having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. Providing a dummy struct avoids this and makes adding Tegra114 clock support in a bisectable way a lot easier. -- Stephen, Should this be a separate patch or should I make this part of new release of the Tegra114 clock series? Signed-off-by: Peter De Schrijver pdeschrij...@nvidia.com --- drivers/clk/tegra/clk.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index a603b9a..f6c141f 100644 --- a/drivers/clk/tegra/clk.c +++ b/drivers/clk/tegra/clk.c @@ -22,7 +22,8 @@ #include clk.h /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops *tegra_cpu_car_ops; Sorry for bringing this up so late... Shouldn't the above be struct tegra_cpu_car_ops tegra_cpu_car_ops;? +static struct tegra_cpu_car_ops *dummy_car_ops; struct +tegra_cpu_car_ops *tegra_cpu_car_ops = dummy_car_ops; void __init tegra_init_dup_clks(struct tegra_clk_duplicate *dup_list, struct clk *clks[], int clk_max) -- 1.7.7.rc0.72.g4b5ea.dirty -- To unsubscribe from this list: send the line unsubscribe linux-tegra in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] clk: tegra: provide dummy cpu car ops
From: linux-tegra-ow...@vger.kernel.org [mailto:linux-tegra- ow...@vger.kernel.org] On Behalf Of Stephen Warren Sent: Wednesday, March 06, 2013 3:43 PM To: Andrew Chew Cc: Peter De Schrijver; linux-te...@vger.kernel.org; linux-arm- ker...@lists.infradead.org; Stephen Warren; Prashant Gaikwad; Mike Turquette; linux-kernel@vger.kernel.org Subject: Re: [PATCH] clk: tegra: provide dummy cpu car ops On 03/06/2013 04:20 PM, Andrew Chew wrote: Subject: [PATCH] clk: tegra: provide dummy cpu car ops tegra_boot_secondary() relies on some of the car ops. This means having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. Providing a dummy struct avoids this and makes adding Tegra114 clock support in a bisectable way a lot easier. -- Stephen, Should this be a separate patch or should I make this part of new release of the Tegra114 clock series? I'm not sure if I answered this. Peter, I intend to apply this patch to a branch right before the CCF, so there's no explicit need to include it in the series, although if you do, that's fine. diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops *tegra_cpu_car_ops; Sorry for bringing this up so late... Shouldn't the above be struct tegra_cpu_car_ops tegra_cpu_car_ops;? +static struct tegra_cpu_car_ops *dummy_car_ops; struct +tegra_cpu_car_ops *tegra_cpu_car_ops = dummy_car_ops; No, the value is used as a pointer in include/linux/clk/tegra.h, e.g.: tegra_cpu_car_ops-wait_for_reset(cpu); Yeah, I get that tegra_cpu_car_ops is a pointer to an ops table. It seems to me that what's happening above is that tegra_cpu_car_ops is getting assigned a pointer to a pointer that's supposed to point to an instance of struct tegra_cpu_car_ops (but it really points to NULL as far as I can tell). In any case, dummy_car_ops never actually gets instantiated. I assume the intention is for dummy_car_ops to be an instance of struct tegra_cpu_car_ops, but with all of its members zero'd. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
Thanks, Alex! Makes sense to me. There's one comment I'm not sure about,
though, described inline.
> On 03/06/2013 08:51 AM, Andrew Chew wrote:
> > The backlight enable regulator is specified in the device tree node
> > for backlight.
> >
> > Signed-off-by: Andrew Chew
> > ---
> > Applied all recommendations from Thierry Reding and Alex Courbot,
> > including making pwm_bl take an optional regulator instead of a GPIO,
> > which solves the platform data issue (platform data will default the
> > regulator to NULL, which is the right thing).
> >
> > .../bindings/video/backlight/pwm-backlight.txt |1 +
> > drivers/video/backlight/pwm_bl.c | 53
> > +---
> > include/linux/pwm_backlight.h |1 +
> > 3 files changed, 48 insertions(+), 7 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > index 1e4fc72..e0bccd30 100644
> > ---
> > a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.
> > +++ txt
> > @@ -14,6 +14,7 @@ Required properties:
> > Optional properties:
> > - pwm-names: a list of names for the PWM devices specified in the
> > "pwms" property (see PWM binding[0])
> > + - en-supply: phandle to the regulator device tree node
>
> You may want to specify what this regulator does - namely, that is enables
> the BL. May I also suggest to rename it to "enable-supply" since the other
> properties do not use abbreviations.
>
> > [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index 069983c..c4da5e2 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -20,10 +20,13 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > struct pwm_bl_data {
> > struct pwm_device *pwm;
> > struct device *dev;
> > + struct regulator*en_supply;
> > + boolen_supply_enabled;
>
> Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled?
> It would also ensure the driver performs correctly no matter what the initial
> state of the regulator is.
Are you sure this works? I'm concerned about the (bizarre and unlikely) case
where this supply is shared with another driver, so I use en_supply_enabled
to track the state of the supply such that I can ignore that case.
> > unsigned intperiod;
> > unsigned intlth_brightness;
> > unsigned int*levels;
> > @@ -35,6 +38,34 @@ struct pwm_bl_data {
> > void(*exit)(struct device *);
> > };
> >
> > +static void pwm_backlight_enable(struct backlight_device *bl) {
> > + struct pwm_bl_data *pb = dev_get_drvdata(>dev);
> > +
> > + pwm_enable(pb->pwm);
> > +
> > + if (pb->en_supply && !pb->en_supply_enabled) {
> > + if (regulator_enable(pb->en_supply) != 0)
> > + dev_warn(>dev, "Failed to enable regulator");
> > + else
> > + pb->en_supply_enabled = true;
> > + }
> > +}
> > +
> > +static void pwm_backlight_disable(struct backlight_device *bl) {
> > + struct pwm_bl_data *pb = dev_get_drvdata(>dev);
> > +
> > + if (pb->en_supply && pb->en_supply_enabled) {
> > + if (regulator_disable(pb->en_supply) != 0)
> > + dev_warn(>dev, "Failed to disable regulator");
> > + else
> > + pb->en_supply_enabled = false;
> > + }
> > +
> > + pwm_disable(pb->pwm);
> > +}
> > +
> > static int pwm_backlight_update_status(struct backlight_device *bl)
> > {
> > struct pwm_bl_data *pb = dev_get_drvdata(>dev); @@ -52,7
> +83,7
> > @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> >
> > if (brightness == 0) {
> > pwm_config(pb->pwm, 0, pb->period);
> > - pwm_disable(pb->pwm);
> > + pwm_backlight_disable(bl);
> > } else {
> > int duty_cycle;
> >
> > @@ -66,7 +97,7 @@ static int pwm_backlight_
[PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
Applied all recommendations from Thierry Reding and Alex Courbot, including
making pwm_bl take an optional regulator instead of a GPIO, which solves
the platform data issue (platform data will default the regulator to NULL,
which is the right thing).
.../bindings/video/backlight/pwm-backlight.txt |1 +
drivers/video/backlight/pwm_bl.c | 53 +---
include/linux/pwm_backlight.h |1 +
3 files changed, 48 insertions(+), 7 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..e0bccd30 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,7 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+ - en-supply: phandle to the regulator device tree node
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c4da5e2 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ pwm_enable(pb->pwm);
+
+ if (pb->en_supply && !pb->en_supply_enabled) {
+ if (regulator_enable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+ else
+ pb->en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ if (pb->en_supply && pb->en_supply_enabled) {
+ if (regulator_disable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+ else
+ pb->en_supply_enabled = false;
+ }
+
+ pwm_disable(pb->pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -52,7 +83,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_enable(bl);
}
if (pb->notify_after)
@@ -146,10 +177,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If "en-supply" is present, use that regulator to enable the
+* backlight. If a GPIO is used to enable the backlight, make
+* a fixed regulator with that particular GPIO and use that
+* regulator for "en-supply".
*/
+ data->en_supply = devm_regulator_get(dev, "en");
+ if (IS_ERR_OR_NULL(data->en_supply)) {
+ ret = PTR_ERR(data->en_supply);
+ data->en_supply = NULL;
+ return ret;
+ }
return 0;
}
@@ -207,6 +245,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
} else
max = data->max_brightness;
+ pb->en_supply = data->en_supply;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
@@ -268,7 +307,7 @@ static int pwm_backlight_remove(struct platform_device
*pdev)
bac
[PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Applied all recommendations from Thierry Reding and Alex Courbot, including
making pwm_bl take an optional regulator instead of a GPIO, which solves
the platform data issue (platform data will default the regulator to NULL,
which is the right thing).
.../bindings/video/backlight/pwm-backlight.txt |1 +
drivers/video/backlight/pwm_bl.c | 53 +---
include/linux/pwm_backlight.h |1 +
3 files changed, 48 insertions(+), 7 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..e0bccd30 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,7 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+ - en-supply: phandle to the regulator device tree node
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c4da5e2 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ pwm_enable(pb-pwm);
+
+ if (pb-en_supply !pb-en_supply_enabled) {
+ if (regulator_enable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+ else
+ pb-en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ if (pb-en_supply pb-en_supply_enabled) {
+ if (regulator_disable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+ else
+ pb-en_supply_enabled = false;
+ }
+
+ pwm_disable(pb-pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -52,7 +83,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
- pwm_enable(pb-pwm);
+ pwm_backlight_enable(bl);
}
if (pb-notify_after)
@@ -146,10 +177,17 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If en-supply is present, use that regulator to enable the
+* backlight. If a GPIO is used to enable the backlight, make
+* a fixed regulator with that particular GPIO and use that
+* regulator for en-supply.
*/
+ data-en_supply = devm_regulator_get(dev, en);
+ if (IS_ERR_OR_NULL(data-en_supply)) {
+ ret = PTR_ERR(data-en_supply);
+ data-en_supply = NULL;
+ return ret;
+ }
return 0;
}
@@ -207,6 +245,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
} else
max = data-max_brightness;
+ pb-en_supply = data-en_supply;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
@@ -268,7 +307,7 @@ static int pwm_backlight_remove(struct platform_device
*pdev)
backlight_device_unregister(bl);
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl
RE: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
Thanks, Alex! Makes sense to me. There's one comment I'm not sure about,
though, described inline.
On 03/06/2013 08:51 AM, Andrew Chew wrote:
The backlight enable regulator is specified in the device tree node
for backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Applied all recommendations from Thierry Reding and Alex Courbot,
including making pwm_bl take an optional regulator instead of a GPIO,
which solves the platform data issue (platform data will default the
regulator to NULL, which is the right thing).
.../bindings/video/backlight/pwm-backlight.txt |1 +
drivers/video/backlight/pwm_bl.c | 53
+---
include/linux/pwm_backlight.h |1 +
3 files changed, 48 insertions(+), 7 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..e0bccd30 100644
---
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-
backlight.
+++ txt
@@ -14,6 +14,7 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+ - en-supply: phandle to the regulator device tree node
You may want to specify what this regulator does - namely, that is enables
the BL. May I also suggest to rename it to enable-supply since the other
properties do not use abbreviations.
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
index 069983c..c4da5e2 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled?
It would also ensure the driver performs correctly no matter what the initial
state of the regulator is.
Are you sure this works? I'm concerned about the (bizarre and unlikely) case
where this supply is shared with another driver, so I use en_supply_enabled
to track the state of the supply such that I can ignore that case.
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,34 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl) {
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ pwm_enable(pb-pwm);
+
+ if (pb-en_supply !pb-en_supply_enabled) {
+ if (regulator_enable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+ else
+ pb-en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl) {
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ if (pb-en_supply pb-en_supply_enabled) {
+ if (regulator_disable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+ else
+ pb-en_supply_enabled = false;
+ }
+
+ pwm_disable(pb-pwm);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev); @@ -52,7
+83,7
@@ static int pwm_backlight_update_status(struct backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +97,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
- pwm_enable(pb-pwm);
+ pwm_backlight_enable(bl);
}
if (pb-notify_after)
@@ -146,10 +177,17 @@ static int pwm_backlight_parse_dt(struct device
*dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If en-supply is present, use that regulator to enable the
+* backlight. If a GPIO is used to enable the backlight, make
+* a fixed regulator with that particular GPIO and use that
+* regulator for en-supply
RE: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
I sent out a new patch that enables/disables the backlight enable gpio. > On 03/05/2013 01:00 PM, Andrew Chew wrote: > > I did come to the same conclusion regarding the platform data breakage. > > I'm expecting that the use of platform data will go away, at least on > > ARM, since we are all aggressively moving what used to be in platform > > data into the device tree. Do other platforms use this driver? > > I can see at least 29 users of platform_pwm_backlight_data, all ARM with the > exception of one unicore32. I guess at least for the foreseeable future > platform data will remain. I'm not sure how to solve this, then. Any suggestions? > > I remember hearing that there is some work in progress to encapsulate > > gpios into a struct, rather than passing it around as a bare integer, > > so when that happens, we can use NULL for no-gpio, which should take > > care of the platform data issue as well. It's kind of difficult to > > work around this problem otherwise. > > Yes, actually I am doing the GPIO rework. If you are not too much in a hurry > you might want for it to happen (should not be too long now that the core > has been reworked). At the same time, GPIO descriptors will also enable the > power sequences, so if you wait even longer (or help me with it), this patch > might not even be needed at all. Of course if you want to support this > *now*, this is still the shortest path. Sadly, I do need this now, and I'd rather do it as cleanly as possible rather than maintaining a hack. The project I am working on is very pedantic. > > I agree that we should be turning on and off the backlight enable gpio > > as needed to save power. I just haven't gotten there yet. I can try > > to modify this patch if that's your preference, or I can follow up > > with a patch to add this in the very near future. > > That's ultimately for Thierry to say, but submitting a new revision makes > more sense IMHO - it is not a big change and there are other issues to > address (uninitialized GPIO in platform data) anyway. Done. > > To answer your last question, yes, this single patch does allow me to > > enable the backlight on some boards (in particular, the one I'm working > on). > > Cool - may I ask which one? All the NV boards I tried to far required more > complex sequences for their panels. This is for t114-dalmore. There may be other gpios that are needed that I'm not aware of off the top of my head. For the backlight itself, this seems to be the only one. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1 v2] pwm_bl: Add support for backlight enable GPIO
The backlight enable GPIO is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
I decided to go ahead with disabling/enabling the backlight via GPIO as
needed. Note that I named the new functions pwm_backlight_enable() and
pwm_backlight_disable() (instead of something more gpio-specific) because
I thought it would be convenient to have a generic hook for when someone
wants to add yet more stuff to be done on enable/disable.
I tested this by going into /sys/class/backlight/backlight.n and manually
adjusting the brightness, and checked the gpio state to see that it had
the appropriate value.
.../bindings/video/backlight/pwm-backlight.txt |2 +
drivers/video/backlight/pwm_bl.c | 50 ++--
include/linux/pwm_backlight.h |2 +
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+ - enable-gpio: a GPIO that needs to be used to enable the backlight
+ - enable-gpio-active-high: polarity of GPIO is active high (default is low)
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..7dd426e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ int enable_gpio;
+ unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,20 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct pwm_bl_data *pb)
+{
+ if (gpio_is_valid(pb->enable_gpio))
+ gpio_set_value(pb->enable_gpio,
+ pb->enable_gpio_flags & GPIOF_INIT_HIGH ? 1 : 0);
+}
+
+static void pwm_backlight_disable(struct pwm_bl_data *pb)
+{
+ if (gpio_is_valid(pb->enable_gpio))
+ gpio_set_value(pb->enable_gpio,
+ pb->enable_gpio_flags & GPIOF_INIT_HIGH ? 0 : 1);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -53,6 +70,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
+ pwm_backlight_disable(pb);
} else {
int duty_cycle;
@@ -67,6 +85,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
pwm_enable(pb->pwm);
+ pwm_backlight_enable(pb);
}
if (pb->notify_after)
@@ -146,10 +165,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If "enable-gpio" is present, use that GPIO to enable the backlight.
+* The presence (or not) of "enable-gpio-active-high" will determine
+* the value of the GPIO.
*/
+ data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
+ if (of_property_read_bool(node, "enable-gpio-active-high"))
+ data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+ else
+ data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
return 0;
}
@@ -207,12 +231,23 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
} else
max = data->max_brightness;
+ pb->enable_gpio = data->enable_gpio;
+ pb->enable_gpio_flags = data->enable_gpio_flags;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
pb->exit = data->exit;
pb->dev = >dev;
+ if (gpio_is_valid(pb->enable_gpio)) {
+ ret = gpio_request_one
RE: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
> From: Alex Courbot
> Sent: Monday, March 04, 2013 7:00 PM
> To: Thierry Reding
> Cc: Andrew Chew; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
>
> On 03/05/2013 07:46 AM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
> >> The backlight enable GPIO is specified in the device tree node for
> >> backlight.
> >>
> >> Signed-off-by: Andrew Chew
> >> ---
> >> .../bindings/video/backlight/pwm-backlight.txt |2 ++
> >> drivers/video/backlight/pwm_bl.c | 32
> >> +--
> -
> >> include/linux/pwm_backlight.h |2 ++
> >> 3 files changed, 32 insertions(+), 4 deletions(-)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> b/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> index 1e4fc72..1ed4f0f 100644
> >> ---
> >> a/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight
> >> +++ .txt
> >> @@ -14,6 +14,8 @@ Required properties:
> >> Optional properties:
> >> - pwm-names: a list of names for the PWM devices specified in the
> >> "pwms" property (see PWM binding[0])
> >> + - enable-gpio: a GPIO that needs to be used to enable the
> >> + backlight
> >> + - enable-gpio-active-high: polarity of GPIO is active high
> >> + (default is low)
> >>
> >> [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> >>
> >> diff --git a/drivers/video/backlight/pwm_bl.c
> >> b/drivers/video/backlight/pwm_bl.c
> >> index 069983c..f29f9c7 100644
> >> --- a/drivers/video/backlight/pwm_bl.c
> >> +++ b/drivers/video/backlight/pwm_bl.c
> >> @@ -20,10 +20,13 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> struct pwm_bl_data {
> >>struct pwm_device *pwm;
> >>struct device *dev;
> >> + int enable_gpio;
> >> + unsigned intenable_gpio_flags;
> >>unsigned intperiod;
> >>unsigned intlth_brightness;
> >>unsigned int*levels;
> >> @@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
> >>}
> >>
> >>/*
> >> - * TODO: Most users of this driver use a number of GPIOs to control
> >> - * backlight power. Support for specifying these needs to be
> >> - * added.
> >> + * If "enable-gpio" is present, use that GPIO to enable the backlight.
> >> + * The presence (or not) of "enable-gpio-active-high" will determine
> >> + * the value of the GPIO.
> >> */
> >> + data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
> >> + if (of_property_read_bool(node, "enable-gpio-active-high"))
> >> + data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
> >> + else
> >> + data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
> >>
> >>return 0;
> >> }
> >> @@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct
> platform_device *pdev)
> >>} else
> >>max = data->max_brightness;
> >>
> >> + pb->enable_gpio = data->enable_gpio;
> >> + pb->enable_gpio_flags = data->enable_gpio_flags;
> >>pb->notify = data->notify;
> >>pb->notify_after = data->notify_after;
> >>pb->check_fb = data->check_fb;
> >>pb->exit = data->exit;
> >>pb->dev = >dev;
> >>
> >> + if (gpio_is_valid(pb->enable_gpio)) {
> >> + ret = gpio_request_one(pb->enable_gpio,
> >> + GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
> >> + if (ret) {
> >> + dev_err(>dev, "failed to allocate bl_en gpio");
> >> + goto err_alloc;
> >> + }
> >> + }
> >> +
> >>pb->pwm = devm_pwm_get(>dev, NULL);
> >>if (IS_ERR(pb->pwm)) {
> >>dev_err(>dev, "unable t
[PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
The backlight enable GPIO is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
.../bindings/video/backlight/pwm-backlight.txt |2 ++
drivers/video/backlight/pwm_bl.c | 32 +---
include/linux/pwm_backlight.h |2 ++
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+ - enable-gpio: a GPIO that needs to be used to enable the backlight
+ - enable-gpio-active-high: polarity of GPIO is active high (default is low)
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ int enable_gpio;
+ unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If "enable-gpio" is present, use that GPIO to enable the backlight.
+* The presence (or not) of "enable-gpio-active-high" will determine
+* the value of the GPIO.
*/
+ data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
+ if (of_property_read_bool(node, "enable-gpio-active-high"))
+ data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+ else
+ data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
return 0;
}
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
} else
max = data->max_brightness;
+ pb->enable_gpio = data->enable_gpio;
+ pb->enable_gpio_flags = data->enable_gpio_flags;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
pb->exit = data->exit;
pb->dev = >dev;
+ if (gpio_is_valid(pb->enable_gpio)) {
+ ret = gpio_request_one(pb->enable_gpio,
+ GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
+ if (ret) {
+ dev_err(>dev, "failed to allocate bl_en gpio");
+ goto err_alloc;
+ }
+ }
+
pb->pwm = devm_pwm_get(>dev, NULL);
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request PWM, trying legacy
API\n");
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request legacy PWM\n");
ret = PTR_ERR(pb->pwm);
- goto err_alloc;
+ goto err_gpio;
}
}
@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;
+err_gpio:
+ if (gpio_is_valid(data->enable_gpio))
+ gpio_free(data->enable_gpio);
err_alloc:
if (data->exit)
data->exit(>dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device
*pdev)
backlight_device_unregister(bl);
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
+ if (gpio_is_valid(pb->enable_gpio))
+ gpio_free(pb->enable_gpio);
if (pb->exit)
pb->exit(>dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@
struct platform_pwm_backlight_data {
int pwm_id;
+ int enable_gpio;
+ unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_bri
[PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
The backlight enable GPIO is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
.../bindings/video/backlight/pwm-backlight.txt |2 ++
drivers/video/backlight/pwm_bl.c | 32 +---
include/linux/pwm_backlight.h |2 ++
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+ - enable-gpio: a GPIO that needs to be used to enable the backlight
+ - enable-gpio-active-high: polarity of GPIO is active high (default is low)
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/of_gpio.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ int enable_gpio;
+ unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If enable-gpio is present, use that GPIO to enable the backlight.
+* The presence (or not) of enable-gpio-active-high will determine
+* the value of the GPIO.
*/
+ data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
+ if (of_property_read_bool(node, enable-gpio-active-high))
+ data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+ else
+ data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
return 0;
}
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
} else
max = data-max_brightness;
+ pb-enable_gpio = data-enable_gpio;
+ pb-enable_gpio_flags = data-enable_gpio_flags;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
pb-exit = data-exit;
pb-dev = pdev-dev;
+ if (gpio_is_valid(pb-enable_gpio)) {
+ ret = gpio_request_one(pb-enable_gpio,
+ GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
+ if (ret) {
+ dev_err(pdev-dev, failed to allocate bl_en gpio);
+ goto err_alloc;
+ }
+ }
+
pb-pwm = devm_pwm_get(pdev-dev, NULL);
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request PWM, trying legacy
API\n);
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request legacy PWM\n);
ret = PTR_ERR(pb-pwm);
- goto err_alloc;
+ goto err_gpio;
}
}
@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;
+err_gpio:
+ if (gpio_is_valid(data-enable_gpio))
+ gpio_free(data-enable_gpio);
err_alloc:
if (data-exit)
data-exit(pdev-dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device
*pdev)
backlight_device_unregister(bl);
pwm_config(pb-pwm, 0, pb-period);
pwm_disable(pb-pwm);
+ if (gpio_is_valid(pb-enable_gpio))
+ gpio_free(pb-enable_gpio);
if (pb-exit)
pb-exit(pdev-dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@
struct platform_pwm_backlight_data {
int pwm_id;
+ int enable_gpio;
+ unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord
RE: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
From: Alex Courbot
Sent: Monday, March 04, 2013 7:00 PM
To: Thierry Reding
Cc: Andrew Chew; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
On 03/05/2013 07:46 AM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
The backlight enable GPIO is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
.../bindings/video/backlight/pwm-backlight.txt |2 ++
drivers/video/backlight/pwm_bl.c | 32
+--
-
include/linux/pwm_backlight.h |2 ++
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt
index 1e4fc72..1ed4f0f 100644
---
a/Documentation/devicetree/bindings/video/backlight/pwm-
backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-
backlight
+++ .txt
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+ - enable-gpio: a GPIO that needs to be used to enable the
+ backlight
+ - enable-gpio-active-high: polarity of GPIO is active high
+ (default is low)
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/of_gpio.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ int enable_gpio;
+ unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device
*dev,
}
/*
- * TODO: Most users of this driver use a number of GPIOs to control
- * backlight power. Support for specifying these needs to be
- * added.
+ * If enable-gpio is present, use that GPIO to enable the backlight.
+ * The presence (or not) of enable-gpio-active-high will determine
+ * the value of the GPIO.
*/
+ data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
+ if (of_property_read_bool(node, enable-gpio-active-high))
+ data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+ else
+ data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
return 0;
}
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct
platform_device *pdev)
} else
max = data-max_brightness;
+ pb-enable_gpio = data-enable_gpio;
+ pb-enable_gpio_flags = data-enable_gpio_flags;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
pb-exit = data-exit;
pb-dev = pdev-dev;
+ if (gpio_is_valid(pb-enable_gpio)) {
+ ret = gpio_request_one(pb-enable_gpio,
+ GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
+ if (ret) {
+ dev_err(pdev-dev, failed to allocate bl_en gpio);
+ goto err_alloc;
+ }
+ }
+
pb-pwm = devm_pwm_get(pdev-dev, NULL);
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request PWM, trying legacy
API\n); @@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct
platform_device *pdev)
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request legacy
PWM\n);
ret = PTR_ERR(pb-pwm);
- goto err_alloc;
+ goto err_gpio;
}
}
@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;
+err_gpio:
+ if (gpio_is_valid(data-enable_gpio))
+ gpio_free(data-enable_gpio);
err_alloc:
if (data-exit)
data-exit(pdev-dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct
platform_device *pdev)
backlight_device_unregister(bl);
pwm_config(pb-pwm, 0, pb-period);
pwm_disable(pb-pwm);
+ if (gpio_is_valid(pb-enable_gpio))
+ gpio_free(pb-enable_gpio);
if (pb-exit)
pb-exit(pdev-dev);
return 0;
diff --git a/include/linux/pwm_backlight.h
b/include/linux/pwm_backlight.h index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@
struct platform_pwm_backlight_data {
int pwm_id;
+ int
[PATCH 1/1 v2] pwm_bl: Add support for backlight enable GPIO
The backlight enable GPIO is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
I decided to go ahead with disabling/enabling the backlight via GPIO as
needed. Note that I named the new functions pwm_backlight_enable() and
pwm_backlight_disable() (instead of something more gpio-specific) because
I thought it would be convenient to have a generic hook for when someone
wants to add yet more stuff to be done on enable/disable.
I tested this by going into /sys/class/backlight/backlight.n and manually
adjusting the brightness, and checked the gpio state to see that it had
the appropriate value.
.../bindings/video/backlight/pwm-backlight.txt |2 +
drivers/video/backlight/pwm_bl.c | 50 ++--
include/linux/pwm_backlight.h |2 +
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+ - enable-gpio: a GPIO that needs to be used to enable the backlight
+ - enable-gpio-active-high: polarity of GPIO is active high (default is low)
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..7dd426e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/of_gpio.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ int enable_gpio;
+ unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,20 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct pwm_bl_data *pb)
+{
+ if (gpio_is_valid(pb-enable_gpio))
+ gpio_set_value(pb-enable_gpio,
+ pb-enable_gpio_flags GPIOF_INIT_HIGH ? 1 : 0);
+}
+
+static void pwm_backlight_disable(struct pwm_bl_data *pb)
+{
+ if (gpio_is_valid(pb-enable_gpio))
+ gpio_set_value(pb-enable_gpio,
+ pb-enable_gpio_flags GPIOF_INIT_HIGH ? 0 : 1);
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -53,6 +70,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
pwm_disable(pb-pwm);
+ pwm_backlight_disable(pb);
} else {
int duty_cycle;
@@ -67,6 +85,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
pwm_enable(pb-pwm);
+ pwm_backlight_enable(pb);
}
if (pb-notify_after)
@@ -146,10 +165,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power. Support for specifying these needs to be
-* added.
+* If enable-gpio is present, use that GPIO to enable the backlight.
+* The presence (or not) of enable-gpio-active-high will determine
+* the value of the GPIO.
*/
+ data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
+ if (of_property_read_bool(node, enable-gpio-active-high))
+ data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+ else
+ data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
return 0;
}
@@ -207,12 +231,23 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
} else
max = data-max_brightness;
+ pb-enable_gpio = data-enable_gpio;
+ pb-enable_gpio_flags = data-enable_gpio_flags;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
pb-exit = data-exit;
pb-dev = pdev-dev;
+ if (gpio_is_valid(pb-enable_gpio)) {
+ ret = gpio_request_one(pb-enable_gpio,
+ GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
+ if (ret
RE: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
I sent out a new patch that enables/disables the backlight enable gpio. On 03/05/2013 01:00 PM, Andrew Chew wrote: I did come to the same conclusion regarding the platform data breakage. I'm expecting that the use of platform data will go away, at least on ARM, since we are all aggressively moving what used to be in platform data into the device tree. Do other platforms use this driver? I can see at least 29 users of platform_pwm_backlight_data, all ARM with the exception of one unicore32. I guess at least for the foreseeable future platform data will remain. I'm not sure how to solve this, then. Any suggestions? I remember hearing that there is some work in progress to encapsulate gpios into a struct, rather than passing it around as a bare integer, so when that happens, we can use NULL for no-gpio, which should take care of the platform data issue as well. It's kind of difficult to work around this problem otherwise. Yes, actually I am doing the GPIO rework. If you are not too much in a hurry you might want for it to happen (should not be too long now that the core has been reworked). At the same time, GPIO descriptors will also enable the power sequences, so if you wait even longer (or help me with it), this patch might not even be needed at all. Of course if you want to support this *now*, this is still the shortest path. Sadly, I do need this now, and I'd rather do it as cleanly as possible rather than maintaining a hack. The project I am working on is very pedantic. I agree that we should be turning on and off the backlight enable gpio as needed to save power. I just haven't gotten there yet. I can try to modify this patch if that's your preference, or I can follow up with a patch to add this in the very near future. That's ultimately for Thierry to say, but submitting a new revision makes more sense IMHO - it is not a big change and there are other issues to address (uninitialized GPIO in platform data) anyway. Done. To answer your last question, yes, this single patch does allow me to enable the backlight on some boards (in particular, the one I'm working on). Cool - may I ask which one? All the NV boards I tried to far required more complex sequences for their panels. This is for t114-dalmore. There may be other gpios that are needed that I'm not aware of off the top of my head. For the backlight itself, this seems to be the only one. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
