Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH] Add initial SX3000b platform related documentation to document tree
From: Rob Herring @ 2017-03-29  0:55 UTC (permalink / raw)
  To: Amit Kama IL
  Cc: corbet@lwn.net, ralf@linux-mips.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mips@linux-mips.org,
	tglx@linutronix.de, jason@lakedaemon.net, marc.zyngier@arm.com,
	linux-doc@vger.kernel.org
In-Reply-To: <AM4PR0201MB21799759E18C64A7032A2C3EE43C0@AM4PR0201MB2179.eurprd02.prod.outlook.com>

On Wed, Mar 22, 2017 at 05:59:49AM +0000, Amit Kama IL wrote:
> Add initial SX3000b platform related documentation to document tree:
>  - Vendor prefix
>  - Platform binding documentation
>  - Interrupt Controller Unit binding documentation.

Probably should be 3 patches. Preferred subject prefix is "dt-bindings: 
..."

> 
> Signed-off-by: Amit Kama <amit.kama@staixfy.com>
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/satixfy-icu.txt b/Documentation/devicetree/bindings/interrupt-controller/satixfy-icu.txt
> index 0000000..1893393
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/satixfy-icu.txt
> @@ -0,0 +1,47 @@
> +Satixfy SX3000B Interrupt Controller Unit (ICU)
> +
> +The ICU routes HW interrupts from the inter-module fabric to the
> +processor. For the MIPS interaptive, all interrupts are then routed

interaptive or interaptiv? I see both in the doc.


> +to the GIC.
> +
> +Required properties:
> +- compatible : Should be "satixfy,icu".

icu is fairly generic sounding. Should be satixfy,sx3000b-icu.

> +- reg - must be present and equal <0x1D4D0000 0x1C0>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt specifier.  Should be 1 - the GIC interrupt number
> +- interrupt-parent - Currently only the MIPS GIC is supported, so
> +<&gic> must be specified as parent
> +- interrupts : in interrupt parent form. For GIC it's
> +<GIC_SHARED x IRQ_TYPE_EDGE_RISING> where x is the interrupt number
> +allocated for ICU in GIC.
> +
> +
> +
> +
> +
> +Example:
> +
> +	icu: interrupt-controller@1d4d0000 {
> +		compatible = "sx,icu";
> +		reg = <0x1D4D0000 0x1C0>;

lowercase

> +
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SHARED 25 IRQ_TYPE_EDGE_RISING>;
> +	};
> +
> +	uart0: uart@1D4D09C0 {

serial@1d4d09c0

> +		compatible = "ns16550a";
> +		reg = <0x1D4D09C0 0x100>;

lowercase

> +
> +		interrupt-parent = <&icu>;
> +		interrupts = <3>;
> +
> +		clock-frequency = <270000000>;
> +
> +		reg-shift = <2>;
> +		reg-io-width = <4>;
> +	};
> diff --git a/Documentation/devicetree/bindings/mips/satixfy/sx3000b.txt b/Documentation/devicetree/bindings/mips/satixfy/sx3000b.txt
> index 0000000..7cae67b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/satixfy/sx3000b.txt
> @@ -0,0 +1,37 @@
> +Satixfy SX3000b SoC
> +=========================
> +
> +Required properties:
> +--------------------
> + - compatible: Must include "satixfy,sx3000".

The "b" in SX3000b is not significant?

> +
> +CPU nodes:
> +----------
> +A "cpus" node is required.  Required properties:
> + - #address-cells: Must be 1.
> + - #size-cells: Must be 0.
> +A CPU sub-node is also required for at least CPU 0.  Since the topology may
> +be probed via CPS, it is not necessary to specify secondary CPUs.  Required
> +properties:
> + - device_type: Must be "cpu".
> + - compatible: Must be "mti,interaptiv".
> + - reg: CPU number.
> + - clocks: Must include the CPU clock.  See ../../clock/clock-bindings.txt for
> +   details on clock bindings.
> +Example:
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "mti,interaptiv";
> +			clocks	= <&ext>;
> +			reg = <0>;
> +		};
> +	};
> +
> +Interrupt controllers:
> +----------------------
> +Two nodes are required:
> + - mips,gic - MIPS Global Interrupt Controller - see Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
> + - satixfy,icu - SX3000b SoC Interrupt Controller Unit - see Documentation/devicetree/bindings/interrupt-controller/satixfy-icu.txt
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index ec0bfb9..76819dd
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -261,6 +261,7 @@ rockchip	Fuzhou Rockchip Electronics Co., Ltd
>  samsung	Samsung Semiconductor
>  samtec	Samtec/Softing company
>  sandisk	Sandisk Corporation
> +satixfy Satixfy Technologies Ltd
>  sbs	Smart Battery System
>  schindler	Schindler
>  seagate	Seagate Technology PLC
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/3] clk: qcom: Update DT bindings for the MSM8660/APQ8060 RPMCC
From: Rob Herring @ 2017-03-29  0:55 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Michael Turquette, Stephen Boyd, linux-clk, linux-arm-msm,
	devicetree
In-Reply-To: <20170322081823.20446-1-linus.walleij@linaro.org>

On Wed, Mar 22, 2017 at 09:18:23AM +0100, Linus Walleij wrote:
> These compatible strings need to be added to extend support
> for the RPM CC to cover MSM8660/APQ8060. We also need to add
> enumberators to the include file for a few clocks that were
> missing.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/clock/qcom,rpmcc.txt | 2 ++
>  include/dt-bindings/clock/qcom,rpmcc.h                 | 4 ++++
>  2 files changed, 6 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH 2/3] clk: qcom: Elaborate on "active" clocks in the RPM clock bindings
From: Rob Herring @ 2017-03-29  0:59 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Michael Turquette, Stephen Boyd, linux-clk-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170322081842.20495-1-linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On Wed, Mar 22, 2017 at 09:18:42AM +0100, Linus Walleij wrote:
> The concept of "active" clocks is just explained in a bried comment in the
> device driver, let's explain it a bit more in the device tree bindings
> so everyone understands this.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/clock/qcom,rpmcc.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> index d470a0187035..cf80a00b7ff2 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> @@ -18,6 +18,14 @@ Required properties :
>  
>  - #clock-cells : shall contain 1
>  
> +The clock enumerators are defined in <dt-bindings/clock/qcom,rpmcc.h>
> +and come in pairs: FOO_CLK followed by FOO_A_CLK. The latter clock
> +is an "active" clock, which means that the consumer only care that the
> +clock is available when the system is active, i.e. not suspended. If
> +it is important that the clock keeps running during system suspend,
> +you need to specify the non-active clock, the one not containing
> +*_A_* in the enumerator name.
> +

Sounds like abuse as the clock id is encoding policy into it. The number 
of clocks should be the number of inputs to a block. I wouldn't be 
opposed to some flags for clocks, but that should be a separate cell.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/2] clk: qcom: clk-smd-rpm: add msm8996 rpmclks
From: Rob Herring @ 2017-03-29  1:21 UTC (permalink / raw)
  To: Rajendra Nayak
  Cc: sboyd-sgV2jX0FEOL9JmXXK+q4OQ, mturquette-rdvid1DuHRBWk0Htik3J/w,
	linux-clk-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <1490175566-4084-2-git-send-email-rnayak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

