Re: [PATCH 2/3] pwm: Add PWM driver for OMAP using dual-mode timers

[Thierry Reding]

On Mon, Nov 02, 2015 at 12:14:21PM +0100, Neil Armstrong wrote:
> Adds support for using a OMAP dual-mode timer with PWM capability
> as a Linux PWM device. The driver controls the timer by using the
> dmtimer API.
> 
> Add a platform_data structure for each pwm-omap-dmtimer nodes containing
> the dmtimers functions in order to get driver not rely on platform
> specific functions.
> 
> Cc: Grant Erickson 
> Cc: NeilBrown 
> Cc: Joachim Eastwood 
> Suggested-by: Tony Lindgren 
> Signed-off-by: Neil Armstrong 
> ---
>  .../devicetree/bindings/pwm/pwm-omap-dmtimer.txt   |  18 ++
>  drivers/pwm/Kconfig|   9 +
>  drivers/pwm/Makefile   |   1 +
>  drivers/pwm/pwm-omap-dmtimer.c | 322 
> +
>  include/linux/platform_data/pwm_omap_dmtimer.h |  69 +
>  5 files changed, 419 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-omap-dmtimer.txt
>  create mode 100644 drivers/pwm/pwm-omap-dmtimer.c
>  create mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

I've applied this with some coding style bikeshedding applied. Also I
think there's a timer leak in the probe function:

> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
[...]
> +static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> +{
[...]
> + dm_timer = pdata->request_by_node(timer);
> + if (!dm_timer)
> + return -EPROBE_DEFER;

dm_timer holds the requested timer now.

> +
> + omap = devm_kzalloc(>dev, sizeof(*omap), GFP_KERNEL);
> + if (!omap)
> + return -ENOMEM;

But it's not released when this allocation fails...

> +
> + omap->pdata = pdata;
> + omap->dm_timer = dm_timer;
> + omap->dm_timer_pdev = of_find_device_by_node(timer);
> + if (!omap->dm_timer_pdev) {
> + dev_err(>dev, "Unable to find timer pdev\n");
> + return -EINVAL;
> + }

... nor when this lookup fails. I've taken the liberty of adding two
calls to omap->pdata->free(dm_timer) to these error paths.

Please take a look at what's in the pwm/for-next branch to see if it
still works correctly.

Thanks,
Thierry


signature.asc
Description: PGP signature

Re: [PATCH 00/10] drivers/pci: avoid module_init in non-modular host/pci*

[Thierry Reding]

On Mon, Dec 14, 2015 at 04:33:51PM +0800, Ley Foon Tan wrote:
> On Mon, Dec 14, 2015 at 4:24 PM, Thierry Reding
> <thierry.red...@gmail.com> wrote:
> > On Mon, Dec 14, 2015 at 09:19:30AM +0100, Geert Uytterhoeven wrote:
> >> Hi Paul,
> >>
> >> On Sun, Dec 13, 2015 at 2:41 AM, Paul Gortmaker
> >> <paul.gortma...@windriver.com> wrote:
> >> > This series of commits is a slice of a larger project to ensure
> >> > people don't have dead code for module removal in non-modular
> >> > drivers.  Overall there was roughly 5k lines of dead code in the
> >> > kernel due to this.  So far we've fixed several areas, like tty,
> >> > x86, net, etc. and we continue to work on other areas.
> >> >
> >> > There are several reasons to not use module_init for code that can
> >> > never be built as a module, but the big ones are:
> >> >
> >> >  (1) it is easy to accidentally code up unused module_exit and remove 
> >> > code
> >> >  (2) it can be misleading when reading the source, thinking it can be
> >> >   modular when the Makefile and/or Kconfig prohibit it
> >> >  (3) it requires the include of the module.h header file which in turn
> >> >  includes nearly everything else.
> >> >
> >> > Here we convert some module_init() calls into device_initcall() and 
> >> > delete
> >> > any module_exit and remove code that gets orphaned in the process, for
> >> > an overall net code reduction, which is always welcome.
> >> >
> >> > The use of device_initcall ensures that the init function ordering
> >> > remains unchanged, but one could argue that PCI host code might be more
> >> > appropriate to be handled under subsys_initcall.  Fortunately we can
> >> > revisit making this extra change at a later date if desired; it does
> >> > not need to happen now, and we reduce the risk of introducing
> >> > regressions at this point in time by separating the two changes.
> >> >
> >> > Over half of the drivers changed here already explicitly disallowed any
> >> > unbind operations.  For the rest we make them the same, since there is
> >> > not really any sensible use case to unbind any built-in bus support that
> >> > I can think of.
> >>
> >> Personally, I think all of these should become tristate, so distro kernels
> >> don't have to build in PCI(e) support for all SoCs. multi_v7_defconfig 
> >> kernels
> >> are becoming too big.
> >>
> >> That does not preclude making these modules un-unloadable, though.
> >
> > Most of these can't be made tristate as-is, because they use symbols
> > that aren't exported. Many of those symbols can easily be exported, so
> > its just a matter of getting the respective patches merged. I disagree
> > with making the modules non-unloadable, though. I have a local branch
> > with changes necessary to unload the host controller driver and it
> > works just fine.
> >
> PCIe host driver that use fixup (DECLARE_PCI_FIXUP_*) can't use tristate.
> Fixup region is in kernel region and this region if not updated when
> loading a module.

Interesting, I hadn't thought about that. I suppose this means that the
module will end up containing an unused section with the fixup code. It
might be useful to add a way for that to trigger a warning at build
time.

Perhaps to fix this a mechanism could be introduced to add a table of
fixups to a host controller driver and that will get applied to all
children of the bridge. It could be problematic to cover all of the
different fixup stages, though.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH 00/10] drivers/pci: avoid module_init in non-modular host/pci*

[Thierry Reding]

On Mon, Dec 14, 2015 at 09:19:30AM +0100, Geert Uytterhoeven wrote:
> Hi Paul,
> 
> On Sun, Dec 13, 2015 at 2:41 AM, Paul Gortmaker
>  wrote:
> > This series of commits is a slice of a larger project to ensure
> > people don't have dead code for module removal in non-modular
> > drivers.  Overall there was roughly 5k lines of dead code in the
> > kernel due to this.  So far we've fixed several areas, like tty,
> > x86, net, etc. and we continue to work on other areas.
> >
> > There are several reasons to not use module_init for code that can
> > never be built as a module, but the big ones are:
> >
> >  (1) it is easy to accidentally code up unused module_exit and remove code
> >  (2) it can be misleading when reading the source, thinking it can be
> >   modular when the Makefile and/or Kconfig prohibit it
> >  (3) it requires the include of the module.h header file which in turn
> >  includes nearly everything else.
> >
> > Here we convert some module_init() calls into device_initcall() and delete
> > any module_exit and remove code that gets orphaned in the process, for
> > an overall net code reduction, which is always welcome.
> >
> > The use of device_initcall ensures that the init function ordering
> > remains unchanged, but one could argue that PCI host code might be more
> > appropriate to be handled under subsys_initcall.  Fortunately we can
> > revisit making this extra change at a later date if desired; it does
> > not need to happen now, and we reduce the risk of introducing
> > regressions at this point in time by separating the two changes.
> >
> > Over half of the drivers changed here already explicitly disallowed any
> > unbind operations.  For the rest we make them the same, since there is
> > not really any sensible use case to unbind any built-in bus support that
> > I can think of.
> 
> Personally, I think all of these should become tristate, so distro kernels
> don't have to build in PCI(e) support for all SoCs. multi_v7_defconfig kernels
> are becoming too big.
> 
> That does not preclude making these modules un-unloadable, though.

Most of these can't be made tristate as-is, because they use symbols
that aren't exported. Many of those symbols can easily be exported, so
its just a matter of getting the respective patches merged. I disagree
with making the modules non-unloadable, though. I have a local branch
with changes necessary to unload the host controller driver and it
works just fine.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] pwm: tipwmss: enable on TI DRA7x and AM437x

[Thierry Reding]

On Tue, Sep 08, 2015 at 08:44:05PM +0530, Sekhar Nori wrote:
> From: Vignesh R 
> 
> TIPWMSS is present on TI's DRA7x and
> AM437x SoCs. Enable its usage.
> 
> Instead of adding each SoC individually,
> use the more generic ARCH_OMAP2PLUS
> instead.
> 
> Signed-off-by: Vignesh R 
> Signed-off-by: Sekhar Nori 
> ---
>  drivers/pwm/Kconfig | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied, thanks. In the future, try to make use of the 78 columns. There
is no need to make lines as short as you did.

Thierry


signature.asc
Description: PGP signature

Re: [RFC/PATCH 00/11] arm: omap: counter32k rework

[Thierry Reding]

On Wed, Sep 30, 2015 at 04:49:53PM +0200, Arnd Bergmann wrote:
> On Wednesday 30 September 2015 16:42:21 Arnd Bergmann wrote:
> > 
> > TEGRA folks: the tegra_read_persistent_clock() implementation apparently
> > predates the Tegra RTC driver and I wonder if they actually do the
> > right thing in combination. Could it be that the wall time forwards
> > twice as fast as it should during resume when the RTC driver is loaded?
> > Could it be that we can simply remove  tegra_read_persistent_clock()
> > and the register_persistent_clock() infrastructure?
> > 
> 
> I found the 'sleeptime_injected' variable now, which takes care of
> forwarding the clock by the correct amount.
> 
> I also found the CLOCK_SOURCE_SUSPEND_NONSTOP flag next to it, which
> should let us use the counter32k driver to provide the correct
> time during suspend without the omap_read_persistent_clock() function.
> We should be able to just delete that code.
> 
> If we decide to also delete the tegra_read_persistent_clock()
> function, we can remove the registration too.

This was discussed a very long time ago, but I can't remember most of
the context. I found this in my inbox:

https://lkml.org/lkml/2014/11/7/605

But I don't remember if there was an outcome or if anything came of
that. I'm currently busy with some other work, but thought I'd drop
this reference here in case somebody wants to dig into it before I
get around to it.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] ARM: Remove __ref on hotplug cpu die path

[Thierry Reding]

On Mon, Sep 14, 2015 at 05:23:17PM -0700, Stephen Boyd wrote:
[...]
>  arch/arm/mach-tegra/hotplug.c| 2 +-

Acked-by: Thierry Reding <tred...@nvidia.com>


signature.asc
Description: PGP signature

Re: [PATCH 2/2] clk: Convert __clk_get_name(hw-clk) to clk_hw_get_name(hw)

[Thierry Reding]

On Wed, Aug 12, 2015 at 04:12:41PM -0700, Stephen Boyd wrote:
 Use the provider based method to get a clock's name so that we
 can get rid of the clk member in struct clk_hw one day. Mostly
 converted with the following coccinelle script.
 
 @@
 struct clk_hw *E;
 @@
 
 -__clk_get_name(E-clk)
 +clk_hw_get_name(E)
 
[...]
  drivers/clk/tegra/clk-pll.c  |  8 

Acked-by: Thierry Reding tred...@nvidia.com


signature.asc
Description: PGP signature

[PATCH] usb: dwc3: Use ASCII space in Kconfig

[Thierry Reding]

From: Thierry Reding tred...@nvidia.com

The USB_DWC3_ULPI Kconfig entry uses a UTF-8 non-breaking space (0xca20)
instead of a regular ASCII space (0x20). Commit 2e0d737fc76f (kconfig:
don't silently ignore unhandled characters) exposes this by warning
about unhandled characters.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/usb/dwc3/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 473bfaa96c30..dede32e809b6 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -13,7 +13,7 @@ if USB_DWC3
 
 config USB_DWC3_ULPI
bool Register ULPI PHY Interface
-   depends on USB_ULPI_BUS=y || USB_ULPI_BUS=USB_DWC3
+   depends on USB_ULPI_BUS=y || USB_ULPI_BUS=USB_DWC3
help
  Select this if you have ULPI type PHY attached to your DWC3
  controller.
-- 
2.4.1

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] ARM: tegra: clock: Provide y2038-safe tegra_read_persistent_clock() replacement

[Thierry Reding]

On Wed, Mar 11, 2015 at 11:24:34AM +0800, Xunlei Pang wrote:
 From: Xunlei Pang pang.xun...@linaro.org
 
 As part of addressing y2038 problem for in-kernel uses, this
 patch adds the y2038-safe tegra_read_persistent_clock64() using
 timespec64.
 
 Because we rely on some subsequent changes to convert arm multiarch
 support, tegra_read_persistent_clock() will be removed then.
 
 Signed-off-by: Xunlei Pang pang.xun...@linaro.org
 ---
  drivers/clocksource/tegra20_timer.c | 21 ++---
  1 file changed, 14 insertions(+), 7 deletions(-)

Acked-by: Thierry Reding tred...@nvidia.com


pgp8stNcxHd3k.pgp
Description: PGP signature

Re: [PATCH 6/8] ARM: time: Provide read_boot_clock64() and read_persistent_clock64()

[Thierry Reding]

On Wed, Mar 11, 2015 at 11:24:35AM +0800, Xunlei Pang wrote:
 From: Xunlei Pang pang.xun...@linaro.org
 
 As part of addressing y2038 problem for in-kernel uses, this
 patch converts read_boot_clock() to read_boot_clock64() and
 read_persistent_clock() to read_persistent_clock64() using
 timespec64 by converting clock_access_fn to use timespec64.
 
 Signed-off-by: Xunlei Pang pang.xun...@linaro.org
 ---
  arch/arm/include/asm/mach/time.h|  3 +--
  arch/arm/kernel/time.c  |  6 +++---
  arch/arm/plat-omap/counter_32k.c| 10 +-
  drivers/clocksource/tegra20_timer.c | 10 +-
  4 files changed, 6 insertions(+), 23 deletions(-)

For the Tegra part:

Acked-by: Thierry Reding tred...@nvidia.com


pgpHMuBLRbsdQ.pgp
Description: PGP signature

Re: [PATCH] treewide: Convert clockevents_notify to use int cpu

[Thierry Reding]

On Wed, Dec 10, 2014 at 03:28:53PM -0800, Joe Perches wrote:
[...]
  arch/arm/mach-tegra/cpuidle-tegra114.c |  4 ++--
  arch/arm/mach-tegra/cpuidle-tegra20.c  |  8 
  arch/arm/mach-tegra/cpuidle-tegra30.c  |  8 
[...]

Acked-by: Thierry Reding tred...@nvidia.com


pgpXYKBHwOOLK.pgp
Description: PGP signature

Re: [PATCH v2 06/21] ARM: tegra: remove old LIC support

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:41PM +, Marc Zyngier wrote:
[...]
 diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
[...]
  void __init tegra_init_irq(void)
  {
 - int i;
 - void __iomem *distbase;
 -
 - if (of_find_matching_node(NULL, tegra_ictlr_match))
 - goto skip_extn_setup;
 -
 - tegra_legacy_irq_syscore_init();
 -
 - distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
 - num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR)  0x1f;
 -
 - if (num_ictlrs  ARRAY_SIZE(ictlr_reg_base)) {
 - WARN(1, Too many (%d) interrupt controllers found. Maximum is 
 %d.,
 - num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
 - num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
 - }
 -
 - for (i = 0; i  num_ictlrs; i++) {
 - void __iomem *ictlr = ictlr_reg_base[i];
 - writel(~0, ictlr + ICTLR_CPU_IER_CLR);
 - writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
 - }
 -
 - gic_arch_extn.irq_ack = tegra_ack;
 - gic_arch_extn.irq_eoi = tegra_eoi;
 - gic_arch_extn.irq_mask = tegra_mask;
 - gic_arch_extn.irq_unmask = tegra_unmask;
 - gic_arch_extn.irq_retrigger = tegra_retrigger;
 - gic_arch_extn.irq_set_wake = tegra_set_wake;
 - gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 + if (!of_find_matching_node(NULL, tegra_ictlr_match))
 + pr_warn(Outdated DT detected, suspend/resume will NOT work\n);

I'm not very happy about the ABI breakage here, but I also realize that
we need this change to properly describe the hardware. To make it more
obvious that people really should update their DTBs, maybe turn this
into a WARN()?

 -skip_extn_setup:
   tegra114_gic_cpu_pm_registration();

I'm not intimately familiar with the GIC, but is this really SoC
specific? Doesn't anybody else need this? Comparing to the GIC spec the
write of 0x1e0 to the GIC_CPU_CTRL register (which I assume corresponds
to GICC_CTLR in the spec), this simply disables the IRQ and FIQ bypass
signals for both group 0 and group 1.

Thierry


pgp2pvsAd1Q18.pgp
Description: PGP signature

Re: [PATCH v2 01/21] ARM: tegra: irq: nuke leftovers from non-DT support

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:36PM +, Marc Zyngier wrote:
 The GIC is now always initialized from DT on tegra, and there is
 no point in keeping non-DT init code.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/mach-tegra/irq.c | 8 
  1 file changed, 8 deletions(-)

Acked-by: Thierry Reding tred...@nvidia.com


pgpfwxvzgiPdc.pgp
Description: PGP signature

Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:37PM +, Marc Zyngier wrote:
 Tegra's LIC (Legacy Interrupt Controller) has been so far only
 supported as a weird extension of the GIC, which is not exactly
 pretty.
 
 The stacked irq domain framework fits this pretty well, and allows

Nit: s/irq/IRQ/

 the LIC code to be turned into a standalone irqchip. In the process,
 make the driver DT aware, something that was sorely missing from
 the mach-tegra implementation.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  drivers/irqchip/Makefile|   1 +
  drivers/irqchip/irq-tegra.c | 335 
 
  2 files changed, 336 insertions(+)
  create mode 100644 drivers/irqchip/irq-tegra.c

This matches largely what I have in a local patch (modulo the stacked
domains vs. gic_arch_extn). A few comments below.

 diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
 index 9516a32..59f34be 100644
 --- a/drivers/irqchip/Makefile
 +++ b/drivers/irqchip/Makefile
 @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_HIP04)  += irq-hip04.o
  obj-$(CONFIG_ARCH_MMP)   += irq-mmp.o
  obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
  obj-$(CONFIG_ARCH_MXS)   += irq-mxs.o
 +obj-$(CONFIG_ARCH_TEGRA) += irq-tegra.o
  obj-$(CONFIG_ARCH_S3C24XX)   += irq-s3c24xx.o

Should these be sorted alphabetically?

 diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
[...]
 +#define TEGRA_MAX_NUM_ICTLRS 5
 +
 +static int num_ictlrs;

This could be unsigned int.

 +struct tegra_ictlr_info {
 + void __iomem *ictlr_reg_base[TEGRA_MAX_NUM_ICTLRS];

Maybe only reg_base or base. The ictlr_ prefix is redundant.

 +#ifdef CONFIG_PM_SLEEP
 + u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
 + u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
 + u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
 + u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
 +
 + u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];

Same here.

 +#endif
 +};
 +
 +static struct tegra_ictlr_info *tegra_ictlr_info;