On Wed, Mar 22, 2017 at 03:09:25PM +0530, Rajendra Nayak wrote:
> Add all RPM controlled clocks on msm8996 platform
> 
> Signed-off-by: Rajendra Nayak <rnayak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> [srini: Fixed various issues with offsets and made names specific to msm8996]
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/clock/qcom,rpmcc.txt       |  1 +
>  drivers/clk/qcom/clk-smd-rpm.c                     | 82 ++++++++++++++++++++++
>  include/dt-bindings/clock/qcom,rpmcc.h             | 14 ++++
>  include/linux/soc/qcom/smd-rpm.h                   |  4 ++
>  4 files changed, 101 insertions(+)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] ipmi: bt-bmc: Add ast2500 compatible string
From: Rob Herring @ 2017-03-29  1:22 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Corey Minyard,
	openipmi-developer-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Cédric Le Goater,
	Andrew Jeffery, Mark Rutland
In-Reply-To: <20170322140112.2479-1-joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

On Thu, Mar 23, 2017 at 12:31:12AM +1030, Joel Stanley wrote:
> The ast2500 SoCs contain the same IPMI BT device.
> 
> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/ipmi/aspeed,ast2400-ibt-bmc.txt | 4 +++-
>  drivers/char/ipmi/bt-bmc.c                                        | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 2/7] rtc: Add rtc-sh
From: Rob Herring @ 2017-03-29  1:24 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Alessandro Zummo, Alexandre Belloni, Mark Rutland, Simon Horman,
	Geert Uytterhoeven, rtc-linux, devicetree, linux-renesas-soc
In-Reply-To: <20170322142754.30888-3-chris.brandt@renesas.com>

On Wed, Mar 22, 2017 at 10:27:49AM -0400, Chris Brandt wrote:
> rtc-sh is an RTC for SuperH and RZ/A SoCs.

Please work on the subject. It's a bit brief. "dt-bindings: rtc: " is 
the preferred prefix.

> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v2:
> * added interrupt-names and clock-names
> * clocks now include counting sources
> * changed 'is a RTC' to 'is an RTC' in commit message
> ---
>  Documentation/devicetree/bindings/rtc/rtc-sh.txt | 29 ++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-sh.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-sh.txt b/Documentation/devicetree/bindings/rtc/rtc-sh.txt
> new file mode 100644
> index 0000000..adbb8af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-sh.txt
> @@ -0,0 +1,29 @@
> +* Real Time Clock for Renesas SH and ARM SoCs
> +
> +Required properties:
> +- compatible: Should be "renesas,r7s72100-rtc" and "renesas,sh-rtc" as a
> +  fallback.
> +- reg: physical base address and length of memory mapped region.
> +- interrupts: 3 interrupts for alarm, period, and carry.
> +- interrupt-names: The interrupts should be labeled as "alarm", "period", and
> +  "carry".
> +- clocks: The functional clock source for the RTC controller must be listed
> +  first (if exists). Additionally, potential clock counting sources are to be
> +  listed.
> +- clock-names: The functional clock must be labeled as "fck". Other clocks
> +  may be named in accordance to the SoC hardware manuals.
> +
> +
> +Example:
> +rtc: rtc@fcff1000 {
> +	compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> +	reg = <0xfcff1000 0x2e>;
> +	interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> +		      GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> +		      GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> +	interrupt-names = "alarm", "period", "carry";
> +	clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> +		 <&rtc_x3_clk>, <&extal_clk>;
> +	clock-names = "fck", "rtc_x1", "rtc_x3", "extal";
> +	power-domains = <&cpg_clocks>;

Not documented.

Rob

^ permalink raw reply

* Re: [PATCH 2/3] clk: qcom: Elaborate on "active" clocks in the RPM clock bindings
From: Linus Walleij @ 2017-03-29  1:33 UTC (permalink / raw)
  To: Rob Herring
  Cc: Michael Turquette, Stephen Boyd, linux-clk,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170329005943.xqs6ouqbqwdzaayf@rob-hp-laptop>

On Wed, Mar 29, 2017 at 2:59 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Mar 22, 2017 at 09:18:42AM +0100, Linus Walleij wrote:
>> The concept of "active" clocks is just explained in a bried comment in the
>> device driver, let's explain it a bit more in the device tree bindings
>> so everyone understands this.
>>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/clock/qcom,rpmcc.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
>> index d470a0187035..cf80a00b7ff2 100644
>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
>> @@ -18,6 +18,14 @@ Required properties :
>>
>>  - #clock-cells : shall contain 1
>>
>> +The clock enumerators are defined in <dt-bindings/clock/qcom,rpmcc.h>
>> +and come in pairs: FOO_CLK followed by FOO_A_CLK. The latter clock
>> +is an "active" clock, which means that the consumer only care that the
>> +clock is available when the system is active, i.e. not suspended. If
>> +it is important that the clock keeps running during system suspend,
>> +you need to specify the non-active clock, the one not containing
>> +*_A_* in the enumerator name.
>> +
>
> Sounds like abuse as the clock id is encoding policy into it. The number
> of clocks should be the number of inputs to a block. I wouldn't be
> opposed to some flags for clocks, but that should be a separate cell.

I'm sorry about that, but I'm just documenting what is already a fact and
was previously just implicit in the name.

I first had no idea what this *_A_* infix notation was about so after some
reading I found a comment in the driver saying this.

I guess Stephen can confirm and/or elaborate on this.

Keeping them around is I guess the lesser evil (as compard to
pulling up the deployed bindings with the roots) at this point.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 1/5] dt-bindings: Document STM32 IWDG bindings
From: Rob Herring @ 2017-03-29  1:40 UTC (permalink / raw)
  To: Yannick Fertre
  Cc: Wim Van Sebroeck, Guenter Roeck, Alexandre TORGUE,
	Benjamin Gaignard, Maxime Coquelin,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Philippe Cornu,
	Gabriel FERNANDEZ, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1490195083-25117-2-git-send-email-yannick.fertre-qxv4g6HH51o@public.gmane.org>

On Wed, Mar 22, 2017 at 04:04:39PM +0100, Yannick Fertre wrote:
> From: Yannick Fertre <yfertre.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

"dt-bindings: watchdog: ..." for subject.
> 
> This adds documentation of device tree bindings for the STM32 IWDG
> (Independent WatchDoG).
> 
> Change-Id: Idadacc806d00205fe9af2e8606af229b4b760558
> Signed-off-by: Yannick Fertre <yannick.fertre-qxv4g6HH51o@public.gmane.org>
> ---
>  .../devicetree/bindings/watchdog/st,stm32-iwdg.txt        | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
> new file mode 100644
> index 0000000..036b040
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
> @@ -0,0 +1,15 @@
> +STM32 Independent WatchDoG (IWDG)
> +---------------------------------
> +
> +Required properties:
> +- compatible: "st,stm32-iwdg"
> +- reg: physical base address and length of the registers set for the device
> +- clocks: must contain a single entry describing the clock input
> +
> +Example:
> +
> +iwdg@40003000 {

watchdog@...

> +	compatible = "st,stm32-iwdg";
> +	reg = <0x40003000 0x400>;
> +	clocks = <&clk_lsi>;
> +};
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v1 1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current
From: Liam Breck @ 2017-03-29  1:42 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sebastian Reichel, Tony Lindgren, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	Hans de Goede, devicetree-u79uwXL29TY76Z2rM5mHXA, Quentin Schulz
In-Reply-To: <20170329003946.3eyqmsozj2sxrtiw@rob-hp-laptop>

Hi Rob,

On Tue, Mar 28, 2017 at 5:39 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>>
>> precharge-current-microamp and endcharge-current-microamp are used
>> by battery chargers at the beginning and end of a charging cycle.
>>
>> Depends-on: https://patchwork.kernel.org/patch/9633605/
>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>> index 53a68c0..494374a 100644
>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -12,6 +12,8 @@ Optional Properties:
>>   - voltage-min-design-microvolt: drained battery voltage
>>   - energy-full-design-microwatt-hours: battery design energy
>>   - charge-full-design-microamp-hours: battery design capacity
>> + - precharge-current-microamp: current for pre-charge phase
>> + - endcharge-current-microamp: current for charge termination phase
>
> current is implied by microamp, so perhaps just pre-charge-microamp and
> end-charge-microamp.

Ah, this is why I want to document the naming scheme for battery node
properties in battery.txt, by referring to a linux header file :-)

The names mirror enum power_supply_property elements wherever
possible. Shortening them as you suggest makes dts code a little more
terse, but obscures their relationship with power_supply sysfs
attributes. I prefer brevity myself, but there is no strong case to
reword the names for DT use.

> I know little about batteries, but don't you also need to know when each
> phase starts/ends?

Meaning at what voltage levels? We don't need it for this
battery/charger pair; no idea about others...

> I mainly ask because we just added the previous properties and now we're
> adding 2 more. While fine to add features to a driver one by one, we
> really shouldn't for bindings. The h/w is not evolving (in a month).

And a third patchset from Quentin Schulz adds another :-)

I think you may need to accept piecemeal assembly in this case... No
one has made a study of all properties that should be in the battery
node. (precharge_current wasn't even in power_supply_property until
this patchset.) Sebastian requested we create this binding in the
process of adding DT support to a fuel gauge, so we coded what that
called for.

I hope that helps...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller
From: Linus Walleij @ 2017-03-29  1:43 UTC (permalink / raw)
  To: jacopo
  Cc: Jacopo Mondi, Geert Uytterhoeven, Laurent Pinchart, Chris Brandt,
	Rob Herring, Mark Rutland, Russell King, Linux-Renesas,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170324164559.GA15925@w540>

On Fri, Mar 24, 2017 at 5:45 PM, jacopo <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:

> I initially created a whole new sub-directory where all the Renesas
> devices with this kind of pin controller would have gone
> (drivers/pinctrl/rz-pfc) but since as of now the only available
> hardware of that type is RZ/A1 and some of its variants, we decided to go
> with a single driver supporting that platform only.
> If more will come, we'll think about supporting them through this
> driver if possible, or create a dedicated directory.
>
> Are you ok with this?

Yep it's OK. It's no big deal to move it into a new subdir if many
new drivers start popping in anyway.

Right now I see the use of renesas,pins as the only big blocker,
I would much like it to use just pins = <>;

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v1 2/7] devicetree: power: Add docs for TI BQ24190 battery charger
From: Liam Breck @ 2017-03-29  1:48 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sebastian Reichel, Tony Lindgren, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	Hans de Goede, devicetree-u79uwXL29TY76Z2rM5mHXA, Liam Breck
In-Reply-To: <20170329004743.6mwpk3lzt5l666gw@rob-hp-laptop>

Hi Rob,

On Tue, Mar 28, 2017 at 5:47 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 21, 2017 at 03:09:16PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>>
>> Document monitored-battery and ti,system-minimum-microvolt properties.
>>
>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>> ---
>>  .../devicetree/bindings/power/supply/bq24190.txt   | 47 ++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/supply/bq24190.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/bq24190.txt b/Documentation/devicetree/bindings/power/supply/bq24190.txt
>> new file mode 100644
>> index 0000000..d252d10
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/bq24190.txt
>> @@ -0,0 +1,47 @@
>> +Binding for TI BQ24190 Li-Ion Battery Charger
>> +
>> +Required properties:
>> +- compatible: Should contain one of the following:
>> +    * "ti,bq24190"
>> +- reg: integer, I2C address of the device.
>> +
>> +Optional properties:
>> +- monitored-battery: phandle of battery information devicetree node
>> +    These battery properties are relevant:
>> +    + precharge-current-microamp: maximum charge current during precharge
>> +      phase (typically 20% of battery capacity).
>> +    + endcharge-current-microamp: a charge cycle terminates when the
>> +      battery voltage is above recharge threshold, and the current is below
>> +      this setting (typically 10% of battery capacity).
>> +    See Documentation/devicetree/bindings/power/supply/battery.txt
>
> This isn't really relevant to the binding. The battery properties
> shouldn't vary with the charger.

Different components need different properties from the battery node.
This charger needs the above two, so we should document that, no?

>> +- ti,system-minimum-microvolt: when power is connected and the battery
>> +    is below minimum system voltage, the system will be regulated above this
>> +    setting.
>> +
>> +Other features:
>> +- Use gpio-hog to set the OTG pin high to enable 500mA charge current on USB SDP port.
>> +
>> +Example:
>> +
>> +bat: battery {
>> +     compatible = "simple-battery";
>> +     precharge-current-microamp = <256000>;
>> +     endcharge-current-microamp = <128000>;
>> +};
>> +
>> +bq24190 charger@6a {
>> +     compatible = "ti,bq24190";
>> +     reg = <0x6a>;
>> +     // interrupt configuration here
>> +     monitored-battery = <&bat>;
>> +     ti,system-minimum-microvolt = <3200000>;
>> +};
>> +
>> +&twl_gpio {
>> +     otg {
>> +             gpio-hog;
>> +             gpios = <6 0>;
>> +             output-high;
>> +             line-name = "otg-gpio";
>> +     };
>> +};
>> --
>> 2.9.3
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver
From: Linus Walleij @ 2017-03-29  1:50 UTC (permalink / raw)
  To: Nandor Han
  Cc: Alexandre Courbot, Rob Herring, Mark Rutland,
	linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <cover.1490595641.git.nandor.han@ge.com>

On Mon, Mar 27, 2017 at 8:22 AM, Nandor Han <nandor.han@ge.com> wrote:

> Testing:
>
> 1. XRA1403 connected to iMX53 MCU
> 2. Export gpio from userspace
> 3. Verify that corresponding gpio directories are created
>    in `/sys/class/gpio/gpioXX`
> 4. Export gpios from first and second bank as output
> 5. Set the output gpio pin to high/low and verify with the
>    oscilloscope that gpio status is according with the configured
>    value.

Do *NOT* use the sysfs for testing GPIO.
This is being phased out.