This variable name could be shorter, say lic, to make some of the code
below more terse.

 +static int tegra_ictlr_suspend(void)
 +{
 + unsigned long flags;
 + int i;

This could be unsigned int, too.

[...]
 +static void tegra_ictlr_resume(void)
 +{
 + unsigned long flags;
 + int i;

And here.

 +static struct syscore_ops tegra_ictlr_syscore_ops = {
 + .suspend= tegra_ictlr_suspend,
 + .resume = tegra_ictlr_resume,
 +};
 +
 +static int tegra_ictlr_syscore_init(void)
 +{
 + register_syscore_ops(tegra_ictlr_syscore_ops);
 +
 + return 0;
 +}
 +#else
 +#define tegra_set_wake   NULL
 +static inline void tegra_ictlr_syscore_init(void) {}
 +#endif

Both prototypes for tegra_ictlr_syscore_init() should match here. Since
it never fails, returning void for both should be fine.

 +static struct irq_chip tegra_ictlr_chip = {
 + .name   = ICTLR,

Maybe name it LIC since that's the more common name for it?

 +static int tegra_ictlr_domain_xlate(struct irq_domain *domain,
 + struct device_node *controller,
 + const u32 *intspec,
 + unsigned int intsize,
 + unsigned long *out_hwirq,
 + unsigned int *out_type)
 +{
 + if (domain-of_node != controller)
 + return -EINVAL; /* Shouldn't happen, really... */
 + if (intsize != 3)
 + return -EINVAL; /* Not GIC compliant */
 + if (intspec[0] != 0)

Maybe use GIC_SPI from dt-bindings/interrupt-controller/arm-gic.h here?
To match the DTS content?

 +static int tegra_ictlr_domain_alloc(struct irq_domain *domain,
 + unsigned int virq,
 + unsigned int nr_irqs, void *data)
 +{
 + struct of_phandle_args *args = data;
 + struct of_phandle_args parent_args;
 + struct tegra_ictlr_info *info = domain-host_data;
 + irq_hw_number_t hwirq;
 + int i;

unsigned int?

 +
 + if (args-args_count != 3)
 + return -EINVAL; /* Not GIC compliant */
 + if (args-args[0] != 0)

GIC_SPI?

 + hwirq = args-args[1];
 + if (hwirq = (num_ictlrs * 32))
 + return -EINVAL; /* Can't deal with this */

Not sure if that comment is necessary here. It's fairly obvious why this
is an error. But it doesn't hurt, so if you prefer to leave it, that's
fine with me.

 +
 + for (i = 0; i  nr_irqs; i++) {
 + int ictlr = (hwirq + i) / 32;

irq_hw_number_t? Or unsigned int?

 +
 + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
 +   tegra_ictlr_chip,
 +   info-ictlr_reg_base[ictlr]);
 + }
 +
 + parent_args = *args;
 + parent_args.np = domain-parent-of_node;
 + return irq_domain_alloc_irqs_parent(domain, 

Re: [PATCH v2 03/21] ARM: tegra: skip gic_arch_extn setup if DT has a LIC node

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:38PM +, Marc Zyngier wrote:
 If we detect that our DT has a LIC node, don't setup gic_arch_extn,
 and skip tegra_legacy_irq_syscore_init as well.
 
 This is only a temporary measure until that code is removed for good.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/mach-tegra/irq.c   | 11 +++
  arch/arm/mach-tegra/tegra.c |  1 -
  2 files changed, 11 insertions(+), 1 deletion(-)
 
 diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
 index 7f87a50..b37141d 100644
 --- a/arch/arm/mach-tegra/irq.c
 +++ b/arch/arm/mach-tegra/irq.c
 @@ -255,11 +255,21 @@ static void tegra114_gic_cpu_pm_registration(void)
  static void tegra114_gic_cpu_pm_registration(void) { }
  #endif
  
 +static const struct of_device_id tegra_ictlr_match[] __initconst = {
 + { .compatible = nvidia,tegra-ictlr },

Like I said elsewhere, I think this should be nvidia,tegra20-ictlr and
there should be another entry for nvidia,tegra30-ictlr.

Otherwise looks good, so with that fixed:

Acked-by: Thierry Reding tred...@nvidia.com


pgp4UqaGf3A6W.pgp
Description: PGP signature

Re: [PATCH v2 05/21] DT: tegra: add binding for the legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:40PM +, Marc Zyngier wrote:
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  .../interrupt-controller/nvidia,tegra-ictlr.txt| 39 
 ++
  1 file changed, 39 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 
 diff --git 
 a/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
  
 b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 new file mode 100644
 index 000..44fd873
 --- /dev/null
 +++ 
 b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra-ictlr.txt
 @@ -0,0 +1,39 @@
 +NVIDIA Legacy Interrupt Controller
 +
 +All Tegra SoCs contain a legacy interrupt controller that routes
 +interrupts to the GIC, and also serves as a wakeup source. It is also
 +refered to as ictlr, hence the name of the binding.
 +
 +The HW block exposes a number of frames, each implementing a set of 32
 +interrupts.

I don't think I've ever seen them referred to as frames. They are
technically separate instances of the same controller. Maybe:

The HW block exposes a number of controllers, ...

?

 +
 +Reguired properties:
 +
 +- compatible : should contain at least nvidia,tegra-ictlr.

As previously discussed I think this should be something along the lines
of:

should be one of:
- nvidia,tegra20-ictlr
- nvidia,tegra30-ictlr

Or similar. In the past, we've used nvidia,tegrachip-foo to wildcard
the compatible string so that we don't have to modify the documentation
for every new chip. The above has the disadvantage that it omits that we
should always provide a most specific compatible string, too, so maybe
something like the following would be even better:

should be: nvidia,tegrachip-ictlr. The LIC on subsequent
SoCs remained backwards-compatible with Tegra30, so on Tegra
generations later than Tegra30 the compatible value should
include nvidia,tegra30-ictlr.

 +- reg : Specifies base physical address and size of the registers.
 +  Each frame must be described separately.

Each controller must be described separately.? Also maybe mention that
this Tegra20 has 4 of them, whereas Tegra30 and later have 5? That way
people will know how many entries are required.

Thierry


pgpyrY_2LuHFm.pgp
Description: PGP signature

Re: [PATCH v2 04/21] ARM: tegra: update DTs to expose legacy interrupt controller

[Thierry Reding]

On Wed, Jan 07, 2015 at 05:42:39PM +, Marc Zyngier wrote:
 Describe the legacy interrupt controller in every tegra DTSI files,
 and make it the parent of most interrupts.
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/boot/dts/tegra114.dtsi | 16 +++-
  arch/arm/boot/dts/tegra124.dtsi | 16 +++-
  arch/arm/boot/dts/tegra20.dtsi  | 15 ++-
  arch/arm/boot/dts/tegra30.dtsi  | 16 +++-
  4 files changed, 59 insertions(+), 4 deletions(-)
 
 diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
 index 4296b53..f70bed0 100644
 --- a/arch/arm/boot/dts/tegra114.dtsi
 +++ b/arch/arm/boot/dts/tegra114.dtsi
 @@ -8,7 +8,7 @@
  
  / {
   compatible = nvidia,tegra114;
 - interrupt-parent = gic;
 + interrupt-parent = ictlr;

Maybe name the label lic because that's the more common name for this
controller? Same for the other DTSI files.

 @@ -134,6 +134,19 @@
 0x50046000 0x2000;
   interrupts = GIC_PPI 9
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH);
 + interrupt-parent = gic;

Is this allowed? It makes the GIC its own parent. I guess we need it to
stop a loop from GIC - LIC - GIC, but it doesn't look quite right.

 + };
 +
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra114-ictlr, nvidia,tegra-ictlr;

As previously discussed, I think the second string should be
nvidia,tegra30-ictlr.

 + reg = 0x60004000 64,

I think the first entry should be 256 bytes long since there's another
block of 192 bytes of registers that's part of the interrupt controller,
albeit maybe not related to the functionality of the interrupt chip. But
they're still part of the same hardware block.

 +   0x60004100 64,

According to the TRM there are 4 more registers in this block, so this
should be 80 in size.

 +   0x60004200 64,
 +   0x60004300 64,
 +   0x60004400 64;

I'd prefer all of these to be hexadecimal.

 + interrupt-controller;
 + #interrupt-cells = 3;
 + interrupt-parent = gic;
   };
  
   timer@60005000 {
 @@ -766,5 +779,6 @@
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW),
   GIC_PPI 10
   (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW);
 + interrupt-parent = gic;

Why does this get to have a non-default parent?

   };
  };
 diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
[...]
 @@ -190,6 +191,18 @@
   status = disabled;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra124-ictlr, nvidia,tegra-ictlr;

Same as for Tegra114.

 + reg = 0x0 0x60004000 0x0 0x40,
 +   0x0 0x60004100 0x0 0x40,
 +   0x0 0x60004200 0x0 0x40,
 +   0x0 0x60004300 0x0 0x40,
 +   0x0 0x60004400 0x0 0x40;

According to the TRM, entries 1-4 should be 0x100 bytes. Even the first
entry could be 0x100 bytes long since there are additional registers in
there. While they may not be directly relevant to the interrupt chip, I
think it would make sense to include them here because they are part of
the same hardware block.

 diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
 index 8acf5d8..ab2f004 100644
 --- a/arch/arm/boot/dts/tegra20.dtsi
 +++ b/arch/arm/boot/dts/tegra20.dtsi
 @@ -7,7 +7,7 @@
  
  / {
   compatible = nvidia,tegra20;
 - interrupt-parent = intc;
 + interrupt-parent = ictlr;

I wonder if we shouldn't rename the intc label to gic for consistency.
Could be in a follow-up patch though, and something that I can easily do
after this patch set.

  
   host1x@5000 {
   compatible = nvidia,tegra20-host1x, simple-bus;
 @@ -142,6 +142,7 @@
  
   timer@50004600 {
   compatible = arm,cortex-a9-twd-timer;
 + interrupt-parent = intc;
   reg = 0x50040600 0x20;
   interrupts = GIC_PPI 13
   (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH);
 @@ -154,6 +155,7 @@
  0x50040100 0x0100;
   interrupt-controller;
   #interrupt-cells = 3;
 + interrupt-parent = intc;
   };
  
   cache-controller@50043000 {
 @@ -165,6 +167,17 @@
   cache-level = 2;
   };
  
 + ictlr: interrupt-controller@60004000 {
 + compatible = nvidia,tegra20-ictlr, nvidia,tegra-ictlr;

As discussed previously, I think the second compatible should be
dropped.

 + reg = 0x60004000 64,
 +   0x60004100 64,
 +   0x60004200 64,
 +   0x60004300 64;

Same comments as for Tegra114, except the quinary controller which
doesn't exist on Tegra20.

 + 

Re: [PATCH] pwm: Fix period and polarity in pwm_get() for non-perfect matches

[Thierry Reding]

On Wed, Aug 13, 2014 at 05:18:53PM +0200, Geert Uytterhoeven wrote:
 If pwm_get() finds a look-up entry with a perfect match (both dev_id and
 con_id match), the loop is aborted, and p still points to the correct
 struct pwm_lookup.
 
 If only an entry with a matching dev_id or con_id is found, the loop
 terminates after traversing the whole list, and p now points to
 arbitrary memory, not part of the pwm_lookup list.
 Then pwm_set_period() and pwm_set_polarity() will set random values for
 period resp. polarity.
 
 To fix this, save period and polarity when finding a new best match,
 just like is done for chip (for the provider) and index.
 
 This fixes the LCD backlight on r8a7740/armadillo-legacy, which was fed
 period 0 and polarity -1068821144 instead of 3 resp. 1.
 
 Fixes: 3796ce1d4d4b330a75005c5eda105603ce9d4071 (pwm: add period and 
 polarity to struct pwm_lookup)
 Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
 Cc: sta...@vger.kernel.org
 ---
  drivers/pwm/core.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

Good catch! One comment below.

 diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
 index 4b66bf09ee55..d2c35920ff08 100644
 --- a/drivers/pwm/core.c
 +++ b/drivers/pwm/core.c
 @@ -606,6 +606,8 @@ struct pwm_device *pwm_get(struct device *dev, const char 
 *con_id)
   unsigned int best = 0;
   struct pwm_lookup *p;
   unsigned int match;
 + unsigned int period;
 + enum pwm_polarity polarity;
  
   /* look up via DT first */
   if (IS_ENABLED(CONFIG_OF)  dev  dev-of_node)
 @@ -653,6 +655,8 @@ struct pwm_device *pwm_get(struct device *dev, const char 
 *con_id)
   if (match  best) {
   chip = pwmchip_find_by_name(p-provider);
   index = p-index;
 + period = p-period;
 + polarity = p-polarity;
  
   if (match != 3)
   best = match;
 @@ -668,8 +672,8 @@ struct pwm_device *pwm_get(struct device *dev, const char 
 *con_id)
   if (IS_ERR(pwm))
   return pwm;
  
 - pwm_set_period(pwm, p-period);
 - pwm_set_polarity(pwm, p-polarity);
 + pwm_set_period(pwm, period);
 + pwm_set_polarity(pwm, polarity);

Could we achieve the same by storing a pointer to the best match and
then use that instead of p? Perhaps something like this:

struct pwm_lookup *entry;

...

if (match  best) {
chip = pwmchip_find_by_name(p-provider);
entry = p;

if (match != 3)
best = match;
else
break;
}

...

if (chip)
pwm = pwm_request_from_chip(chip, entry-index,
con_id ?: dev_id);
if (IS_ERR(pwm))
return pwm;

pwm_set_period(pwm, entry-period);
pwm_set_polarity(pwm, entry-polarity);

?

Thierry


pgp1Olv9Qi_Zr.pgp
Description: PGP signature

Re: [PATCH] pwm: Fix period and polarity in pwm_get() for non-perfect matches

[Thierry Reding]

On Mon, Aug 18, 2014 at 10:38:00AM +0200, Geert Uytterhoeven wrote:
 Hi Thierry,
 
 On Mon, Aug 18, 2014 at 10:20 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  Could we achieve the same by storing a pointer to the best match and
  then use that instead of p? Perhaps something like this:
 
  struct pwm_lookup *entry;
 
  ...
 
  if (match  best) {
  chip = pwmchip_find_by_name(p-provider);
  entry = p;
 
  if (match != 3)
  best = match;
  else
  break;
  }
 
  ...
 
  if (chip)
  pwm = pwm_request_from_chip(chip, entry-index,
  con_id ?: dev_id);
  if (IS_ERR(pwm))
  return pwm;
 
  pwm_set_period(pwm, entry-period);
  pwm_set_polarity(pwm, entry-polarity);
 
  ?
 
 That's possible. But that will add complexity, as you have to move the
 mutex_unlock(pwm_lookup_lock); after the last user of entry again,
 and add a goto for the IS_ERR(pwm) case.
 So I'm not sure it's worth the effort.

Oh, right. It would've been nice to avoid all the temporary variables,
but we can always refactor if that turns out to become too messy. I'll
apply this one for now.

I'll shorten the SHA-1 in the Fixes: line to 12 characters if you don't
mind to match the guidelines in Documentation/SubmittingPatches.

Thierry


pgpWhdonsyCnN.pgp
Description: PGP signature

Re: [PATCH] arm/arm64: DT: Fix GICv2 CPU interface size

[Thierry Reding]

On Wed, Jun 25, 2014 at 11:37:54AM +0100, Marc Zyngier wrote:
 All the Cortex-{A7,A15} implementations are using a GICv2. Same for
 the current arm64 platforms.
 
 Turns out that most of these platforms have described their GIC CPU
 interface size as being 4kB. while it is actually 8kB (the GICC_DIR
 register lives at offset 0x1000).
 
 This was found when converting the GIC driver to use EOImode==1 on
 GICv2-based systems. It uses the GICC_DIR register, and the result
 is a very early firework...
 
 Signed-off-by: Marc Zyngier marc.zyng...@arm.com
 ---
  arch/arm/boot/dts/axm55xx.dtsi  | 2 +-
  arch/arm/boot/dts/dra7.dtsi | 2 +-
  arch/arm/boot/dts/ecx-2000.dts  | 2 +-
  arch/arm/boot/dts/exynos3250.dtsi   | 2 +-
  arch/arm/boot/dts/exynos5.dtsi  | 2 +-
  arch/arm/boot/dts/exynos5260.dtsi   | 2 +-
  arch/arm/boot/dts/exynos5410.dtsi   | 2 +-
  arch/arm/boot/dts/exynos5440.dtsi   | 2 +-
  arch/arm/boot/dts/omap5.dtsi| 2 +-
  arch/arm/boot/dts/r8a73a4.dtsi  | 2 +-
  arch/arm/boot/dts/r8a7790.dtsi  | 2 +-
  arch/arm/boot/dts/r8a7791.dtsi  | 2 +-
  arch/arm/boot/dts/sun6i-a31.dtsi| 2 +-
  arch/arm/boot/dts/sun7i-a20.dtsi| 2 +-
  arch/arm/boot/dts/tegra114.dtsi | 2 +-
  arch/arm/boot/dts/tegra124.dtsi | 2 +-
  arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts | 2 +-
  arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts  | 2 +-
  arch/arm64/boot/dts/apm-storm.dtsi  | 2 +-
  arch/arm64/boot/dts/foundation-v8.dts   | 2 +-
  arch/arm64/boot/dts/rtsm_ve-aemv8a.dts  | 2 +-
  21 files changed, 21 insertions(+), 21 deletions(-)

For the Tegra114 and Tegra124 patches:

Tested-by: Thierry Reding tred...@nvidia.com
Acked-by: Thierry Reding tred...@nvidia.com


pgpWE1cQD5evb.pgp
Description: PGP signature

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Thierry Reding]

On Mon, Jun 02, 2014 at 04:14:23PM -0700, Tony Lindgren wrote:
 * Tony Lindgren t...@atomide.com [140602 16:06]:
  * Linus Walleij linus.wall...@linaro.org [140425 00:53]:
   On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
   thierry.red...@gmail.com wrote:
   
From: Thierry Reding tred...@nvidia.com
   
Many bindings use the -gpio suffix in property names. Support this in
addition to the -gpios suffix when requesting GPIOs using the new
descriptor-based API.
   
Signed-off-by: Thierry Reding tred...@nvidia.com
   
   It appears this can save quite a lot of code in drivers, work that
   I trust Thierry to persue based on this to some extent so patch is
   tentatively applied unless something comes up.
  
  Looks like this patch causes a regression where GPIOs on I2C will
  no longer return -EPROBE_DEFER but seem to return -ENOENT instead.
  
  This breaks drivers using things like devm_gpiod_get_index()
  on a GPIO that's on a I2C bus not probed yet.
  
  Reverting commit dd34c37aa3e (gpio: of: Allow -gpio suffix for
  property names) fixes things.
 
 Looks like something like below fixes the issue.
 
 Regards,
 
 Tony
 
 8 ---
 From: Tony Lindgren t...@atomide.com
 Date: Mon, 2 Jun 2014 16:13:46 -0700
 Subject: [PATCH] gpio: of: Fix handling for deferred probe for -gpio suffix
 
 Commit dd34c37aa3e (gpio: of: Allow -gpio suffix for property names)
 added parsing for both -gpio and -gpios suffix but also changed
 the handling for deferred probe unintentionally. Because of the
 looping the second name will now return -ENOENT instead of
 -EPROBE_DEFER. Fix the issue by breaking out of the loop if
 -EPROBE_DEFER is encountered.
 
 Signed-off-by: Tony Lindgren t...@atomide.com
 
 --- a/drivers/gpio/gpiolib.c
 +++ b/drivers/gpio/gpiolib.c
 @@ -2614,7 +2614,7 @@ static struct gpio_desc *of_find_gpio(struct device 
 *dev, const char *con_id,
  
   desc = of_get_named_gpiod_flags(dev-of_node, prop_name, idx,
   of_flags);
 - if (!IS_ERR(desc))
 + if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
   break;
   }

This looks good to me:

Reviewed-by: Thierry Reding tred...@nvidia.com


pgpM5lu5mabwx.pgp
Description: PGP signature

Re: [PATCHv2 resend 00/11] improve PWM lookup support without device tree

[Thierry Reding]

On Mon, May 19, 2014 at 10:42:31PM +0200, Alexandre Belloni wrote:
 Hi,
 
 Originally sent on Apr 14th, note that this series is blocking another 16
 patches series, I would like it to be taken in 3.16 if we can agree on this
 implementation.
 
 A patch set as suggested by Thierry to make lookup with the lookup table
 instead of device tree behave more like when using device tree.
 
 The first patch adds a period and a polarity member to the lookup table and 
 use
 those to set period and polarity.
 
 Patch 2, 4 and 5 are making use of those new members from the board files.
 Patch 3 removes useless code since setting the polarity is now handled by the
 PWM core.
 
 I couldn't decide on a good name for the extended PWM_LOOKUP macro and I 
 believe
 we won't have to add members to that structure soon so:
 Patch 6 modifies the PWM_LOOKUP macro to also initialize period and polarity
 and
 Patch 7-9 are making use of the new PWM_LOOKUP macro in the board files
 
 Patch 10 and 11 are making the leds-pwm and pwm_bl drivers get the period from
 the PWM before using pwm_period_ns if it is not already set.
 
 Patch 10 will obviously conflict with the series of Russell reworking the
 leds-pwm probing. I can rebase if necessary
 
 The final goal would be to get rid of .pwm_period_ns in leds-pwm and pwm_bl
 after moving all the remaining users (still around 25) to pwm_lookup.
 
 Changes in v2:
  - correctly unlock the pwm_lookup_lock mutex before returning.
  - don't change PWM_LOOKUP atomically
  - remove tpu_pwm_platform_data and the associated header file
  - make the leds-pwm and pwm_bl drivers get the period from the PWM
 
 Alexandre Belloni (11):
   pwm: add period and polarity to struct pwm_lookup
   ARM: shmobile: Armadillo 800 EVA: initialize all struct pwm_lookup
 members
   pwm: renesas-tpu: remove useless struct tpu_pwm_platform_data
   ARM: OMAP3: Beagle: initialize all the struct pwm_lookup members
   ARM: pxa: hx4700: initialize all the struct pwm_lookup members
   pwm: modify PWM_LOOKUP to initialize all struct pwm_lookup members
   ARM: OMAP3: Beagle: use PWM_LOOKUP to initialize struct pwm_lookup
   ARM: shmobile: Armadillo 800 EVA: use PWM_LOOKUP to initialize struct
 pwm_lookup
   ARM: pxa: hx4700: use PWM_LOOKUP to initialize struct pwm_lookup
   leds: leds-pwm: retrieve configured pwm period
   backlight: pwm_bl: retrieve configured pwm period
 
  Documentation/pwm.txt  |  3 ++-
  arch/arm/mach-omap2/board-omap3beagle.c|  3 ++-
  arch/arm/mach-pxa/hx4700.c |  3 ++-
  arch/arm/mach-shmobile/board-armadillo800eva.c | 14 +++---
  drivers/leds/leds-pwm.c|  5 -
  drivers/pwm/core.c |  8 +++-
  drivers/pwm/pwm-renesas-tpu.c  | 19 +++
  drivers/video/backlight/pwm_bl.c   |  8 +---
  include/linux/platform_data/pwm-renesas-tpu.h  | 16 
  include/linux/pwm.h|  6 +-
  10 files changed, 33 insertions(+), 52 deletions(-)
  delete mode 100644 include/linux/platform_data/pwm-renesas-tpu.h

I've applied this whole series with some minor fixups (mostly adding
detail to commit messages). Test builds show no breakage, so I've pushed
this to the for-next branch.

Thanks,
Thierry


pgpFs_eAIYPI_.pgp
Description: PGP signature

Re: [PATCHv2 resend 00/11] improve PWM lookup support without device tree

[Thierry Reding]

On Mon, May 19, 2014 at 10:42:31PM +0200, Alexandre Belloni wrote:
 Hi,
 
 Originally sent on Apr 14th, note that this series is blocking another 16
 patches series, I would like it to be taken in 3.16 if we can agree on this
 implementation.

It's kind of late for 3.16 now, but it doesn't look very risky from a
quick look. I'll sleep on it and if I don't feel too uncomfortable about
it in the morning I'll apply it for linux-next and see how that goes.

Thierry


pgpCh3ixGuR6u.pgp
Description: PGP signature

Re: [PATCH v3 5/6] pwm: enable TI PWMSS if the IIO tiecap driver is selected

[Thierry Reding]

On Wed, Feb 05, 2014 at 02:01:40PM -0500, Matt Porter wrote:
 The IIO TI ECAP driver depends on the TI PWMSS management
 driver in this subsystem. Enable PWMSS when the IIO TI ECAP
 driver is selected.
 
 Signed-off-by: Matt Porter mpor...@linaro.org
 ---
  drivers/pwm/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 index 22f2f28..bd3cc65 100644
 --- a/drivers/pwm/Kconfig
 +++ b/drivers/pwm/Kconfig
 @@ -219,7 +219,7 @@ config  PWM_TIEHRPWM
  
  config  PWM_TIPWMSS
   bool
 - default y if SOC_AM33XX  (PWM_TIECAP || PWM_TIEHRPWM)
 + default y if SOC_AM33XX  (IIO_TIECAP || PWM_TIECAP || PWM_TIEHRPWM)
   help
 PWM Subsystem driver support for AM33xx SOC.

Perhaps this module should move out of drivers/pwm if it's no longer a
PWM specific module.

Thierry


pgpdSQvc3sF6U.pgp
Description: PGP signature

Re: [PATCH 1/3] pwm: core: Rearrange pwm lock.

[Thierry Reding]

On Wed, Dec 18, 2013 at 05:06:53PM +0530, Sourav Poddar wrote:
 When tiecap is used as a module, then while doing a rmmod I
 get the following dump.
 
 root@am437x-evm:/# rmmod pwm_tiecap
 [  219.539245]
 [  219.540771] ==
 [  219.546936] [ INFO: possible circular locking dependency detected ]
 [  219.553192] 3.12.4-01557-g9921cde-dirty #134 Not tainted
 [  219.558471] ---
 [  219.564727] rmmod/1517 is trying to acquire lock:
 [  219.569427]  (s_active#35){.+}, at: [c017ab00] 
 sysfs_hash_and_remove+0x4c/0x8c
 [  219.577239]
 [  219.577239] but task is already holding lock:
 [  219.583068]  (pwm_lock){+.+.+.}, at: [c0303598] pwmchip_remove+0x14/0xf8
 [  219.589996]
 [  219.589996] which lock already depends on the new lock.
 [  219.589996]
 [  219.598144]
 [  219.598144] the existing dependency chain (in reverse order) is:
 [  219.605590]
 - #1 (pwm_lock){+.+.+.}:
 [  219.609497][c00a2d1c] lock_acquire+0x9c/0x128
 [  219.614746][c0639bc0] mutex_lock_nested+0x50/0x3dc
 [  219.620391][c0303974] pwm_request_from_chip+0x38/0x6c
 [  219.626312][c0303fe0] pwm_export_store+0x50/0x140
 [  219.631896][c039aba8] dev_attr_store+0x18/0x24
 [  219.637207][c017aff0] sysfs_write_file+0x16c/0x1a0
 [  219.642883][c0119084] vfs_write+0xb0/0x188
 [  219.647857][c0119478] SyS_write+0x3c/0x70
 [  219.652770][c0014100] ret_fast_syscall+0x0/0x48
 [  219.658172]
 - #0 (s_active#35){.+}:
 [  219.662353][c00a2778] __lock_acquire+0x1b28/0x1b70
 [  219.667999][c00a2d1c] lock_acquire+0x9c/0x128
 [  219.673248][c017c780] sysfs_addrm_finish+0xe8/0x158
 [  219.678985][c017ab00] sysfs_hash_and_remove+0x4c/0x8c
 [  219.684906][c017e224] remove_files+0x38/0x74
 [  219.690063][c017e2a4] sysfs_remove_group+0x44/0x108
 [  219.695800][c017e38c] sysfs_remove_groups+0x24/0x34
 [  219.701538][c039bc2c] device_del+0xec/0x178
 [  219.706604][c039bcc4] device_unregister+0xc/0x18
 [  219.712097][c0303658] pwmchip_remove+0xd4/0xf8
 [  219.717407][c039fdc4] platform_drv_remove+0x18/0x1c
 [  219.723175][c039e6c4] __device_release_driver+0x70/0xc8
 [  219.729248][c039eec8] driver_detach+0xb4/0xb8
 [  219.734497][c039e4ec] bus_remove_driver+0x8c/0xd0
 [  219.740081][c00abd2c] SyS_delete_module+0x118/0x22c
 [  219.745819][c0014100] ret_fast_syscall+0x0/0x48
 [  219.751220]
 [  219.751220] other info that might help us debug this:
 [  219.751220]
 [  219.759216]  Possible unsafe locking scenario:
 [  219.759216]
 [  219.765106]CPU0CPU1
 [  219.769622]
 [  219.774139]   lock(pwm_lock);
 [  219.777130]lock(s_active#35);
 [  219.782897]lock(pwm_lock);
 [  219.788391]   lock(s_active#35);
 [  219.791656]
 [  219.791656]  *** DEADLOCK ***
 [  219.791656]
 [  219.797546] 3 locks held by rmmod/1517:
 [  219.801391]  #0:  (__lockdep_no_validate__){..}, at: [c039ee58] 
 driver_detach+0x44/0xb8
 [  219.810028]  #1:  (__lockdep_no_validate__){..}, at: [c039ee64] 
 driver_detach+0x50/0xb8
 [  219.818695]  #2:  (pwm_lock){+.+.+.}, at: [c0303598] 
 pwmchip_remove+0x14/0xf8
 [  219.826049]
 [  219.826049] stack backtrace:
 [  219.830413] CPU: 0 PID: 1517 Comm: rmmod Not tainted 
 3.12.4-01557-g9921cde-dirty #134
 [  219.838256] [c001cc98] (unwind_backtrace+0x0/0xf0) from [c0018124] 
 (show_stack+0x10/0x14)
 [  219.846771] [c0018124] (show_stack+0x10/0x14) from [c0636728] 
 (dump_stack+0x74/0xb4)
 [  219.854858] [c0636728] (dump_stack+0x74/0xb4) from [c06344e4] 
 (print_circular_bug+0x284/0x2d8)
 [  219.863830] [c06344e4] (print_circular_bug+0x284/0x2d8) from 
 [c00a2778] (__lock_acquire+0x1b28/0x1b70)
 [  219.873443] [c00a2778] (__lock_acquire+0x1b28/0x1b70) from [c00a2d1c] 
 (lock_acquire+0x9c/0x128)
 [  219.882476] [c00a2d1c] (lock_acquire+0x9c/0x128) from [c017c780] 
 (sysfs_addrm_finish+0xe8/0x158)
 [  219.891601] [c017c780] (sysfs_addrm_finish+0xe8/0x158) from [c017ab00] 
 (sysfs_hash_and_remove+0x4c/0x8c)
 [  219.901397] [c017ab00] (sysfs_hash_and_remove+0x4c/0x8c) from 
 [c017e224] (remove_files+0x38/0x74)
 [  219.910614] [c017e224] (remove_files+0x38/0x74) from [c017e2a4] 
 (sysfs_remove_group+0x44/0x108)
 [  219.919647] [c017e2a4] (sysfs_remove_group+0x44/0x108) from [c017e38c] 
 (sysfs_remove_groups+0x24/0x34)
 [  219.929260] [c017e38c] (sysfs_remove_groups+0x24/0x34) from [c039bc2c] 
 (device_del+0xec/0x178)
 [  219.938201] [c039bc2c] (device_del+0xec/0x178) from [c039bcc4] 
 (device_unregister+0xc/0x18)
 [  219.946899] [c039bcc4] (device_unregister+0xc/0x18) from [c0303658] 
 (pwmchip_remove+0xd4/0xf8)
 [  219.955841] [c0303658] (pwmchip_remove+0xd4/0xf8) from [c039fdc4] 
 (platform_drv_remove+0x18/0x1c)
 [  219.965057] 

Re: [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call.

[Thierry Reding]

On Wed, Dec 18, 2013 at 05:06:55PM +0530, Sourav Poddar wrote:
 Writing to ecap register on second insmod crashes with an external
 abort. This happens becuase the STOP_CLK bit remains set(from rmmod) 
 during the second insmod thereby not allowing the clocks to get enabled.
 
 So, we disable STOP clock bit while doing a clock enable.
 
 Signed-off-by: Sourav Poddar sourav.pod...@ti.com
 ---
  drivers/pwm/pwm-tipwmss.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/pwm/pwm-tipwmss.c b/drivers/pwm/pwm-tipwmss.c
 index 3b119bc..4749866 100644
 --- a/drivers/pwm/pwm-tipwmss.c
 +++ b/drivers/pwm/pwm-tipwmss.c
 @@ -40,6 +40,8 @@ u16 pwmss_submodule_state_change(struct device *dev, int 
 set)
  
   mutex_lock(info-pwmss_lock);
   val = readw(info-mmio_base + PWMSS_CLKCONFIG);
 + if (set == PWMSS_ECAPCLK_EN)
 + val = ~PWMSS_ECAPCLK_STOP_REQ;

Should this be done for set == PWMSS_EPWMCLK_EN as well? Also how does
this behave if somebody goes and passes:

PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ

as the set parameter.

I think that perhaps the pwmss_submodule_state_change() API should be
rethought. Instead of taking a value that's directly written into the
register, perhaps it should abstract away what this does.

From my understanding this is used to enable (or disable) the clock for
a specific submodule (ECAP or EHRPWM). Perhaps an interface like the
following would be more intuitive:

bool pwmss_module_enable(struct device *dev, enum pwmss_module module)
{
struct pwmss_info *info = dev_get_drvdata(dev);
u16 value, mask, state, ack;

switch (module) {
case PWMSS_MODULE_ECAP:
mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
state = PWMSS_ECAPCLK_EN;
ack = PWMSS_ECAPCLK_EN_ACK;
break;

case PWMSS_MODULE_EPWM:
mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
state = PWMSS_EPWMCLK_EN;
ack = PWMSS_ECAPCLK_EN_ACK;
break;

default:
return false;
}

mutex_lock(info-pwmss_lock);
value = readw(info-mmio + PWMSS_CLKCONFIG);
value = ~mask;
value |= state;
writew(value, info-mmio + PWMSS_CLKCONFIG);
mutex_unlock(info-pwmss_lock);

value = readw(info-mmio + PWMSS_CLKSTATUS);
return (value  ack) != 0;
}

void pwmss_module_disable(struct device *dev, enum pwmss_module module)
{
struct pwmss_info *info = dev_get_drvdata(dev);
u16 value, mask, state;

switch (module) {
case PWMSS_MODULE_ECAP:
mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
state = PWMSS_ECAPCLK_STOP_REQ;
break;

case PWMSS_MODULE_EPWM:
mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
state = PWMSS_EPWMCLK_STOP_REQ;
break;

default:
return false;
}

mutex_lock(info-pwmss_lock);
value = readw(info-mmio + PWMSS_CLKCONFIG);
value = ~mask;
value |= state;
writew(value, info-mmio + PWMSS_CLKCONFIG);
mutex_unlock(info-pwmss_lock);
}

One possible other interesting alternative would be to export this
functionality via the common clock framework, since you're essentially
exposing clk_enable() and clk_disable(). That's probably overkill,
though.

Thierry


pgpBsLMscUhj5.pgp
Description: PGP signature

Re: [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call.

[Thierry Reding]

On Wed, Dec 18, 2013 at 05:06:54PM +0530, Sourav Poddar wrote:
 Remove duplicate 'pm_runtime_put_sync' in the remove path.
 
 Signed-off-by: Sourav Poddar sourav.pod...@ti.com
 ---
  drivers/pwm/pwm-tiecap.c |1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

Applied, thanks.

Thierry


pgpSBPgNnxvs5.pgp
Description: PGP signature

[PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

When devices are probed from the device tree, any interrupts that they
reference are resolved at device creation time. This causes problems if
the interrupt provider hasn't been registered yet at that time, which
results in the interrupt being set to 0.

This is especially bad for platform devices because they are created at
a very early stage during boot when the majority of interrupt providers
haven't had a chance to be probed yet. One of the platform where this
causes major issues is OMAP.

Note that this patch is the easy way out to fix a large part of the
problems for now. A more proper solution for the long term would be to
transition drivers to an API that always resolves resources of any kind
(not only interrupts) at probe time.

For some background and discussion on possible solutions, see:

https://lkml.org/lkml/2013/11/22/520

Acked-by: Rob Herring robherri...@gmail.com
Signed-off-by: Thierry Reding tred...@nvidia.com
---
Note that this is somewhat urgent and should if at all possible go into
v3.13 before the release.

 drivers/base/platform.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b799f166..c894d1af3a5e 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -13,6 +13,7 @@
 #include linux/string.h
 #include linux/platform_device.h
 #include linux/of_device.h
+#include linux/of_irq.h
 #include linux/module.h
 #include linux/init.h
 #include linux/dma-mapping.h
@@ -87,7 +88,12 @@ int platform_get_irq(struct platform_device *dev, unsigned 
int num)
return -ENXIO;
return dev-archdata.irqs[num];
 #else
-   struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+   struct resource *r;
+
+   if (IS_ENABLED(CONFIG_OF)  dev-dev.of_node)
+   return irq_of_parse_and_map(dev-dev.of_node, num);
+
+   r = platform_get_resource(dev, IORESOURCE_IRQ, num);
 
return r ? r-start : -ENXIO;
 #endif
-- 
1.8.4.2

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

On Wed, Jan 08, 2014 at 02:41:19PM +0100, Arnd Bergmann wrote:
 On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote:
  When devices are probed from the device tree, any interrupts that they
  reference are resolved at device creation time. This causes problems if
  the interrupt provider hasn't been registered yet at that time, which
  results in the interrupt being set to 0.
 
 Thanks for looking at this problem, it has bothered a lot of people
 for a long time. I'm sorry I wasn't there for the discussion in November,
 but when it came up before, I suggested a different solution that
 apparently didn't get implemented.
 
  Note that this patch is the easy way out to fix a large part of the
  problems for now. A more proper solution for the long term would be to
  transition drivers to an API that always resolves resources of any kind
  (not only interrupts) at probe time.
  
  For some background and discussion on possible solutions, see:
  
  https://lkml.org/lkml/2013/11/22/520
 
 I hope I read this thread correctly, sorry if I missed an important
 part. My idea was to add the code not in platform_get_irq() but add
 the resource in platform_drv_probe(), and just bail out with
 -EPROBE_DEFER there if necessary.

One of the reasons we can't do that just yet is that we don't actually
get back an accurate error code from the OF IRQ helpers. Therefore if we
want to support deferred probing we either have to return -EPROBE_DEFER
for all errors (which is a bad idea because some errors just can't be
resolved by deferral) or we change the OF IRQ functions to propagate the
error code properly so that we know exactly when -EPROBE_DEFER makes
sense (i.e. the IRQ domain for an interrupt-parent hasn't been
registered yet).

Actually I posted a round of patches quite a while back that did exactly
this for interrupts. The changes were somewhat involved because it means
propagating an error code from fairly deep down in the OF code back to
the various higher-level helpers. If you're interested, the last version
of that is here:

https://lkml.org/lkml/2013/9/18/216

Grant in particular seemed to be uncomfortable about how invasive the
patch series is.

Perhaps I should step back for a minute and provide some background on
how the initial idea came about. This was first triggered by the IOMMU
probe ordering issue that you may have heard about. One of the reasons
to do this for interrupts was because they are an existing type of
resource yet suffer from a very similar problem, so I wanted to solve
the issue for interrupts and thereby get a better overview of what
needed to be done for IOMMUs.

The most logical place for this code to reside seemed to be in the probe
path of platform devices (or I2C and other devices for that matter). The
patch series introduced an of_platform_probe() function that's called
from platform_drv_probe(). This would automatically give us deferred
probe support provided that we could propagate the appropriate error
code while at the same time giving us some flexibility to hook up other
resource types (such as IOMMUs).

One problem with the IOMMU patches is that they've received some strong
pushback from both Greg and Grant, arguing that it doesn't belong in the
core. If you want to read up on that, here's a link to the latest
version:

https://lkml.org/lkml/2013/12/12/74

Some things had been discussed in earlier iterations of that series, but
this should give you the basic idea.

It stands to reason that if they push back on the IOMMU variant of what
is essentially the same thing, they will push back on the IRQ variant as
well. One alternative I proposed was to, just as you suggested earlier,
move the code into platform_drv_probe() or rather a function called from
it. That proposal never got any replies, though.

https://lkml.org/lkml/2013/12/14/39

 We could then skip adding the resources at device creation time.
 Is this something you already plan to do later, or is there a reason
 it wouldn't work?

The current thread here suggests that it would be preferable not to have
any static allocations at all, but rather introduce a new API to resolve
things at probe time if necessary. I think the idea was to have a set of
functions like:

int device_get_irq(struct device *dev, unsigned int num);
struct resource *device_get_mem_region(struct device *dev,
   unsigned int num);

Or even possible the more generic:

struct resource *device_get_resource(struct device *dev,
 unsigned int type,
 unsigned int num);

If every driver used these functions, all resources could trivially be
resolved at probe time. That solution is also the most invasive one of
course, because it requires all existing drivers to be converted. At
least the API would be all new and therefore the conversion could happen
piecemeal.

One downside

Re: [PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote:
 On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote:
  On Wed, Jan 08, 2014 at 02:41:19PM +0100, Arnd Bergmann wrote:
   On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote:
[...]
  Actually I posted a round of patches quite a while back that did exactly
  this for interrupts. The changes were somewhat involved because it means
  propagating an error code from fairly deep down in the OF code back to
  the various higher-level helpers. If you're interested, the last version
  of that is here:
  
  https://lkml.org/lkml/2013/9/18/216
  
  Grant in particular seemed to be uncomfortable about how invasive the
  patch series is.
 
 Interesting. It seems like a worthwhile thing to do, but I can understand
 Grant's reluctance.

To be fair, Grant didn't say outright no. Given how easily this could
turn into a regression nightmare I do understand the reluctance as well.
Merging things piece by piece would make it somewhat less risky but at
the same time makes it hard to keep at it.

  It stands to reason that if they push back on the IOMMU variant of what
  is essentially the same thing, they will push back on the IRQ variant as
  well. One alternative I proposed was to, just as you suggested earlier,
  move the code into platform_drv_probe() or rather a function called from
  it. That proposal never got any replies, though.
  
  https://lkml.org/lkml/2013/12/14/39
 
 I guess putting it into the platform_drv_probe function seems reasonable,
 I would be more scared of the implications of a notifier based method.

I fully agree. Of course if we decide against moving things into the
core and in favour of a more generic API that drivers should use, then
this issue goes away silently at least for resources that the driver
needs to use explicitly (memory-mapped regions, interrupts, ...).

The issue remains for IOMMU which is meant to be used transparently
through the DMA API. Perhaps a good compromise would be to have some
sort of generic helper that can be called to initialize IOMMU support
for a particular device and support probe deferral on error. Something
like this perhaps:

int iommu_attach(struct device *dev);
int iommu_detach(struct device *dev);

I still don't like very much how that needs to be done in each driver
explicitly, but if we can't do it in the core, then the only other clean
way to handle it would be to treat it like any other sort of resource
and handle it explicitly. Perhaps handing out some sort of cookie would
be preferable to just an error code?

   We could then skip adding the resources at device creation time.
   Is this something you already plan to do later, or is there a reason
   it wouldn't work?
  
  The current thread here suggests that it would be preferable not to have
  any static allocations at all, but rather introduce a new API to resolve
  things at probe time if necessary. I think the idea was to have a set of
  functions like:
  
  int device_get_irq(struct device *dev, unsigned int num);
  struct resource *device_get_mem_region(struct device *dev,
 unsigned int num);
  
  Or even possible the more generic:
  
  struct resource *device_get_resource(struct device *dev,
   unsigned int type,
   unsigned int num);
  
  If every driver used these functions, all resources could trivially be
  resolved at probe time. That solution is also the most invasive one of
  course, because it requires all existing drivers to be converted. At
  least the API would be all new and therefore the conversion could happen
  piecemeal.
 
 Right, that does sound nice, and has the added benefit of saving
 some memory allocations. I'd prefer the less generic variant here,
 but I haven't given it much thought.

I do prefer the less generic ones as well. They seem to be more
convenient to use.

  One downside of that approach is that, while it maps well to platform
  devices or generic devices that have some sort of firmware interface
  such as OF or ACPI, I don't see how it can be made to work with an I2C
  client that's registered from board setup code for example. Well, I
  suppose that problem could be solved by throwing another lookup table at
  it, just like we do for clocks, regulators, PWMs and GPIOs.
 
 Wouldn't you still be able to attach resources in the traditional
 way for those, but use the same new interface to get at them?

I wouldn't know how. For instance platform devices store the IRQ number
within a struct resource of type IORESOURCE_IRQ, whereas I2C clients
store them in the struct i2c_client's .irq field.

So without actually introspecting the struct device (possibly using the
.bus field for example) and upcasting you won't know how to get at the
resources. One possibility to remedy that would be to try and unify the
resources within struct device

Re: [PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

On Wed, Jan 08, 2014 at 08:40:41AM -0800, Tony Lindgren wrote:
 * Thierry Reding thierry.red...@gmail.com [140108 04:55]:
  When devices are probed from the device tree, any interrupts that they
  reference are resolved at device creation time. This causes problems if
  the interrupt provider hasn't been registered yet at that time, which
  results in the interrupt being set to 0.
  
  This is especially bad for platform devices because they are created at
  a very early stage during boot when the majority of interrupt providers
  haven't had a chance to be probed yet. One of the platform where this
  causes major issues is OMAP.
  
  Note that this patch is the easy way out to fix a large part of the
  problems for now. A more proper solution for the long term would be to
  transition drivers to an API that always resolves resources of any kind
  (not only interrupts) at probe time.
  
  For some background and discussion on possible solutions, see:
  
  https://lkml.org/lkml/2013/11/22/520
  
  Acked-by: Rob Herring robherri...@gmail.com
  Signed-off-by: Thierry Reding tred...@nvidia.com
  ---
  Note that this is somewhat urgent and should if at all possible go into
  v3.13 before the release.
  
   drivers/base/platform.c | 8 +++-
   1 file changed, 7 insertions(+), 1 deletion(-)
  
  diff --git a/drivers/base/platform.c b/drivers/base/platform.c
  index 3a94b799f166..c894d1af3a5e 100644
  --- a/drivers/base/platform.c
  +++ b/drivers/base/platform.c
  @@ -13,6 +13,7 @@
   #include linux/string.h
   #include linux/platform_device.h
   #include linux/of_device.h
  +#include linux/of_irq.h
   #include linux/module.h
   #include linux/init.h
   #include linux/dma-mapping.h
  @@ -87,7 +88,12 @@ int platform_get_irq(struct platform_device *dev, 
  unsigned int num)
  return -ENXIO;
  return dev-archdata.irqs[num];
   #else
  -   struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);
  +   struct resource *r;
  +
  +   if (IS_ENABLED(CONFIG_OF)  dev-dev.of_node)
  +   return irq_of_parse_and_map(dev-dev.of_node, num);
  +
  +   r = platform_get_resource(dev, IORESOURCE_IRQ, num);
   
  return r ? r-start : -ENXIO;
   #endif
 
 Hmm actually testing this patch, it does not fix fix the $Subject bug :(
 
 irq: no irq domain found for /ocp/pinmux@48002030 !
 [0.301361] [ cut here ]
 [0.301391] WARNING: CPU: 0 PID: 1 at drivers/of/platform.c:171 
 of_device_alloc+0x144/0x184()
 [0.301422] Modules linked in:
 [0.301452] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
 3.13.0-rc7-2-g4d998a6 #17
 [0.301513] [c0015c04] (unwind_backtrace+0x0/0xf4) from [c00127b0] 
 (show_stack+0x14/0x1c)
 [0.301544] [c00127b0] (show_stack+0x14/0x1c) from [c05685a4] 
 (dump_stack+0x6c/0xa0)
 [0.301574] [c05685a4] (dump_stack+0x6c/0xa0) from [c00425b4] 
 (warn_slowpath_common+0x64/0x84)
 [0.301605] [c00425b4] (warn_slowpath_common+0x64/0x84) from 
 [c00425f0] (warn_slowpath_null+0x1c/0x24)
 [0.301635] [c00425f0] (warn_slowpath_null+0x1c/0x24) from [c0485210] 
 (of_device_alloc+0x144/0x184)
 [0.301635] [c0485210] (of_device_alloc+0x144/0x184) from [c0485294] 
 (of_platform_device_create_pdata+0x44/0x9c)
 [0.301666] [c0485294] (of_platform_device_create_pdata+0x44/0x9c) from 
 [c04853bc] (of_platform_bus_create+0xd0/0x170)
 [0.301696] [c04853bc] (of_platform_bus_create+0xd0/0x170) from 
 [c0485418] (of_platform_bus_create+0x12c/0x170)
 [0.301727] [c0485418] (of_platform_bus_create+0x12c/0x170) from 
 [c04854bc] (of_platform_populate+0x60/0x98)
 [0.301757] [c04854bc] (of_platform_populate+0x60/0x98) from 
 [c07ca53c] (pdata_quirks_init+0x28/0x78)
 [0.301788] [c07ca53c] (pdata_quirks_init+0x28/0x78) from [c07bab20] 
 (customize_machine+0x20/0x48)
 [0.301818] [c07bab20] (customize_machine+0x20/0x48) from [c000882c] 
 (do_one_initcall+0x2c/0x150)
 [0.301849] [c000882c] (do_one_initcall+0x2c/0x150) from [c07b75d8] 
 (do_basic_setup+0x94/0xd4)
 [0.301879] [c07b75d8] (do_basic_setup+0x94/0xd4) from [c07b7694] 
 (kernel_init_freeable+0x7c/0x120)
 [0.301910] [c07b7694] (kernel_init_freeable+0x7c/0x120) from 
 [c05667ec] (kernel_init+0x8/0x120)
 [0.301940] [c05667ec] (kernel_init+0x8/0x120) from [c000e908] 
 (ret_from_fork+0x14/0x2c)
 [0.302124] ---[ end trace 2b87f5de2f86f809 ]---
 ...
 
 There's nothing wrong with the interrupt related code paths, we're just
 trying to call the functions at a wrong time when thing are not yet
 initialized.

The patch won't get rid of that warning, but it should at least restore
things to a working state at runtime. At least for well-behaved drivers
that use platform_get_irq() rather than those that try to access the
resources directly.

 Below is a repost of what works for me without using notifiers. Anybody
 got any better ideas for a minimal fix?

That patch is somewhat big for something that should be a minimal fix.
Being the size that it is it might have

Re: [PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

On Wed, Jan 08, 2014 at 05:25:17PM +0100, Arnd Bergmann wrote:
 On Wednesday 08 January 2014, Thierry Reding wrote:
  On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote:
   On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote:
It stands to reason that if they push back on the IOMMU variant of what
is essentially the same thing, they will push back on the IRQ variant as
well. One alternative I proposed was to, just as you suggested earlier,
move the code into platform_drv_probe() or rather a function called from
it. That proposal never got any replies, though.

https://lkml.org/lkml/2013/12/14/39
   
   I guess putting it into the platform_drv_probe function seems reasonable,
   I would be more scared of the implications of a notifier based method.
  
  I fully agree. Of course if we decide against moving things into the
  core and in favour of a more generic API that drivers should use, then
  this issue goes away silently at least for resources that the driver
  needs to use explicitly (memory-mapped regions, interrupts, ...).
  
  The issue remains for IOMMU which is meant to be used transparently
  through the DMA API. Perhaps a good compromise would be to have some
  sort of generic helper that can be called to initialize IOMMU support
  for a particular device and support probe deferral on error. Something
  like this perhaps:
  
  int iommu_attach(struct device *dev);
  int iommu_detach(struct device *dev);
  
  I still don't like very much how that needs to be done in each driver
  explicitly, but if we can't do it in the core, then the only other clean
  way to handle it would be to treat it like any other sort of resource
  and handle it explicitly. Perhaps handing out some sort of cookie would
  be preferable to just an error code?
 
 The more I think about the iommu case, the more I am convinced that it
 does belong into the core, in whatever form we can find. As far as I
 can tell from the little reliable information I have on the topic, I
 would assume that we can keep it in the DT probing code, as there won't
 be a need for multiple arbitrary IOMMUs with ACPI or with board files.

I think part of the problem is that we don't have any DT probing code
yet. of_platform_probe() would have been that code. Perhaps if you weigh
in Grant and Greg will reconsider.

One downside of that approach is that, while it maps well to platform
devices or generic devices that have some sort of firmware interface
such as OF or ACPI, I don't see how it can be made to work with an I2C
client that's registered from board setup code for example. Well, I
suppose that problem could be solved by throwing another lookup table at
it, just like we do for clocks, regulators, PWMs and GPIOs.
   
   Wouldn't you still be able to attach resources in the traditional
   way for those, but use the same new interface to get at them?
  
  I wouldn't know how. For instance platform devices store the IRQ number
  within a struct resource of type IORESOURCE_IRQ, whereas I2C clients
  store them in the struct i2c_client's .irq field.
 
 Good point, I forgot about the special case for i2c_client-irq.
 I looked now and noticed that very few i2c devices actually use this
 field, but larger number uses platform_data, which has a similar
 problem.

Yeah. It's kind of messy. The more I think about it, the more I begin to
like the lookup table option. One big advantage of that is that we could
actually unify much of the lookup code, much like we do for other types
of resources. It's a pattern that has worked fairly well in a number of
cases, so it seems natural to use it for interrupts as well.

An even more extreme option would be to go all the way and introduce
struct irq, along the same lines of the new struct gpiod. That would
allow nice things such as storing trigger types and such within the IRQ
handle and propagate them automatically.

  So without actually introspecting the struct device (possibly using the
  .bus field for example) and upcasting you won't know how to get at the
  resources. One possibility to remedy that would be to try and unify the
  resources within struct device. But that doesn't feel right.
  
  One other thing I had considered at one point was to extend the bus_type
  structure and give it a way to obtain resources in a bus-specific way,
  but that feel even more wrong.
  
  Perhaps I'm missing something obvious, though, and this is actually much
  more trivial to solve.
 
 No trivial solution that I can see. I think we can deal with the case
 where platform code uses platform_device-resources, and everything else
 comes down to having multiple code branches in the driver, as we already
 have to deal with platform_data and DT properties describing stuff that
 doesn't fit in the resources.

That would be another argument in favour of the lookup table. It would
provide a unified mechanism to define static interrupts

Re: [PATCH] driver-core: platform: Resolve DT interrupt references late

[Thierry Reding]

On Wed, Jan 08, 2014 at 09:09:17PM +0100, Arnd Bergmann wrote:
 On Wednesday 08 January 2014 20:59:10 Thierry Reding wrote:
  On Wed, Jan 08, 2014 at 05:25:17PM +0100, Arnd Bergmann wrote:
   On Wednesday 08 January 2014, Thierry Reding wrote:
On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote:
   The more I think about the iommu case, the more I am convinced that it
   does belong into the core, in whatever form we can find. As far as I
   can tell from the little reliable information I have on the topic, I
   would assume that we can keep it in the DT probing code, as there won't
   be a need for multiple arbitrary IOMMUs with ACPI or with board files.
  
  I think part of the problem is that we don't have any DT probing code
  yet. of_platform_probe() would have been that code. Perhaps if you weigh
  in Grant and Greg will reconsider.
 
 For all I know, we don't even have a binding proposal, but I may
 have missed that.

Yeah, last time I checked there was no concensus on that. I'll try to
dig up the thread and get it going again, adding you on Cc if you don't
mind (and in case you weren't Cc'ed in the first place anyway).

   Good point, I forgot about the special case for i2c_client-irq.
   I looked now and noticed that very few i2c devices actually use this
   field, but larger number uses platform_data, which has a similar
   problem.
  
  Yeah. It's kind of messy. The more I think about it, the more I begin to
  like the lookup table option. One big advantage of that is that we could
  actually unify much of the lookup code, much like we do for other types
  of resources. It's a pattern that has worked fairly well in a number of
  cases, so it seems natural to use it for interrupts as well.
  
  An even more extreme option would be to go all the way and introduce
  struct irq, along the same lines of the new struct gpiod. That would
  allow nice things such as storing trigger types and such within the IRQ
  handle and propagate them automatically.
 
 We already have struct irq_desc, and I believe everybody who works
 with interrupts supports migrating from irq number interfaces to
 irq descriptors as soon as we can find someone willing to do that
 work.

I didn't know that. I had always assumed irq_desc was only for internal
use by the IRQ code. Perhaps I'll look into that at some point.

   No trivial solution that I can see. I think we can deal with the case
   where platform code uses platform_device-resources, and everything else
   comes down to having multiple code branches in the driver, as we already
   have to deal with platform_data and DT properties describing stuff that
   doesn't fit in the resources.
  
  That would be another argument in favour of the lookup table. It would
  provide a unified mechanism to define static interrupts if no firmware
  interface is available *independent* of the device type. You could have
  something like:
  
  static const struct irq_lookup board_irq_lookup[] = {
  IRQ_LOOKUP(gpio, 0, 0-0040, NULL), /* I2C client via GPIO 
  expander */
  IRQ_LOOKUP(intc, 0, ehci.1, NULL), /* platform device via 
  INTC */
  };
  
  Along with:
  
  struct irq *irq_get(struct device *dev, const char *con_id);
  
  To look it up via DT, ACPI, lookup table. That obviously means a more or
  less complete change in how interrupts are handled in the kernel, and it
  may not be worth it in the end.
 
 It would certainly need a long migration period, and a plan for how to
 get there without breaking things in the meantime. Rather than a lookup
 table like the kind we have for clocks, I'd prefer a general way to
 attach named properties to devices. My feeling is that building upon
 devres would be a good plan, since it already provides a way to attach
 arbitrary data to a device.

The problem with devres, or any other solution for that matter, is that
for the cases where we'd need something like this (that is, statically
allocated devices in board setup code) we don't have a fully initialized
struct device. We need at least device_initialize() to have been called
before devres can be used. Therefore we still need some sort of lookup
table that can be used to seed devres objects. Also devres will go away
completely when a driver is unloaded, so it'll have to be repopulated
everytime.

I don't think that would buy us much over a simple table lookup.

Thierry


pgp1duHtWnTs9.pgp
Description: PGP signature

Re: [PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[Thierry Reding]

On Tue, Oct 29, 2013 at 02:23:18PM -0700, Tony Lindgren wrote:
 * NeilBrown ne...@suse.de [131023 23:36]:
  
  I submitted this in December last year.  I got lots of good feedback
  and fixed some things, but it never got accepted.  Not entirely sure
  why, maybe I dropped the ball.
  
  Anyway, here is again with device-tree support added.
  
  This is only an RFC and not a real submission for two reasons, both of which
  are really directed as Jon.
  
  1/ I have to 
  
  #include ../arch/arm/plat-omap/include/plat/dmtimer.h
  
  which is incredibly ugly.
  Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?
 
 Yes that's what at least dw_apb_timer and sh_timer are doing.
  
  2/ I found that I need to call
  
  omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
  
   when using device-tree.  This is because with devicetree
   omap_timer_restore_context() is called much more often and it sets the 
  counter
   register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN 
  from there.
  
   Now I don't object to calling omap_dm_timer_write_counter (though it might 
  be nice if
   omap_dm_timer_set_load wrote the one value to both LOAD_REG and 
  COUNTER_REG).
   However it seems wrong that I have to call it *after* starting the counter.
   Currently _write_counter refuses to run if the timer hasn't been started.
  
   So Jon: 
 a/ can we change omap_dm_timer_write_counter to work when the timer isn't
running?
 b/ can we have omap_dm_timer_set_load also set the counter?
  
  
  For anyone else generous enough to read my code: is this otherwise 
  acceptable?
 
 Did not look beyond the dmtimer stuff, but let's start by moving dmtimer.c to
 live under drivers and move the header, then do the pwm patches.

Why does the driver have to move? There is certainly some precedent for
having arch-specific code in arch/ but expose the public API via some
header in include/linux. Sometimes there's just no proper place for the
driver elsewhere, so rather than moving it somewhere more or less random
keeping it in arch/arm/plat-omap is just as good.

Thierry


pgpgGVxwWFraD.pgp
Description: PGP signature

Re: [PATCH/RFC] pwm: omap: Add PWM support using dual-mode timers

[Thierry Reding]

On Thu, Oct 24, 2013 at 05:36:20PM +1100, NeilBrown wrote:
 
 I submitted this in December last year.  I got lots of good feedback
 and fixed some things, but it never got accepted.  Not entirely sure
 why, maybe I dropped the ball.
 
 Anyway, here is again with device-tree support added.
 
 This is only an RFC and not a real submission for two reasons, both of which
 are really directed as Jon.
 
 1/ I have to 
 
 #include ../arch/arm/plat-omap/include/plat/dmtimer.h
 
 which is incredibly ugly.
 Is there any reason this cannot be moved to include/linux/omap-dmtimer.h?
 
 2/ I found that I need to call
 
   omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
 
  when using device-tree.  This is because with devicetree
  omap_timer_restore_context() is called much more often and it sets the 
 counter
  register to 0 .. it takes a long time to count up to DM_TIMER_LOAD_MIN from 
 there.
 
  Now I don't object to calling omap_dm_timer_write_counter (though it might 
 be nice if
  omap_dm_timer_set_load wrote the one value to both LOAD_REG and COUNTER_REG).
  However it seems wrong that I have to call it *after* starting the counter.
  Currently _write_counter refuses to run if the timer hasn't been started.
 
  So Jon: 
a/ can we change omap_dm_timer_write_counter to work when the timer isn't
   running?
b/ can we have omap_dm_timer_set_load also set the counter?
 
 
 For anyone else generous enough to read my code: is this otherwise acceptable?
 
 Thanks,
 NeilBrown
 
 -
 This patch is based on an earlier patch by Grant Erickson
 which provided PWM devices using the 'legacy' interface.
 
 This driver instead uses the new framework interface.
 
 The choice of dmtimer to be used can be made through platform_data
 by requesting a specific timer, or though devicetree by giving
 the appropriate device-tree node.
 
 Lots of clean-ups and improvements thanks to Thierry Reding
 and Jon Hunter.
 
 Cc: Grant Erickson maratho...@gmail.com
 Signed-off-by: NeilBrown ne...@suse.de
 
 diff --git a/Documentation/devicetree/bindings/pwm/pwm-omap.txt 
 b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
 new file mode 100644
 index 000..5f03048
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/pwm/pwm-omap.txt
 @@ -0,0 +1,32 @@
 +* PWM for OMAP using dmtimers
 +
 +TI's OMAP SoCs contains dual mode timers (dmtimers), some of
 +which can driver output pins and so provide services as

s/driver/drive/

 +a PWM.  When using this driver it will normally be necessary
 +to programmer an appropriate pin to server as a timer output.

s/programmer/program/ and s/server/serve/

 +
 +Required properties:
 +- compatible: must be
 +   ti,omap-pwm
 +
 +- timers: a device-tree node for the dmtimer to use
 +
 +- #pwm-cells: must be 2.

The canonical form to write this these days is:

- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
  the cells format.

 +
 +Example:
 +
 + pwm: omap-pwm {
 +   compatible = ti,omap-pwm;
 +   timers = timer11;
 +   #pwm-cells = 2;
 +
 +   pinctrl-names = default;
 +   pinctrl-0 = pwm-pins;
 + };
 +
 +...
 + pwm-pins: pinmux_pwm_pins {

I don't think dashes work in labels or phandles. They do work in node
names, though, so this could be:

pwm_pins: pinmux-pwm-pins {
...
};

 diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 index 75840b5..0e3cf83 100644
 --- a/drivers/pwm/Kconfig
 +++ b/drivers/pwm/Kconfig
 @@ -110,6 +110,15 @@ config PWM_PCA9685
 To compile this driver as a module, choose M here: the module
 will be called pwm-pca9685.
  
 +config PWM_OMAP

This doesn't seem to be properly ordered now. I suspect that back when
you posted that last time PCA9685 support hadn't been merged yet and the
rebase messed this up.

I wonder: does the OMAP not have dedicated PWM hardware?

 diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
 index 77a8c18..322ddf0 100644
 --- a/drivers/pwm/Makefile
 +++ b/drivers/pwm/Makefile
 @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_JZ4740)  += pwm-jz4740.o
  obj-$(CONFIG_PWM_LPC32XX)+= pwm-lpc32xx.o
  obj-$(CONFIG_PWM_MXS)+= pwm-mxs.o
  obj-$(CONFIG_PWM_PCA9685)+= pwm-pca9685.o
 +obj-$(CONFIG_PWM_OMAP)   += pwm-omap.o
  obj-$(CONFIG_PWM_PUV3)   += pwm-puv3.o

Same ordering issue here.

 diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
 +/*
 + *Copyright (c) 2012 NeilBrown ne...@suse.de

I guess that'd include 2013 now?

 +#include linux/export.h
 +#include linux/kernel.h
 +#include linux/platform_device.h
 +#include linux/slab.h
 +#include linux/err.h
 +#include linux/clk.h
 +#include linux/io.h
 +#include linux/pwm.h
 +#include linux/module.h
 +#include linux/platform_data/omap-pwm.h

I'd prefer these to be sorted alphabetically.

 +#include ../arch/arm/plat-omap/include/plat/dmtimer.h
 +
 +#define DM_TIMER_LOAD_MIN0xFFFE
 +
 +struct

Re: [PATCH 10/10] pwm-backlight: Allow backlight to remain disabled on boot

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:50:51PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
[...]
  +   if (gpio_is_valid(pb-enable_gpio)) {
  +   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
  +   gpio_set_value(pb-enable_gpio, 0);
  +   else
  +   gpio_set_value(pb-enable_gpio, 1);
  +   }
 
 ... That assumes that the backlight is on at boot, and hence presumably
 after this patch still always turns on the backlight, only to turn it
 off very quickly once the following code in this patch executes:

I was just going to fix this, but then saw that the code in .probe() is
actually this:

if (gpio_is_valid(pb-enable_gpio)) {
unsigned long flags;

if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
flags = GPIOF_OUT_INIT_HIGH;
else
flags = GPIOF_OUT_INIT_LOW;

ret = gpio_request_one(pb-enable_gpio, flags, enable);
...
}

Which sets the backlight up to be disabled by default and is consistent
with the PWM and regulator setup. Only later, depending on the value of
boot_off it gets enabled (or stays disabled).

 (and perhaps we also need to avoid turning the backlight off then on if
 it was already on at boot)

That's true. Although I'm not sure if that's even possible. I think the
pinctrl driver doesn't touch the registers, but what about the GPIO and
PWM drivers. It might be impossible to always query the boot status and
set the internal state based on that. The easiest would probably be if
both the bootloader and the kernel use the same DT, in which case the
bootloader could set the backlight-boot-on property, in which case we
wouldn't need to do anything at .probe() time really.

Thierry


pgpeu0VkrWADK.pgp
Description: PGP signature

Re: [PATCH 01/10] pwm-backlight: Refactor backlight power on/off

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:26:07PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:40 PM, Thierry Reding wrote:
  In preparation for adding an optional regulator and enable GPIO to the
  driver, split the power on and power off sequences into separate
  functions to reduce code duplication at the multiple call sites.
 
  diff --git a/drivers/video/backlight/pwm_bl.c 
  b/drivers/video/backlight/pwm_bl.c
 
  +static void pwm_backlight_power_off(struct pwm_bl_data *pb)
  +{
  +   pwm_disable(pb-pwm);
 
 Both the call-sites you're replacing do the following before pwm_disable():
 
   pwm_config(pb-pwm, 0, pb-period);
 
 While I agree that probably shouldn't be necessary, I think it's at
 least worth mentioning that in the commit description just to make it
 obvious that it was a deliberate change. Splitting that change into a
 separate patch might be reasonable in order to keep refactoring and
 functional changes separate, although perhaps it's not worth it.

Actually I'll add that back. I'm not sure exactly why I dropped it (it
may just have been oversight) and there have been reports that some HW
actually requires pwm_config(..., 0, ...) before pwm_disable() to turn
off properly.

 There are also a couple unrelated whitespace changes thrown in here.

I think they increase readability, but I can certainly move them into a
separate patch.

Thierry


pgpctxOW5l0O1.pgp
Description: PGP signature

Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
  The GPIO API defines 0 as being a valid GPIO number, so this field needs
  to be initialized explicitly.
 
   static void __init smdkv210_map_io(void)
 
  @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata samsung_dfl_bl_data 
  __initdata = {
  .max_brightness = 255,
  .dft_brightness = 255,
  .pwm_period_ns  = 78770,
  +   .enable_gpio= -1,
  .init   = samsung_bl_init,
  .exit   = samsung_bl_exit,
  },
  @@ -121,6 +122,10 @@ void __init samsung_bl_set(struct samsung_bl_gpio_info 
  *gpio_info,
  samsung_bl_data-lth_brightness = bl_data-lth_brightness;
  if (bl_data-pwm_period_ns)
  samsung_bl_data-pwm_period_ns = bl_data-pwm_period_ns;
  +   if (bl_data-enable_gpio)
  +   samsung_bl_data-enable_gpio = bl_data-enable_gpio;
  +   if (bl_data-enable_gpio_flags)
  +   samsung_bl_data-enable_gpio_flags = bl_data-enable_gpio_flags;
 
 Won't this cause the core pwm_bl driver to request/manipulate the GPIO,
 whereas this driver already does that inside the samsung_bl_init/exit
 callbacks? I think you either need to adjust those callbacks, or not set
 the new standard GPIO property in samsung_bl_data.

I don't think so. The samsung_bl_data is a copy of samsung_dfl_bl_data
augmented by board-specific settings. So in fact copying these values
here is essential to allow boards to override the enable_gpio and flags
fields. Currently no board sets the enable_gpio to a valid GPIO so it's
all still handled by the callbacks only.

Thierry


pgpbvsUk3QTMs.pgp
Description: PGP signature

Re: [PATCH 08/10] pwm-backlight: Use new enable_gpio field

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:39:36PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
  Make use of the new enable_gpio field and allow it to be set from DT as
  well. Now that all legacy users of platform data have been converted to
  initialize this field to an invalid value, it is safe to use the field
  from the driver.
 
  diff --git 
  a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
  b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
 
   Optional properties:
 
  +  - enable-gpios: a list of GPIOs to enable and disable the backlight
 
 That seems to imply that multiple GPIOs are legal. Was that intended? If
 not, I would suggest:
 
 - enable-gpios: contains a single GPIO specifier for the GPIO which
   enables/disables the backlight. See [1] for the format.
 
   
   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 + [1]: Documentation/devicetree/bindings/gpio/gpio.txt

Yes, that sounds better. Indeed only a single GPIO is supported. Adding
more than one would require representing the timing between the two and
that will take us back to where we left off with the power sequences.

  diff --git a/drivers/video/backlight/pwm_bl.c 
  b/drivers/video/backlight/pwm_bl.c
 
  @@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
  *pb, int brightness,
   pb-lth_brightness;
   
  pwm_config(pb-pwm, duty_cycle, pb-period);
  +
  +   if (gpio_is_valid(pb-enable_gpio)) {
  +   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
  +   gpio_set_value(pb-enable_gpio, 0);
  +   else
  +   gpio_set_value(pb-enable_gpio, 1);
  +   }
 
 Feel completely free to ignore this, but when the difference in two
 code-paths is solely in function parameters, I prefer to calculate the
 parameter inside the if statement, then call the function outside the
 conditional code, to highlight the common/different parts:
 
 int value;
 
 /* or an if statement for the next 1 line, if you don't like ?: */
 value = (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW) ? 0 : 1;
 gpio_set_value((pb-enable_gpio, value);

I actually tried that, but it looked cluttered, so I opted for the
alternative. Not that this matters all that much, because beginning with
3.13 the GPIO subsystem should be able to handle the active low flag
internally using the new gpiod_*() API. I plan on converting the driver
to use that during the next cycle.

  @@ -148,11 +168,10 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
  +   data-enable_gpio = of_get_named_gpio_flags(node, enable-gpios, 0,
  +   flags);
  +   if (gpio_is_valid(data-enable_gpio)  (flags  OF_GPIO_ACTIVE_LOW))
  +   data-enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
 
 This doesn't seem to handle deferred probe correctly; I would expect
 something more like:
 
   data-enable_gpio = of_get_named_gpio_flags(...);
   if (data-enable_gpio == -EPROBE_DEFERRED)
   return data-enable_gpio;
   if (gpio_is_valid(...))
   ...
   return 0;
 
 I suppose it's possible that the value filters down to
 gpio_request_one() and this actually does work out OK, but that feels
 very accidental/implicit to me.

Yes, I think it does indeed propagate to gpio_request_one(), but you're
right, it should be caught explicitly.

Thierry


pgpQ6o_fNIUqu.pgp
Description: PGP signature

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
  Many backlights require a power supply to work properly. This commit
  uses a power-supply regulator, if available, to power up and power down
  the panel.
 
 I think that all backlights require a power supply, albeit the supply
 may not be SW-controllable. Hence, shouldn't the regulator be mandatory
 in the binding, yet the driver be defensively coded such that if one
 isn't specified, the driver continues to work?

That has already changed in my local version of this patch.

  diff --git a/drivers/video/backlight/pwm_bl.c 
  b/drivers/video/backlight/pwm_bl.c
 
  @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device 
  *pdev)
  }
  }
   
  +   pb-power_supply = devm_regulator_get_optional(pdev-dev, power);
 
 ... so I think that should be devm_regulator_get(), since the regulator
 isn't really optional.
 
  +   if (IS_ERR(pb-power_supply)) {
  +   if (PTR_ERR(pb-power_supply) != -ENODEV) {
  +   ret = PTR_ERR(pb-power_supply);
  +   goto err_gpio;
  +   }
  +
  +   pb-power_supply = NULL;
 
 If devm_regulator_get_optional() returns an error value or a valid
 value, then I don't think that this driver should transmute error values
 into NULL; NULL might be a perfectly valid regulator value. Related, I
 think the if (pb-power_supply) tests should be replaced with if
 (!IS_ERR(pb-power_supply)) instead.

All of that is already done in my local tree. This actually turns out to
work rather smoothly with the new support for optional regulators. The
regulator core will give you a dummy regulator (assuming it's there
physically but hasn't been wired up in software) that's always on, so
the driver doesn't even have to special case it anymore.

Thierry


pgpMjIbcA3a4c.pgp
Description: PGP signature

Re: [PATCH 10/10] pwm-backlight: Allow backlight to remain disabled on boot

[Thierry Reding]

On Tue, Oct 01, 2013 at 12:50:51PM -0600, Stephen Warren wrote:
 On 09/23/2013 03:41 PM, Thierry Reding wrote:
  The default for backlight devices is to be enabled immediately when
  registering with the backlight core. This can be useful for setups that
  use a simple framebuffer device and where the backlight cannot otherwise
  be hooked up to the panel.
  
  However, when dealing with more complex setups, such as those of recent
  ARM SoCs, this can be problematic. Since the backlight is usually setup
  separately from the display controller, the probe order is not usually
  deterministic. That can lead to situations where the backlight will be
  powered up and the panel will show an uninitialized framebuffer.
  
  Furthermore, subsystems such as DRM have advanced functionality to set
  the power mode of a panel. In order to allow such setups to power up the
  panel at exactly the right moment, a way is needed to prevent the
  backlight core from powering the backlight up automatically when it is
  registered.
  
  This commit introduces a new boot_off field in the platform data (and
  also implements getting the same information from device tree). When set
  the initial backlight power mode will be set to off.
 
  diff --git 
  a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
  b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
 
  +  - backlight-boot-off: keep the backlight disabled on boot
 
 A few thoughts:
 
 1) Does this property indicate that:
 
 a) The current state of the backlight at boot. In which case, this will
 need bootloader involvement to modify the value in the DT at boot time
 based on whether the bootloader turned on the backlight:-(

I was pretty much following the regulator bindings here. But the meaning
is different indeed, see below.

 b) That the driver should not turn on the backlight immediately? That
 seems to describe driver behaviour more than HW. Is it appropriate to
 put that into DT?

That's what it was supposed to mean. The idea is, and I mentioned that
in the commit message, that when wired up with a higher-level API such
as DRM, then usually that framework knows exactly when to enable the
backlight. Having the backlight enable itself on probe may cause the
panel to light up with no content or garbage.

 Your suggestion to make the backlight not turn on by default might be a
 better fix?

I think so too, but the problem in this case is that users of this
driver heretofore assumed that it would be turned on by default. I think
that's been common practice for all backlight drivers. Adding a property
to DT was supposed to be a way to keep backwards compatibility with any
existing users while at the same time allowing the backlight to be used
in a more modern system. I don't really have any other ideas how to
achieve this.

It is quite possible that the only reason why turning on the backlight
at probe time is the current default is because backlight_properties'
.power field is by default initialized to 0, which turns out to be the
value of FB_BLANK_UNBLANK. That's been the source of quite a bit of
confusion (I could tell you stories of how people tried to convince me
that there must be a bug in the Linux kernel because writing a 0 to the
backlight's bl_power field in sysfs turns the backlight on instead of
off). The same is true for DPMS modes in DRM and X, where on has the
value 0.

Now there have been plans to overhaul the backlight subsystem for quite
some time. One of the goals was to decouple it from the FB subsystem,
which might help solve this to some degree. At the same time sysfs is an
ABI, so we can't just go and change it randomly. The same is true for
the default behaviour of enabling the backlight at boot. That can
equally be considered an ABI and changing it will cause the majority of
devices to regress, and that's not something that I look forward to
taking the blame for...

 2) Should the driver instead attempt to read the current state of the
 GPIO output to determine whether the backlight is on? This may not be
 possible on all HW.
 
 3) Doesn't the following code in probe() (added in a previous patch)
 need to be updated?
 
  +   if (gpio_is_valid(pb-enable_gpio)) {
  +   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
  +   gpio_set_value(pb-enable_gpio, 0);
  +   else
  +   gpio_set_value(pb-enable_gpio, 1);
  +   }
 
 ... That assumes that the backlight is on at boot, and hence presumably
 after this patch still always turns on the backlight, only to turn it
 off very quickly once the following code in this patch executes:
 
 (and perhaps we also need to avoid turning the backlight off then on if
 it was already on at boot)

Yes, that's a good point. Depending on the outcome of the above
discussion I'll update this to match the expectations.

Thierry


pgpOKJtRJ_AH6.pgp
Description: PGP signature

Re: [PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field

[Thierry Reding]

On Tue, Oct 01, 2013 at 02:58:22PM -0600, Stephen Warren wrote:
 On 10/01/2013 02:43 PM, Thierry Reding wrote:
  On Tue, Oct 01, 2013 at 12:31:04PM -0600, Stephen Warren wrote:
  On 09/23/2013 03:41 PM, Thierry Reding wrote:
  The GPIO API defines 0 as being a valid GPIO number, so this
  field needs to be initialized explicitly.
  
  static void __init smdkv210_map_io(void)
  
  @@ -70,6 +70,7 @@ static struct samsung_bl_drvdata
  samsung_dfl_bl_data __initdata = { .max_brightness = 255, 
  .dft_brightness = 255, .pwm_period_ns  = 78770, + .enable_gpio
  = -1, .init   = samsung_bl_init, .exit   =
  samsung_bl_exit, }, @@ -121,6 +122,10 @@ void __init
  samsung_bl_set(struct samsung_bl_gpio_info *gpio_info, 
  samsung_bl_data-lth_brightness = bl_data-lth_brightness; if
  (bl_data-pwm_period_ns) samsung_bl_data-pwm_period_ns =
  bl_data-pwm_period_ns; + if (bl_data-enable_gpio) +
  samsung_bl_data-enable_gpio = bl_data-enable_gpio; +if
  (bl_data-enable_gpio_flags) +
  samsung_bl_data-enable_gpio_flags =
  bl_data-enable_gpio_flags;
  
  Won't this cause the core pwm_bl driver to request/manipulate the
  GPIO, whereas this driver already does that inside the
  samsung_bl_init/exit callbacks? I think you either need to adjust
  those callbacks, or not set the new standard GPIO property in
  samsung_bl_data.
  
  I don't think so. The samsung_bl_data is a copy of
  samsung_dfl_bl_data augmented by board-specific settings. So in
  fact copying these values here is essential to allow boards to
  override the enable_gpio and flags fields. Currently no board sets
  the enable_gpio to a valid GPIO so it's all still handled by the
  callbacks only.
 
 Oh yes, you're right. I was confusing the new enable_gpio field in
 pwm_bl's platform data with some other field in a custom data structure.
 
 One minor point though:
 
  + if (bl_data-enable_gpio) + samsung_bl_data-enable_gpio =
  bl_data-enable_gpio;
 
 That assumes that enable_gpio==0 means none, whereas you've gone to
 great pains in the rest of the series to allow 0 to be a valid GPIO
 ID. right now, the default value of samsung_bl_data-enable_gpio is
 -1, and if !bl_data-enable_gpio, that value won't be propagated across.

Right, that check should now be:

if (bl_data-enable_gpio = 0)

Well, it depends. It would be possible for the default to specify a
valid GPIO and for a board to override it with -1 (and provide a set of
corresponding callbacks). In that case the right thing to do here would
be not to check at all.

Then again, I don't think that will ever happen, because no fixed GPIO
will ever be a good default. So changing to = 0 instead of != 0 should
work fine.

Again, starting with 3.13 this should become a lot easier to handle
since the GPIO subsystem will gain functionality to use a per-board
lookup table, similarly to what the regulator and PWM subsystems do.
Once that's in place I plan to make another pass over all users of the
pwm-backlight driver and replace the enable_gpio field with a GPIO
lookup table, so that the driver can uniformly request them using a
simple gpiod_get().

Thierry


pgp6CLx44qy6C.pgp
Description: PGP signature

Re: [PATCH 09/10] pwm-backlight: Use an optional power supply

[Thierry Reding]

On Tue, Oct 01, 2013 at 02:59:43PM -0600, Stephen Warren wrote:
 On 10/01/2013 02:53 PM, Thierry Reding wrote:
  On Tue, Oct 01, 2013 at 12:43:57PM -0600, Stephen Warren wrote:
  On 09/23/2013 03:41 PM, Thierry Reding wrote:
  Many backlights require a power supply to work properly. This
  commit uses a power-supply regulator, if available, to power up
  and power down the panel.
  
  I think that all backlights require a power supply, albeit the
  supply may not be SW-controllable. Hence, shouldn't the regulator
  be mandatory in the binding, yet the driver be defensively coded
  such that if one isn't specified, the driver continues to work?
  
  That has already changed in my local version of this patch.
  
  diff --git a/drivers/video/backlight/pwm_bl.c
  b/drivers/video/backlight/pwm_bl.c
  
  @@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct
  platform_device *pdev) } }
  
  + pb-power_supply = devm_regulator_get_optional(pdev-dev,
  power);
  
  ... so I think that should be devm_regulator_get(), since the
  regulator isn't really optional.
  
  + if (IS_ERR(pb-power_supply)) { +   if
  (PTR_ERR(pb-power_supply) != -ENODEV) { +ret =
  PTR_ERR(pb-power_supply); +  goto err_gpio; +
  } + +
  pb-power_supply = NULL;
  
  If devm_regulator_get_optional() returns an error value or a
  valid value, then I don't think that this driver should transmute
  error values into NULL; NULL might be a perfectly valid regulator
  value. Related, I think the if (pb-power_supply) tests should be
  replaced with if (!IS_ERR(pb-power_supply)) instead.
  
  All of that is already done in my local tree. This actually turns
  out to work rather smoothly with the new support for optional
  regulators. The regulator core will give you a dummy regulator
  (assuming it's there physically but hasn't been wired up in
  software) that's always on, so the driver doesn't even have to
  special case it anymore.
 
 OK, hopefully it (the regulator core) complains about the missing DT
 property though; I assume you're using regulator_get() not
 regulator_get_optional(), since the supply really is not optional.

It doesn't always. There's a pr_warn() in _regulator_get(), but that's
only for non-DT (since for DT, has_full_constraints is set to true in
regulator_init_complete()). Actually that would mean that the regulator
core will complain as long as init isn't complete. But since, like many
other drivers, pwm-backlight could use deferred probing it's likely to
end up being probed after init.

Cc'ing Mark Brown.

Thierry


pgpqgdHYE7QEt.pgp
Description: PGP signature

Re: [PATCH 00/10] pwm-backlight: Add GPIO and power supply support

[Thierry Reding]

On Tue, Sep 24, 2013 at 05:14:46PM +0900, Simon Horman wrote:
 [ Cc: Olof Johansson, Kevin Hilman and Arnd Bergman: arm-soc maintainers ]
 
 On Mon, Sep 23, 2013 at 11:40:57PM +0200, Thierry Reding wrote:
  This series adds the ability to specify a GPIO and a power supply to
  enable a backlight.
  
  Patch 1 refactors the power on and power off sequences into separate
  functions in preparation for subsequent patches.
  
  Patch 2 adds an optional GPIO to enable a backlight. This patch only
  includes the field within the platform data so that it can be properly
  setup before actually being put to use.
  
  Patches 3 to 7 convert all users of the pwm-backlight driver to use the
  new field. For most of them, this just initializes the field to -1,
  marking the field as unused.
 
  Patch 8 uses the new field within the pwm-backlight driver and at the
  same time allows it to be parsed from device tree.
  
  Patch 9 implements support for an optional power supply. This relies on
  the regulator core to return a dummy regulator when no supply has been
  otherwise setup so the driver doesn't have to handle that specially nor
  require all users to be updated.
  
  Patch 10 adds a way to keep a backlight turned off at boot. This is
  useful when hooking up a backlight with a subsystem such as DRM which
  has more explicit semantics as to when a backlight should be turned on.
  
  Due to the dependencies within the series, I propose to take all these
  patches through the PWM tree, so I'll need acks from OMAP, PXA, Samsung,
  shmobile and Unicore32 maintainers.
 
 I received some feedback regarding shmobile conflicts when
 arm-soc was merged between v3.11 and v3.12-rc1. With this
 in mind I now have a strong preference for changes inside
 arch/arm/mach-shmobile/ to be taken through my renesas
 tree and thus more importantly through arm-soc if possible.

I understand. Unfortunately the nature of patche series such as this is
that they require the whole series to be applied atomically (or at least
in a very specific order). So the patch that uses the new enable_gpio
field can only be applied after all previous patches. The only
reasonable way to ensure that is to take all of the patches through one
tree. Furthermore this patch is tiny (it adds a single line) and touches
code that's unlikely to be modified by anyone else.

But if it makes you more comfortable, I could provide a stable branch
that contains this series for you to merge into the shmobile tree. That
should enable you to handle all conflict resolution prior to submitting
to arm-soc.

Thierry


pgpWmaBVZWHdw.pgp
Description: PGP signature

[PATCH 00/10] pwm-backlight: Add GPIO and power supply support

[Thierry Reding]

This series adds the ability to specify a GPIO and a power supply to
enable a backlight.

Patch 1 refactors the power on and power off sequences into separate
functions in preparation for subsequent patches.

Patch 2 adds an optional GPIO to enable a backlight. This patch only
includes the field within the platform data so that it can be properly
setup before actually being put to use.

Patches 3 to 7 convert all users of the pwm-backlight driver to use the
new field. For most of them, this just initializes the field to -1,
marking the field as unused.

Patch 8 uses the new field within the pwm-backlight driver and at the
same time allows it to be parsed from device tree.

Patch 9 implements support for an optional power supply. This relies on
the regulator core to return a dummy regulator when no supply has been
otherwise setup so the driver doesn't have to handle that specially nor
require all users to be updated.

Patch 10 adds a way to keep a backlight turned off at boot. This is
useful when hooking up a backlight with a subsystem such as DRM which
has more explicit semantics as to when a backlight should be turned on.

Due to the dependencies within the series, I propose to take all these
patches through the PWM tree, so I'll need acks from OMAP, PXA, Samsung,
shmobile and Unicore32 maintainers.

Thierry

Thierry Reding (10):
  pwm-backlight: Refactor backlight power on/off
  pwm-backlight: Add optional enable GPIO
  ARM: OMAP: Initialize PWM backlight enable_gpio field
  ARM: pxa: Initialize PWM backlight enable_gpio field
  ARM: SAMSUNG: Initialize PWM backlight enable_gpio field
  ARM: shmobile: Initialize PWM backlight enable_gpio field
  unicore32: Initialize PWM backlight enable_gpio field
  pwm-backlight: Use new enable_gpio field
  pwm-backlight: Use an optional power supply
  pwm-backlight: Allow backlight to remain disabled on boot

 .../bindings/video/backlight/pwm-backlight.txt |   6 +
 arch/arm/mach-omap2/board-zoom-peripherals.c   |   1 +
 arch/arm/mach-pxa/cm-x300.c|   1 +
 arch/arm/mach-pxa/colibri-pxa270-income.c  |   1 +
 arch/arm/mach-pxa/ezx.c|   1 +
 arch/arm/mach-pxa/hx4700.c |   1 +
 arch/arm/mach-pxa/lpd270.c |   1 +
 arch/arm/mach-pxa/magician.c   |   1 +
 arch/arm/mach-pxa/mainstone.c  |   1 +
 arch/arm/mach-pxa/mioa701.c|   1 +
 arch/arm/mach-pxa/palm27x.c|   1 +
 arch/arm/mach-pxa/palmtc.c |  35 +
 arch/arm/mach-pxa/palmte2.c|   1 +
 arch/arm/mach-pxa/pcm990-baseboard.c   |   1 +
 arch/arm/mach-pxa/raumfeld.c   |   1 +
 arch/arm/mach-pxa/tavorevb.c   |   2 +
 arch/arm/mach-pxa/viper.c  |   1 +
 arch/arm/mach-pxa/z2.c |   2 +
 arch/arm/mach-pxa/zylonite.c   |   1 +
 arch/arm/mach-s3c24xx/mach-h1940.c |   1 +
 arch/arm/mach-s3c24xx/mach-rx1950.c|   1 +
 arch/arm/mach-s3c64xx/mach-crag6410.c  |   1 +
 arch/arm/mach-s3c64xx/mach-hmt.c   |   1 +
 arch/arm/mach-s3c64xx/mach-smartq.c|   1 +
 arch/arm/mach-s3c64xx/mach-smdk6410.c  |   1 +
 arch/arm/mach-s5p64x0/mach-smdk6440.c  |   1 +
 arch/arm/mach-s5p64x0/mach-smdk6450.c  |   1 +
 arch/arm/mach-s5pc100/mach-smdkc100.c  |   1 +
 arch/arm/mach-s5pv210/mach-smdkv210.c  |   1 +
 arch/arm/mach-shmobile/board-armadillo800eva.c |   1 +
 arch/arm/plat-samsung/dev-backlight.c  |   5 +
 arch/unicore32/kernel/puv3-nb0916.c|   1 +
 drivers/video/backlight/pwm_bl.c   | 142 -
 include/linux/pwm_backlight.h  |   7 +
 34 files changed, 162 insertions(+), 64 deletions(-)

-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/10] pwm-backlight: Allow backlight to remain disabled on boot

[Thierry Reding]

The default for backlight devices is to be enabled immediately when
registering with the backlight core. This can be useful for setups that
use a simple framebuffer device and where the backlight cannot otherwise
be hooked up to the panel.

However, when dealing with more complex setups, such as those of recent
ARM SoCs, this can be problematic. Since the backlight is usually setup
separately from the display controller, the probe order is not usually
deterministic. That can lead to situations where the backlight will be
powered up and the panel will show an uninitialized framebuffer.

Furthermore, subsystems such as DRM have advanced functionality to set
the power mode of a panel. In order to allow such setups to power up the
panel at exactly the right moment, a way is needed to prevent the
backlight core from powering the backlight up automatically when it is
registered.

This commit introduces a new boot_off field in the platform data (and
also implements getting the same information from device tree). When set
the initial backlight power mode will be set to off.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
Note: Perhaps it would be more useful to make this the default behaviour
in the backlight core? Many other subsystems and frameworks assume that
a resource is off unless explicitly enabled.
---
 .../devicetree/bindings/video/backlight/pwm-backlight.txt | 1 +
 drivers/video/backlight/pwm_bl.c  | 8 
 include/linux/pwm_backlight.h | 2 ++
 3 files changed, 11 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 3898f26..1271886 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -16,6 +16,7 @@ Optional properties:
pwms property (see PWM binding[0])
   - enable-gpios: a list of GPIOs to enable and disable the backlight
   - power-supply: regulator for supply voltage
+  - backlight-boot-off: keep the backlight disabled on boot
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index a2b3876..3b2d9cf 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -184,6 +184,8 @@ static int pwm_backlight_parse_dt(struct device *dev,
if (gpio_is_valid(data-enable_gpio)  (flags  OF_GPIO_ACTIVE_LOW))
data-enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
 
+   data-boot_off = of_property_read_bool(node, backlight-boot-off);
+
return 0;
 }
 
@@ -318,6 +320,12 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
}
 
bl-props.brightness = data-dft_brightness;
+
+   if (data-boot_off)
+   bl-props.power = FB_BLANK_POWERDOWN;
+   else
+   bl-props.power = FB_BLANK_UNBLANK;
+
backlight_update_status(bl);
 
platform_set_drvdata(pdev, bl);
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 2de2e27..ea4a239 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -18,6 +18,8 @@ struct platform_pwm_backlight_data {
unsigned int *levels;
int enable_gpio;
unsigned long enable_gpio_flags;
+   bool boot_off;
+
int (*init)(struct device *dev);
int (*notify)(struct device *dev, int brightness);
void (*notify_after)(struct device *dev, int brightness);
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/10] pwm-backlight: Use an optional power supply

[Thierry Reding]

Many backlights require a power supply to work properly. This commit
uses a power-supply regulator, if available, to power up and power down
the panel.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 .../bindings/video/backlight/pwm-backlight.txt  |  2 ++
 drivers/video/backlight/pwm_bl.c| 21 +
 2 files changed, 23 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 052eb03..3898f26 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -15,6 +15,7 @@ Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
   - enable-gpios: a list of GPIOs to enable and disable the backlight
+  - power-supply: regulator for supply voltage
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -28,4 +29,5 @@ Example:
default-brightness-level = 6;
 
enable-gpios = gpio 58 0;
+   power-supply = vdd_bl_reg;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 506810c..a2b3876 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -21,6 +21,7 @@
 #include linux/err.h
 #include linux/pwm.h
 #include linux/pwm_backlight.h
+#include linux/regulator/consumer.h
 #include linux/slab.h
 
 struct pwm_bl_data {
@@ -29,6 +30,7 @@ struct pwm_bl_data {
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
+   struct regulator*power_supply;
int enable_gpio;
unsigned long   enable_gpio_flags;
int (*notify)(struct device *,
@@ -56,6 +58,12 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, 
int brightness,
 
pwm_config(pb-pwm, duty_cycle, pb-period);
 
+   if (pb-power_supply) {
+   err = regulator_enable(pb-power_supply);
+   if (err  0)
+   dev_err(pb-dev, failed to enable power supply\n);
+   }
+
if (gpio_is_valid(pb-enable_gpio)) {
if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
gpio_set_value(pb-enable_gpio, 0);
@@ -76,6 +84,9 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
else
gpio_set_value(pb-enable_gpio, 0);
}
+
+   if (pb-power_supply)
+   regulator_disable(pb-power_supply);
 }
 
 static int pwm_backlight_update_status(struct backlight_device *bl)
@@ -253,6 +264,16 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
}
}
 
+   pb-power_supply = devm_regulator_get_optional(pdev-dev, power);
+   if (IS_ERR(pb-power_supply)) {
+   if (PTR_ERR(pb-power_supply) != -ENODEV) {
+   ret = PTR_ERR(pb-power_supply);
+   goto err_gpio;
+   }
+
+   pb-power_supply = NULL;
+   }
+
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);
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] ARM: SAMSUNG: Initialize PWM backlight enable_gpio field

[Thierry Reding]

The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/mach-s3c24xx/mach-h1940.c| 1 +
 arch/arm/mach-s3c24xx/mach-rx1950.c   | 1 +
 arch/arm/mach-s3c64xx/mach-crag6410.c | 1 +
 arch/arm/mach-s3c64xx/mach-hmt.c  | 1 +
 arch/arm/mach-s3c64xx/mach-smartq.c   | 1 +
 arch/arm/mach-s3c64xx/mach-smdk6410.c | 1 +
 arch/arm/mach-s5p64x0/mach-smdk6440.c | 1 +
 arch/arm/mach-s5p64x0/mach-smdk6450.c | 1 +
 arch/arm/mach-s5pc100/mach-smdkc100.c | 1 +
 arch/arm/mach-s5pv210/mach-smdkv210.c | 1 +
 arch/arm/plat-samsung/dev-backlight.c | 5 +
 11 files changed, 15 insertions(+)

diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c 
b/arch/arm/mach-s3c24xx/mach-h1940.c
index 74dd479..952b6a0 100644
--- a/arch/arm/mach-s3c24xx/mach-h1940.c
+++ b/arch/arm/mach-s3c24xx/mach-h1940.c
@@ -504,6 +504,7 @@ static struct platform_pwm_backlight_data backlight_data = {
.dft_brightness = 50,
/* tcnt = 0x31 */
.pwm_period_ns  = 36296,
+   .enable_gpio= -1,
.init   = h1940_backlight_init,
.notify = h1940_backlight_notify,
.exit   = h1940_backlight_exit,
diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c 
b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 206b1f7..034b7fe 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -522,6 +522,7 @@ static struct platform_pwm_backlight_data 
rx1950_backlight_data = {
.max_brightness = 24,
.dft_brightness = 4,
.pwm_period_ns = 48000,
+   .enable_gpio = -1,
.init = rx1950_backlight_init,
.notify = rx1950_backlight_notify,
.exit = rx1950_backlight_exit,
diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c 
b/arch/arm/mach-s3c64xx/mach-crag6410.c
index 1a911df..b319bf9 100644
--- a/arch/arm/mach-s3c64xx/mach-crag6410.c
+++ b/arch/arm/mach-s3c64xx/mach-crag6410.c
@@ -114,6 +114,7 @@ static struct platform_pwm_backlight_data 
crag6410_backlight_data = {
.max_brightness = 1000,
.dft_brightness = 600,
.pwm_period_ns  = 10,   /* about 1kHz */
+   .enable_gpio= -1,
 };
 
 static struct platform_device crag6410_backlight_device = {
diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c
index e806404..614a03a 100644
--- a/arch/arm/mach-s3c64xx/mach-hmt.c
+++ b/arch/arm/mach-s3c64xx/mach-hmt.c
@@ -114,6 +114,7 @@ static struct platform_pwm_backlight_data 
hmt_backlight_data = {
.max_brightness = 100 * 256,
.dft_brightness = 40 * 256,
.pwm_period_ns  = 10 / (100 * 256 * 20),
+   .enable_gpio= -1,
.init   = hmt_bl_init,
.notify = hmt_bl_notify,
.exit   = hmt_bl_exit,
diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c 
b/arch/arm/mach-s3c64xx/mach-smartq.c
index 0f47237..a6b338f 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -151,6 +151,7 @@ static struct platform_pwm_backlight_data 
smartq_backlight_data = {
.max_brightness = 1000,
.dft_brightness = 600,
.pwm_period_ns  = 10 / (1000 * 20),
+   .enable_gpio= -1,
.init   = smartq_bl_init,
 };
 
diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c 
b/arch/arm/mach-s3c64xx/mach-smdk6410.c
index 2a7b32c..d5ea938 100644
--- a/arch/arm/mach-s3c64xx/mach-smdk6410.c
+++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c
@@ -625,6 +625,7 @@ static struct samsung_bl_gpio_info smdk6410_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6410_bl_data = {
.pwm_id = 1,
+   .enable_gpio = -1,
 };
 
 static struct s3c_hsotg_plat smdk6410_hsotg_pdata;
diff --git a/arch/arm/mach-s5p64x0/mach-smdk6440.c 
b/arch/arm/mach-s5p64x0/mach-smdk6440.c
index 0b00304..9efdcc0 100644
--- a/arch/arm/mach-s5p64x0/mach-smdk6440.c
+++ b/arch/arm/mach-s5p64x0/mach-smdk6440.c
@@ -223,6 +223,7 @@ static struct samsung_bl_gpio_info smdk6440_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6440_bl_data = {
.pwm_id = 1,
+   .enable_gpio = -1,
 };
 
 static void __init smdk6440_map_io(void)
diff --git a/arch/arm/mach-s5p64x0/mach-smdk6450.c 
b/arch/arm/mach-s5p64x0/mach-smdk6450.c
index 5949296..c3cacc0 100644
--- a/arch/arm/mach-s5p64x0/mach-smdk6450.c
+++ b/arch/arm/mach-s5p64x0/mach-smdk6450.c
@@ -242,6 +242,7 @@ static struct samsung_bl_gpio_info smdk6450_bl_gpio_info = {
 
 static struct platform_pwm_backlight_data smdk6450_bl_data = {
.pwm_id = 1,
+   .enable_gpio = -1,
 };
 
 static void __init smdk6450_map_io(void)
diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c 
b/arch/arm/mach-s5pc100/mach-smdkc100.c
index 7c57a22..9e256b9 100644
--- a/arch/arm/mach-s5pc100/mach-smdkc100.c
+++ b/arch/arm/mach-s5pc100/mach-smdkc100.c
@@ -216,6 +216,7 @@ static struct samsung_bl_gpio_info smdkc100_bl_gpio_info = {
 
 static

[PATCH 04/10] ARM: pxa: Initialize PWM backlight enable_gpio field

[Thierry Reding]

The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

A special case is the Palm Tungsten|C board. Since it doesn't use any
quirks that would require the existing .init() or .exit() hooks it can
simply use the new enable_gpio field.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/mach-pxa/cm-x300.c   |  1 +
 arch/arm/mach-pxa/colibri-pxa270-income.c |  1 +
 arch/arm/mach-pxa/ezx.c   |  1 +
 arch/arm/mach-pxa/hx4700.c|  1 +
 arch/arm/mach-pxa/lpd270.c|  1 +
 arch/arm/mach-pxa/magician.c  |  1 +
 arch/arm/mach-pxa/mainstone.c |  1 +
 arch/arm/mach-pxa/mioa701.c   |  1 +
 arch/arm/mach-pxa/palm27x.c   |  1 +
 arch/arm/mach-pxa/palmtc.c| 35 +--
 arch/arm/mach-pxa/palmte2.c   |  1 +
 arch/arm/mach-pxa/pcm990-baseboard.c  |  1 +
 arch/arm/mach-pxa/raumfeld.c  |  1 +
 arch/arm/mach-pxa/tavorevb.c  |  2 ++
 arch/arm/mach-pxa/viper.c |  1 +
 arch/arm/mach-pxa/z2.c|  2 ++
 arch/arm/mach-pxa/zylonite.c  |  1 +
 17 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/arch/arm/mach-pxa/cm-x300.c b/arch/arm/mach-pxa/cm-x300.c
index f942349..584439b 100644
--- a/arch/arm/mach-pxa/cm-x300.c
+++ b/arch/arm/mach-pxa/cm-x300.c
@@ -310,6 +310,7 @@ static struct platform_pwm_backlight_data 
cm_x300_backlight_data = {
.max_brightness = 100,
.dft_brightness = 100,
.pwm_period_ns  = 1,
+   .enable_gpio= -1,
 };
 
 static struct platform_device cm_x300_backlight_device = {
diff --git a/arch/arm/mach-pxa/colibri-pxa270-income.c 
b/arch/arm/mach-pxa/colibri-pxa270-income.c
index 2d4a7b4..3aa2646 100644
--- a/arch/arm/mach-pxa/colibri-pxa270-income.c
+++ b/arch/arm/mach-pxa/colibri-pxa270-income.c
@@ -189,6 +189,7 @@ static struct platform_pwm_backlight_data 
income_backlight_data = {
.max_brightness = 0x3ff,
.dft_brightness = 0x1ff,
.pwm_period_ns  = 100,
+   .enable_gpio= -1,
 };
 
 static struct platform_device income_backlight = {
diff --git a/arch/arm/mach-pxa/ezx.c b/arch/arm/mach-pxa/ezx.c
index fe2eb83..ab93441 100644
--- a/arch/arm/mach-pxa/ezx.c
+++ b/arch/arm/mach-pxa/ezx.c
@@ -54,6 +54,7 @@ static struct platform_pwm_backlight_data ezx_backlight_data 
= {
.max_brightness = 1023,
.dft_brightness = 1023,
.pwm_period_ns  = 78770,
+   .enable_gpio= -1,
 };
 
 static struct platform_device ezx_backlight_device = {
diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index 133109e..a7c30eb 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -561,6 +561,7 @@ static struct platform_pwm_backlight_data backlight_data = {
.max_brightness = 200,
.dft_brightness = 100,
.pwm_period_ns  = 30923,
+   .enable_gpio= -1,
 };
 
 static struct platform_device backlight = {
diff --git a/arch/arm/mach-pxa/lpd270.c b/arch/arm/mach-pxa/lpd270.c
index 1255ee0..9f6ec16 100644
--- a/arch/arm/mach-pxa/lpd270.c
+++ b/arch/arm/mach-pxa/lpd270.c
@@ -269,6 +269,7 @@ static struct platform_pwm_backlight_data 
lpd270_backlight_data = {
.max_brightness = 1,
.dft_brightness = 1,
.pwm_period_ns  = 78770,
+   .enable_gpio= -1,
 };
 
 static struct platform_device lpd270_backlight_device = {
diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index f44532f..fab30d6 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -378,6 +378,7 @@ static struct platform_pwm_backlight_data backlight_data = {
.max_brightness = 272,
.dft_brightness = 100,
.pwm_period_ns  = 30923,
+   .enable_gpio= -1,
.init   = magician_backlight_init,
.notify = magician_backlight_notify,
.exit   = magician_backlight_exit,
diff --git a/arch/arm/mach-pxa/mainstone.c b/arch/arm/mach-pxa/mainstone.c
index dd70343..08ccc07 100644
--- a/arch/arm/mach-pxa/mainstone.c
+++ b/arch/arm/mach-pxa/mainstone.c
@@ -338,6 +338,7 @@ static struct platform_pwm_backlight_data 
mainstone_backlight_data = {
.max_brightness = 1023,
.dft_brightness = 1023,
.pwm_period_ns  = 78770,
+   .enable_gpio= -1,
 };
 
 static struct platform_device mainstone_backlight_device = {
diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
index acc9d3c..f70583f 100644
--- a/arch/arm/mach-pxa/mioa701.c
+++ b/arch/arm/mach-pxa/mioa701.c
@@ -186,6 +186,7 @@ static struct platform_pwm_backlight_data 
mioa701_backlight_data = {
.max_brightness = 100,
.dft_brightness = 50,
.pwm_period_ns  = 4000 * 1024,  /* Fl = 250kHz */
+   .enable_gpio= -1,
 };
 
 /*
diff --git a/arch/arm/mach-pxa/palm27x.c b/arch/arm/mach-pxa

[PATCH 06/10] ARM: shmobile: Initialize PWM backlight enable_gpio field

[Thierry Reding]

The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/mach-shmobile/board-armadillo800eva.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c 
b/arch/arm/mach-shmobile/board-armadillo800eva.c
index 7f8f607..6ccb854 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -423,6 +423,7 @@ static struct platform_pwm_backlight_data 
pwm_backlight_data = {
.max_brightness = 255,
.dft_brightness = 255,
.pwm_period_ns = 3, /* 30kHz */
+   .enable_gpio = -1,
 };
 
 static struct platform_device pwm_backlight_device = {
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/10] pwm-backlight: Use new enable_gpio field

[Thierry Reding]

Make use of the new enable_gpio field and allow it to be set from DT as
well. Now that all legacy users of platform data have been converted to
initialize this field to an invalid value, it is safe to use the field
from the driver.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 .../bindings/video/backlight/pwm-backlight.txt |  3 ++
 drivers/video/backlight/pwm_bl.c   | 54 +++---
 2 files changed, 50 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..052eb03 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])
+  - enable-gpios: a list of GPIOs to enable and disable the backlight
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -25,4 +26,6 @@ Example:
 
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 6;
+
+   enable-gpios = gpio 58 0;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8a49b30..506810c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -10,6 +10,8 @@
  * published by the Free Software Foundation.
  */
 
+#include linux/gpio.h
+#include linux/of_gpio.h
 #include linux/module.h
 #include linux/kernel.h
 #include linux/init.h
@@ -27,6 +29,8 @@ struct pwm_bl_data {
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
+   int enable_gpio;
+   unsigned long   enable_gpio_flags;
int (*notify)(struct device *,
  int brightness);
void(*notify_after)(struct device *,
@@ -51,12 +55,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, 
int brightness,
 pb-lth_brightness;
 
pwm_config(pb-pwm, duty_cycle, pb-period);
+
+   if (gpio_is_valid(pb-enable_gpio)) {
+   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
+   gpio_set_value(pb-enable_gpio, 0);
+   else
+   gpio_set_value(pb-enable_gpio, 1);
+   }
+
pwm_enable(pb-pwm);
 }
 
 static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 {
pwm_disable(pb-pwm);
+
+   if (gpio_is_valid(pb-enable_gpio)) {
+   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
+   gpio_set_value(pb-enable_gpio, 1);
+   else
+   gpio_set_value(pb-enable_gpio, 0);
+   }
 }
 
 static int pwm_backlight_update_status(struct backlight_device *bl)
@@ -108,6 +127,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
  struct platform_pwm_backlight_data *data)
 {
struct device_node *node = dev-of_node;
+   enum of_gpio_flags flags;
struct property *prop;
int length;
u32 value;
@@ -148,11 +168,10 @@ 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.
-*/
+   data-enable_gpio = of_get_named_gpio_flags(node, enable-gpios, 0,
+   flags);
+   if (gpio_is_valid(data-enable_gpio)  (flags  OF_GPIO_ACTIVE_LOW))
+   data-enable_gpio_flags |= PWM_BACKLIGHT_GPIO_ACTIVE_LOW;
 
return 0;
 }
@@ -210,12 +229,30 @@ 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)) {
+   unsigned long flags;
+
+   if (pb-enable_gpio_flags  PWM_BACKLIGHT_GPIO_ACTIVE_LOW)
+   flags = GPIOF_OUT_INIT_HIGH;
+   else
+   flags = GPIOF_OUT_INIT_LOW;
+
+   ret = gpio_request_one(pb-enable_gpio, flags, enable);
+   if (ret  0) {
+   dev_err(pdev-dev, failed to request GPIO#%d: %d\n,
+   pb-enable_gpio, ret);
+   goto err_alloc

[PATCH 07/10] unicore32: Initialize PWM backlight enable_gpio field

[Thierry Reding]

The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/unicore32/kernel/puv3-nb0916.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/unicore32/kernel/puv3-nb0916.c 
b/arch/unicore32/kernel/puv3-nb0916.c
index 181108b..0c6618e 100644
--- a/arch/unicore32/kernel/puv3-nb0916.c
+++ b/arch/unicore32/kernel/puv3-nb0916.c
@@ -54,6 +54,7 @@ static struct platform_pwm_backlight_data 
nb0916_backlight_data = {
.max_brightness = 100,
.dft_brightness = 100,
.pwm_period_ns  = 70 * 1024,
+   .enable_gpio= -1,
 };
 
 static struct gpio_keys_button nb0916_gpio_keys[] = {
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10] pwm-backlight: Refactor backlight power on/off

[Thierry Reding]

In preparation for adding an optional regulator and enable GPIO to the
driver, split the power on and power off sequences into separate
functions to reduce code duplication at the multiple call sites.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 drivers/video/backlight/pwm_bl.c | 59 
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 36db5d9..8a49b30 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -35,6 +35,30 @@ struct pwm_bl_data {
void(*exit)(struct device *);
 };
 
+static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness,
+  int max)
+{
+   int duty_cycle, err;
+
+   if (pb-levels) {
+   duty_cycle = pb-levels[brightness];
+   max = pb-levels[max];
+   } else {
+   duty_cycle = brightness;
+   }
+
+   duty_cycle = (duty_cycle * (pb-period - pb-lth_brightness) / max) +
+pb-lth_brightness;
+
+   pwm_config(pb-pwm, duty_cycle, pb-period);
+   pwm_enable(pb-pwm);
+}
+
+static void pwm_backlight_power_off(struct pwm_bl_data *pb)
+{
+   pwm_disable(pb-pwm);
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
struct pwm_bl_data *pb = bl_get_data(bl);
@@ -49,24 +73,10 @@ static int pwm_backlight_update_status(struct 
backlight_device *bl)
if (pb-notify)
brightness = pb-notify(pb-dev, brightness);
 
-   if (brightness == 0) {
-   pwm_config(pb-pwm, 0, pb-period);
-   pwm_disable(pb-pwm);
-   } else {
-   int duty_cycle;
-
-   if (pb-levels) {
-   duty_cycle = pb-levels[brightness];
-   max = pb-levels[max];
-   } else {
-   duty_cycle = brightness;
-   }
-
-   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);
-   }
+   if (brightness  0)
+   pwm_backlight_power_on(pb, brightness, max);
+   else
+   pwm_backlight_power_off(pb);
 
if (pb-notify_after)
pb-notify_after(pb-dev, brightness);
@@ -267,10 +277,11 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
struct pwm_bl_data *pb = bl_get_data(bl);
 
backlight_device_unregister(bl);
-   pwm_config(pb-pwm, 0, pb-period);
-   pwm_disable(pb-pwm);
+   pwm_backlight_power_off(pb);
+
if (pb-exit)
pb-exit(pdev-dev);
+
return 0;
 }
 
@@ -282,10 +293,12 @@ static int pwm_backlight_suspend(struct device *dev)
 
if (pb-notify)
pb-notify(pb-dev, 0);
-   pwm_config(pb-pwm, 0, pb-period);
-   pwm_disable(pb-pwm);
+
+   pwm_backlight_power_off(pb);
+
if (pb-notify_after)
pb-notify_after(pb-dev, 0);
+
return 0;
 }
 
@@ -294,6 +307,7 @@ static int pwm_backlight_resume(struct device *dev)
struct backlight_device *bl = dev_get_drvdata(dev);
 
backlight_update_status(bl);
+
return 0;
 }
 #endif
@@ -317,4 +331,3 @@ module_platform_driver(pwm_backlight_driver);
 MODULE_DESCRIPTION(PWM based Backlight Driver);
 MODULE_LICENSE(GPL);
 MODULE_ALIAS(platform:pwm-backlight);
-
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10] ARM: OMAP: Initialize PWM backlight enable_gpio field

[Thierry Reding]

The GPIO API defines 0 as being a valid GPIO number, so this field needs
to be initialized explicitly.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 arch/arm/mach-omap2/board-zoom-peripherals.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c 
b/arch/arm/mach-omap2/board-zoom-peripherals.c
index a90375d..5ed2884 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c
@@ -227,6 +227,7 @@ static struct platform_pwm_backlight_data 
zoom_backlight_data = {
.max_brightness = 127,
.dft_brightness = 127,
.pwm_period_ns = 7812500,
+   .enable_gpio = -1,
 };
 
 static struct platform_device zoom_backlight_pwm = {
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10] pwm-backlight: Add optional enable GPIO

[Thierry Reding]

To support a wider variety of backlight setups, introduce an optional
enable GPIO. Legacy users of the platform data already have a means of
supporting GPIOs by using the .init(), .exit() and .notify() hooks. DT
users however cannot use those, so an alternative method is required.

In order to ease the introduction of the optional enable GPIO, make it
available in the platform data first, so that existing users can be
converted. Once that has happened a second patch will add code to make
use of it in the driver.

Signed-off-by: Thierry Reding tred...@nvidia.com
---
 include/linux/pwm_backlight.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2de2e27 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -6,6 +6,9 @@
 
 #include linux/backlight.h
 
+/* TODO: convert to gpiod_*() API once it has been merged */
+#define PWM_BACKLIGHT_GPIO_ACTIVE_LOW  (1  0)
+
 struct platform_pwm_backlight_data {
int pwm_id;
unsigned int max_brightness;
@@ -13,6 +16,8 @@ struct platform_pwm_backlight_data {
unsigned int lth_brightness;
unsigned int pwm_period_ns;
unsigned int *levels;
+   int enable_gpio;
+   unsigned long enable_gpio_flags;
int (*init)(struct device *dev);
int (*notify)(struct device *dev, int brightness);
void (*notify_after)(struct device *dev, int brightness);
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] i2c: move of helpers into the core

[Thierry Reding]

On Mon, Aug 19, 2013 at 07:59:40PM +0200, Wolfram Sang wrote:
[...]
 diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
[...]
 +#if IS_ENABLED(CONFIG_OF)
 +static void of_i2c_register_devices(struct i2c_adapter *adap)
 +{
[...]
 +}
[...]
 +#endif /* CONFIG_OF */

Isn't this missing the dummy implementation for !OF.

  static int i2c_do_add_adapter(struct i2c_driver *driver,
 struct i2c_adapter *adap)
  {
 @@ -1058,6 +1160,8 @@ static int i2c_register_adapter(struct i2c_adapter 
 *adap)
  
  exit_recovery:
   /* create pre-declared device nodes */
 + of_i2c_register_devices(adap);

Alternatively you could remove the of_i2c_register_devices() from the
#ifdef CONFIG_OF block so that it will always be compiled. You could
turn the above into

if (IS_ENABLED(CONFIG_OF))
of_i2c_register_devices(adap);

and let the compiler throw the static function away if it sees that the
condition is always false.

Thierry


pgpbXf7EO5KsX.pgp
Description: PGP signature

Re: [PATCH v2 0/4] Add PWM polarity flag macro for DT

[Thierry Reding]

On Thu, Jul 18, 2013 at 12:54:20AM +0200, Laurent Pinchart wrote:
 Hello,
 
 Here's a small patch set that replaces PWM polarity numerical constants with
 macros in DT.
 
 The series is the result of splitting the original patch into four patches
 that
 
 - add the flag macro (both in a header file and in the PWM DT binding core
   documentation)
 - use the macro in the PWM core code
 - update existing DT bindings to refer to the PWM DT bindings core
   documentation
 - update existing DT sources to use the new macro
 
 I believe I've taken all comments received so far into account. Most notable
 changes include
 
 - splitting the original patch
 - removing the PWM_POLARITY_NORMAL flag, which wasn't a flag but was defined
   as 0
 - renaming the PWM_POLARITY_INVERSED DT flag to PWM_POLARITY_INVERTED
 - not relying on DT flags and PWM C flags having identical names and values
 
 Laurent Pinchart (4):
   pwm: Add PWM polarity flag macro for DT
   pwm: Use the DT macro directly when parsing PWM DT flags
   pwm: Update DT bindings to reference pwm.txt for cells documentation
   ARM: dts: Use the PWM polarity flags
 
  Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt|  8 +++-
  Documentation/devicetree/bindings/pwm/imx-pwm.txt  |  4 ++--
  Documentation/devicetree/bindings/pwm/mxs-pwm.txt  |  4 ++--
  .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt |  5 ++---
  Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt  |  4 ++--
  Documentation/devicetree/bindings/pwm/pwm-samsung.txt  | 10 +++---
  Documentation/devicetree/bindings/pwm/pwm-tiecap.txt   |  8 +++-
  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt |  8 +++-
  Documentation/devicetree/bindings/pwm/pwm.txt  |  7 ---
  Documentation/devicetree/bindings/pwm/spear-pwm.txt|  5 ++---
  Documentation/devicetree/bindings/pwm/ti,twl-pwm.txt   |  4 ++--
  Documentation/devicetree/bindings/pwm/ti,twl-pwmled.txt|  4 ++--
  Documentation/devicetree/bindings/pwm/vt8500-pwm.txt   |  8 +++-
  arch/arm/boot/dts/am335x-evmsk.dts |  3 ++-
  arch/arm/boot/dts/wm8850-w70v2.dts |  3 ++-
  drivers/pwm/core.c |  7 +++
  include/dt-bindings/pwm/pwm.h  | 14 
 ++
  17 files changed, 54 insertions(+), 52 deletions(-)
  create mode 100644 include/dt-bindings/pwm/pwm.h

Applied, thanks.

Thierry


pgpsqWUj7fotS.pgp
Description: PGP signature

Re: [PATCH v2 0/4] Add PWM polarity flag macro for DT

[Thierry Reding]

On Fri, Jul 19, 2013 at 01:29:13PM +0200, Laurent Pinchart wrote:
 Hi Stephen,
 
 On Thursday 18 July 2013 10:55:56 Stephen Warren wrote:
  On 07/17/2013 04:54 PM, Laurent Pinchart wrote:
   Hello,
   
   Here's a small patch set that replaces PWM polarity numerical constants
   with macros in DT.
  
  The series,
  Reviewed-by: Stephen Warren swar...@nvidia.com
  
  I'm (very very) slightly hesitant about patch 3/4, since it's moving towards
  all PWMs having to use the same specifier format, whereas specifiers are at
  least potentially binding-specific, not device-type-specific. However,
  consistency is good; there's no need to do something different just for the
  heck of it. Equally, there's nothing actually stopping a new binding from
  defining its own format rather than simply deferring to pwm.txt if it
  absolutely has to, so I think this will turn out fine.
 
 Exactly, that's why I don't think it's an issue. pwm.txt defines a common 
 format, individual bindings are free to use it or not.
 
 Thierry, if you're fine with the patches, could you take them in your tree 
 with Stephen's Reviewed-by, or should I report them and send you a pull 
 request ?

They look good to me. I'll take them into my tree and add Stephen's
Reviwed-by. It might take me another week, though, as I'm currently
rather busy with other things.

Thierry


signature.asc
Description: Digital signature

Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Thierry Reding]

On Wed, Jul 17, 2013 at 11:11:19AM -0600, Stephen Warren wrote:
 On 07/17/2013 05:00 AM, Laurent Pinchart wrote:
  On Monday 15 July 2013 21:39:31 Stephen Warren wrote:
 ...
  But then there's a problem where people assume that the common flags are
  always available, and somewhere they aren't... Care is needed in the
  choice of which common flags to define and/or how they're used.
  
  Exactly. That's why I think listing the supported common flags in 
  individual 
  bindings makes sense when some of the flags are not supported by all 
  devices. 
  As the only PWM flags currently used are common to all PWM devices I can 
  leave 
  this out now. I have no strong preference, I'll follow your opinion on this.
 
 Yes, I guess separating the concept of defining common flags and which
 devices use them is good. And then indeed individual devices need to
 define which of the common flags they support. I'd still like to see the
 *definition* of those common flags in some central place (i.e. pwm.txt
 or a header that defines constants for it), and the other device
 bindings simply reference that for the actual definitions.

That sounds reasonable to me.

Thierry


signature.asc
Description: Digital signature

Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Thierry Reding]

On Fri, Jul 12, 2013 at 08:40:07AM -0600, Stephen Warren wrote:
 On 07/12/2013 04:41 AM, Laurent Pinchart wrote:
  Hi Stephen,
[...]
  What about splitting it in three patches that
  
  - add the include/dt-bindings/pwm/pwm.h header, and update 
  include/linux/pwm.h 
  and Documentation/devicetree/bindings/pwm/pwm.txt
  
  - update the rest of the documentation
  
  - update the .dts files
 
 I think that sounds reasonable.

Shouldn't the addition of include/dt-bindings/pwm/pwm.h be separate from
its inclusion in include/linux/pwm.h so that it can be moved more easily
(cherry-picked) to a separate repository?

  diff --git a/include/linux/pwm.h b/include/linux/pwm.h
 
   enum pwm_polarity {
 
  - PWM_POLARITY_NORMAL,
  - PWM_POLARITY_INVERSED,
  + PWM_POLARITY_NORMAL = 0,
  + PWM_POLARITY_INVERSED = 1,
 
   };
 
  Rather than manually editing that to ensure the enum matches the DT 
  bindings
  header, the whole point of making a separate dt-bindings/... directory 
  was
  that drivers could include the binding header files directly to avoid 
  having
  to duplicate the constant definitions. Can't linux/pwm.h include dt-
  bindings/pwm.h and remove that enum?
  
  We could do that, but we would then need to modify all drivers to replace 
  enum_pwm_polarity with unsigned int. Thierry, what's your opinion on this ?
 
 Or perhaps we could keep the enums around, but force the values to match
 the DT constants:
 
 enum pwm_polarity {
   PWM_POLARITY_NORMAL = PWM_POLARITY_NORMAL,
   PWM_POLARITY_INVERTED = PWM_POLARITY_INVERTED,
 };
 
 (although obviously you'd need to avoid the enum and DT constants having
 the same name).

I think I've seen stuff like the following done in a few header files to
keep compatibility between enums and defines.

enum foo {
BAR,
#define BAR BAR
BAZ,
#define BAZ BAZ
};

Which, as I understand it, won't work in this case because DTC can only
cope with plain cpp files?

 Although this brings up one point: let's say we support ACPI/.. bindings
 in the future. The enum possibly can't match the binding values from
 every different kind of binding definition (DT, ACPI, ...) so perhaps
 rather than changing the enum definition in linux/pwm.h, what we
 should be doing is mapping between the different name-spaces in whatever
 of_xlate function exists for the PWM flags cell. That would be more
 flexible.

I'm not quite sure what exactly you are suggesting here. Can you
elaborate?

Thierry


signature.asc
Description: Digital signature

Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Thierry Reding]

On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote:
[...]
 diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt 
 b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
 index de0eaed..be09be4 100644
 --- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
 +++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
 @@ -4,9 +4,9 @@ Required properties:
  - compatible: should be atmel,tcb-pwm
  - #pwm-cells: Should be 3.  The first cell specifies the per-chip index
of the PWM to use, the second cell is the period in nanoseconds and
 -  bit 0 in the third cell is used to encode the polarity of PWM output.
 -  Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity 
 -  set to 0 for normal polarity.
 +  the third cell is used to encode the polarity of PWM output. Set the
 +  PWM_POLARITY_NORMAL flag for normal polarity or the PWM_POLARITY_INVERSED
 +  flag for inverted polarity. PWM flags are defined in 
 dt-bindings/pwm/pwm.h.
  - tc-block: The Timer Counter block to use as a PWM chip.
  
  Example:

I'd prefer for the original text to stay in place and the reference to
the dt-bindings/pwm/pwm.h file to go below that block. The reason is
that perhaps somebody will look at an older version of a given DT (with
the numerical value) and not have access to the include and therefore
not know which flag was being set by just looking at the documentation.
I'm also not sure about what the plans are with respect to shipping
device trees in a separate repository and if they are, whether that
repository would ship the includes as well.

Another issue might be that people without access to recent versions of
DTC won't be able to use the new #include feature, so keeping the
documentation backwards compatible seems like a good idea.

Perhaps the include file should even only be mentioned in the general
PWM binding documentation.

Perhaps Grant and Rob (Cc'ed) can comment on how they want to handle
this.

 diff --git a/Documentation/devicetree/bindings/pwm/pwm-samsung.txt 
 b/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
 index ac67c68..bece18b 100644
 --- a/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
 +++ b/Documentation/devicetree/bindings/pwm/pwm-samsung.txt
 @@ -24,8 +24,9 @@ Required properties:
   - phandle to PWM controller node
   - index of PWM channel (from 0 to 4)
   - PWM signal period in nanoseconds
 - - bitmask of optional PWM flags:
 -0x1 - invert PWM signal
 + - bitmask of optional PWM flags as defined in dt-bindings/pwm/pwm.h:
 +PWM_POLARITY_NORMAL - normal polarity
 + PWM_POLARITY_INVERSED - inverted polarity

This part mixes spaces and tabs for indentation. Please stick to spaces.

 diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt 
 b/Documentation/devicetree/bindings/pwm/pwm.txt
 index 06e6724..38c357a 100644
 --- a/Documentation/devicetree/bindings/pwm/pwm.txt
 +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
 @@ -43,13 +43,15 @@ because the name backlight would be used as fallback 
 anyway.
  pwm-specifier typically encodes the chip-relative PWM number and the PWM
  period in nanoseconds.
  
 -Optionally, the pwm-specifier can encode a number of flags in a third cell:
 -- bit 0: PWM signal polarity (0: normal polarity, 1: inverse polarity)
 +Optionally, the pwm-specifier can encode a number of flags (defined in
 +dt-bindings/gpio/gpio.h) in a third cell:
 +- PWM_POLARITY_NORMAL: use the normal PWM signal polarity
 +- PWM_POLARITY_INVERSED: invert the PWM signal polarity

You'd have a hard time finding those in the GPIO header. =)

 diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
 new file mode 100644
 index 000..f82be30
 --- /dev/null
 +++ b/include/dt-bindings/pwm/pwm.h
 @@ -0,0 +1,15 @@
 +/*
 + * This header provides constants for most PWM bindings.
 + *
 + * Most PWM bindings can include a flags cell as part of the PWM specifier.
 + * In most cases, the format of the flags cell uses the standard values
 + * defined in this header.
 + */
 +
 +#ifndef _DT_BINDINGS_PWM_PWM_H
 +#define _DT_BINDINGS_PWM_PWM_H
 +
 +#define PWM_POLARITY_NORMAL  (0  0)
 +#define PWM_POLARITY_INVERSED(1  0)
 +
 +#endif

I think this should go into a patch separate from the DT changes above
because they'll likely go in via different trees. Or maybe they won't,
but it'd still be good to introduce the header first and only then
change the DTS files.

 diff --git a/include/linux/pwm.h b/include/linux/pwm.h
[...]
  enum pwm_polarity {
 - PWM_POLARITY_NORMAL,
 - PWM_POLARITY_INVERSED,
 + PWM_POLARITY_NORMAL = 0,
 + PWM_POLARITY_INVERSED = 1,

That's what the enum values will be by default, so there's no need to be
explicit here.

Thierry


signature.asc
Description: Digital signature

Re: [PATCH 2/2] pwm: Add PWM polarity flag macros for DT

[Thierry Reding]

On Thu, Jul 11, 2013 at 11:50:48AM -0600, Stephen Warren wrote:
 On 07/11/2013 09:36 AM, Thierry Reding wrote:
  On Thu, Jul 11, 2013 at 04:37:48PM +0200, Laurent Pinchart wrote: 
  [...]
  diff --git
  a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
  b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt index
  de0eaed..be09be4 100644 ---
  a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt +++
  b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt @@ -4,9
  +4,9 @@ Required properties: - compatible: should be
  atmel,tcb-pwm - #pwm-cells: Should be 3.  The first cell
  specifies the per-chip index of the PWM to use, the second cell
  is the period in nanoseconds and -  bit 0 in the third cell is
  used to encode the polarity of PWM output. -  Set bit 0 of the
  third cell in PWM specifier to 1 for inverse polarity  -  set to
  0 for normal polarity. +  the third cell is used to encode the
  polarity of PWM output. Set the +  PWM_POLARITY_NORMAL flag for
  normal polarity or the PWM_POLARITY_INVERSED +  flag for inverted
  polarity. PWM flags are defined in dt-bindings/pwm/pwm.h. -
  tc-block: The Timer Counter block to use as a PWM chip.
  
  Example:
  
  I'd prefer for the original text to stay in place and the reference
  to the dt-bindings/pwm/pwm.h file to go below that block.
 
 I disagree here. The whole point of creating header files for the
 constants in binding definitions was so that you wouldn't have to
 duplicate all the values into the binding definitions. Rather, you'd
 simply say see dt-bindings/xxx.h.

But that's not something that this patch solves. And it could be solved
even in the absence of the header file defining the symbolic constants.
If all the standard flags that dt-bindings/pwm/pwm.txt now specifies
were to be listed in pwm.txt (they actually are) then referring to that
document as the canonical source works equally well.

  The reason is that perhaps somebody will look at an older version
  of a given DT (with the numerical value) and not have access to the
  include and therefore not know which flag was being set by just
  looking at the documentation.
 
 It's pretty trivial to find the header that defines the values; just
 grab the latest source. If you're looking at an old version of the
 .dts file, it's almost certain that's within the context of an old
 Linux kernel soruce tree, in which case you trivially have access to
 the old version of the binding document that spelled out the values.
 
  I'm also not sure about what the plans are with respect to
  shipping device trees in a separate repository and if they are,
  whether that repository would ship the includes as well.
 
 It would have to. That separate repository would be upstream for
 dt-bindings/ files, which would be imported into the kernel
 periodically as drivers wanted to make use of any new headers/values
 defined there.

If we can take both of the above for granted, then sure, let's refer to
the header from within the generic pwm.txt file and add a reference to
that in bindings for drivers that use the standard flags.

  Another issue might be that people without access to recent
  versions of DTC won't be able to use the new #include feature, so
  keeping the documentation backwards compatible seems like a good
  idea.
 
 The dtc source tree is duplicated into the kernel source tree, so that
 isn't an issue for now.
 
 Besides, the dtc version is an entirely unrelated issue to how the
 documentation is written.

Well, not really. If the documentation specifies the binding in a way
that the DTC can't handle that's still a problem. People will end up
with a DTS that their DTC can't compile. I guess that can be resolved
by adding a note to the upstream device tree repository about the
minimum required version of DTC.

  Perhaps the include file should even only be mentioned in the
  general PWM binding documentation.
 
 I wondered about that too. It's slightly indirect in that you'd read
 foo-pwm.txt which would say something like:
 
 -
 the third cell encodes standard PWM flags, as defined by the core PWM
 binding in pwm.txt in this directory.
 -
 
 and pwm.txt would say:
 
 -
 The PWM specifier should include a flags cell to contain standard PWM
 flags. Values for that cell are defined in dt-bindings/pwm/pwm.h.
 -
 
 Which is somewhat like following a lot of symlinks. Still, I agree
 that's arguably the correct thing to do.

Yes, I like this a whole lot better than the current situation. It gets
us almost automatic consistency, which is harder to do with the current
approach.

Thierry


signature.asc
Description: Digital signature

Re: Latest randconfig build errors

[Thierry Reding]

On Sun, Apr 14, 2013 at 12:29:46AM -0400, Rob Clark wrote:
 On Sat, Apr 13, 2013 at 5:45 PM, Thierry Reding
[...]
  I had been thinking about this on and off for a while, but I haven't
  come up with anything concrete. Ideally we could just have some kind of
  event that userspace would listen for, so that new outputs can be
  dynamically added and userspace informed about them. Last time I checked
  most of the helpers assumed that the complete output configuration is
  known when the DRM device is registered, so some major rework will be
  required to efficiently make use of such dynamicity.
 
 I'm less worried about the kernel re-work.. more worried about the
 fact that we have no way to know whether userspace knows to listen for
 this new event.  So anything down this path could, I think, be
 considered as breaking userspace.

Yes, that's probably true. Although the typical use-case would be that
the application would run on any of the detected outputs and if there is
none to begin with it will just fail. If there already is one, it will
try to use that instead and only fail to use a second one if it becomes
available.

 I think in the end, we need some way to have sort of dummy
 connectors for output drivers which might or might not be probed, so
 that from userspace perspective, non-present panels appear as displays
 that are not plugged in.

I can imagine that quite a few userspace programs will be broken by this
as well. For instance if you have an LVDS panel, I'm pretty sure people
will just assume that it can't be in a disconnected state and therefore
won't deal with that case.

From a driver's point of view this isn't all that different from the
case you're trying to solve now. You still need to find out which
outputs can eventually become available. But instead of waiting until
they do before registering the DRM device you'd have to create dummy
outputs and keep checking whether the device is actually there each time
somebody interacts with the output.

But maybe this case is different. For Tegra the problem is that some
outputs (such as HDMI) require other drivers to be loaded first because
they provide resources (regulators, GPIOs, ...) that the output drivers
require. All of the drivers use deferred probing to resolve this
dependency and we can be reasonably sure that eventually probing of the
output will succeed (and therefore the output will become available).
Postponing the DRM registration until that point doesn't hurt. From what
I understand your use-case is similar. You need to load drivers for
different panels based on a runtime decision. So the only thing you need
is to be able to postpone the DRM registration until the panel driver
has been loaded, right?

Thierry


pgp21kauR35ES.pgp
Description: PGP signature

Re: Latest randconfig build errors

[Thierry Reding]

On Sat, Apr 13, 2013 at 08:54:22AM -0400, Rob Clark wrote:
 On Mon, Mar 4, 2013 at 1:46 PM, Tony Lindgren t...@atomide.com wrote:
 
  drivers/gpu/drm/tilcdc/tilcdc_slave.o:(.data+0x54): multiple definition of 
  `__mod_of_device_table'
  drivers/gpu/drm/tilcdc/tilcdc_tfp410.o:(.data+0x54): first defined here
  drivers/gpu/drm/tilcdc/tilcdc_panel.o:(.data+0x54): multiple definition of 
  `__mod_of_device_table'
  drivers/gpu/drm/tilcdc/tilcdc_tfp410.o:(.data+0x54): first defined here
  drivers/gpu/drm/tilcdc/tilcdc_drv.o:(.data+0x184): multiple definition of 
  `__mod_of_device_table'
  drivers/gpu/drm/tilcdc/tilcdc_tfp410.o:(.data+0x54): first defined here
 
  Rob, I assume you'll do a patch for this one?
 
 
 oh, I apologize for the late reply, I didn't see this email...
 
 There is a patch that we can merge to make tilcdc bool instead of
 tristate[1], which I suppose is ok for a temporary fix.  Although I'm
 all-ears if someone has a better idea about how to fix this.  The
 problem is that we have multiple sub-devices for different possible
 panel drivers, so that depending on DT tables, the correct ones get
 loaded for the hw.  But they are all built into a single module.
 Splitting them into multiple modules will be problematic, as panel
 drivers which are present really need to get probed before the
 toplevel drm device..

You could look at the Tegra driver. I had to solve a similar problem
there. What I did is basically parse the DT in the host1x driver and add
all device nodes which are required by DRM to a list. Later when the
individual devices are probed they are removed from that list, so that
when the list becomes empty we are sure that all required devices are
there and only then call the drm_platform_init() function.

This fits very well with how Tegra hardware is designed because host1x
is the parent for all DRM subdevices (DC, RGB/LVDS, HDMI, ...). So it is
probed before any of its children and it can easily parse the DT upfront
and initialize the list of required devices.

 I suppose in theory it is possible to make drm
 cope better with dynamically loaded outputs, but I'm not sure that
 there is any way to do this without breaking userspace which expects
 that all of the connectors/encoders are present once the drm device is
 loaded.

I had been thinking about this on and off for a while, but I haven't
come up with anything concrete. Ideally we could just have some kind of
event that userspace would listen for, so that new outputs can be
dynamically added and userspace informed about them. Last time I checked
most of the helpers assumed that the complete output configuration is
known when the DRM device is registered, so some major rework will be
required to efficiently make use of such dynamicity.

Thierry


pgpUedh6e2UTT.pgp
Description: PGP signature

Re: [PATCH V3 1/2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Mon, Apr 08, 2013 at 03:16:57PM -0700, Tony Lindgren wrote:
 * Thierry Reding thierry.red...@avionic-design.de [130408 15:01]:
  On Mon, Apr 08, 2013 at 02:46:24PM -0700, Tony Lindgren wrote:
   * Andrew Chew ac...@nvidia.com [130313 15:37]:
The pwm-backlight driver now takes a mandatory regulator that is gotten
during driver probe.  Initialize a dummy regulator to satisfy this
requirement.

Signed-off-by: Andrew Chew ac...@nvidia.com
---
Changed the device name of the backlight regulator supply to 
pwm-backlight,
per Peter's comment.

Changed the name of the regulator to backlight-enable, per Thierry's
suggestion.
   
   Thanks applying into omap-for-v3.10/board. Note that I'm not taking the
   second one, that should be resent to the related driver maintainers.
   You can get that list by running scripts/get_maintainer.pl against it.
  
  The plan was to take these all through one tree, preferably the PWM tree
  because it's all PWM related and the pwm-backlight change will go
  through that tree as well. Technically these individual patches can be
  applied as is and aren't harmful, but keeping track of the dependencies
  might be difficult if they go in via separate trees.
 
 Registering the regulator alone should not do anything. Also the driver
 should do the right thing if the regulator is not yet registered.
 
 Can you please check your driver patch so the driver won't do anything
 if the regulator is not (yet) registered and repost it alone as I've
 already applied the board-*.c change.

That's not the way that the regulator subsystem works, unfortunately.
There's no way you can distinguish between the case where a regulator
just hasn't been registered yet, or whether it's missing. That's the
whole reason why we need to add the dummy regulators in the first
place.

 The reason why we want to do queue these patches separately is to cut
 away the dependency between drivers and the core arch/arm changes. 
 Otherwise we'll end up with pointless merge conflicts as we've seen
 earlier several times with the USB patches for example.

We could possibly postpone merging the pwm-backlight changes until all
other patches have been merged. If you have this in you for-3.10 branch
I guess it will go into linux-next when the 3.9 merge window closes? If
so I could possibly base my for-3.10 branch on your branch if you can
provide a stable one that you can guarantee not to rebase. There are
other alternatives too, but certainly the easiest would've been to take
all patches through the PWM tree. The potential for merge conflicts
should be rather minimal.

Thierry


pgpeLKFSlr5GV.pgp
Description: PGP signature

Re: [PATCH V3 1/2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Tue, Apr 09, 2013 at 09:40:04AM -0700, Tony Lindgren wrote:
[...]
 But then the regulator is not found and the driver should just exit,
 or do nothing. If this is an optional regulator, then that should be
 indicated in some platform data flags?

Yes, if the regulator isn't found then the driver fails. However the
goal was to maintain bisectability. If we apply them in the wrong order
we can't guarantee that because pwm-backlight will fail to work between
both patches.

 The driver parts really must be done in independently from any platform
 data or .dts changes. The only common part needed should be changes
 to include/linux/platform_data/*.h files.

We don't even need to touch platform data because the regulators are
looked up via a global table. And the changes are all done independently
but as I mentioned above, bisectability isn't maintained, so the
preferred patch order is the one in which pwm-backlight keeps working at
each point in the commit history.

Thierry


pgpObfBiWBDf0.pgp
Description: PGP signature

Re: [PATCH V3 1/2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Tue, Apr 09, 2013 at 01:17:46PM -0700, Tony Lindgren wrote:
 * Thierry Reding thierry.red...@avionic-design.de [130409 12:45]:
  On Tue, Apr 09, 2013 at 09:40:04AM -0700, Tony Lindgren wrote:
  [...]
   But then the regulator is not found and the driver should just exit,
   or do nothing. If this is an optional regulator, then that should be
   indicated in some platform data flags?
  
  Yes, if the regulator isn't found then the driver fails. However the
  goal was to maintain bisectability. If we apply them in the wrong order
  we can't guarantee that because pwm-backlight will fail to work between
  both patches.
 
 But it's fixing something that's not working anyways for board-4430sdp.c,
 It seems so as these patches just add new features?

Not quite. It's adding a dummy regulator to represent hardware where the
enable pin is always high. So I think pwm-backlight will work in the
current state, but if we make the pwm-backlight driver change without
adding the dummy regulator, then pwm-backlight will fail to probe and
therefore the PWM won't be enabled either and the display will stay
black.

   The driver parts really must be done in independently from any platform
   data or .dts changes. The only common part needed should be changes
   to include/linux/platform_data/*.h files.
  
  We don't even need to touch platform data because the regulators are
  looked up via a global table. And the changes are all done independently
  but as I mentioned above, bisectability isn't maintained, so the
  preferred patch order is the one in which pwm-backlight keeps working at
  each point in the commit history.
 
 Bisectability is a good point. But in the 4430sdp case I'm sure it's enough
 that it builds and boots no matter how the patches get merged :)

If you don't mind some intermediary breakage, I will no longer object.

Thierry


pgpTcneQMYGuc.pgp
Description: PGP signature

Re: [PATCH V3 1/2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Mon, Apr 08, 2013 at 02:46:24PM -0700, Tony Lindgren wrote:
 * Andrew Chew ac...@nvidia.com [130313 15:37]:
  The pwm-backlight driver now takes a mandatory regulator that is gotten
  during driver probe.  Initialize a dummy regulator to satisfy this
  requirement.
  
  Signed-off-by: Andrew Chew ac...@nvidia.com
  ---
  Changed the device name of the backlight regulator supply to 
  pwm-backlight,
  per Peter's comment.
  
  Changed the name of the regulator to backlight-enable, per Thierry's
  suggestion.
 
 Thanks applying into omap-for-v3.10/board. Note that I'm not taking the
 second one, that should be resent to the related driver maintainers.
 You can get that list by running scripts/get_maintainer.pl against it.

The plan was to take these all through one tree, preferably the PWM tree
because it's all PWM related and the pwm-backlight change will go
through that tree as well. Technically these individual patches can be
applied as is and aren't harmful, but keeping track of the dependencies
might be difficult if they go in via separate trees.

Thierry


pgp5neLsaWD6U.pgp
Description: PGP signature

Re: [PATCH 1/1] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Tue, Mar 12, 2013 at 12:11:54PM -0700, Andrew Chew wrote:
  From: Thierry Reding [mailto:thierry.red...@avionic-design.de]
  Sent: Tuesday, March 12, 2013 12:01 AM
  To: Andrew Chew
  Cc: peter.ujfal...@ti.com; Alex Courbot; linux-omap@vger.kernel.org
  Subject: Re: [PATCH 1/1] ARM: OMAP: board-4430sdp: Provide regulator to
  pwm-backlight
  
  * PGP Signed by an unknown key
  
  On Mon, Mar 11, 2013 at 06:54:26PM -0700, Andrew Chew wrote:
   The pwm-backlight driver now takes a mandatory regulator that is
   gotten during driver probe.  Initialize a dummy regulator to satisfy
   this requirement.
  
   Signed-off-by: Andrew Chew ac...@nvidia.com
   ---
   This patch, along with many more soon to follow, attempts to satisfy
   the new mandatory regulator that pwm-backlight will grab during probe.
   The only board in mach-omap2 to use the pwm-backlight appears to be the
  4430sdp.
  
   I thought I'd start small and use this board as an example.
  
   I tested similar code in my Tegra board, so it should be okay.  Of
   course, I don't have a 4430sdp to test with.
  
  Hi Andrew,
  
  This looks good, one minor comment below. I think it might make sense to
  post this patch as part of a series that includes the change to the pwm-
  backlight driver. This will make things easier on potential testers.
  You can later extend that series to include all the other users and 
  eventually
  the whole series can be merged.
 
 Do you mean I should do a single patch series, accumulating other boards
 as I go?

Yes, that way everything needed will be in one patch series and people
can easily apply that to their trees. It will also make things easier if
we decide to merge all of them through one tree in the end.

 And by the way, do I have your reviewed-by for the pwm-backlight regulator
 patch?

I think it looks good, but I'll go over it again in your repost of the
whole series. I don't think you need my Reviewed-by, though, since I
maintain the pwm-backlight driver. =)

Thierry


pgpgrlZEo63Kk.pgp
Description: PGP signature

Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Thierry Reding]

On Wed, Mar 13, 2013 at 09:56:57AM +0100, Peter Ujfalusi wrote:
 On 03/12/2013 11:22 PM, Andrew Chew wrote:
[...]
  +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;
  +}
 
 Would it not be better to have some locking here since the code started to use
 flag for state tracking?

I don't think that's necessary. The backlight core already uses the
ops_lock mutex to serial accesses. I just noticed that the documentation
mentions that update_lock is used for this purpose, but the code doesn't
use it after it is initialized. Still, the ops_lock should be enough.

Thierry


pgp0kA3PVkoTc.pgp
Description: PGP signature

Re: [PATCH 1/2 v2] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Wed, Mar 13, 2013 at 01:38:31PM -0700, Andrew Chew wrote:
   +/* Dummy regulator for pwm-backlight driver */ static struct
   +regulator_consumer_supply backlight_supply =
   + REGULATOR_SUPPLY(enable, NULL);
  
  'enable' is just too generic, the device name should be also provided:
  REGULATOR_SUPPLY(enable, pwm-backlight);
 
 You're right.  I don't like how generic it is as well.  But I don't think
 pwm-backlight works...at least, not for me when I test it.  What
 does work is backlight.x where x is some number (for me, it's 1).
 Problem is, I don't know what it would be for you.  If only there
 was a way to wildcard that...
 
 Would it be better if we called the regulator something other than
 enable?  Maybe backlight-enable, or bl-enable for brevity?

The second parameter needs to match the device name. For the 4430sdp
board this should be pwm-backlight since the name will be generated
from the .name and .id fields of the struct platform_device. .id = -1
will result in no .id suffix being attached, so the device should be
named pwm-backlight. The first parameter needs to match the name of
the supply that the driver requests, so enable is correct since the
call to regulator_get() uses that.

Thierry


pgp_6Rdqha4r3.pgp
Description: PGP signature

Re: [PATCH 2/2 v2] pwm_bl: Add mandatory backlight enable regulator

[Thierry Reding]

On Wed, Mar 13, 2013 at 02:10:16PM -0700, Andrew Chew wrote:
   +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)
  
  I would loose the '!= 0'
 
 I think I prefer the '!= 0'.  Without it, it looks at first glance
 like regulator_enable() is following boolean semantics,
 so it reads kind of weird.  But I'll defer to Thierry on this
 one.  Thierry, what's your preference?

Why not assign the return value of regulator_enable() to a variable, for
instance err, and make that part of the warning message so that people
will have a better chance to diagnose what's going wrong?

Thierry


pgpKB6UF_rada.pgp
Description: PGP signature

Re: [PATCH 1/1] ARM: OMAP: board-4430sdp: Provide regulator to pwm-backlight

[Thierry Reding]

On Mon, Mar 11, 2013 at 06:54:26PM -0700, Andrew Chew wrote:
 The pwm-backlight driver now takes a mandatory regulator that is gotten
 during driver probe.  Initialize a dummy regulator to satisfy this
 requirement.
 
 Signed-off-by: Andrew Chew ac...@nvidia.com
 ---
 This patch, along with many more soon to follow, attempts to satisfy the new
 mandatory regulator that pwm-backlight will grab during probe.  The only
 board in mach-omap2 to use the pwm-backlight appears to be the 4430sdp.
 
 I thought I'd start small and use this board as an example.
 
 I tested similar code in my Tegra board, so it should be okay.  Of course,
 I don't have a 4430sdp to test with.

Hi Andrew,

This looks good, one minor comment below. I think it might make sense to
post this patch as part of a series that includes the change to the
pwm-backlight driver. This will make things easier on potential testers.
You can later extend that series to include all the other users and
eventually the whole series can be merged.

  arch/arm/mach-omap2/board-4430sdp.c |5 +
  1 file changed, 5 insertions(+)
 
 diff --git a/arch/arm/mach-omap2/board-4430sdp.c 
 b/arch/arm/mach-omap2/board-4430sdp.c
 index 35f3ad0..62022c0 100644
 --- a/arch/arm/mach-omap2/board-4430sdp.c
 +++ b/arch/arm/mach-omap2/board-4430sdp.c
 @@ -291,6 +291,10 @@ static struct platform_device sdp4430_leds_pwm = {
   },
  };
  
 +/* Dummy regulator for pwm-backlight driver */
 +static struct regulator_consumer_supply backlight_supply =
 + REGULATOR_SUPPLY(enable, NULL);
 +
  static struct platform_pwm_backlight_data sdp4430_backlight_data = {
   .max_brightness = 127,
   .dft_brightness = 127,
 @@ -718,6 +722,7 @@ static void __init omap_4430sdp_init(void)
  
   omap4_i2c_init();
   omap_sfh7741prox_init();
 + regulator_register_always_on(-1, bl-enable, backlight_supply, 1, 0);

I think I'd prefer to spell out backlight-enable. I know it's rather
long but I find it is much easier on the eye to look at the various
outputs where the string is used and see backlight instead of bl.
But it's really up to the OMAP crowd, if everybody else is fine with
bl-enable I'll shut up.

Thierry


pgp5VNNsEIYk_.pgp
Description: PGP signature

Re: [PATCH v2] pwm: pwm-tiehrpwm: Update the clock handling of pwm-tiehrpwm driver

[Thierry Reding]

On Thu, Jan 10, 2013 at 06:35:26PM +0530, Philip Avinash wrote:
 From: Philip, Avinash avinashphi...@ti.com
 
 The clock framework has changed and it's now better to invoke
 clock_prepare_enable() and clk_disable_unprepare() rather than the
 legacy clk_enable() and clk_disable() calls. This patch converts the
 pwm-tiehrpwm driver to the new framework.
 
 Signed-off-by: Philip Avinash avinashphi...@ti.com
 ---
 Changes Since v1:
   - Check the return value of TBCLK enable in .pwm_enable()
 
 In 3.8-rc1, common clock frame work support added to AM335x.
 
  drivers/pwm/pwm-tiehrpwm.c |   10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

I had applied this to my for-next branch, but reading the commit message
again it would seem that this should actually go into 3.8, right?

Thierry


pgpHyKBMHb6Dj.pgp
Description: PGP signature

Re: [PATCH v2 1/2] pwm: pwm-tiehrpwm: Low power sleep support

[Thierry Reding]

On Thu, Jan 17, 2013 at 02:50:02PM +0530, Philip Avinash wrote:
 In low power modes of AM33XX platforms, peripherals power is cut off.
 This patch supports low power sleep transition support for EHRPWM
 driver.
 
 Signed-off-by: Philip Avinash avinashphi...@ti.com
 ---
 Changes since v1:
   - check the enabled status of pwm device for handling module
 enable/disable on resume/suspend
 
  drivers/pwm/pwm-tiehrpwm.c |   83 
 
  1 file changed, 83 insertions(+)

Applied, thanks.

Thierry


pgpC79fLUWLAR.pgp
Description: PGP signature

Re: [PATCH v2 2/2] pwm: pwm-tiecap: Low power sleep support

[Thierry Reding]

On Thu, Jan 17, 2013 at 02:50:03PM +0530, Philip Avinash wrote:
 In low power modes of AM33XX platforms, peripherals power is cut off.
 This patch supports low power sleep transition support for ECAP driver.
 
 Signed-off-by: Philip Avinash avinashphi...@ti.com
 ---
 Changes since v1:
   - check the enabled status of pwm device for handling module
 enable/disable on resume/suspend
 
  drivers/pwm/pwm-tiecap.c |   53 
 ++
  1 file changed, 53 insertions(+)

Applied, thanks.

Thierry


pgpjqf4IIWAHK.pgp
Description: PGP signature

Re: [PATCH v2] pwm: pwm-tiehrpwm: Update the clock handling of pwm-tiehrpwm driver

[Thierry Reding]

On Fri, Jan 18, 2013 at 04:18:27AM +, Philip, Avinash wrote:
 On Thu, Jan 17, 2013 at 21:22:18, Thierry Reding wrote:
  On Thu, Jan 10, 2013 at 06:35:26PM +0530, Philip Avinash wrote:
   From: Philip, Avinash avinashphi...@ti.com
   
   The clock framework has changed and it's now better to invoke
   clock_prepare_enable() and clk_disable_unprepare() rather than the
   legacy clk_enable() and clk_disable() calls. This patch converts the
   pwm-tiehrpwm driver to the new framework.
   
   Signed-off-by: Philip Avinash avinashphi...@ti.com
   ---
   Changes Since v1:
 - Check the return value of TBCLK enable in .pwm_enable()
   
   In 3.8-rc1, common clock frame work support added to AM335x.
   
drivers/pwm/pwm-tiehrpwm.c |   10 --
1 file changed, 8 insertions(+), 2 deletions(-)
  
  I had applied this to my for-next branch, but reading the commit message
  again it would seem that this should actually go into 3.8, right?
 
 Common clock framework support added for am335x in 3.8 only, still EHRPWM
 tbclk not present in clock tree. I expect this to be in 3.9. So this patch
 also requires to be in 3.9 only.

Okay, I'll leave it in my for-next branch then, which gets merged into
linux-next and will go into 3.9.

Thierry


pgpkXs4tKB7rN.pgp
Description: PGP signature

Re: [PATCH 1/2] pwm: pwm-tiehrpwm: Low power sleep support

[Thierry Reding]

On Wed, Jan 16, 2013 at 12:14:01PM +, Philip, Avinash wrote:
 On Mon, Jan 14, 2013 at 12:38:56, Thierry Reding wrote:
  On Thu, Jan 10, 2013 at 06:33:43PM +0530, Philip Avinash wrote:
  [...]
   diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
  [...]
   +static int ehrpwm_pwm_suspend(struct device *dev)
   +{
   + struct ehrpwm_pwm_chip *pc = dev_get_drvdata(dev);
   +
   + ehrpwm_pwm_context_save(pc);
   + pm_runtime_put_sync(dev);
   + return 0;
   +}
   +
   +static int ehrpwm_pwm_resume(struct device *dev)
   +{
   + struct ehrpwm_pwm_chip *pc = dev_get_drvdata(dev);
   +
   + pm_runtime_get_sync(dev);
   + ehrpwm_pwm_context_restore(pc);
   + return 0;
   +}
  
  According to Documentation/power/runtime_pm.txt, the PM core runs the
  pm_runtime_get_noresume() and pm_runtime_put_sync() before executing the
  .suspend() and .resume() callbacks. Are you sure you need to call them
  here explicitly?
 
 I understand the problem of calling pm_runtime_get_sync() from .resume().
 But this has to be called if pwm was running while going to suspend so that
 pwm can continues to run after resume.

Okay. I misread the documentation and/or your patch. The documentation
says that the core calls pm_runtime_get_noresume() before executing the
.suspend() callback so you're not in fact calling it twice. Sorry for
the confusion.

 So I will add check of test_bit(PWMF_ENABLED, pwm-flags) before 
 pm_runtime_get/put_sync calls.

Yes, that sounds reasonable.

Thierry


pgpsAZiZ6CmuS.pgp
Description: PGP signature

Re: [PATCH v2] pwm: pwm-tiehrpwm: Update the clock handling of pwm-tiehrpwm driver

[Thierry Reding]

On Thu, Jan 10, 2013 at 06:35:26PM +0530, Philip Avinash wrote:
 From: Philip, Avinash avinashphi...@ti.com
 
 The clock framework has changed and it's now better to invoke
 clock_prepare_enable() and clk_disable_unprepare() rather than the
 legacy clk_enable() and clk_disable() calls. This patch converts the
 pwm-tiehrpwm driver to the new framework.
 
 Signed-off-by: Philip Avinash avinashphi...@ti.com

Applied, thanks.

Thierry


pgpk6097w2TnP.pgp
Description: PGP signature

Re: [PATCH 3/7] pwm: pwm-tiehrpwm: Update the clock handling of pwm-tiehrpwm driver

[Thierry Reding]

On Wed, Jan 02, 2013 at 06:54:50PM +0530, Philip Avinash wrote:
 The clock framework has changed and it's now better to invoke
 clock_prepare_enable() and clk_disable_unprepare() rather than the
 legacy clk_enable() and clk_disable() calls. This patch converts the
 pwm-tiehrpwm driver to the new framework.
 
 Signed-off-by: Philip Avinash avinashphi...@ti.com
 Cc: Thierry Reding thierry.red...@avionic-design.de
 ---
 In 3.8-rc1, common clock frame work support added to AM335x.
 
  drivers/pwm/pwm-tiehrpwm.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
 index 72a6dd4..af6f162 100644
 --- a/drivers/pwm/pwm-tiehrpwm.c
 +++ b/drivers/pwm/pwm-tiehrpwm.c
 @@ -341,7 +341,7 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, 
 struct pwm_device *pwm)
   configure_polarity(pc, pwm-hwpwm);
  
   /* Enable TBCLK before enabling PWM device */
 - clk_enable(pc-tbclk);
 + clk_prepare_enable(pc-tbclk);

I apparently didn't catch this before, but maybe it would be useful to
check the return value here to make sure we only proceed if the clock
can actually be enabled.

Thierry


pgpUc4tbcCsHB.pgp
Description: PGP signature

Re: [RFC] drm/lcdc: add TI LCD Controller DRM driver

[Thierry Reding]

On Mon, Dec 17, 2012 at 10:36:10AM -0600, Rob Clark wrote:
 On Mon, Dec 17, 2012 at 9:26 AM, Sekhar Nori nori.sek...@gmail.com wrote:
  Hi Rob,
 
 
  On Monday, December 17, 2012, Rob Clark wrote:
 
  On Mon, Dec 17, 2012 at 8:39 AM, Rob Clark robdcl...@gmail.com wrote:
   I'm not very enthusiastic about adding ti-lcdc specific panel/chip
   drivers. It's not really a big deal if it's only kernel code, but you
   add device-tree bindings also, which is an external API that you need
   to
   support after adding it.
  
   I'd rather see the energy put to common display framework, and get this
   whole panel/chip driver issue solved in a generic manner.
  
   yeah, I was expecting to migrate to CDF once it exists, but needed
   something for now.  I'm using the exercise to get my thoughts straight
   on how CDF should fit into KMS.  (One thing I plan to add support for
   is an i2c connected hdmi encoder.. which looks like it would fit well
   in drivers/gpu/drm/i2c.. so the drm encoder-slave stuff might be the
   way.)
  
   If you have any suggestions on the DT bindings, I'd like to hear 'em.
 
  btw, a little bit of-topic, but speaking of DT...
 
  Anybody have any clue about how backlight devices are supposed to work
  in this brave new DT world?
 
 
  See Runtime interpreted power sequences here:
   http://lkml.indiana.edu/hypermail/linux/kernel/1208.2/00029.html
 
  It is an attempt to address this need.
 
 hmm, I'm not really sure that is what is needed..  or rather, it might
 perhaps make sense to have a generic backlight driver implementation
 that could be used where appropriate, but I'm a bit suspicious about
 that trying to cover absolutely everything.
 
 From the drm/display driver we don't even want to care how the
 backlight is implemented.  You could have (just making something up
 hypothetically) a backlight controlled via a uart or some sort of
 other crazy magic.. and eventually the generic interpreter gets out of
 hand.
 
 Really I think we just want a way to retrieve a 'struct
 backlight_device *' that is created somewhere else.  We don't care how
 that backlight driver is implemented.  I don't think we want an
 interpreter.. we want a way to lookup backlight device by name or
 phandle or something like that.

I posted a patch for this a while back. It adds a new function called
of_find_backlight_by_node() that does exactly that. The way it is
supposed to be used is somewhat like this:

panel {
backlight = backlight;
};

backlight: backlight {
...
};

Then look up the phandle in the panel/DRM driver and call the new
function on the corresponding struct device_node *.

The patch went into 3.8-rc1.

And by the way, the future of the power-sequences series isn't very
clear. It was initially developed to allow the DT to contain power
sequencing for panels as part of the pwm-backlight driver, but that
idea was more or less killed by the CDF. The latest I know of is that
they could still be used as part of CDF to implement the actual power
sequences for drivers but not use their DT representation because
several people were unhappy about it.

Thierry


pgpZn0bQUdnsk.pgp
Description: PGP signature

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
 
 
 This patch is based on an earlier patch by Grant Erickson
 which provided pwm devices using the 'legacy' interface.
 
 This driver instead uses the new framework interface.

I'd prefer some kind of description about the driver here. Also the
subject should be something like:

pwm: Add OMAP support using dual-mode timers

or

pwm: omap: Add PWM support using dual-mode timers

I take this description to mean that OMAP doesn't have dedicated PWM
hardware? Otherwise it might be bad to call this pwm-omap.

Also please use all-caps when referring to PWM devices in prose. A few
other comments inline below.

 Cc: Grant Erickson maratho...@gmail.com
 Signed-off-by: NeilBrown ne...@suse.de
 
 diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
 index ed81720..7df573a 100644
 --- a/drivers/pwm/Kconfig
 +++ b/drivers/pwm/Kconfig
 @@ -85,6 +85,15 @@ config PWM_MXS
 To compile this driver as a module, choose M here: the module
 will be called pwm-mxs.
  
 +config PWM_OMAP
 + tristate OMAP pwm support

OMAP PWM support

 diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
 + *The 'id' number for the device encodes the number of the dm timer
 + *to use, and the polarity of the output.
 + *lsb is '1' of active-high, and '0' for active low
 + *remaining bit a timer number and need to be shifted down before use.

I don't know if this is such a good idea. Usually you number platform
devices sequentially, while this would leave gaps in the numbering. I
know that adding platform data may sound a bit like overkill, but I
really think the added clarity and consistency is worth it.

 +#define pr_fmt(fmt) pwm-omap:  fmt

You don't seem to be using any of the pr_*() logging functions, so this
isn't needed.

 +#include linux/export.h
 +#include linux/kernel.h
 +#include linux/platform_device.h
 +#include linux/slab.h
 +#include linux/err.h
 +#include linux/clk.h
 +#include linux/io.h
 +#include linux/pwm.h
 +#include linux/module.h
 +
 +#include plat/dmtimer.h
 +
 +#define DM_TIMER_LOAD_MIN0xFFFE
 +
 +struct omap_chip {
 + struct platform_device  *pdev;

I don't see this field being used anywhere.

 + struct omap_dm_timer*dm_timer;
 + unsigned intpolarity;

The PWM subsystem already has enum pwm_polarity for this.

 + const char  *label;

This isn't used anywhere either.

 +
 + unsigned intduty_ns, period_ns;
 + struct pwm_chip chip;
 +};
 +
 +#define to_omap_chip(chip)   container_of(chip, struct omap_chip, chip)
 +
 +#define  pwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)

This is never used.

 +
 +/**
 + * pwm_calc_value - determines the counter value for a clock rate and period.

Nit: You should either start the sentence with a capital or not
terminate it with a full stop.

 + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
 + *counter value for.
 + * @ns: The period, in nanoseconds, to computer the counter value for.

compute

 + *
 + * Returns the PWM counter value for the specified clock rate and period.
 + */
 +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
 +{
 + const unsigned long nanoseconds_per_second = 10;

Maybe use NSEC_PER_SEC instead?

 + int cycles;
 + __u64 c;

I think for in-kernel use, the custom is to stick with simply u64.

 +
 + c = (__u64)clk_rate * ns;
 + do_div(c, nanoseconds_per_second);
 + cycles = c;
 +
 + return DM_TIMER_LOAD_MIN - cycles;

Can't you just do DM_TIMER_LOAD_MIN - c and get rid of the cycles
variable altogether?

 +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 +{
 + struct omap_chip *omap = to_omap_chip(chip);
 + int status = 0;
 +
 + /* Enable the counter--always--before attempting to write its
 +  * registers and then set the timer to its minimum load value to
 +  * ensure we get an overflow event right away once we start it.
 +  */

Block comments should be in the following format:

/*
 * foo...
 * bar...
 */

 +
 + omap_dm_timer_enable(omap-dm_timer);
 + omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
 + omap_dm_timer_start(omap-dm_timer);
 + omap_dm_timer_disable(omap-dm_timer);

So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?

 + return status;

return 0; and drop the status variable.

 +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 +int duty_ns, int period_ns)
 +{
 + struct omap_chip *omap = to_omap_chip(chip);
 + int status = 0;

This one can be dropped as well.

 + const bool enable = true;
 + const bool autoreload = true;
 + const bool toggle = true;
 + const int trigger = 

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote:
 
 [Thierry: question for you near the end - thanks]
 
 On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote:
 
  Hi Neil,
  
  On 12/12/2012 02:24 AM, NeilBrown wrote:
[...]
   +{
   + struct omap_chip *omap = platform_get_drvdata(pdev);
   + int status = 0;
   +
   + status = pwmchip_remove(omap-chip);
   + if (status  0)
   + goto done;
   +
   + omap_dm_timer_free(omap-dm_timer);
  
  Is it guaranteed that the timer will be disabled at this point?
 
 Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
 might not be disabled.
 Thierry: should pwm_put do that, or do I need a 'free' function in my chip
 ops to do that?

To be honest, I haven't decided yet. =) There have been discussions that
resulted in a request to run pwm_disable() from pwmchip_remove() on all
PWM devices a chip provides.

This isn't implemented yet and I'm not sure about all the side-effects.
I think for now the best way would be to implement .free() within this
driver, or even do an explicit pwm_disable() in the driver's .remove()
function to do this. When I've come to a decision I'll refactor all of
that in one patch across the whole subsystem.

Thierry


pgptvXzqqXKUo.pgp
Description: PGP signature

Re: [PATCH] OMAP: add pwm driver using dmtimers.

[Thierry Reding]

On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote:
 On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
 thierry.red...@avionic-design.de wrote:
 
  On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[...]
   + struct omap_dm_timer*dm_timer;
   + unsigned intpolarity;
  
  The PWM subsystem already has enum pwm_polarity for this.
  
 
 I'll use that then  and as there  is a pwm_set_polarity() interface, that
 probably means that I don't need to configure the polarity via the platform
 data?  That would be a lot cleaner.

I guess the answer to that question is: it depends. If the user can set
the polarity (via platform or other means), then yes, you don't have to
pass it in here. However there may be users that don't support setting
the polarity or there may even be situations where the PWM goes through
an additional inverter on the board and therefore doesn't need the
polarity inversed after all, even if the user driver requests it.

Generally though I think that it is up to the user drivers to take care
of this and call pwm_set_polarity() as appropriate, so yes, I don't
think you have to explicitly pass it via platform data at all.

   + if (omap-duty_ns == duty_ns 
   + omap-period_ns == period_ns)
   + /* No change - don't cause any transients */
   + return 0;
  
  Note to self: this might be a candidate to put in the core.
 
 might be useful, though the core doesn't currently know the current values.

Yes, but that can be changed. PWM is still a very young subsystem and
I'm trying to be cautious not to add too much cruft to it unless it's
really worth it.

   + omap_dm_timer_set_pwm(omap-dm_timer,
   +   !omap-polarity,
   +   toggle,
   +   trigger);
  
  This doesn't either. Also you should be explicit about the polarity
  parameter, since enum pwm_polarity is an enum and therefore negating it
  isn't very nice (it should work though).
  
  You could solve this by doing something like:
  
  if (omap-polarity == PWM_POLARITY_NORMAL)
  polarity = 1;
  else
  polarity = 0;
 
 (omap-polarity == PWM_POLARITY_NORMAL)
 
 would have the same effect.

Yes, that should work as well. However I'm not a friend of using such
expressions in a function call. But since you'll probably be reworking
this anyway because of the pwm_set_polarity() comments from above you
might just want to stick the proper value into omap-polarity in your
.set_polarity() implementation and not need the extra negation here.

   +static int __devinit omap_pwm_probe(struct platform_device *pdev)
  
  No more __devinit, please.
 
 If you say so (having no idea what it did :-)

This was used to mark functions depending on whether HOTPLUG was enabled
or not. For instance functions marked __devinit could be discarded after
the init stage if HOTPLUG was disabled because it would be guaranteed to
not be called after the init stage. Recently however HOTPLUG was changed
to be always enabled because the gains were very small and most people
would get them wrong anyway.

   +#if CONFIG_PM
   +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t 
   state)
   +{
   + struct omap_chip *omap = platform_get_drvdata(pdev);
   + /* No one preserve these values during suspend so reset them
   +  * Otherwise driver leaves PWM unconfigured if same values
   +  * passed to pwm_config
   +  */
   + omap-period_ns = 0;
   + omap-duty_ns = 0;
   +
   + return 0;
   +}
   +#else
   +#define omap_pwm_suspend NULL
   +#endif
  
  This doesn't look right. You should implement .resume() if you really
  care, in which case the resume callback would have to reconfigure with
  the cached values. In that case maybe you should switch to dev_pm_ops
  and SIMPLE_DEV_PM_OPS() as well.
  
  If you don't, just resetting these values will not make the PWM work
  properly after resume either since it will have to be explicitly
  reconfigured.
 
 I just copied that from pwm-samsung.c
 
 I think the point is to avoid the no transients short-circuit in
 omap_pwm_config if the config is unchanged.
 
 The assumption is that pwm_disable() will be called before suspend and
 pwm_config()/pwm_enable() after resume.  So there is no point actually
 configuring anything in .resume() - it makes sense to wait until pwm_config()
 is called (if ever).  But we want to make sure that pwm_config actually does
 something.

Okay, that makes sense. User drivers should actually be better suited to
reset PWM devices to their proper state on resume.

   +MODULE_AUTHOR(Grant Erickson maratho...@gmail.com);
   +MODULE_AUTHOR(NeilBrown ne...@suse.de);
  
  Shouldn't this be Neil Brown? I noticed you use the concatenated form
  in the email address as well, so maybe that's on purpose?
 
 Yes, it is on purpose.  With a surname like Brown, one likes finding ways
 to be distinctive :-)

Hehe, alright then =).

Thierry


pgpOn9FPjHxZS.pgp
Description

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

[Thierry Reding]

On Fri, Nov 30, 2012 at 10:20:38AM +, Grant Likely wrote:
 On Fri, 30 Nov 2012 07:47:52 +0100, Thierry Reding 
 thierry.red...@avionic-design.de wrote:
  On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
   On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi 
   peter.ujfal...@ti.com wrote:
Hi Grant, Lars, Thierry,

On 11/26/2012 04:46 PM, Grant Likely wrote:
 You're effectively asking the pwm layer to behave like a gpio (which
 is completely reasonable). Having a completely separate translation 
 node
 really doesn't make sense because it is entirely a software construct.
 In fact, the way your using it is *entirely* to make the Linux driver
 model instantiate the translation code. It has *nothing* to do with 
 the
 structure of the hardware. It makes complete sense that if a PWM is
 going to be used as a GPIO, then the PWM node should conform to the 
 GPIO
 binding.

I understand your point around this. I might say I agree with it as 
well...
I spent yesterday with prototyping and I'm not really convinced that it 
is a
good approach from C code point of view. I got it working, yes.
In essence this is what I have on top of the slightly modified 
gpio-pwm.c
driver I have submitted:

DTS files:
twl_pwm: pwm {
/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
compatible = ti,twl6030-pwm;
#pwm-cells = 2;

/* Enable GPIO us of the PWMs */
gpio-controller = 1;
   
   This line should be simply (the property shouldn't have any data):
 gpio-controller;
   
#gpio-cells = 2;
pwm,period_ns = 7812500;
   
   Nit: property names should use '-' instead of '_'.
   
};

leds {
compatible = gpio-leds;
backlight {
label = omap4::backlight;
gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
};

keypad {
label = omap4::keypad;
gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
};
};

The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device 
when
it is requested going to look something like this. I have removed the 
error
checks for now and I still don't have the code to clean up the allocated
memory for the created device on error, or in case the module is 
unloaded. We
should also prevent the pwm core from removal when the pwm-gpo driver 
is loaded.
We need to create the platform device for gpo-pwm, create the pdata 
structure
for it and fill it in. We also need to hand craft the pwm_lookup table 
so we
can use pwm_get() to request the PWM. I have other minor changes around 
this
to get things working when we booted with DT.
So the function to do the heavy lifting is something like this:
static void of_pwmchip_as_gpio(struct pwm_chip *chip)
{
struct platform_device *pdev;
struct gpio_pwm *gpos;
struct gpio_pwm_pdata *pdata;
struct pwm_lookup *lookup;
char gpodev_name[15];
int i;
u32 gpio_mode = 0;
u32 period_ns = 0;

of_property_read_u32(chip-dev-of_node, gpio-controller,
 gpio_mode);
if (!gpio_mode)
return;

of_property_read_u32(chip-dev-of_node, pwm,period_ns, 
period_ns);
if (!period_ns) {
dev_err(chip-dev,
period_ns is not specified for GPIO use\n);
return;
}
   
   This property name seems ambiguous. What do you need to encode here? It
   looks like there is a specific PWM period used for the 'on' state. What
   about the 'off' state? Would different pwm outputs have different
   frequencies required for GPIO usage.
   
   Actually, I'm a bit surprised here that a period value is needed at all.
   I would expect if a PWM is used as a GPIO then the driver would already
   know how to set it up that way.
  
  Just to make sure we're talking about the same thing here: if a PWM is
  used as GPIO the assumption is that it would be set to 0% duty-cycle
  when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
  The period will still need to be set here, otherwise how would the PWM
  core know what the hardware even supports?
 
 Umm, I agree with you on duty cycle, but that's got nothing to do with
 period. 100% duty cycle looks exactly the same whether the period is
 10ns or 100s.

Yes, that's true. My concern was more that any value for period that we
choose more or less arbitrarily as the default for GPIO operation would
end up not being supported by some hardware.

  Unless you're proposing to not include that in the PWM core but rather
  in individual drivers. Then I suppose

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

[Thierry Reding]

On Thu, Nov 29, 2012 at 04:10:24PM +, Grant Likely wrote:
 On Wed, 28 Nov 2012 09:54:57 +0100, Peter Ujfalusi peter.ujfal...@ti.com 
 wrote:
  Hi Grant, Lars, Thierry,
  
  On 11/26/2012 04:46 PM, Grant Likely wrote:
   You're effectively asking the pwm layer to behave like a gpio (which
   is completely reasonable). Having a completely separate translation node
   really doesn't make sense because it is entirely a software construct.
   In fact, the way your using it is *entirely* to make the Linux driver
   model instantiate the translation code. It has *nothing* to do with the
   structure of the hardware. It makes complete sense that if a PWM is
   going to be used as a GPIO, then the PWM node should conform to the GPIO
   binding.
  
  I understand your point around this. I might say I agree with it as well...
  I spent yesterday with prototyping and I'm not really convinced that it is a
  good approach from C code point of view. I got it working, yes.
  In essence this is what I have on top of the slightly modified gpio-pwm.c
  driver I have submitted:
  
  DTS files:
  twl_pwm: pwm {
  /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
  compatible = ti,twl6030-pwm;
  #pwm-cells = 2;
  
  /* Enable GPIO us of the PWMs */
  gpio-controller = 1;
 
 This line should be simply (the property shouldn't have any data):
   gpio-controller;
 
  #gpio-cells = 2;
  pwm,period_ns = 7812500;
 
 Nit: property names should use '-' instead of '_'.
 
  };
  
  leds {
  compatible = gpio-leds;
  backlight {
  label = omap4::backlight;
  gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
  };
  
  keypad {
  label = omap4::keypad;
  gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
  };
  };
  
  The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
  it is requested going to look something like this. I have removed the error
  checks for now and I still don't have the code to clean up the allocated
  memory for the created device on error, or in case the module is unloaded. 
  We
  should also prevent the pwm core from removal when the pwm-gpo driver is 
  loaded.
  We need to create the platform device for gpo-pwm, create the pdata 
  structure
  for it and fill it in. We also need to hand craft the pwm_lookup table so we
  can use pwm_get() to request the PWM. I have other minor changes around this
  to get things working when we booted with DT.
  So the function to do the heavy lifting is something like this:
  static void of_pwmchip_as_gpio(struct pwm_chip *chip)
  {
  struct platform_device *pdev;
  struct gpio_pwm *gpos;
  struct gpio_pwm_pdata *pdata;
  struct pwm_lookup *lookup;
  char gpodev_name[15];
  int i;
  u32 gpio_mode = 0;
  u32 period_ns = 0;
  
  of_property_read_u32(chip-dev-of_node, gpio-controller,
   gpio_mode);
  if (!gpio_mode)
  return;
  
  of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
  if (!period_ns) {
  dev_err(chip-dev,
  period_ns is not specified for GPIO use\n);
  return;
  }
 
 This property name seems ambiguous. What do you need to encode here? It
 looks like there is a specific PWM period used for the 'on' state. What
 about the 'off' state? Would different pwm outputs have different
 frequencies required for GPIO usage.
 
 Actually, I'm a bit surprised here that a period value is needed at all.
 I would expect if a PWM is used as a GPIO then the driver would already
 know how to set it up that way.

Just to make sure we're talking about the same thing here: if a PWM is
used as GPIO the assumption is that it would be set to 0% duty-cycle
when the GPIO value is set to 0 and 100% duty-cycle when set to the 1.
The period will still need to be set here, otherwise how would the PWM
core know what the hardware even supports?

Unless you're proposing to not include that in the PWM core but rather
in individual drivers. Then I suppose the driver could choose some
sensible default.

One other problem is that some PWM devices cannot be setup to achieve a
0% or 100% duty-cycle but instead will toggle for at least one period.
This would be another argument in favour of moving the functionality to
the individual drivers, perhaps with some functionality provided by the
core to do the gpio_chip registration (a period could be passed to that
function at registration time), which will likely be the same for all
hardware that can and wants to support this feature.

Thierry


pgpIoVSMCWr2a.pgp
Description: PGP signature

Re: [PATCH v4 0/3] pwm: Drivers for twl4030/6030 PWMs and LEDs

[Thierry Reding]

On Tue, Nov 27, 2012 at 11:09:56AM +0100, Peter Ujfalusi wrote:
 Hello,
 
 Changes since v3:
 - pwm-twl-led driver's comment fix patch squashed to the original patch
 - Documentation for the DT bindings of the PWM drivers
 Comments from Thierry Reding addressed:
 - pwm-twl6030 has been removed in the last patch
 - macro for twl_pwm_chip/twl_pwmled_chip lookup
 - locking for read-modify-write sequences
 - separate pwm_ops strictures for twl4030 and twl6030 class
 - _devexit_P() removed
 - Do not select HAVE_PWM in Kconfig file since it is going away AFAIK:
   https://patchwork.kernel.org/patch/1544851/
 - Remaining (smaller) comments from Thierry also has been addressed.
 
 Changes since v2:
 - New PWM cycle calculation for twl-pwm driver and twl4030's pwm-twl-led with
   comment to document the expected behaviour.
 
 Changes cince v1:
 - The turn on/off sequence has been corrected for twl4030 PWMs as suggested by
   Grazvydas Ignotas
 - Comment section added to the new drivers to describe what they are 
 controlling
 
 The series has been tested (with additional, pending patches):
 Zoom2 for twl4030 PWM0 (keypad light), PWM1 (LCD backlight)
 BeagleBoard for twl4030 LEDB (PWMB - pmu_stat led)
 OMAP4 Blaze (SDP4430) for twl6030 PWM1 (keypad light), PWM2 (LCD backlight) 
 and
 LED (charging indication led).
 
 Intro mail from v1:
 
 The currently available pwm-twl6030.c driver only supports TWL6030's Charging
 indication LED.
 Remove this driver and add two new ones which implements support for all PWM
 driven outputs:
 pwm-twl driver supports twl4030 (PWM 0/1) and twl6030 (PWM 1/2) instances
 pwm-twl-led driver supports twl4030 (PWM driven LED A/B ports) and twl6030's
 Charging indication LED (PWM driven).
 
 Regards,
 Peter
 ---
 Peter Ujfalusi (3):
   pwm: New driver to support PWMs on TWL4030/6030 series of PMICs
   pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of
 PMICs
   pwm: Remove pwm-twl6030 driver
 
  .../devicetree/bindings/pwm/ti,twl-pwm.txt |  17 +
  .../devicetree/bindings/pwm/ti,twl-pwmled.txt  |  17 +
  drivers/pwm/Kconfig|  17 +-
  drivers/pwm/Makefile   |   3 +-
  drivers/pwm/pwm-twl-led.c  | 342 
  drivers/pwm/pwm-twl.c  | 357 
 +
  drivers/pwm/pwm-twl6030.c  | 184 ---
  7 files changed, 748 insertions(+), 189 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/pwm/ti,twl-pwm.txt
  create mode 100644 Documentation/devicetree/bindings/pwm/ti,twl-pwmled.txt
  create mode 100644 drivers/pwm/pwm-twl-led.c
  create mode 100644 drivers/pwm/pwm-twl.c
  delete mode 100644 drivers/pwm/pwm-twl6030.c

Applied with minor fixups, thanks.

Thierry


pgp1IaMpHyBNe.pgp
Description: PGP signature

Re: [PATCH v5 00/12] Support for AM33xx PWM Subsystem

[Thierry Reding]

On Tue, Nov 27, 2012 at 02:18:05PM +0530, Philip, Avinash wrote:
 In AM33xx PWM sub modules like ECAP, EHRPWM  EQEP are  integrated to
 PWM subsystem. All these submodules shares the resources (clock)  has
 a clock gating register in PWM Subsystem. This patch series creates a
 parent PWM Subsystem driver to handle access synchronization of shared
 resources  clock gating from PWM Subsystem configuration space.
 Also Device tree nodes populated to support parent child relation
 between PWMSS, ECAP  EHRPWM submodules.
 In addition EHRPWM module requires explicit clock gating from control
 module  is handled by patch #6  7.
 
 As suggested by  Thierry for handling clock gating for PWM submodules
 should handle with a global function. This requires config space
 handling done independent from driver and is done at parent driver.
 
 So the parent-child relation adopted to handle
 1. pm runtime synchronization
 2. PWM subsystem common config space clock gating for PWM submodules.
 
 Patches supports
 - Driver support for parent child relation handled patch #1
 - HWMOD data correction in EPWMSS at patch #2
 - Parent child in HWMOD relation handled at patch #3
 - Optional EHRPWM tb clock in patch #6
 - Support for TBCLK clock gating at patch #7.
 - Device tree binding support handled in patch #4,8 
 - Pinctrl support in patch #5  9.
 - DT node populated in patch #10 ,11  12.
 
 This patch series based on omap_dt/for_3.8/dts_part2 and tested
 on am335x-evm  am335x-evmsk.
 
 This patch series will come under 3 subsystem basically. But sent
 it in single patch series to have a clearer picture of why the
 OMAP subsystem changes required.
 
 Patches come under PWM (1,4,5,7,8,9)
 Patches come under OMAP (2,3,6)
 Patches come under DT (10,11,12).
 
 It depends on [1]
 1. 
 https://gitorious.org/linux-pwm/linux-pwm/commit/83af24027b3df1af5c5a9aa9adcdcfeb3429d3be
 pwm: Device tree support for PWM polarity
 
 Changes since v3:
   - Rebased on top of omap_dt/for_3.8/dts_part2
   - Add pwm backlight for am335xevm_sk
   - Moved tipwmss.h to pwm-tipwmss.h
 
 Philip, Avinash (12):
   PWMSS: Add PWM Subsystem driver for parent-child relationship
   ARM: OMAP: AM33xx hwmod: Corrects PWM subsystem HWMOD entries
   ARM: OMAP: AM33xx hwmod: Add parent-child relationship for PWM
 subsystem
   pwm: tiecap: Add device-tree binding
   pwm: pwm-tiecap: pinctrl support
   ARM: AM33XX: clk: Add clock node for EHRPWM TBCLK
   pwm: pwm-tiehrpwm: Adding TBCLK gating support.
   pwm: tiehrpwm: Add device-tree binding
   pwm: pwm-tiehrpwm: pinctrl support
   ARM: dts: AM33XX: Add PWMSS device tree nodes
   ARM: dts: AM33XX: Add PWM backlight DT data to am335x-evm
   ARM: dts: AM33XX: Add PWM backlight DT data to am335x-evmsk

Patches 1, 4, 5, 7, 8 and 9 applied, with a minor fixup to the subject
line of patch 1. Thanks.

Thierry


pgpQeygLxXjOO.pgp
Description: PGP signature

Re: [PATCH] gpio: New driver for GPO emulation using PWM generators

[Thierry Reding]

On Wed, Nov 28, 2012 at 09:54:57AM +0100, Peter Ujfalusi wrote:
 Hi Grant, Lars, Thierry,
 
 On 11/26/2012 04:46 PM, Grant Likely wrote:
  You're effectively asking the pwm layer to behave like a gpio (which
  is completely reasonable). Having a completely separate translation node
  really doesn't make sense because it is entirely a software construct.
  In fact, the way your using it is *entirely* to make the Linux driver
  model instantiate the translation code. It has *nothing* to do with the
  structure of the hardware. It makes complete sense that if a PWM is
  going to be used as a GPIO, then the PWM node should conform to the GPIO
  binding.
 
 I understand your point around this. I might say I agree with it as well...
 I spent yesterday with prototyping and I'm not really convinced that it is a
 good approach from C code point of view. I got it working, yes.
 In essence this is what I have on top of the slightly modified gpio-pwm.c
 driver I have submitted:
 
 DTS files:
 twl_pwm: pwm {
   /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
   compatible = ti,twl6030-pwm;
   #pwm-cells = 2;
 
   /* Enable GPIO us of the PWMs */
   gpio-controller = 1;
   #gpio-cells = 2;
   pwm,period_ns = 7812500;
 };
 
 leds {
   compatible = gpio-leds;
   backlight {
   label = omap4::backlight;
   gpios = twl_pwm 1 0; /* PWM1 of twl6030 */
   };
 
   keypad {
   label = omap4::keypad;
   gpios = twl_pwm 0 0; /* PWM0 of twl6030 */
   };
 };
 
 The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
 it is requested going to look something like this. I have removed the error
 checks for now and I still don't have the code to clean up the allocated
 memory for the created device on error, or in case the module is unloaded. We
 should also prevent the pwm core from removal when the pwm-gpo driver is 
 loaded.
 We need to create the platform device for gpo-pwm, create the pdata structure
 for it and fill it in. We also need to hand craft the pwm_lookup table so we
 can use pwm_get() to request the PWM. I have other minor changes around this
 to get things working when we booted with DT.
 So the function to do the heavy lifting is something like this:
 static void of_pwmchip_as_gpio(struct pwm_chip *chip)
 {
   struct platform_device *pdev;
   struct gpio_pwm *gpos;
   struct gpio_pwm_pdata *pdata;
   struct pwm_lookup *lookup;
   char gpodev_name[15];
   int i;
   u32 gpio_mode = 0;
   u32 period_ns = 0;
 
   of_property_read_u32(chip-dev-of_node, gpio-controller,
gpio_mode);
   if (!gpio_mode)
   return;
 
   of_property_read_u32(chip-dev-of_node, pwm,period_ns, period_ns);
   if (!period_ns) {
   dev_err(chip-dev,
   period_ns is not specified for GPIO use\n);
   return;
   }
 
   lookup = devm_kzalloc(chip-dev, sizeof(*lookup) * chip-npwm,
 GFP_KERNEL);
   pdata = devm_kzalloc(chip-dev, sizeof(*pdata), GFP_KERNEL);
   gpos = devm_kzalloc(chip-dev, sizeof(*gpos) * chip-npwm,
   GFP_KERNEL);
 
   pdata-gpos = gpos;
   pdata-num_gpos = chip-npwm;
   pdata-gpio_base = -1;
 
   pdev = platform_device_alloc(pwm-gpo, chip-base);
   pdev-dev.parent = chip-dev;
 
   sprintf(gpodev_name, pwm-gpo.%d, chip-base);
   for (i = 0; i  chip-npwm; i++) {
   struct gpio_pwm *gpo = gpos[i];
   struct pwm_lookup *pl = lookup[i];
   char con_id[15];
 
   sprintf(con_id, pwm-gpo.%d, chip-base + i);
 
   /* prepare GPO information */
   gpo-pwm_period_ns = period_ns;
   gpo-name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
 
   /* prepare pwm lookup table */
   pl-provider = dev_name(chip-dev);
   pl-index = i;
   pl-dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
GFP_KERNEL);
   pl-con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
   }
 
   platform_device_add_data(pdev, pdata, sizeof(*pdata));
   pwm_add_table(lookup, chip-npwm);
   platform_device_add(pdev);
 }
 
 PS: as I have said I have removed the error check just to make the code
 snippet more readable and yes we need to do some memory cleanup as well at the
 right time.
 
 Is this something you would like to see?

I must say I'm not terribly thrilled to integrate something like this
into the PWM subsystem. I wish hardware engineers wouldn't come up with
such designs. But since this seems to be a real use-case, if we want to
support it we should try and find the right solution.

Reading the above sounds overly complicated. Couldn't we, instead of
instantiating an extra driver, just register a GPIO chip if a chip wants
to support this 

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Thierry Reding]

On Mon, Nov 26, 2012 at 09:30:03AM +0100, Peter Ujfalusi wrote:
 On 11/23/2012 04:04 PM, Thierry Reding wrote:
  On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
  The driver supports the following LED outputs as generic PWM driver:
  TWL4030 LEDA and LEDB (PWMA and PWMB)
  TWL6030 Charging indicator LED (PWM LED)
 
  On TWL6030 when the PWM requested LED is configured to be controlled by SW.
  In this case the user can enable/disable and set the duty period freely.
  When the PWM has been freed, the LED driver is put back to HW control.
 
  Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
  ---
   drivers/pwm/Kconfig   |  10 ++
   drivers/pwm/Makefile  |   1 +
   drivers/pwm/pwm-twl-led.c | 303 
  ++
   3 files changed, 314 insertions(+)
   create mode 100644 drivers/pwm/pwm-twl-led.c
  
  Doesn't this belong in the drivers/leds subsystem? Besides that, the
  same comments as for the previous patch apply. One additional note
  below.
 
 The PINs itself are called as LED but they are PWMs at the end. If we
 represent them as PWMs they can be used for different purposes which is going
 to be needed for example in BeagleBoard, where the LEDA (PWMA) is used as a
 GPO to enable/disable the USB host power.

Heh, that's an interesting use-case for a PWM. =)

 Also the removed 'twl6030-pwm' driver was only controlled the LED part of 
 twl6030.
 With this series I enable the use of the PWMs and the PWMs behind of the LED
 functions to give us flexibility on how we are using them.

Alright, we can keep it in the PWM subsystem then.

  +static struct platform_driver twl_pwmled_driver = {
  +  .driver = {
  +  .name = twl-pwmled,
  +  .of_match_table = of_match_ptr(twl_pwmled_of_match),
  +  },
  +  .probe = twl_pwmled_probe,
  +  .remove = __devexit_p(twl_pwmled_remove),
  
  You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
  isn't needed here either.
 
 Oh yes, I have also received patches from a series which removes the
 _devexit_p() from the kernel.
 But should the __devexit need to be added to the remove function?

__devexit_p without a corresponding __devexit doesn't make sense. But as
all of __devinit, __devexit and __devexit_p will be removed sooner or
later, new code just shouldn't bother adding it. In this case, just drop
__devexit_p.

Thierry


pgpQDvwjBEQsV.pgp
Description: PGP signature

Re: [PATCH v4 05/11] pwm: pwm-tiecap: pinctrl support

[Thierry Reding]

On Fri, Nov 23, 2012 at 01:48:51PM +0100, Peter Korsgaard wrote:
  Thierry == Thierry Reding thierry.red...@avionic-design.de writes:
 
 Hi,
 
  Thierry Everybody seems to be doing it with a warning, so I guess
  Thierry that's fine for now. I just find it strange that if you
  Thierry request the default pin group to be selected when in fact the
  Thierry hardware doesn't support pinctrl at all you shouldn't be
  Thierry getting an error either.
 
 There's several different situations:
 
 - Platform without pinctrl support
 - Platform with pinctrl support but no pinmux specified in dt for device
   (E.G. pinmux setup in bootloader)
 - Pinmux specified in dt
 - Some kind of misconfiguration in dt
 
 You could argue that devm_pinctrl_get_select_default() shouldn't return
 an error for the first situation, but how should it be able to know the
 difference between 2 and 4?

In case where the platform supports pinctrl but no pinmux is specified
for the device it should just assume that no pinmuxing is needed. That
sounds like the most logical behaviour to me. In those cases pinctrl
could just assume that the default has already been selected and not
return an error.

But you can't reasonably expect to cope with misconfigured DT content.
Heck, there's no way for you to even know if it is misconfigured.

That said, I'm not sure how much of an issue this really is. Pinmuxing
is only used for functions local to a given chip, right? So if an SoC
supports pinctrl and a given peripheral needs pinmuxing then we can
reasonably assume that your second case can't happen, can't we?

Thierry


pgp6VT4c0OQmm.pgp
Description: PGP signature

Re: [PATCH v4 05/11] pwm: pwm-tiecap: pinctrl support

[Thierry Reding]

On Fri, Nov 23, 2012 at 10:34:02AM +, Philip, Avinash wrote:
 On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote:
  On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote:
  [...]
   + pinctrl = devm_pinctrl_get_select_default(pdev-dev);
   + if (IS_ERR(pinctrl))
   + dev_warn(pdev-dev, failed to configure pins from driver\n);
  
  I think we already discussed this, but shouldn't this be a fatal error?
 
 I had checked relevant discussion and found this can be warning message.
 This is because most boards don't have pinctrl implemented at this point,
 or may never have.
 
 https://lkml.org/lkml/2012/9/11/369
 
 Any way I will change to
 dev_warn(pdev-dev, unable to select pin group\n);
 as in other drivers.

But in case pinctrl isn't implemented, shouldn't the implementation of
devm_pinctrl_get_select_default() just be a no-op?

Thierry


pgpF87YzRz2dC.pgp
Description: PGP signature

Re: [PATCH v4 05/11] pwm: pwm-tiecap: pinctrl support

[Thierry Reding]

On Fri, Nov 23, 2012 at 11:12:15AM +, Philip, Avinash wrote:
 On Fri, Nov 23, 2012 at 16:21:10, Thierry Reding wrote:
  On Fri, Nov 23, 2012 at 10:34:02AM +, Philip, Avinash wrote:
   On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote:
On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote:
[...]
 + pinctrl = devm_pinctrl_get_select_default(pdev-dev);
 + if (IS_ERR(pinctrl))
 + dev_warn(pdev-dev, failed to configure pins from 
 driver\n);

I think we already discussed this, but shouldn't this be a fatal error?
   
   I had checked relevant discussion and found this can be warning message.
   This is because most boards don't have pinctrl implemented at this point,
   or may never have.
   
   https://lkml.org/lkml/2012/9/11/369
   
   Any way I will change to
   dev_warn(pdev-dev, unable to select pin group\n);
   as in other drivers.
  
  But in case pinctrl isn't implemented, shouldn't the implementation of
  devm_pinctrl_get_select_default() just be a no-op?
 
 So driver can still exist but pin mux functionality missing. Then warning
 message will give hints.
 I am also ok with error message  failure of driver with EPROBE_DEFER,
 if you want?

Everybody seems to be doing it with a warning, so I guess that's fine
for now. I just find it strange that if you request the default pin
group to be selected when in fact the hardware doesn't support pinctrl
at all you shouldn't be getting an error either.

I'm adding LinusW on Cc, perhaps he can shed some light on it.

Thierry


pgpFMGc5JhlDt.pgp
Description: PGP signature

Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs

[Thierry Reding]

On Tue, Nov 20, 2012 at 10:56:21AM +0100, Peter Ujfalusi wrote:
 The driver supports the following PWM outputs:
 TWL4030 PWM0 and PWM1
 TWL6030 PWM1 and PWM2
 
 On TWL4030 the PWM signals are muxed. Upon requesting the PWM the driver
 will select the correct mux so the PWM can be used. When the PWM has been
 freed the original configuration going to be restored.

configuration is going to be

 +config PWM_TWL
 + tristate TWL4030/6030 PWM support
 + select HAVE_PWM

Why do you select HAVE_PWM?

 +static int twl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 +   int duty_ns, int period_ns)
 +{
 + int duty_cycle = DIV_ROUND_UP(duty_ns * TWL_PWM_MAX, period_ns);
 + u8 pwm_config[2] = {1, 0};
 + int base, ret;
 +
 + /*
 +  * To configure the duty period:
 +  * On-cycle is set to 1 (the minimum allowed value)
 +  * The off time of 0 is not configurable, so the mapping is:
 +  * 0 - off cycle = 2,
 +  * 1 - off cycle = 2,
 +  * 2 - off cycle = 3,
 +  * 126 -  off cycle 127,
 +  * 127 -  off cycle 1
 +  * When on cycle == off cycle the PWM will be always on
 +  */
 + duty_cycle += 1;

The canonical form to write this would be duty_cycle++, but maybe it
would even be better to add it to the expression that defines
duty_cycle?

 +static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 +{
 + int ret;
 + u8 val;
 +
 + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0) {
 + dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
 + return ret;
 + }
 +
 + val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
 +
 + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0)
 + dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
 +
 + val |= TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
 +
 + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0)
 + dev_err(chip-dev, %s: Failed to enable PWM\n, pwm-label);
 +
 + return ret;
 +}

Does this perhaps need some locking to make sure the read-modify-write
sequence isn't interrupted?

 +static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device 
 *pwm)
 +{
 + int ret;
 + u8 val;
 +
 + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0) {
 + dev_err(chip-dev, %s: Failed to read GPBR1\n, pwm-label);
 + return;
 + }
 +
 + val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMX_ENABLE);
 +
 + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0)
 + dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
 +
 + val = ~TWL4030_PWM_TOGGLE(pwm-hwpwm, TWL4030_PWMXCLK_ENABLE);
 +
 + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
 + if (ret  0)
 + dev_err(chip-dev, %s: Failed to disable PWM\n, pwm-label);
 +}

Same here.

 +static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 +{
 + struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
 + chip);

This could use a macro like to_twl(chip).

 + int ret;
 + u8 val, mask, bits;
 +
 + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
 + if (ret  0) {
 + dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
 + return ret;
 + }
 +
 + if (pwm-hwpwm) {
 + /* PWM 1 */
 + mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
 + bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
 + } else {
 + /* PWM 0 */
 + mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
 + bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
 + }
 +
 + /* Save the current MUX configuration for the PWM */
 + twl-twl4030_pwm_mux = ~mask;
 + twl-twl4030_pwm_mux |= (val  mask);
 +
 + /* Select PWM functionality */
 + val = ~mask;
 + val |= bits;
 +
 + ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
 + if (ret  0)
 + dev_err(chip-dev, %s: Failed to request PWM\n, pwm-label);
 +
 + return ret;
 +}

Again, more read-modify-write without locking.

 +
 +static void twl4030_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 +{
 + struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
 + chip);
 + int ret;
 + u8 val, mask;
 +
 + ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
 + if (ret  0) {
 + dev_err(chip-dev, %s: Failed to read PMBR1\n, pwm-label);
 + return;
 + }
 +
 + if (pwm-hwpwm)
 + /* PWM 1 */
 + mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
 + else
 + 

Re: [PATCH v3 3/3] pwm: New driver to support PWM driven LEDs on TWL4030/6030 series of PMICs

[Thierry Reding]

On Tue, Nov 20, 2012 at 10:56:22AM +0100, Peter Ujfalusi wrote:
 The driver supports the following LED outputs as generic PWM driver:
 TWL4030 LEDA and LEDB (PWMA and PWMB)
 TWL6030 Charging indicator LED (PWM LED)
 
 On TWL6030 when the PWM requested LED is configured to be controlled by SW.
 In this case the user can enable/disable and set the duty period freely.
 When the PWM has been freed, the LED driver is put back to HW control.
 
 Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
 ---
  drivers/pwm/Kconfig   |  10 ++
  drivers/pwm/Makefile  |   1 +
  drivers/pwm/pwm-twl-led.c | 303 
 ++
  3 files changed, 314 insertions(+)
  create mode 100644 drivers/pwm/pwm-twl-led.c

Doesn't this belong in the drivers/leds subsystem? Besides that, the
same comments as for the previous patch apply. One additional note
below.

 +static struct platform_driver twl_pwmled_driver = {
 + .driver = {
 + .name = twl-pwmled,
 + .of_match_table = of_match_ptr(twl_pwmled_of_match),
 + },
 + .probe = twl_pwmled_probe,
 + .remove = __devexit_p(twl_pwmled_remove),

You didn't annotate twl_pwmled_remove() with __devexit, so __devexit_p
isn't needed here either.

Thierry


pgpOYHLTcsefh.pgp
Description: PGP signature

Re: [PATCH v3 1/3] pwm: Remove pwm-twl6030 driver

[Thierry Reding]

On Tue, Nov 20, 2012 at 10:56:20AM +0100, Peter Ujfalusi wrote:
 This driver only supported the Charging indicator LED.
 New set of drivers going to provide support for both PWMs and LEDs for twl4030
 and twl6030 series of PMICs.
 
 Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
 ---
  drivers/pwm/Kconfig   |   9 ---
  drivers/pwm/Makefile  |   1 -
  drivers/pwm/pwm-twl6030.c | 184 
 --
  3 files changed, 194 deletions(-)
  delete mode 100644 drivers/pwm/pwm-twl6030.c

I think I'd rather see this patch as the last in the series to make sure
that no functionality is dropped across the series.

Thierry


pgphwo2JBrmxI.pgp
Description: PGP signature

Re: [PATCH v4 01/11] PWMSS: Add PWM Subsystem driver for parent-child relationship

[Thierry Reding]

On Wed, Nov 21, 2012 at 06:40:58PM +0530, Philip, Avinash wrote:
[...]
 +static const struct of_device_id pwmss_of_match[] = {
 + {
 + .compatible = ti,am33xx-pwmss,
 + },

For consistency with other drivers this should be all on one line.

 +static const struct dev_pm_ops pwmss_pm_ops = {
 + SET_SYSTEM_SLEEP_PM_OPS(pwmss_suspend, pwmss_resume)
 +};

This could be even shorter:

static SIMPLE_DEV_PM_OPS(pwmss_pm_ops, pwmss_suspend, pwmss_resume);

Thierry


pgpCwZQKrm7f1.pgp
Description: PGP signature

Re: [PATCH v4 04/11] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

[Thierry Reding]

On Wed, Nov 21, 2012 at 06:41:01PM +0530, Philip, Avinash wrote:
[...]
 +static const struct of_device_id ecap_of_match[] = {
 + {
 + .compatible = ti,am33xx-ecap,
 + },

Same here, can be shorter by putting it on one line instead of three.

Thierry


pgpwJOp4RGP99.pgp
Description: PGP signature

Re: [PATCH v4 05/11] pwm: pwm-tiecap: pinctrl support

[Thierry Reding]

On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote:
[...]
 + pinctrl = devm_pinctrl_get_select_default(pdev-dev);
 + if (IS_ERR(pinctrl))
 + dev_warn(pdev-dev, failed to configure pins from driver\n);

I think we already discussed this, but shouldn't this be a fatal error?

Thierry


pgpSOUcGyJWBq.pgp
Description: PGP signature

Re: [PATCH v4 06/11] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

[Thierry Reding]

On Wed, Nov 21, 2012 at 06:41:03PM +0530, Philip, Avinash wrote:
[...]
 +static const struct of_device_id ehrpwm_of_match[] = {
 + {
 + .compatible = ti,am33xx-ehrpwm,
 + },

Same comment as for patch 4.

 @@ -437,16 +451,41 @@ static int __devinit ehrpwm_pwm_probe(struct 
 platform_device *pdev)
   dev_err(pdev-dev, pwmchip_add() failed: %d\n, ret);
   return ret;
   }
 -

I think this blank line actually improves readability so it can stay in.

Thierry


pgpgT19DwQRz0.pgp
Description: PGP signature