Use the tools in tools/gpio/* so that you exercise the
character device instead of the old deprecated ABI.

Yours,
Linus Walleij

^ permalink raw reply

* Re: [PATCH v7 1/7] clocksource/drivers/clksrc-evt-probe: Describe with the DT both the clocksource and the clockevent
From: Rob Herring @ 2017-03-29  1:51 UTC (permalink / raw)
  To: Alexander Kochetkov
  Cc: Mark Rutland, devicetree, Huang Tao, Heiko Stuebner,
	Daniel Lezcano, linux-kernel, Russell King, linux-rockchip,
	Thomas Gleixner, linux-arm-kernel, Caesar Wang
In-Reply-To: <1490197714-25415-2-git-send-email-al.kochet@gmail.com>

On Wed, Mar 22, 2017 at 06:48:28PM +0300, Alexander Kochetkov wrote:
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> The CLOCKSOURCE_OF_DECLARE() was introduced before the
> CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> clockevent and the clocksource are both initialized in the same init
> routine.
> 
> With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> now split the clocksource and the clockevent init code. However, the
> device tree may specify a single node, so the same node will be passed
> to the clockevent/clocksource's init function, with the same base
> address.
> 
> with this patch it is possible to specify an attribute to the timer's node to
> specify if it is a clocksource or a clockevent and define two timers node.

Daniel and I discussed and agreed against this a while back. What 
changed?

> 
> For example:
> 
>         timer: timer@98400000 {
>                 compatible = "moxa,moxart-timer";
>                 reg = <0x98400000 0x42>;

This overlaps the next node. You can change this to 0x10, but are these 
really 2 independent h/w blocks? Don't design the nodes around the 
current needs of Linux.

>                 interrupts = <19 1>;
>                 clocks = <&coreclk>;
>                 clockevent;

This is not needed. The presence of "interrupts" is enough to say use 
this timer for clockevent.

>         };
> 
>         timer: timer@98400010 {
>                 compatible = "moxa,moxart-timer";
>                 reg = <0x98400010 0x42>;
>                 clocks = <&coreclk>;
>                 clocksource;

Likewise.

>         };
> 
> With this approach, we allow a mechanism to clearly define a clocksource or a
> clockevent without aerobatics we can find around in some drivers:
> 	timer-sp804.c, arc-timer.c, dw_apb_timer_of.c, mps2-timer.c,
> 	renesas-ostm.c, time-efm32.c, time-lpc32xx.c.

These all already have bindings and work. What problem are you trying to 
solve other than restructuring Linux?

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> ---
>  Documentation/devicetree/bindings/timer/timer.txt |   38 +++++++++++++++++++++
>  drivers/clocksource/clkevt-probe.c                |    7 ++++
>  drivers/clocksource/clksrc-probe.c                |    7 ++++
>  3 files changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/timer.txt b/Documentation/devicetree/bindings/timer/timer.txt
> new file mode 100644
> index 0000000..f1ee0cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/timer.txt
> @@ -0,0 +1,38 @@
> +
> +Specifying timer information for devices
> +========================================
> +
> +The timer can be declared via the macro:
> +
> +CLOCKSOURCE_OF_DECLARE(name, init) if it is a clocksource
> +CLOCKEVENT_OF_DECLARE(name, init) if it is a clockevent
> +
> +The CLOCKSOURCE_OF_DECLARE() was introduced before the
> +CLOCKEVENT_OF_DECLARE() and that resulted in some abuse where the
> +clockevent and the clocksource are both initialized in the same init
> +routine.
> +
> +With the introduction of the CLOCKEVENT_OF_DECLARE(), the driver can
> +now split the clocksource and the clockevent init code. However, the
> +device tree may specify a single node, so the same node will be passed
> +to the clockevent/clocksource's init function, with the same base
> +address. It is possible to specify an attribute to the timer's node to
> +specify if it is a clocksource or a clockevent and define two timers
> +node.

This is all Linux details and doesn't belong in binding docs.

> +
> +Example:
> +
> +	timer: timer@98400000 {
> +		compatible = "moxa,moxart-timer";
> +		reg = <0x98400000 0x42>;
> +		interrupts = <19 1>;
> +		clocks = <&coreclk>;
> +		clockevent;
> +	};
> +
> +	timer: timer@98400010 {
> +		compatible = "moxa,moxart-timer";
> +		reg = <0x98400010 0x42>;
> +		clocks = <&coreclk>;
> +		clocksource;
> +	};
> diff --git a/drivers/clocksource/clkevt-probe.c b/drivers/clocksource/clkevt-probe.c
> index eb89b50..fa02ac1 100644
> --- a/drivers/clocksource/clkevt-probe.c
> +++ b/drivers/clocksource/clkevt-probe.c
> @@ -37,6 +37,13 @@ int __init clockevent_probe(void)
>  
>  		init_func = match->data;
>  
> +		/*
> +		 * The device node describes a clocksource, ignore it
> +		 * as we are in the clockevent init routine.
> +		 */
> +		if (of_property_read_bool(np, "clocksource"))
> +			continue;
> +
>  		ret = init_func(np);
>  		if (ret) {
>  			pr_warn("Failed to initialize '%s' (%d)\n",
> diff --git a/drivers/clocksource/clksrc-probe.c b/drivers/clocksource/clksrc-probe.c
> index bc62be9..ce50f33 100644
> --- a/drivers/clocksource/clksrc-probe.c
> +++ b/drivers/clocksource/clksrc-probe.c
> @@ -38,6 +38,13 @@ void __init clocksource_probe(void)
>  
>  		init_func_ret = match->data;
>  
> +		/*
> +		 * The device node describes a clockevent, ignore it
> +		 * as we are in the clocksource init routine.
> +		 */
> +		if (of_property_read_bool(np, "clockevent"))
> +			continue;
> +
>  		ret = init_func_ret(np);
>  		if (ret) {
>  			pr_err("Failed to initialize '%s': %d",
> -- 
> 1.7.9.5
> 

^ permalink raw reply

* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Leo Yan @ 2017-03-29  1:54 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Michael Turquette, Stephen Boyd, Guodong Xu, John Stultz,
	linux-doc, linux-kernel, devicetree, linux-arm-kernel,
	linux-arm-msm, linux-soc, linux-clk, mike.leach, Suzuki.Poulose,
	sudeep.holla
In-Reply-To: <20170328165010.GA21937@linaro.org>

Hi Mathieu,

On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
> On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:

[...]

> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> > +{
> > +	int timeout = DEBUG_WAIT_TIMEOUT;
> > +
> > +	drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +
> > +	CS_UNLOCK(drvdata->base);
> > +
> > +	/* Bail out if CPU is powered up yet */
> > +	if (drvdata->edprsr & EDPRSR_PU)
> > +		goto out_powered_up;
> > +
> > +	/*
> > +	 * Send request to power management controller and assert
> > +	 * DBGPWRUPREQ signal; if power management controller has
> > +	 * sane implementation, it should enable CPU power domain
> > +	 * in case CPU is in low power state.
> > +	 */
> > +	drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > +	drvdata->edprsr |= EDPRCR_COREPURQ;
> > +	writel(drvdata->edprsr, drvdata->base + EDPRCR);
> 
> Here ->edprsr is used but EDPRCR is accessed.  Is this intentional or a
> copy/paste error?  The same is true for accesses in the out_powered_up section.

Thanks for pointing out. This is a typo error and will fix.

> > +
> > +	/* Wait for CPU to be powered up (timeout~=32ms) */
> > +	while (timeout--) {
> > +		drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +		if (drvdata->edprsr & EDPRSR_PU)
> > +			break;
> > +
> > +		usleep_range(1000, 2000);
> > +	}
> 
> See if function readx_poll_timeout() can be used.

Will use it.

> > +
> > +	/*
> > +	 * Unfortunately the CPU cannot be powered up, so return
> > +	 * back and later has no permission to access other
> > +	 * registers. For this case, should set 'idle_constraint'
> > +	 * to ensure CPU power domain is enabled!
> > +	 */
> > +	if (!(drvdata->edprsr & EDPRSR_PU)) {
> > +		pr_err("%s: power up request for CPU%d failed\n",
> > +			__func__, drvdata->cpu);
> > +		goto out;
> > +	}
> > +
> > +out_powered_up:
> > +	debug_os_unlock(drvdata);
> > +
> > +	/*
> > +	 * At this point the CPU is powered up, so set the no powerdown
> > +	 * request bit so we don't lose power and emulate power down.
> > +	 */
> > +	drvdata->edprsr = readl(drvdata->base + EDPRCR);
> > +	drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> 
> If we are here the core is already up.  Shouldn't we need to set
> EDPRCR_CORENPDRQ only?

Yeah. Will fix.

> > +	writel(drvdata->edprsr, drvdata->base + EDPRCR);
> 
> This section is a little racy - between the time the PU bit has been
> checked and the time COREPDRQ has been flipped, the state of PU may have
> changed.  You can probably get around this by checking edprsr.PU rigth here.  If
> it is not set you go through the process again.  Note that doing this will
> probably force a refactoring of the whole function.  

Agree. Will handle this.

[...]

> > +static ssize_t debug_func_knob_write(struct file *f,
> > +		const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > e	u8 on;
> > +	int ret;
> > +
> > +	ret = kstrtou8_from_user(buf, count, 2, &on);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&debug_lock);
> > +
> > +	if (!on ^ debug_enable)
> > +		goto out;
> 
> I had to read this condition too many times - please refactor.

Will do it.

> > +
> > +	if (on) {
> > +		ret = debug_enable_func();
> > +		if (ret) {
> > +			pr_err("%s: unable to disable debug function: %d\n",
> > +			       __func__, ret);
> 
> Based on the semantic this is the wrong error message.

Yeah. Will fix.

> > +			goto err;
> > +		}
> > +	} else
> > +		debug_disable_func();
> 
> Did checkpatch.pl complain about extra curly braces?  If not please add them.

checkpatch.pl doesn't report for this. Will add.

> > +
> > +	debug_enable = on;
> 
> Here we can't set debug_enable if we just called debug_disable_func().  Maybe
> I'm missing something.  If that's the case a comment in the code would be worth
> it.

After called debug_disable_func(), debug_enable is set to 0 (on = 0).

> > +
> > +out:
> > +	ret = count;
> > +err:
> > +	mutex_unlock(&debug_lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t debug_func_knob_read(struct file *f,
> > +		char __user *ubuf, size_t count, loff_t *ppos)
> > +{
> > +	char val[] = { '0' + debug_enable, '\n' };
> > +
> > +	return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val));
> 
> Use the debug_lock to avoid race conditions.

Will do it.

> > +}
> > +
> > +static ssize_t debug_idle_constraint_write(struct file *f,
> > +		const char __user *buf, size_t count, loff_t *ppos)
> > +{
> > +	int val;
> > +	ssize_t ret;
> > +
> > +	ret = kstrtoint_from_user(buf, count, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&debug_lock);
> > +	idle_constraint = val;
> > +
> > +	if (debug_enable)
> > +		pm_qos_update_request(&debug_qos_req, idle_constraint);
> > +
> > +	mutex_unlock(&debug_lock);
> > +	return count;
> > +}
> > +
> > +static ssize_t debug_idle_constraint_read(struct file *f,
> > +		char __user *ubuf, size_t count, loff_t *ppos)
> > +{
> > +	char buf[32];
> > +	int len;
> > +
> > +	if (*ppos)
> > +		return 0;
> > +
> > +	len = sprintf(buf, "%d\n", idle_constraint);
> > +	return simple_read_from_buffer(ubuf, count, ppos, buf, len);
> 
> Use the debug_lock to avoid race conditions.

Will do it.

> > +}
> > +
> > +static const struct file_operations debug_func_knob_fops = {
> > +	.open	= simple_open,
> > +	.read	= debug_func_knob_read,
> > +	.write	= debug_func_knob_write,
> > +};
> > +
> > +static const struct file_operations debug_idle_constraint_fops = {
> > +	.open	= simple_open,
> > +	.read	= debug_idle_constraint_read,
> > +	.write	= debug_idle_constraint_write,
> > +};
> > +
> > +static int debug_func_init(void)
> > +{
> > +	struct dentry *file;
> > +	int ret;
> > +
> > +	/* Create debugfs node */
> > +	debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> > +	if (!debug_debugfs_dir) {
> > +		pr_err("%s: unable to create debugfs directory\n", __func__);
> > +		return -ENOMEM;
> 
> return PTR_ERR(debug_debugfs_dir);

Here cannot use PTR_ERR(debug_debugfs_dir). If create debugfs failed
the pointer is NULL value, so finally we will return zero value for
PTR_ERR(debug_debugfs_dir).

[...]

> > +	}
> > +
> > +	file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> > +			debug_debugfs_dir, NULL, &debug_func_knob_fops);
> > +	if (!file) {
> > +		pr_err("%s: unable to create enable knob file\n", __func__);
> > +		ret = -ENOMEM;
> 
> Same as above.
> 
> > +		goto err;
> > +	}
> > +
> > +	file = debugfs_create_file("idle_constraint", S_IRUGO | S_IWUSR,
> > +			debug_debugfs_dir, NULL, &debug_idle_constraint_fops);
> > +	if (!file) {
> > +		pr_err("%s: unable to create idle constraint file\n", __func__);
> > +		ret = -ENOMEM;
> 
> Same as above.
> 
> > +		goto err;
> > +	}
> > +
> > +	/* Use sysfs node to enable functionality */
> > +	if (!debug_enable)
> > +		return 0;
> > +
> > +	/* Enable debug module at boot time */
> > +	ret = debug_enable_func();
> > +	if (ret) {
> > +		pr_err("%s: unable to disable debug function: %d\n",
> > +		       __func__, ret);
> > +		goto err;
> > +	}
> 
> Use the debug_lock to avoid race conditions.

I'm struggling to understand what's race condition at here? The
function pairs debug_func_init()/debug_func_exit() are used for
module's probing and removing, so naturally module's probing and
removing are sequential, right?

> > +
> > +	return 0;
> > +
> > +err:
> > +	debugfs_remove_recursive(debug_debugfs_dir);
> > +	return ret;
> > +}
> > +
> > +static void debug_func_exit(void)
> > +{
> > +	debugfs_remove_recursive(debug_debugfs_dir);
> > +
> > +	/* Disable functionality if has enabled */
> > +	if (debug_enable)
> > +		debug_disable_func();
> > +}
> > +
> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > +	void __iomem *base;
> > +	struct device *dev = &adev->dev;
> > +	struct debug_drvdata *drvdata;
> > +	struct resource *res = &adev->res;
> > +	struct device_node *np = adev->dev.of_node;
> > +	int ret;
> > +
> > +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> > +	if (per_cpu(debug_drvdata, drvdata->cpu)) {
> > +		dev_err(dev, "CPU's drvdata has been initialized\n");
> 
> Might be worth adding the CPU number in the error message.

Yeah, will add it.

[...]

> This driver doesn't call the pm_runtime_put/get() operations needed to handle the
> debug power domain.  See the other CoreSight drivers for details. 

Sure, will do it.

> Also, from the conversation that followed the previous post we agreed that we wouldn't
> deal with CPUidle issues in this driver.  We deal with the CPU power domain
> using the EDPRCR register (like you did) and that's it.  System that don't honor that register
> can use other (external) means to solve this.  As such please remove the
> pm_qos_xyz() functions. 

>From previous discussion, Mike reminds the CPU power domain design is
quite SoC specific and usually the SoC has many different low power
states, e.g. except CPU level and cluster level low power states, they
also can define SoC level low power states. Any SoC with any power
state is possible finally impact CPU power domain, so this is why I add
this interface to let user can have the final decision based on their
working platform.

We can rely on "nohlt" and "cpuidle.off=1" in kernel command line to
disable whole SoC low power states at boot time; or we can use sysfs
node "echo 1 > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disble" to
disable CPU low power states at runtime. But that means we need use
different interfaces to control CPU power domain for booting and
runtime, it's not nice for usage.

So this is why add "idle_constraint" as a central place to control
power domain for CPU debug purpose and I also think this is more
friendly for hardware design, e.g. some platforms can enable partial
low power states to save power and avoid overheat after using this
driver.

How about you think for this?

Thanks,
Leo Yan

^ permalink raw reply

* Re: [PATCH v8 2/3] dt-bindings: input: touchscreen: Add max11801-ts binding
From: Rob Herring @ 2017-03-29  1:57 UTC (permalink / raw)
  To: Jagan Teki
  Cc: linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jagan Teki, Dmitry Torokhov,
	Mark Rutland, Shawn Guo, Michael Trimarchi
In-Reply-To: <1490209518-20790-2-git-send-email-jagan-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>

On Thu, Mar 23, 2017 at 12:35:17AM +0530, Jagan Teki wrote:
> From: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> 
> Add missing documentation of max11801-ts dt-binding details.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Michael Trimarchi <michael-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
> Changes for v8:
> - Use IRQ_TYPE_EDGE_FALLING for interrupt trigger type. 
> Changes for v7:
> - add vendor prefix as maxim
> Changes for v6:
> - Replace the lable and name of the node
>    ts: max11801 => max11801: touchscreen@48
> Changes for v5:
> - Newly added patch
> 
>  .../bindings/input/touchscreen/max11801-ts.txt         | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RESEND PATCH v2 13/53] mtd: nand: denali_dt: enable HW_ECC_FIXUP for Altera SOCFPGA variant
From: Rob Herring @ 2017-03-29  2:00 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: laurent.monat, Boris Brezillon, thorsten.christiansson,
	Richard Weinberger, Marek Vasut, Artem Bityutskiy,
	Cyrille Pitchen, Mark Rutland, linux-kernel, Dinh Nguyen,
	devicetree, linux-mtd, Masami Hiramatsu, Chuanxiao Dong,
	Jassi Brar, Brian Norris, Enrico Jorns, David Woodhouse,
	Graham Moore
In-Reply-To: <1490213273-8571-14-git-send-email-yamada.masahiro@socionext.com>

On Thu, Mar 23, 2017 at 05:07:12AM +0900, Masahiro Yamada wrote:
> There are various customizable parameters, so several variants for
> this IP.  A generic compatible like "denali,denali-nand-dt" is
> useless.  Moreover, there are multiple things wrong with this string.
> (Refer to Rob's comment [1])
> 
> The denali_dt.c was split out and this compatible was added by Altera
> for the SOCFPGA port.  So, replace it with a more specific string.
> 
> The Denali IP on SOCFPGA incorporates the hardware ECC fixup feature.
> So, this capability should be associated with the compatible string.
> 
> [1] https://lkml.org/lkml/2016/12/1/450
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2:
>   - Add caps flag to of_device_id data, not hard-code in driver code
>   - Add Altera-specific compatible.
> 
>  Documentation/devicetree/bindings/mtd/denali-nand.txt |  5 +++--
>  drivers/mtd/nand/denali_dt.c                          | 14 ++++++++++----
>  2 files changed, 13 insertions(+), 6 deletions(-)

You should also mention there are no users (in-tree) of the old string.

Acked-by: Rob Herring <robh@kernel.org>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply

* Re: [PATCH] input: matrix_keypad: add option to drive inactive columns
From: Rob Herring @ 2017-03-29  2:02 UTC (permalink / raw)
  To: David Rivshin
  Cc: Dmitry Torokhov, Mark Rutland, linux-input-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170322201650.9072-1-drivshin-WHIoityCHfoAvxtiuMwx3w@public.gmane.org>

On Wed, Mar 22, 2017 at 04:16:50PM -0400, David Rivshin wrote:
> From: David Rivshin <DRivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
> 
> The gpio-matrix-keypad driver normally sets inactive columns as inputs
> while scanning. This does not work for all hardware, which may require
> the inactive columns to be actively driven in order to overcome any
> pull-ups/downs on the columns.
> 
> Signed-off-by: David Rivshin <drivshin-5fOYsn7Fw8lBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/input/gpio-matrix-keypad.txt        |  2 ++
>  drivers/input/keyboard/matrix_keypad.c                      | 13 +++++++++----
>  include/linux/input/matrix_keypad.h                         |  3 +++
>  3 files changed, 14 insertions(+), 4 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCHv2] phy: cpcap-usb: Add CPCAP PMIC USB support
From: Rob Herring @ 2017-03-29  2:04 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Kishon Vijay Abraham I, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-usb-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Marcel Partap, Michael Scott
In-Reply-To: <20170322234602.5888-1-tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

On Wed, Mar 22, 2017 at 04:46:02PM -0700, Tony Lindgren wrote:
> Some Motorola phones like droid 4 use a custom CPCAP PMIC that has a
> multiplexing USB PHY.
> 
> This USB PHY can operate at least in four modes using pin multiplexing
> and two control GPIOS:
> 
> - Pass through companion PHY for the SoC USB PHY
> - ULPI PHY for the SoC
> - Pass through USB for the modem
> - UART debug console for the SoC
> 
> This patch adds support for droid 4 USB PHY and debug UART modes,
> support for other modes can be added later on as needed.
> 
> Both peripheral and host mode are working for the USB. The
> host mode depends on the cpcap-charger driver for VBUS.
> 
> VBUS and ID pin detection are done using cpcap-adc IIO ADC
> driver.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Marcel Partap <mpartap-hi6Y0CQ0nG0@public.gmane.org>
> Cc: Michael Scott <michael.scott-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Tested-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> ---
> 
> Changes since v1:
> 
> - Use iio_read_channel_processed() instead of iio_read_channel_scaled()
>   as changed in the v2 of the ADC driver 
> 
> - Keep Tested-by from Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> as the change
>   from v1 is trivial
> 
> ---
>  .../devicetree/bindings/phy/phy-cpcap-usb.txt      |  40 ++

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>  drivers/phy/Kconfig                                |   8 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/phy-cpcap-usb.c                        | 695 +++++++++++++++++++++
>  4 files changed, 744 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-cpcap-usb.txt
>  create mode 100644 drivers/phy/phy-cpcap-usb.c
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
From: Linus Walleij @ 2017-03-29  2:07 UTC (permalink / raw)
  To: Nandor Han
  Cc: Alexandre Courbot, Rob Herring, Mark Rutland,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Semi Malinen
In-Reply-To: <6bf04ba1761f0692cb461558f0c8836f0d1f7ad8.1490595641.git.nandor.han-JJi787mZWgc@public.gmane.org>

On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org> wrote:

> This is a simple driver that provides a /sys/class/gpio
> interface for controlling and configuring the GPIO lines.

Use the gpio tools in tools/gpio, use the characcter device.
Do not use sysfs. Change this to reference the tools.

> It does not provide support for chip select or interrupts.
>
> Signed-off-by: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> Signed-off-by: Semi Malinen <semi.malinen-JJi787mZWgc@public.gmane.org>
(...)
> +exar   Exar Corporation

Send this as a separate patch to the DT bindings maintainer
(Rob Herring.)

> +static int xra1403_get_byte(struct xra1403 *xra, unsigned int addr)
> +{
> +       return spi_w8r8(xra->spi, XRA_READ | (addr << 1));
> +}
> +
> +static int xra1403_get_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit)
> +{
> +       int ret;
> +
> +       ret = xra1403_get_byte(xra, addr + (bit > 7));
> +       if (ret < 0)
> +               return ret;
> +
> +       return !!(ret & BIT(bit % 8));
> +}

This looks like it can use regmap-spi right off, do you agree?

git grep devm_regmap_init_spi
should give you some examples of how to use it.

If it's not off-the shelf regmap drivers like
drivers/iio/pressure/mpl115_spi.c
give examples of how to make more elaborate custom
SPI transfers with regmap.

> +static int xra1403_set_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit, int value)
> +{
> +       int ret;
> +       u8 mask;
> +       u8 tx[2];
> +
> +       addr += bit > 7;
> +
> +       mutex_lock(&xra->lock);
> +
> +       ret = xra1403_get_byte(xra, addr);
> +       if (ret < 0)
> +               goto out_unlock;
> +
> +       mask = BIT(bit % 8);
> +       if (value)
> +               value = ret | mask;
> +       else
> +               value = ret & ~mask;
> +
> +       if (value != ret) {
> +               tx[0] = addr << 1;
> +               tx[1] = value;
> +               ret = spi_write(xra->spi, tx, sizeof(tx));
> +       } else {
> +               ret = 0;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&xra->lock);
> +
> +       return ret;
> +}

Classical mask-and-set implementation right?
With regmap this becomes simply regmap_update_bits(map, addr, mask, set)

> +static int xra1403_probe(struct spi_device *spi)
> +{
> +       struct xra1403 *xra;
> +       struct gpio_desc *reset_gpio;
> +
> +       xra = devm_kzalloc(&spi->dev, sizeof(*xra), GFP_KERNEL);
> +       if (!xra)
> +               return -ENOMEM;
> +
> +       /* bring the chip out of reset */
> +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(reset_gpio))
> +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> +       else if (reset_gpio)
> +               gpiod_put(reset_gpio);

I don't think you should put it, other than in the remove()
function and in that case you need to have it in the
state container.

> +       mutex_init(&xra->lock);
> +
> +       xra->chip.direction_input = xra1403_direction_input;
> +       xra->chip.direction_output = xra1403_direction_output;

Please implement .get_direction(). This is very nice to have.

> +static int xra1403_remove(struct spi_device *spi)
> +{
> +       struct xra1403 *xra = spi_get_drvdata(spi);
> +
> +       gpiochip_remove(&xra->chip);

Use devm_gpiochip_add_data() and this remove is not
needed at all.

> +static int __init xra1403_init(void)
> +{
> +       return spi_register_driver(&xra1403_driver);
> +}
> +
> +/*
> + * register after spi postcore initcall and before
> + * subsys initcalls that may rely on these GPIOs
> + */
> +subsys_initcall(xra1403_init);
> +
> +static void __exit xra1403_exit(void)
> +{
> +       spi_unregister_driver(&xra1403_driver);
> +}
> +module_exit(xra1403_exit);

This seems like tricksy. Just module_spi_driver()
should be fine don't you think?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/3] doc,dts - add XRA1403 DTS binding documentation
From: Linus Walleij @ 2017-03-29  2:09 UTC (permalink / raw)
  To: Nandor Han
  Cc: Alexandre Courbot, Rob Herring, Mark Rutland,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <39ba92bacf48da957f8f85d5cb11d3254fe3d68f.1490595641.git.nandor.han-JJi787mZWgc@public.gmane.org>

On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org> wrote:

> Add the XRA1403 DTS binding documentation.
>
> Signed-off-by: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>

There is no big problem with this but:

> +The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available:
> +       - Individually programmable inputs:
> +               - Internal pull-up resistors
> +               - Polarity inversion
> +               - Individual interrupt enable
> +               - Rising edge and/or Falling edge interrupt
> +               - Input filter

Since you mention that it has interrupts maybe you want to add bindings
for the cascaded interrupt and the interrupt-controller; keyword etc
already now.

We just document what the hardware can do, we don't have to do all
of it in the first Linux driver submission.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] ipmi: bt-bmc: Add ast2500 compatible string
From: Corey Minyard @ 2017-03-29  2:16 UTC (permalink / raw)
  To: Rob Herring, Joel Stanley
  Cc: openipmi-developer-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Cédric Le Goater,
	Andrew Jeffery, Mark Rutland
In-Reply-To: <20170329012224.jeil7uxvi2iefzvh@rob-hp-laptop>

On 03/28/2017 08:22 PM, Rob Herring wrote:
> On Thu, Mar 23, 2017 at 12:31:12AM +1030, Joel Stanley wrote:
>> The ast2500 SoCs contain the same IPMI BT device.
>>
>> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/ipmi/aspeed,ast2400-ibt-bmc.txt | 4 +++-
>>   drivers/char/ipmi/bt-bmc.c                                        | 1 +
>>   2 files changed, 4 insertions(+), 1 deletion(-)
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

This is in the IPMI tree for the next kernel.

Thanks all,

-corey

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/2] leds: Add driver for Qualcomm LPG
From: Rob Herring @ 2017-03-29  2:17 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Bjorn Andersson, Richard Purdie, Jacek Anaszewski,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-leds-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170323203749.GB8563@amd>

On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> Hi!
> 
> > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > PMICs from Qualcomm. It can operate on fixed parameters or based on a
> > lookup-table, altering the duty cycle over time - which provides the
> > means for e.g. hardware assisted transitions of LED brightness.
> 
> Ok, this is not first hardware that supports something like this. We
> have similar hardware that can do blinking on Nokia N900 -- please
> take a look at leds-lp55*.c

And perhaps some alignment on the bindings too if the N900 has bindings.

> And it would be really good to provide hardware abstraction. We really
> don't want to have different userspace for LPG and for N900 and for

I'm interested in what this looks like as several AOSP platforms do 
tri-color LEDs with custom sysfs extensions.

Do any of the Dragonboards have tri-color LEDs?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support
From: Linus Walleij @ 2017-03-29  2:18 UTC (permalink / raw)
  To: Gregory CLEMENT
  Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
	Andrew Lunn, Sebastian Hesselbarth, Thomas Petazzoni,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Nadav Haklai, Victor Gu, Marcin Wojtas, Wilson Ding, Hua Jing,
	Neta Zur Hershkovits
In-Reply-To: <87mvc5a3hh.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

On Tue, Mar 28, 2017 at 4:19 PM, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>  On mar., mars 28 2017, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

>> What you're doing is mocking around with core irqchip semantics.
>> Is ->mask really supposed to be played around with from the outsid
>> like this?
>
> According to the documentation mask is a "precomputed bitmask for
> accessing the chip registers" and it is exactly the way it is used in
> this driver.
>
> Moreover, currently ->mask is only used by the generic irqchip framework
> and not by the core of the irqchip subsystem. So the mask initialization
> is not done from the oustide but at the same level as the generic
> irqchip which is not used here.

OK excellent, sorry for my ignorance. We should rather point to your driver
as a good example of how to do this. Care to add some few lines of comment
as to what is going on for others that need to do the same?

Actually it would even be good to have something in
Documentation/gpio/driver.txt

>> It has irq_create_mapping(gpiochip->irqdomain, offset); that get
>> called for every IRQ, and that will eventually call irq_of_parse_and_map()
>> if the IRQs are defined in the device tree. (IIRC)
>
> When I followed the functions called I never find a call to
> irq_of_parse_and_map(), the closer things related to device tree I found
> was:
> "virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node),
> NULL);"
> http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507

I don't know if I'm rambling or what. I'm pretty sure it gets called, maybe
even earlier, like when the DT is parsed for the platform. We have so many
drivers not seemingly needing this, but if your driver needs it, all others
may need to be fixed too.

Can you put a print in irq_of_parse_and_map() and see what happens?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding
From: Rob Herring @ 2017-03-29  2:26 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Richard Purdie, Jacek Anaszewski, Pavel Machek, Mark Rutland,
	linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170323055435.29197-2-bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On Wed, Mar 22, 2017 at 10:54:35PM -0700, Bjorn Andersson wrote:
> This adds the binding document describing the three hardware blocks
> related to the Light Pulse Generator found in a wide range of Qualcomm
> PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 194 +++++++++++++++++++++
>  1 file changed, 194 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> new file mode 100644
> index 000000000000..fb9edd89119d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> @@ -0,0 +1,194 @@
> +Binding for Qualcomm Light Pulse Generator
> +
> +The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> +a ramp generator with lookup table, the light pulse generator and a three
> +channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
> +Each of these are described individually below.
> +
> += Lookup Table (LUT)
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-lpg-lut"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the LUT block
> +
> +- qcom,lut-size:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: number of elements available in the lookup table
> +
> += Light Pulse Generator (LPG)
> +The Light Pulse Generator can operate either as a standard PWM controller or in
> +a more advanced lookup-table based mode. These are described separately below.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-lpg"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the LPG block
> +
> +== PWM mode
> +
> +- #pwm-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 1
> +
> +== Lookup-table mode
> +
> +- cell-index:

This is a standard though not used property name. Perhaps "reg" or a 
vendor property instead.

> +	Usage: required, when referencing a LUT
> +	Value type: <u32>
> +	Definition: id of the LPG, used to associate the LPG with a particular
> +		    ramp generator in the LUT block
> +
> +- default-state:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: default state, as defined in common.txt
> +
> +- label:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: label of the LED, as defined in common.txt
> +
> +- linux,default-trigger:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: default trigger, as defined in common.txt
> +
> +- qcom,tri-led:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: a phandle of a TRILED node and a single u32 denoting which
> +		    output channel to control
> +
> +- qcom,lut:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: phandle of a LUT node
> +
> +- qcom,dtest:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: configures the output into an internal test line of the
> +		    pmic. A first u32 defines which test line to use and the
> +		    second cell configures how the value should be outputed
> +		    (available lines and configuration differs between PMICs)
> +
> +- qcom,pattern:
> +	Usage: optional
> +	Value type: <u16-list>
> +	Definition: list of 16 bit duty cycle values to make up the pattern to
> +		    be programmed into the LUT. Values should be in the range
> +		    [0,512).
> +
> +- qcom,pattern-length-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, of the ramp generator running
> +		    one pass over the defined pattern
> +
> +- qcom,pattern-pause-lo-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    before iterating over the pattern
> +
> +- qcom,pattern-pause-hi-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    after iterating over the pattern
> +
> +- qcom,pattern-ping-pong:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should reverse direction
> +		    when reaching the end of the pattern, instead of wrapping
> +		    to the beginning
> +
> +- qcom,pattern-oneshot:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should stop after a single
> +		    pass over the pattern
> +
> +- qcom,pattern-reverse:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should operate backwards
> +		    over the pattern

The pattern related properties should be common if we put them in DT 
which I think is debatable.

> +
> += LED Current Sink (TRILED)
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-tri-led"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the TRILED block
> +
> +- qcom,power-source:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: power-source used to drive the output, as defined in the
> +		    datasheet
> +
> += EXAMPLE:
> +The following example defines a single output of the PMI8994, sinking current
> +into a LED in a natural pulsating pattern:
> +
> +&spmi_bus {
> +	pmic@3 {
> +		compatible = "qcom,pmi8994", "qcom,spmi-pmic";

typo.

> +		reg = <0x3 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmi8994_lpg_lut: lpg-lut@b000 {
> +			compatible = "qcom,spmi-lpg-lut";
> +			reg = <0xb000>;
> +
> +			qcom,lut-size = <24>;
> +		};
> +
> +		lpg@b200 {
> +			compatible = "qcom,spmi-lpg";
> +			reg = <0xb200>;
> +
> +			cell-index = <2>;
> +
> +			label = "lpg:green:user0";
> +
> +			qcom,tri-led = <&pmi8994_tri_led 1>;
> +			qcom,lut = <&pmi8994_lpg_lut>;
> +
> +			qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
> +						  425 469 491 502 507>;
> +			qcom,pattern-length-ms = <1337>;
> +			qcom,pattern-ping-pong;
> +
> +			default-state = "on";
> +		};
> +
> +		pmi8994_tri_led: tri-led@d000 {

It may make more sense to make the LED(s) and their properties a sub 
node of this. You could always use the PWM binding to link back to the 
LPG. The pattern/LUT is really just a queue of PWM settings. That's not 
all that different than a PWM based audio buzzer. There was a DMA based 
PWM binding the other day for audio.

Rob

> +			compatible = "qcom,spmi-tri-led";
> +			reg = <0xd000>;
> +
> +			qcom,power-source = <1>;
> +		};
> +	};
> +};
> -- 
> 2.12.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc
From: Linus Walleij @ 2017-03-29  2:30 UTC (permalink / raw)
  To: jacopo
  Cc: Geert Uytterhoeven, Jacopo Mondi, Geert Uytterhoeven,
	Laurent Pinchart, Chris Brandt, Rob Herring, Mark Rutland,
	Russell King, Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <b9eaac8a63dced229bb152ba412e8ec1@jmondi.org>

On Tue, Mar 28, 2017 at 4:38 PM,  <jacopo@jmondi.org> wrote:

>> The fact that historically all the early adopters of pinctrl in device
>> tree
>> have these funky custom bindings is unfortunate but just something
>> that we need to live with.
>>
>
> To avoid any confusion, please bear with me and clarify this once and for
> all,
> since I'm not certain I fully got you here.
>
> Are you suggesting:
>
> 1) Use "pins" property with the currently implemented ABI (which slightly
> differs
>    from the standard documented one as explained above. Not sure it is fine
> overriding
>    it or not)

Correction: you should be using the property "pinmux", because you
are setting group and function at the same time.

See for example:
include/dt-bindings/pinctrl/mt65xx.h

And how that is used in:
arch/arm/boot/dts/mt2701-pinfunc.h
arch/arm/boot/dts/mt2701-evb.dts

The docs are here:
Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt

I'm sorry that "pinmux" is not part of the generic documentation, it'd be
great if you would like to add it with a patch.

Yours,
Linus Walleij

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox