* [PATCH V3 2/4] pwm: tegra: Increase precision in pwm rate calculation
From: Laxman Dewangan @ 2017-04-07 9:34 UTC (permalink / raw)
To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, jonathanh-DDmLM1+adcrQT0dZR+AlfA
Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Laxman Dewangan
In-Reply-To: <1491557642-15940-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
The rate of the PWM calculated as follows:
hz = NSEC_PER_SEC / period_ns;
rate = (rate + (hz / 2)) / hz;
This has the precision loss in lower PWM rate.
Change this to have more precision as:
hz = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC * 100, period_ns);
rate = DIV_ROUND_CLOSEST(rate * 100, hz)
Example:
1. period_ns = 16672000, PWM clock rate is 200KHz.
Based on old formula
hz = NSEC_PER_SEC / period_ns
= 1000000000ul/16672000
= 59 (59.98)
rate = (200K + 59/2)/59 = 3390
Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(200000*100, 5998) = 3334
If we measure the PWM signal rate, we will get more accurate period
with rate value of 3334 instead of 3390.
2. period_ns = 16803898, PWM clock rate is 200KHz.
Based on old formula:
hz = 59, rate = 3390
Based on new formula:
hz = 5951, rate = 3360
The PWM signal rate of 3360 is more near to requested period than 3333.
Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from v1:
- None
Changes from V2:
- Fix the commit message with exact formula used.
---
drivers/pwm/pwm-tegra.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 0a688da..21518be 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -76,6 +76,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
unsigned long long c = duty_ns;
unsigned long rate, hz;
+ unsigned long long ns100 = NSEC_PER_SEC;
u32 val = 0;
int err;
@@ -94,9 +95,11 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
* cycles at the PWM clock rate will take period_ns nanoseconds.
*/
rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
- hz = NSEC_PER_SEC / period_ns;
- rate = (rate + (hz / 2)) / hz;
+ /* Consider precision in PWM_SCALE_WIDTH rate calculation */
+ ns100 *= 100;
+ hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);
+ rate = DIV_ROUND_CLOSEST(rate * 100, hz);
/*
* Since the actual PWM divider is the register's frequency divider
--
2.1.4
^ permalink raw reply related
* [PATCH V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume
From: Laxman Dewangan @ 2017-04-07 9:34 UTC (permalink / raw)
To: thierry.reding, robh+dt, jonathanh
Cc: mark.rutland, linux-pwm, devicetree, linux-tegra, linux-kernel,
Laxman Dewangan
In-Reply-To: <1491557642-15940-1-git-send-email-ldewangan@nvidia.com>
In some of NVIDIA Tegra's platform, PWM controller is used to
control the PWM controlled regulators. PWM signal is connected to
the VID pin of the regulator where duty cycle of PWM signal decide
the voltage level of the regulator output.
When system enters suspend, some PWM client/slave regulator devices
require the PWM output to be tristated.
Add DT binding details to provide the pin configuration state
from PWM and pinctrl DT node in suspend and active state of
the system.
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from v1:
- Use standard pinctrl names for sleep and active state.
Changes from V2:
- Fix the commit message and details
---
.../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
index b4e7377..c57e11b 100644
--- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
@@ -19,6 +19,19 @@ Required properties:
- reset-names: Must include the following entries:
- pwm
+Optional properties:
+============================
+In some of the interface like PWM based regulator device, it is required
+to configure the pins differently in different states, especially in suspend
+state of the system. The configuration of pin is provided via the pinctrl
+DT node as detailed in the pinctrl DT binding document
+ Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+The PWM node will have following optional properties.
+pinctrl-names: Pin state names. Must be "default" and "sleep".
+pinctrl-0: phandle for the default/active state of pin configurations.
+pinctrl-1: phandle for the sleep state of pin configurations.
+
Example:
pwm: pwm@7000a000 {
@@ -29,3 +42,35 @@ Example:
resets = <&tegra_car 17>;
reset-names = "pwm";
};
+
+
+Example with the pin configuration for suspend and resume:
+=========================================================
+Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
+it requires PWM output to be tristated when system enters suspend.
+Following will be DT binding to achieve this:
+
+#include <dt-bindings/pinctrl/pinctrl-tegra.h>
+
+ pinmux@700008d4 {
+ pwm_active_state: pwm_active_state {
+ pe7 {
+ nvidia,pins = "pe7";
+ nvidia,tristate = <TEGRA_PIN_DISABLE>;
+ };
+ };
+
+ pwm_sleep_state: pwm_sleep_state {
+ pe7 {
+ nvidia,pins = "pe7";
+ nvidia,tristate = <TEGRA_PIN_ENABLE>;
+ };
+ };
+ };
+
+ pwm@7000a000 {
+ /* Mandatory PWM properties */
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&pwm_active_state>;
+ pinctrl-1 = <&pwm_sleep_state>;
+ };
--
2.1.4
^ permalink raw reply related
* [PATCH V3 4/4] pwm: tegra: Add support to configure pin state in suspends/resume
From: Laxman Dewangan @ 2017-04-07 9:34 UTC (permalink / raw)
To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, jonathanh-DDmLM1+adcrQT0dZR+AlfA
Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Laxman Dewangan
In-Reply-To: <1491557642-15940-1-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
In some of NVIDIA Tegra's platform, PWM controller is used to
control the PWM controlled regulators. PWM signal is connected to
the VID pin of the regulator where duty cycle of PWM signal decide
the voltage level of the regulator output.
When system enters suspend, some PWM client/slave regulator devices
require the PWM output to be tristated.
Add support to configure the pin state via pinctrl frameworks in
suspend and active state of the system.
Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from v1:
- Use standard pinctrl names for sleep and active state.
- Use API pinctrl_pm_select_*()
Changes from V2:
- Use returns of pinctrl_pm_select_*()
- Rephrase commit message.
---
drivers/pwm/pwm-tegra.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 21518be..9c7f180 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -29,6 +29,7 @@
#include <linux/of_device.h>
#include <linux/pwm.h>
#include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/slab.h>
#include <linux/reset.h>
@@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(&pc->chip);
}
+#ifdef CONFIG_PM_SLEEP
+static int tegra_pwm_suspend(struct device *dev)
+{
+ return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int tegra_pwm_resume(struct device *dev)
+{
+ return pinctrl_pm_select_default_state(dev);
+}
+#endif
+
static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
};
@@ -271,10 +284,15 @@ static const struct of_device_id tegra_pwm_of_match[] = {
MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
+static const struct dev_pm_ops tegra_pwm_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
+};
+
static struct platform_driver tegra_pwm_driver = {
.driver = {
.name = "tegra-pwm",
.of_match_table = tegra_pwm_of_match,
+ .pm = &tegra_pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
--
2.1.4
--
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 related
* Re: [PATCH v4 04/14] GPIO: Add gpio-ingenic driver
From: Linus Walleij @ 2017-04-07 9:34 UTC (permalink / raw)
To: Paul Cercueil
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Ralf Baechle,
Boris Brezillon, Thierry Reding, Bartlomiej Zolnierkiewicz,
Maarten ter Huurne, Lars-Peter Clausen, Paul Burton, James Hogan,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux MIPS,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170402204244.14216-5-paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
> This driver handles the GPIOs of all the Ingenic JZ47xx SoCs
> currently supported by the upsteam Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
I guess you saw the Kconfig complaint from the build robot, please fix that
so we get silent builds.
> v2: Consider it's a new patch. Completely rewritten from v1.
> v3: Add missing include <linux/pinctrl/consumer.h> and drop semicolon after }
> v4: Completely rewritten from v3.
I really like v4 :)
> +static inline bool gpio_get_value(struct ingenic_gpio_chip *jzgc, u8 offset)
Actually the return value should be an int.
I know, it is a historical artifact, if we change it we need to change
it everywhere.
> + /* DO NOT EXPAND THIS: FOR BACKWARD GPIO NUMBERSPACE COMPATIBIBILITY
> + * ONLY: WORK TO TRANSITION CONSUMERS TO USE THE GPIO DESCRIPTOR API IN
> + * <linux/gpio/consumer.h> INSTEAD.
> + */
> + jzgc->gc.base = cell->id * 32;
OK then :)
This is merge material.
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] arm64: dts: Add coresight DT nodes for hi6220-hikey
From: Leo Yan @ 2017-04-07 9:38 UTC (permalink / raw)
To: Li Pengcheng
Cc: xuwei5-C8/M+/jPZTeaMJb+Lgu22Q, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
suzhuangluan-C8/M+/jPZTeaMJb+Lgu22Q,
dan.zhao-C8/M+/jPZTeaMJb+Lgu22Q, lizhong11-C8/M+/jPZTeaMJb+Lgu22Q,
Mathieu Poirier
In-Reply-To: <1491552418-74386-1-git-send-email-lipengcheng8-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Hi Pengcheng,
[ + Mathieu ]
On Fri, Apr 07, 2017 at 04:06:58PM +0800, Li Pengcheng wrote:
> Add coresight DT nodes for hikey board.
>
> Signed-off-by: Li Pengcheng <lipengcheng8-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Li Zhong <lizhong11-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
> ---
> .../arm64/boot/dts/hisilicon/hi6220-coresight.dtsi | 318 +++++++++++++++++++++
> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 1 +
> 2 files changed, 319 insertions(+)
> create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-coresight.dtsi
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220-coresight.dtsi
> new file mode 100644
> index 0000000..a523e43
> --- /dev/null
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-coresight.dtsi
> @@ -0,0 +1,318 @@
> +/*
> + * Hisilicon Ltd. Hi6220 SoC
> + *
> + * Copyright (C) 2015-2016 Hisilicon Ltd.
> + * Author: lipengcheng <lipengcheng8-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * publishhed by the Free Software Foundation.
> + */
> +/ {
> + amba {
Here I'm curious if can use the similiar implementation with Juno?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/arm/juno-cs-r1r2.dtsi?id=refs/tags/v4.11-rc5
So finally can include this file into hi6220.dtsi 'soc' section; this
can reflect the hardware topology more clearly:
soc {
[...]
#include "hi6220-coresight.dtsi"
}
> + /* A53 cluster0 internal coresight */
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "arm,amba-bus";
Usually the compatible string should be documented in
Documentation/devicetree/bindings/, so please drop it.
I think "#address-cells" and "#size-cells" also can remove. This is
defined by its parent node.
> + ranges;
> + etm@0,f659c000 {
Change to etm@f659c000?
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf659c000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu0>;
> + port {
> + etm0_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port0>;
> + };
> + };
> + };
> +
> + etm@1,f659d000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf659d000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu1>;
> + port {
> + etm1_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port1>;
> + };
> + };
> +
> + };
> +
> + etm@2,f659e000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf659e000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu2>;
> + port {
> + etm2_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port2>;
> + };
> + };
> + };
> +
> + etm@3,f659f000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf659f000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu3>;
> + port {
> + etm3_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port3>;
> + };
> + };
> + };
> +
> + /* A53 cluster1 internal coresight */
> + etm@4,f65dc000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf65dc000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu4>;
> + port {
> + etm4_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port4>;
Wrong indent.
> + };
> + };
> + };
> +
> + etm@5,f65dd000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf65dd000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu5>;
> + port {
> + etm5_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port5>;
Wrong indent.
> + };
> + };
> + };
> +
> + etm@6,f65de000 {
Change to etm@f65de000?
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf65de000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu6>;
> + port {
> + etm6_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port6>;
> + };
> + };
> + };
> +
> + etm@7,f65df000 {
Same with above.
> + compatible = "arm,coresight-etm4x","arm,primecell";
> + reg = <0 0xf65df000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + cpu = <&cpu7>;
> + port {
> + etm7_out_port: endpoint {
> + remote-endpoint = <&funnel0_in_port7>;
> + };
> + };
> + };
> +
> + funnel0:funnel@0,f6501000 {
funnel0@f6501000.
> + compatible = "arm,coresight-funnel","arm,primecell";
> + reg = <0 0xf6501000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* funnel output port */
> + port@0 {
> + reg = <0>;
> + funnel0_out_port: endpoint {
> + remote-endpoint = <&funnel1_in_port>;
> + };
> + };
> +
> + /* funnel input ports */
> + port@1 {
> + reg = <0>;
> + funnel0_in_port0: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm0_out_port>;
> + };
> + };
> +
> + port@2 {
> + reg = <1>;
> + funnel0_in_port1: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm1_out_port>;
> + };
> + };
> +
> + port@3 {
> + reg = <2>;
> + funnel0_in_port2: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm2_out_port>;
> + };
> + };
> +
> + port@4 {
> + reg = <3>;
> + funnel0_in_port3: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm3_out_port>;
> + };
> + };
> +
> + port@5 {
> + reg = <4>;
> + funnel0_in_port4: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm4_out_port>;
> + };
> + };
> +
> + port@6 {
> + reg = <5>;
> + funnel0_in_port5: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm5_out_port>;
> + };
> + };
> +
> + port@7 {
> + reg = <6>;
> + funnel0_in_port6: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm6_out_port>;
> + };
> + };
> +
> + port@8 {
> + reg = <7>;
> + funnel0_in_port7: endpoint {
> + slave-mode;
> + remote-endpoint = <&etm7_out_port>;
> + };
> + };
> + };
> + };
> +
> + funnel1:funnel@1,f6401000 {
funnel1@f6401000
> + compatible = "arm,coresight-funnel","arm,primecell";
> + reg = <0 0xf6401000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + /* funnel1 output port */
> + port@0 {
> + reg = <0>;
> + funnel1_out_port: endpoint {
> + remote-endpoint = <&etf_in_port>;
> + };
> + };
> +
> + /* funnel1 input port */
> + port@1 {
> + reg = <0>;
> + funnel1_in_port: endpoint {
> + slave-mode;
> + remote-endpoint = <&funnel0_out_port>;
> + };
> + };
> + };
> + };
Extra blank line.
> + etf:etf@0,f6402000 {
> + compatible = "arm,coresight-tmc","arm,primecell";
> + reg = <0 0xf6402000 0 0x1000>;
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + /* etf input port */
> + port@0 {
> + reg = <0>;
> + etf_in_port: endpoint {
> + slave-mode;
> + remote-endpoint = <&funnel1_out_port>;
> + };
> + };
> + /* etf output port */
> + port@1 {
> + reg = <0>;
> + etf_out_port: endpoint {
> + remote-endpoint = <&replicator0_in_port>;
> + };
> + };
> + };
> + };
> +
> + replicator@0{
> + compatible = "arm,coresight-replicator";
> +
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + /* replicator input port */
> + port@0 {
> + reg = <0>;
> + replicator0_in_port: endpoint{
> + slave-mode;
> + remote-endpoint = <&etf_out_port>;
Wrong indent.
> + };
> + };
> + /* replicator out port */
> + port@1 {
> + reg = <0>;
> + replicator0_out_port: endpoint {
> + remote-endpoint = <&etr0_in_port>;
> + };
> + };
> + };
> + };
> +
> + etr@0,f6404000 {
> + compatible = "arm,coresight-tmc","arm,primecell";
> + reg = <0 0xf6404000 0 0x1000>;
> +
> + coresight-default-sink;
> + clocks = <&sys_ctrl HI6220_CS_ATB>;
> + clock-names = "apb_pclk";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + /* etr input port */
> + port@0 {
> + etr0_in_port: endpoint{
> + slave-mode;
> + remote-endpoint = <&replicator0_out_port>;
Wrong indent.
> + };
> + };
> + };
> + };
> + };
> +};
> +
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index dba3c13..fb70c9b 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -8,6 +8,7 @@
> /dts-v1/;
> #include "hi6220.dtsi"
> #include "hikey-pinctrl.dtsi"
> +#include "hi6220-coresight.dtsi"
Please review upper comments for adding it into 'soc' node.
BTW, it's good to use script ./scripts/checkpatch.pl to check the
patch.
> #include <dt-bindings/gpio/gpio.h>
>
> / {
> --
> 2.1.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 v4 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs
From: Linus Walleij @ 2017-04-07 9:41 UTC (permalink / raw)
To: Paul Cercueil, Lee Jones
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Ralf Baechle,
Boris Brezillon, Thierry Reding, Bartlomiej Zolnierkiewicz,
Maarten ter Huurne, Lars-Peter Clausen, Paul Burton, James Hogan,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Linux MIPS,
linux-mmc@vger.kernel.org
In-Reply-To: <20170402204244.14216-4-paul@crapouillou.net>
On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> This driver handles pin configuration and pin muxing for the
> JZ4740 and JZ4780 SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
(...)
> + select MFD_CORE
(...)
> +#include <linux/mfd/core.h>
That's unorthodox. Still quite pretty!
I would nee Lee Jones to say something about this, as it is
essentially hijacking MFD into the pinctrl subsystem.
> +static struct mfd_cell ingenic_pinctrl_mfd_cells[] = {
> + {
> + .id = 0,
> + .name = "GPIOA",
> + .of_compatible = "ingenic,gpio-bank-a",
> + },
> + {
> + .id = 1,
> + .name = "GPIOB",
> + .of_compatible = "ingenic,gpio-bank-b",
> + },
> + {
> + .id = 2,
> + .name = "GPIOC",
> + .of_compatible = "ingenic,gpio-bank-c",
> + },
> + {
> + .id = 3,
> + .name = "GPIOD",
> + .of_compatible = "ingenic,gpio-bank-d",
> + },
> + {
> + .id = 4,
> + .name = "GPIOE",
> + .of_compatible = "ingenic,gpio-bank-e",
> + },
> + {
> + .id = 5,
> + .name = "GPIOF",
> + .of_compatible = "ingenic,gpio-bank-f",
> + },
> +};
(...)
> + err = devm_mfd_add_devices(dev, 0, ingenic_pinctrl_mfd_cells,
> + ARRAY_SIZE(ingenic_pinctrl_mfd_cells), NULL, 0, NULL);
> + if (err) {
> + dev_err(dev, "Failed to add MFD devices\n");
> + return err;
> + }
I guess the alternative would be to reimplement the MFD structure.
Did you check the approach to use "simple-mfd" and just let the subnodes
spawn as devices that way? I guess you did and this adds something
necessary.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 09/16] pinctrl: madera: Add driver for Cirrus Logic Madera codecs
From: Richard Fitzgerald @ 2017-04-07 9:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Rob Herring, Thomas Gleixner, Jason Cooper,
Lee Jones, Mark Brown,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CACRpkdZK3QXu4t2jud0-LPDj0LDVruAm33N4Lazjk44C3ndwwQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2017-04-07 at 10:54 +0200, Linus Walleij wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>
> > These codecs have a variable number of I/O lines each of which
> > is individually selectable to a wide range of possible functions.
> >
> > The functionality is slightly different from the traditional muxed
> > GPIO since most of the functions can be mapped to any pin (and even
> > the same function to multiple pins). Most pins have a dedicated
> > "alternate" function that is only available on that pin. The
> > alternate functions are usually a group of signals, though it is
> > not always necessary to enable the full group, depending on the
> > alternate function and how it is to be used. The mapping between
> > alternate functions and GPIO pins varies between codecs depending
> > on the number of alternate functions and available pins.
> >
> > Signed-off-by: Richard Fitzgerald <rf-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>
> > .../bindings/pinctrl/cirrus,madera-pinctrl.txt | 103 ++
>
> This should ideally be split into its own patch but I don't care
> much if the DT people are happy.
>
> > +See also
> > + the core bindings for the parent MFD driver:
> > + Documentation/devicetree/bindings/mfd/madera.txt
> > +
> > + the generic pinmix bindings:
> > + Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>
> Nice.
>
> > +Required properties of parent mfd node:
> > + - pinctrl-names : must be "defaults"
>
> Do you mean "default"
Yes. I'll fix that.
>
> Apart from this the bindings and example look very good to
> me, good job! I like it when people "just get it" with pin control
> and that is where we need to be with this subsystem.
>
> > +config PINCTRL_MADERA
> > + bool
> > + default y if MFD_MADERA=y
>
There was something special to do with the way dependencies are
processed, but I can't remember right now what that was. I'd have to
take another look at this to see if this "default y" pattern is still
necessary for this driver.
> Isn't it even proper for MFD_MADERA to explicitly
> select this driver. I see it hard how the chip would even
> work without this. (Maybe it already does select it but then
> default y is not necessary.)
> > +config PINCTRL_CS47L35
> > + bool
> > + default y if MFD_CS47L35=y
>
> Similar comment for the subdrivers.
>
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_PINCTRL_AMD) += pinctrl-amd.o
> > obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
> > obj-$(CONFIG_PINCTRL_DIGICOLOR) += pinctrl-digicolor.o
> > obj-$(CONFIG_PINCTRL_FALCON) += pinctrl-falcon.o
> > +obj-$(CONFIG_PINCTRL_MADERA) += pinctrl-madera.o
>
> Is it all in one file... despite all the Kconfig symbols... hm.
> I guess we can create drivers/pinctrl/cirrus the day we need more
> space.
>
I have no objection to moving the source into pinctrl/cirrus
> > +/*
> > + * Pins are named after their GPIO number
>
> So don't they have real names? Like the pin name on the underside of
> the chip? That is what this naming convention is actually for.
>
Those are real names. Each pin is dual labelled with a "GPIOn" name and
also its alternate function (if it has one). The mapping of alternate
functions to GPIO pins isn't 1:1 across codecs. The GPIOn name is
consistent across codecs. If your pinctrl config is needing to refer to
the pin name, instead of the alternate function group, it can only be to
use it as a GPIO so the GPIO name is more relevant. This is what I was
trying to imply in my comment but using fewer words.
> > +/*
> > + * All single-pin functions can be mapped to any GPIO, however pinmux applies
> > + * functions to pin groups and only those groups declared as supporting that
> > + * function. To make this work we must put each pin in its own dummy group so
> > + * that the functions can be described as applying to all pins.
> > + * Since these do not correspond to anything in the actual hardware - they are
> > + * merely an adaptation to pinctrl's view of the world - we use the same name
> > + * as the pin to avoid confusion when comparing with datasheet instructions
> > + */
> > +static const char * const madera_pin_single_group_names[] = {
> > + "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7",
> > + "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", "gpio14",
> > + "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", "gpio21",
> > + "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", "gpio28",
> > + "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", "gpio35",
> > + "gpio36", "gpio37", "gpio38", "gpio39", "gpio40",
> > +};
>
> If they are called "gpioN" in the datasheet I guess it is all right.
> That is how e.g. the Qualcomm driver is done.
>
> > +#ifdef CONFIG_PINCTRL_CS47L85
>
> So this makes me feel maybe we should create drivers/pinctrl/cirrus
> and split this driver into subdrivers per chip like others do.
>
> The coding style document does say that ifdefs are ugly.
>
> Would you consider splitting it up?
>
I can do that.
> > +static void madera_pin_dbg_show(struct pinctrl_dev *pctldev,
> > + struct seq_file *s,
> > + unsigned int offset)
> > +{
> > + seq_puts(s, " madera-pinctrl");
> > +}
>
> I don't think the pinctrl debugfs callback is compulsory.
> It would be nice if this added some actual useful information
> about the pin.
>
Yes, I'll add some info
>
> > + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > + mask[0] |= MADERA_GP1_OP_CFG_MASK;
> > + conf[0] |= MADERA_GP1_OP_CFG;
> > + break;
> > + case PIN_CONFIG_DRIVE_PUSH_PULL:
> > + mask[0] |= MADERA_GP1_OP_CFG_MASK;
> > + conf[0] &= ~MADERA_GP1_OP_CFG;
> > + break;
>
> This will be possible to reuse from a GPIO driver as back-end, nice!
>
>
> > + case PIN_CONFIG_INPUT_DEBOUNCE:
> > + mask[0] |= MADERA_GP1_DB_MASK;
> > +
> > + /*
> > + * we can't configure debounce time per-pin so value
> > + * is just a flag
> > + */
> > + val = pinconf_to_config_argument(*configs);
> > + if (val)
> > + conf[0] |= MADERA_GP1_DB;
> > + else
> > + conf[0] &= ~MADERA_GP1_DB;
> > + break;
>
> This too.
>
> Overall it looks very nice.
>
> 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 v2 2/8] v4l: fwnode: Support generic fwnode for parsing standardised properties
From: Laurent Pinchart @ 2017-04-07 9:44 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-3-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:04 Sakari Ailus wrote:
> The fwnode_handle is a more generic way than OF device_node to describe
> firmware nodes. Instead of the OF API, use more generic fwnode API to
> obtain the same information.
I would mention that this is a copy of v4l2-of.c with the OF API replaced with
the fwnode API.
> As the V4L2 fwnode support will be required by a small minority of e.g.
> ACPI based systems (the same might actually go for OF), make this a module
> instead of embedding it in the videodev module.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/Kconfig | 3 +
> drivers/media/v4l2-core/Makefile | 1 +
> drivers/media/v4l2-core/v4l2-fwnode.c | 353 +++++++++++++++++++++++++++++++
> include/media/v4l2-fwnode.h | 104 ++++++++++
> 4 files changed, 461 insertions(+)
> create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
> create mode 100644 include/media/v4l2-fwnode.h
>
> diff --git a/drivers/media/v4l2-core/Kconfig
> b/drivers/media/v4l2-core/Kconfig index 6b1b78f..a35c336 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -55,6 +55,9 @@ config V4L2_FLASH_LED_CLASS
>
> When in doubt, say N.
>
> +config V4L2_FWNODE
> + tristate
> +
> # Used by drivers that need Videobuf modules
> config VIDEOBUF_GEN
> tristate
> diff --git a/drivers/media/v4l2-core/Makefile
> b/drivers/media/v4l2-core/Makefile index 795a535..cf77a63 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -13,6 +13,7 @@ endif
> ifeq ($(CONFIG_OF),y)
> videodev-objs += v4l2-of.o
> endif
> +obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> ifeq ($(CONFIG_TRACEPOINTS),y)
> videodev-objs += vb2-trace.o v4l2-trace.o
> endif
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c new file mode 100644
> index 0000000..4f69b11
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -0,0 +1,353 @@
> +/*
> + * V4L2 fwnode binding parsing library
> + *
> + * Copyright (c) 2016 Intel Corporation.
> + * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> + *
> + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/acpi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-fwnode.h>
> +
> +static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle *fwn,
> + struct v4l2_fwnode_endpoint
*vfwn)
> +{
> + struct v4l2_fwnode_bus_mipi_csi2 *bus = &vfwn->bus.mipi_csi2;
> + bool have_clk_lane = false;
> + unsigned int flags = 0, lanes_used = 0;
> + unsigned int i;
> + u32 v;
> + int rval;
I would have used "ret" instead of "rval" ;-)
> + rval = fwnode_property_read_u32_array(fwn, "data-lanes", NULL, 0);
> + if (rval > 0) {
> + u32 array[ARRAY_SIZE(bus->data_lanes)];
> +
> + bus->num_data_lanes =
> + min_t(int, ARRAY_SIZE(bus->data_lanes), rval);
> +
> + fwnode_property_read_u32_array(
> + fwn, "data-lanes", array, bus->num_data_lanes);
Is there anything wrong with the usual way to wrap code ?
fwnode_property_read_u32_array(fwn, "data-lanes", array,
bus->num_data_lanes);
> +
> + for (i = 0; i < bus->num_data_lanes; i++) {
> + if (lanes_used & BIT(array[i]))
> + pr_warn("duplicated lane %u in data-lanes\n",
> + array[i]);
> + lanes_used |= BIT(array[i]);
> +
> + bus->data_lanes[i] = array[i];
> + }
> + }
> +
> + rval = fwnode_property_read_u32_array(fwn, "lane-polarities", NULL,
0);
> + if (rval > 0) {
> + u32 array[ARRAY_SIZE(bus->lane_polarities)];
> +
> + if (rval < 1 + bus->num_data_lanes /* clock + data */) {
> + pr_warn("too few lane-polarities entries (need %u, got
%u)\n",
> + 1 + bus->num_data_lanes, rval);
> + return -EINVAL;
> + }
> +
> + fwnode_property_read_u32_array(
> + fwn, "lane-polarities", array, 1 + bus-
>num_data_lanes);
Ditto with
fwnode_property_read_u32_array(fwn, "lane-polarities", array,
1 + bus->num_data_lanes);
> + for (i = 0; i < 1 + bus->num_data_lanes; i++)
> + bus->lane_polarities[i] = array[i];
> + }
> +
> + if (!fwnode_property_read_u32(fwn, "clock-lanes", &v)) {
> + if (lanes_used & BIT(v))
> + pr_warn("duplicated lane %u in clock-lanes\n", v);
> + lanes_used |= BIT(v);
> +
> + bus->clock_lane = v;
> + have_clk_lane = true;
> + }
> +
> + if (fwnode_property_present(fwn, "clock-noncontinuous"))
> + flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> + else if (have_clk_lane || bus->num_data_lanes > 0)
> + flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +
> + bus->flags = flags;
> + vfwn->bus_type = V4L2_MBUS_CSI2;
> +
> + return 0;
> +}
> +
> +static void v4l2_fwnode_endpoint_parse_parallel_bus(
> + struct fwnode_handle *fwn, struct v4l2_fwnode_endpoint *vfwn)
> +{
> + struct v4l2_fwnode_bus_parallel *bus = &vfwn->bus.parallel;
> + unsigned int flags = 0;
> + u32 v;
> +
> + if (!fwnode_property_read_u32(fwn, "hsync-active", &v))
> + flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH :
> + V4L2_MBUS_HSYNC_ACTIVE_LOW;
> +
> + if (!fwnode_property_read_u32(fwn, "vsync-active", &v))
> + flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH :
> + V4L2_MBUS_VSYNC_ACTIVE_LOW;
> +
> + if (!fwnode_property_read_u32(fwn, "field-even-active", &v))
> + flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH :
> + V4L2_MBUS_FIELD_EVEN_LOW;
> + if (flags)
> + vfwn->bus_type = V4L2_MBUS_PARALLEL;
> + else
> + vfwn->bus_type = V4L2_MBUS_BT656;
> +
> + if (!fwnode_property_read_u32(fwn, "pclk-sample", &v))
> + flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> + V4L2_MBUS_PCLK_SAMPLE_FALLING;
> +
> + if (!fwnode_property_read_u32(fwn, "data-active", &v))
> + flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
> + V4L2_MBUS_DATA_ACTIVE_LOW;
> +
> + if (fwnode_property_present(fwn, "slave-mode"))
> + flags |= V4L2_MBUS_SLAVE;
> + else
> + flags |= V4L2_MBUS_MASTER;
> +
> + if (!fwnode_property_read_u32(fwn, "bus-width", &v))
> + bus->bus_width = v;
> +
> + if (!fwnode_property_read_u32(fwn, "data-shift", &v))
> + bus->data_shift = v;
> +
> + if (!fwnode_property_read_u32(fwn, "sync-on-green-active", &v))
> + flags |= v ? V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH :
> + V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW;
> +
> + bus->flags = flags;
> +
> +}
> +
> +/**
> + * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> + * @fwn: pointer to fwnode_handle
I would say "the endpoint's fwnode handle", or "pointer to the endpoint's
fwnode handle". Otherwise which fwnode handle should be passed won't be clear.
> + * @vfwn: pointer to the V4L2 fwnode data structure
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.
> + * If any properties that identify the bus as parallel are found and
> + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> recognise
> + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> + * The caller should hold a reference to @node.
s/@node/@fwn/
By the way, have you tried to compile the documentation ?
> + *
> + * NOTE: This function does not parse properties the size of which is
> + * variable without a low fixed limit. Please use
> + * v4l2_fwnode_endpoint_alloc_parse() in new drivers instead.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwn,
> + struct v4l2_fwnode_endpoint *vfwn)
Is there a specific reason not to keep the original parameter name "endpoint"
? vfwn seems to stand for v4l2_fwnode, not v4l2_fwnode_endpoint. Same comment
for all other locations in this patch.
> +{
> + int rval;
> +
> + fwnode_graph_parse_endpoint(fwn, &vfwn->base);
> +
> + /* Zero fields from bus_type to until the end */
> + memset(&vfwn->bus_type, 0, sizeof(*vfwn) -
> + offsetof(typeof(*vfwn), bus_type));
> +
> + rval = v4l2_fwnode_endpoint_parse_csi_bus(fwn, vfwn);
> + if (rval)
> + return rval;
> + /*
> + * Parse the parallel video bus properties only if none
> + * of the MIPI CSI-2 specific properties were found.
> + */
> + if (vfwn->bus.mipi_csi2.flags == 0)
> + v4l2_fwnode_endpoint_parse_parallel_bus(fwn, vfwn);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_fwnode_endpoint_parse);
According to Mauro in http://www.mail-archive.com/linux-media@vger.kernel.org/msg110809.html you should use EXPORT_SYMBOL_GPL.
> +
> +/*
> + * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> + * v4l2_fwnode_endpoint_alloc_parse()
> + * @fwn - the V4L2 fwnode the resources of which are to be released
Mayeb "the V4L2 fwnode whose resources are to be released" ?
> + *
> + * It is safe to call this function with NULL argument or on an
s/on an/on a/
> + * V4L2 fwnode the parsing of which failed.
"whose parsing failed" ?
> + */
> +void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vfwn)
> +{
> + if (IS_ERR_OR_NULL(vfwn))
> + return;
> +
> + kfree(vfwn->link_frequencies);
> + kfree(vfwn);
> +}
> +EXPORT_SYMBOL(v4l2_fwnode_endpoint_free);
> +
> +/**
> + * v4l2_fwnode_endpoint_alloc_parse() - parse all fwnode node properties
> + * @node: pointer to fwnode_handle
s/@node/@fwn/
Here too, I would say "the endpoint's fwnode handle", or "pointer to the
endpoint's fwnode handle".
> + *
> + * All properties are optional. If none are found, we don't set any flags.
> + * This means the port has a static configuration and no properties have
> + * to be specified explicitly.
> + * If any properties that identify the bus as parallel are found and
> + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we
> recognise
> + * the bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> + * The caller should hold a reference to @node.
s/@node/@fwn/
> + *
> + * v4l2_fwnode_endpoint_alloc_parse() has two important differences to
> + * v4l2_fwnode_endpoint_parse():
> + *
> + * 1. It also parses variable size data and
Nitpicking, a valid sentence doesn't end with "and$". You could write
1. It also parses variable size data.
2. The memory it has allocated to store the variable size data must
be freed using v4l2_fwnode_endpoint_free() when no longer needed.
or
1. it also parses variable size data; and
2. the memory it has allocated to store the variable size data must
be freed using v4l2_fwnode_endpoint_free() when no longer needed.
> + *
> + * 2. The memory it has allocated to store the variable size data must
> + * be freed using v4l2_fwnode_endpoint_free() when no longer needed.
> + *
> + * Return: Pointer to v4l2_fwnode_endpoint if successful, on error a
> + * negative error code.
The function returns an error pointer on error.
> + */
> +struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> + struct fwnode_handle *fwn)
> +{
> + struct v4l2_fwnode_endpoint *vfwn;
> + int rval;
ret ? :-)
> +
> + vfwn = kzalloc(sizeof(*vfwn), GFP_KERNEL);
> + if (!vfwn)
> + return ERR_PTR(-ENOMEM);
> +
> + rval = v4l2_fwnode_endpoint_parse(fwn, vfwn);
> + if (rval < 0)
> + goto out_err;
> +
> + rval = fwnode_property_read_u64_array(fwn, "link-frequencies",
> + NULL, 0);
> +
No need for a blank line.
> + if (rval < 0)
> + goto out_err;
> +
> + vfwn->link_frequencies =
> + kmalloc_array(rval, sizeof(*vfwn->link_frequencies),
> + GFP_KERNEL);
> + if (!vfwn->link_frequencies) {
> + rval = -ENOMEM;
> + goto out_err;
> + }
> +
> + vfwn->nr_of_link_frequencies = rval;
> +
> + rval = fwnode_property_read_u64_array(
> + fwn, "link-frequencies", vfwn->link_frequencies,
> + vfwn->nr_of_link_frequencies);
rval = fwnode_property_read_u64_array(fwn, "link-frequencies",
vfwn->link_frequencies,
vfwn->nr_of_link_frequencies);
> + if (rval < 0)
> + goto out_err;
> +
> + return vfwn;
> +
> +out_err:
> + v4l2_fwnode_endpoint_free(vfwn);
> + return ERR_PTR(rval);
> +}
> +EXPORT_SYMBOL(v4l2_fwnode_endpoint_alloc_parse);
> +
> +/**
> + * v4l2_fwnode_endpoint_parse_link() - parse a link between two endpoints
> + * @node: pointer to the fwnode at the local end of the link
The parameter is called __fwn. I believe you should rename it to fwn,
otherwise the documentation will look weird.
As explained before, you should mention that this is an endpoint fwnode
handle.
> + * @link: pointer to the V4L2 fwnode link data structure
> + *
> + * Fill the link structure with the local and remote nodes and port
> numbers.
> + * The local_node and remote_node fields are set to point to the local and
> + * remote port's parent nodes respectively (the port parent node being the
> + * parent node of the port node if that node isn't a 'ports' node, or the
> + * grand-parent node of the port node otherwise).
> + *
> + * A reference is taken to both the local and remote nodes, the caller
> + * must use v4l2_fwnode_endpoint_put_link() to drop the references
> + * when done with the link.
Just curious, is there a reason to wrap earlier than the 80 columns limit ?
* A reference is taken to both the local and remote nodes, the caller must
use
* v4l2_fwnode_endpoint_put_link() to drop the references when done with the
* link.
would work.
> + * Return: 0 on success, or -ENOLINK if the remote fwnode can't be found.
Here too you should mention remote endpoint.
> + */
> +int v4l2_fwnode_parse_link(struct fwnode_handle *__fwn,
> + struct v4l2_fwnode_link *link)
> +{
> + struct fwnode_handle *fwn;
> + const char *port_prop = is_of_node(__fwn) ? "reg" : "port";
> +
> + memset(link, 0, sizeof(*link));
> +
> + fwn = fwnode_get_parent(__fwn);
> + fwnode_property_read_u32(fwn, port_prop, &link->local_port);
> + fwn = fwnode_get_next_parent(fwn);
> + if (is_of_node(fwn)) {
> + if (of_node_cmp(to_of_node(fwn)->name, "ports") == 0)
> + fwn = fwnode_get_next_parent(fwn);
> + } else {
> + /* The "ports" node is always there in ACPI. */
> + fwn = fwnode_get_next_parent(fwn);
> + }
To avoid duplicating the fwnode_get_next_parent(fwn) call, you could write
if (!is_of_node(fwn) ||
of_node_cmp(to_of_node(fwn)->name, "ports") == 0) {
/* The "ports" node is always there in ACPI. */
fwn = fwnode_get_next_parent(fwn);
}
> + link->local_node = fwn;
> +
> + fwn = fwnode_graph_get_remote_endpoint(fwn);
> + if (!fwn) {
> + fwnode_handle_put(fwn);
> + return -ENOLINK;
> + }
> +
> + fwn = fwnode_get_parent(fwn);
> + fwnode_property_read_u32(fwn, port_prop, &link->remote_port);
> + fwn = fwnode_get_next_parent(fwn);
> + if (is_of_node(fwn)) {
> + if (of_node_cmp(to_of_node(fwn)->name, "ports") == 0)
> + fwn = fwnode_get_next_parent(fwn);
> + } else {
> + /* The "ports" node is always there in ACPI. */
> + fwn = fwnode_get_next_parent(fwn);
> + }
Same comment here.
> + link->remote_node = fwn;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(v4l2_fwnode_parse_link);
> +
> +/**
> + * v4l2_fwnode_put_link() - drop references to nodes in a link
> + * @link: pointer to the V4L2 fwnode link data structure
> + *
> + * Drop references to the local and remote nodes in the link. This
> + * function must be called on every link parsed with
> + * v4l2_fwnode_parse_link().
> + */
> +void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> +{
> + fwnode_handle_put(link->local_node);
> + fwnode_handle_put(link->remote_node);
> +}
> +EXPORT_SYMBOL(v4l2_fwnode_put_link);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> new file mode 100644
> index 0000000..a675d8a
> --- /dev/null
> +++ b/include/media/v4l2-fwnode.h
> @@ -0,0 +1,104 @@
> +/*
> + * V4L2 fwnode binding parsing library
> + *
> + * Copyright (c) 2016 Intel Corporation.
> + * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> + *
> + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * Copyright (C) 2012 Renesas Electronics Corp.
> + * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _V4L2_FWNODE_H
> +#define _V4L2_FWNODE_H
> +
> +#include <linux/list.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/of_graph.h>
Alphabetical order please.
> +#include <media/v4l2-mediabus.h>
> +
> +struct fwnode_handle;
> +
> +/**
> + * struct v4l2_fwnode_bus_mipi_csi2 - MIPI CSI-2 bus data structure
> + * @flags: media bus (V4L2_MBUS_*) flags
> + * @data_lanes: an array of physical data lane indexes
> + * @clock_lane: physical lane index of the clock lane
> + * @num_data_lanes: number of data lanes
> + * @lane_polarities: polarity of the lanes. The order is the same of
> + * the physical lanes.
> + */
> +struct v4l2_fwnode_bus_mipi_csi2 {
> + unsigned int flags;
> + unsigned char data_lanes[4];
> + unsigned char clock_lane;
> + unsigned short num_data_lanes;
> + bool lane_polarities[5];
> +};
> +
> +/**
> + * struct v4l2_fwnode_bus_parallel - parallel data bus data structure
> + * @flags: media bus (V4L2_MBUS_*) flags
> + * @bus_width: bus width in bits
> + * @data_shift: data shift in bits
> + */
> +struct v4l2_fwnode_bus_parallel {
> + unsigned int flags;
> + unsigned char bus_width;
> + unsigned char data_shift;
> +};
> +
> +/**
> + * struct v4l2_fwnode_endpoint - the endpoint data structure
> + * @base: fwnode endpoint of the v4l2_fwnode
> + * @bus_type: bus type
> + * @bus: bus configuration data structure
> + * @link_frequencies: array of supported link frequencies
> + * @nr_of_link_frequencies: number of elements in link_frequenccies array
> + */
> +struct v4l2_fwnode_endpoint {
> + struct fwnode_endpoint base;
> + /*
> + * Fields below this line will be zeroed by
> + * v4l2_fwnode_parse_endpoint()
> + */
> + enum v4l2_mbus_type bus_type;
> + union {
> + struct v4l2_fwnode_bus_parallel parallel;
> + struct v4l2_fwnode_bus_mipi_csi2 mipi_csi2;
> + } bus;
> + u64 *link_frequencies;
> + unsigned int nr_of_link_frequencies;
> +};
> +
> +/**
> + * struct v4l2_fwnode_link - a link between two endpoints
> + * @local_node: pointer to device_node of this endpoint
> + * @local_port: identifier of the port this endpoint belongs to
> + * @remote_node: pointer to device_node of the remote endpoint
> + * @remote_port: identifier of the port the remote endpoint belongs to
> + */
> +struct v4l2_fwnode_link {
> + struct fwnode_handle *local_node;
> + unsigned int local_port;
> + struct fwnode_handle *remote_node;
> + unsigned int remote_port;
> +};
> +
> +int v4l2_fwnode_endpoint_parse(struct fwnode_handle *fwn,
> + struct v4l2_fwnode_endpoint *vfwn);
> +struct v4l2_fwnode_endpoint *v4l2_fwnode_endpoint_alloc_parse(
> + struct fwnode_handle *fwn);
> +void v4l2_fwnode_endpoint_free(struct v4l2_fwnode_endpoint *vfwn);
> +int v4l2_fwnode_parse_link(struct fwnode_handle *fwn,
> + struct v4l2_fwnode_link *link);
> +void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> +
> +#endif /* _V4L2_FWNODE_H */
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers
From: Linus Walleij @ 2017-04-07 9:44 UTC (permalink / raw)
To: Paul Cercueil
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Ralf Baechle,
Boris Brezillon, Thierry Reding, Bartlomiej Zolnierkiewicz,
Maarten ter Huurne, Lars-Peter Clausen, Paul Burton, James Hogan,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux MIPS,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170402204244.14216-7-paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
> ---
> arch/mips/boot/dts/ingenic/jz4740.dtsi | 61 ++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> v2: Changed the devicetree bindings to match the new driver
> v3: No changes
> v4: Update the bindings for the v4 version of the drivers
(...)
> + pinctrl: ingenic-pinctrl@10010000 {
> + compatible = "ingenic,jz4740-pinctrl";
> + reg = <0x10010000 0x400>;
> +
> + gpa: gpio-controller@0 {
> + compatible = "ingenic,gpio-bank-a", "ingenic,jz4740-gpio";
As Sergei and Rob notes, the bank compatible properties look
a bit strange. Especially if they are all the same essentially.
I like Sergei's idea to simply use the reg property if what you want
is really a unique ID number. What do you think about this?
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 1/3] pinctrl: Add bindings for ARTPEC-6 pinmux
From: Linus Walleij @ 2017-04-07 9:47 UTC (permalink / raw)
To: Jesper Nilsson
Cc: Jesper Nilsson, Lars Persson, Niklas Cassel, Rob Herring,
Mark Rutland, Greg Kroah-Hartman, David S. Miller,
Geert Uytterhoeven, Mauro Carvalho Chehab,
linux-arm-kernel-VrBV9hrLPhE,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170330112744.GE29118-VrBV9hrLPhE@public.gmane.org>
On Thu, Mar 30, 2017 at 1:27 PM, Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org> wrote:
> Add the bindings for the pinmux functions in the
> ARTPEC-6 SoC, including bias and drive strength.
>
> Signed-off-by: Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org>
Patch applied with Rob's ACK.
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 v2 3/8] v4l: async: Add fwnode match support
From: Laurent Pinchart @ 2017-04-07 9:49 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-4-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:05 Sakari Ailus wrote:
> Add fwnode matching to complement OF node matching. And fwnode may also be
> an OF node.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 12 ++++++++++++
> include/media/v4l2-async.h | 5 +++++
> include/media/v4l2-subdev.h | 3 +++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 96cc733..384ad5e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -46,6 +46,11 @@ static bool match_of(struct v4l2_subdev *sd, struct
> v4l2_async_subdev *asd) of_node_full_name(asd->match.of.node));
> }
>
> +static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> +{
> + return sd->fwnode == asd->match.fwnode.fwn;
> +}
> +
> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
> if (!asd->match.custom.match)
> @@ -80,6 +85,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier * case V4L2_ASYNC_MATCH_OF:
> match = match_of;
> break;
> + case V4L2_ASYNC_MATCH_FWNODE:
> + match = match_fwnode;
> + break;
> default:
> /* Cannot happen, unless someone breaks us */
> WARN_ON(true);
> @@ -158,6 +166,7 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev, case V4L2_ASYNC_MATCH_DEVNAME:
> case V4L2_ASYNC_MATCH_I2C:
> case V4L2_ASYNC_MATCH_OF:
> + case V4L2_ASYNC_MATCH_FWNODE:
> break;
> default:
> dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev :
NULL,
> @@ -282,6 +291,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> */
> if (!sd->of_node && sd->dev)
> sd->of_node = sd->dev->of_node;
> + if (!sd->fwnode && sd->dev)
> + sd->fwnode = sd->dev->of_node ?
> + &sd->dev->of_node->fwnode : sd->dev->fwnode;
>
> mutex_lock(&list_lock);
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8e2a236..8f552d2 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -32,6 +32,7 @@ struct v4l2_async_notifier;
> * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
> * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
> * @V4L2_ASYNC_MATCH_OF: Match will use OF node
> + * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> *
> * This enum is used by the asyncrhronous sub-device logic to define the
> * algorithm that will be used to match an asynchronous device.
> @@ -41,6 +42,7 @@ enum v4l2_async_match_type {
> V4L2_ASYNC_MATCH_DEVNAME,
> V4L2_ASYNC_MATCH_I2C,
> V4L2_ASYNC_MATCH_OF,
> + V4L2_ASYNC_MATCH_FWNODE,
> };
>
> /**
> @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> const struct device_node *node;
> } of;
> struct {
> + struct fwnode_handle *fwn;
I'd name this "node". The rationale is that code should be as independent as
possible of whether we use device_node or fwnode_handle. Naming both variable
"node" helps in that regard, and is in my opinion easier to read. This applies
to the other patches in the series too.
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + } fwnode;
> + struct {
> const char *name;
> } device_name;
> struct {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 0ab1c5d..5f1669c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -788,6 +788,8 @@ struct v4l2_subdev_platform_data {
> * @devnode: subdev device node
> * @dev: pointer to the physical device, if any
> * @of_node: The device_node of the subdev, usually the same as
> dev->of_node. + * @fwnode: The fwnode_handle of the subdev, usually the
> same as
> + * either dev->of_node->fwnode or dev->fwnode (whichever is non-
NULL).
> * @async_list: Links this subdev to a global subdev_list or
> @notifier->done * list.
> * @asd: Pointer to respective &struct v4l2_async_subdev.
> @@ -819,6 +821,7 @@ struct v4l2_subdev {
> struct video_device *devnode;
> struct device *dev;
> struct device_node *of_node;
> + struct fwnode_handle *fwnode;
> struct list_head async_list;
> struct v4l2_async_subdev *asd;
> struct v4l2_async_notifier *notifier;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 3/3] ARM: dts: ARTPEC-6: Add pinctrl configuration
From: Linus Walleij @ 2017-04-07 9:51 UTC (permalink / raw)
To: Jesper Nilsson
Cc: Jesper Nilsson, Lars Persson, Niklas Cassel, Rob Herring,
Mark Rutland, Russell King, linux-arm-kernel-VrBV9hrLPhE,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170330113349.GG29118-VrBV9hrLPhE@public.gmane.org>
On Thu, Mar 30, 2017 at 1:33 PM, Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org> wrote:
> Enable the pinctrl driver for ARTPEC-6 in the artpec6.dtsi
> with all main pinmux functions.
> Add pinctrl information to the relevant (uart) nodes.
>
> Signed-off-by: Jesper Nilsson <jesper.nilsson-VrBV9hrLPhE@public.gmane.org>
Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Please get this applied through the ARM SoC tree.
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 10/16] gpio: madera: Support Cirrus Logic Madera class codecs
From: Richard Fitzgerald @ 2017-04-07 9:54 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Rob Herring, Thomas Gleixner, Jason Cooper,
Lee Jones, Mark Brown,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
open list:WOLFSON MICROELECTRONICS DRIVERS,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CACRpkdatoJOg1U218Q-NteRdz6B+w_yr1PWvnfa1P1EgGm7zug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2017-04-07 at 11:11 +0200, Linus Walleij wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>
> > This adds support for the GPIOs on Cirrus Logic Madera class codecs.
>
> A bit terse commit message, could you elaborate a bit on their
> specifics?
>
Sure.
> > .../devicetree/bindings/gpio/gpio-madera.txt | 24 +++
>
> Again should probably be a separate patch. Again, I don't care much
> as long as the DT people are happy.
>
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
> > @@ -0,0 +1,24 @@
> > +Cirrus Logic Madera class audio codecs gpio driver
> > +
> > +This is a subnode of the parent mfd node.
> > +
> > +See also the core bindings for the parent MFD driver:
> > +See Documentation/devicetree/bindings/mfd/madera.txt
> > +
> > +Required properties:
> > + - compatible : must be "cirrus,madera-gpio"
> > + - gpio-controller : Indicates this device is a GPIO controller.
> > + - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
> > + is reserved for future use and must be zero
> > +
> > +Example:
> > +
> > +codec: cs47l85@0 {
> > + compatible = "cirrus,cs47l85";
> > +
> > + gpio {
> > + compatible = "cirrus,madera-gpio";
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + }
>
> Maybe you want to use the gpio-line-names = ; property in the example
> to show how nice it is to name the lines?
>
I'll take a look at that.
> > +config GPIO_MADERA
> > + tristate "Cirrus Logic Madera class codecs"
> > + depends on MFD_MADERA
> > + help
> > + Support for GPIOs on Cirrus Logic Madera class codecs.
>
> I wonder if you should not depend on the pin controller instead.
> It seems closer and also likely to act as a back-end for the
> GPIOs.
>
> > +static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
> > + struct madera *madera = madera_gpio->madera;
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(madera->regmap,
> > + MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (val & MADERA_GP1_LVL_MASK)
> > + return 1;
> > + else
> > + return 0;
>
> Just do this:
>
> return !!(val & MADERA_GP1_LVL_MASK);
>
Ok. Personally I like the clarity of the more verbose version rather
than the !! but I can change it.
> > +static struct gpio_chip template_chip = {
> > + .label = "madera",
> > + .owner = THIS_MODULE,
> > + .direction_input = madera_gpio_direction_in,
> > + .get = madera_gpio_get,
> > + .direction_output = madera_gpio_direction_out,
> > + .set = madera_gpio_set,
> > + .can_sleep = true,
> > +};
>
> - Implement .get_direction()
>
Ok
> Also consider implementing:
>
> - request/free/set_config looking like this:
>
> .request = gpiochip_generic_request,
> .free = gpiochip_generic_free,
> .set_config = gpiochip_generic_config,
>
> If you also implement the corresponding
> .pin_config_set in struct pinconf_ops and
> .gpio_request_enable() and .gpio_disable_free()
> in struct pinmux_ops, you get a pin control back-end
> that will mux in the pins to GPIO mode if they are wrong
> set, and also set up debounce and/or open drain for the
> GPIO line using the standard GPIO callbacks with pin
> control as a back-end.
>
> If you also specify "strict" in struct pinmux_ops you block
> the collisions between users of GPIO and other functions
> in the pin control driver.
>
> (Please go back and look at your pin control driver
> for this.)
>
I'll take a look at these things.
> Example driver using pin control as GPIO back-end:
> drivers/pinctrl/intel/pinctrl-intel.c
>
> Other than this it looks fine.
>
> 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 v4 0/3] Add Mediatek CIRQ interrupt controller
From: Youlin Pei @ 2017-04-07 9:54 UTC (permalink / raw)
To: Marc Zyngier
Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper,
Mark Rutland, Russell King, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
hongkun.cao-NuS5LvNUpcJWk0Htik3J/w,
yong.wu-NuS5LvNUpcJWk0Htik3J/w, erin.lo-NuS5LvNUpcJWk0Htik3J/w
In-Reply-To: <771954fe-c994-96be-76ab-ece75383d058-5wv7dgnIgG8@public.gmane.org>
On Fri, 2017-04-07 at 09:52 +0100, Marc Zyngier wrote:
> On 07/04/17 09:06, Youlin Pei wrote:
> > In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to
> > works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC.
> >
> > The CIRQ controller is integrated in between MCUSYS and interrupt sources
> > as the second level interrupt controller. The external interrupts which
> > outside MCUSYS will feed through CIRQ then bypass to GIC.
> >
> > In normal mode(where MCUSYS is active), CIRQ is disabled and interrupts
> > will directly issue to MCUSYS. When MCUSYS enters sleep mode, where GIC
> > is power downed. CIRQ will be enabled and monitor all edge trigger
> > interrupts(only edge trigger interrupts will be lost in this scenario).
> > When an edge interrupt is triggered, CIRQ will record the status and
> > generated a pulse signal to GIC when flush command is executed.
> >
> > With CIRQ, MCUSYS can be completely turned off to improve the system
> > power consumption without losing interrupts.
> >
> > change in v4:
> > 1. add some comment to explain CIRQ suspend callback.
> > 2. rebase on 4.11
>
> Hi Youlin,
>
> I'm happy to take the first two patches through the irq tree. How do we
> deal with the third one? It seems to me that it'd be better routed via
> armsoc.
>
> Let me know what you and Matthias want to do.
Hi Marc,
Thanks for your review. I think that driver and dtsi can merged
separately.
Hi Matthias,
Could you help to review the dtsi patch?
Thanks a lot!
>
> Thanks,
>
> M.
--
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 8/8] v4l: Remove V4L2 OF framework in favour of V4L2 fwnode framework
From: Laurent Pinchart @ 2017-04-07 9:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-9-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:10 Sakari Ailus wrote:
> All drivers have been converted from V4L2 OF to V4L2 fwnode. The V4L2 OF
> framework is now unused. Remove it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/Makefile | 3 -
> drivers/media/v4l2-core/v4l2-of.c | 327 -----------------------------------
> include/media/v4l2-of.h | 128 ---------------
> 3 files changed, 458 deletions(-)
> delete mode 100644 drivers/media/v4l2-core/v4l2-of.c
> delete mode 100644 include/media/v4l2-of.h
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 7/8] docs-rst: media: Switch documentation to V4L2 fwnode API
From: Laurent Pinchart @ 2017-04-07 9:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-8-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:09 Sakari Ailus wrote:
> Instead of including the V4L2 OF header in ReST documentation, use the
> V4L2 fwnode header instead.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> Documentation/media/kapi/v4l2-core.rst | 2 +-
> Documentation/media/kapi/v4l2-fwnode.rst | 3 +++
> Documentation/media/kapi/v4l2-of.rst | 3 ---
> 3 files changed, 4 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/media/kapi/v4l2-fwnode.rst
> delete mode 100644 Documentation/media/kapi/v4l2-of.rst
>
> diff --git a/Documentation/media/kapi/v4l2-core.rst
> b/Documentation/media/kapi/v4l2-core.rst index e967715..1bc8a14 100644
> --- a/Documentation/media/kapi/v4l2-core.rst
> +++ b/Documentation/media/kapi/v4l2-core.rst
> @@ -19,7 +19,7 @@ Video2Linux devices
> v4l2-mc
> v4l2-mediabus
> v4l2-mem2mem
> - v4l2-of
> + v4l2-fwnode
I wonder whether we should keep this alphabetically sorted.
Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> v4l2-rect
> v4l2-tuner
> v4l2-common
> diff --git a/Documentation/media/kapi/v4l2-fwnode.rst
> b/Documentation/media/kapi/v4l2-fwnode.rst new file mode 100644
> index 0000000..6c8bccd
> --- /dev/null
> +++ b/Documentation/media/kapi/v4l2-fwnode.rst
> @@ -0,0 +1,3 @@
> +V4L2 fwnode kAPI
> +^^^^^^^^^^^^^^^^
> +.. kernel-doc:: include/media/v4l2-fwnode.h
> diff --git a/Documentation/media/kapi/v4l2-of.rst
> b/Documentation/media/kapi/v4l2-of.rst deleted file mode 100644
> index 1ddf76b..0000000
> --- a/Documentation/media/kapi/v4l2-of.rst
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -V4L2 Open Firmware kAPI
> -^^^^^^^^^^^^^^^^^^^^^^^
> -.. kernel-doc:: include/media/v4l2-of.h
--
Regards,
Laurent Pinchart
--
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 6/8] v4l: media/drv-intf/soc_mediabus.h: include dependent header file
From: Laurent Pinchart @ 2017-04-07 10:01 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-7-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:08 Sakari Ailus wrote:
> media/drv-intf/soc_mediabus.h does depend on struct v4l2_mbus_config which
> is defined in media/v4l2-mediabus.h. Include it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Was this provided indirectly before, through v4l2-of.h perhaps ? If so,
shouldn't this patch be moved before 5/8 ? Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
> include/media/drv-intf/soc_mediabus.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/media/drv-intf/soc_mediabus.h
> b/include/media/drv-intf/soc_mediabus.h index 2ff7737..0449788 100644
> --- a/include/media/drv-intf/soc_mediabus.h
> +++ b/include/media/drv-intf/soc_mediabus.h
> @@ -14,6 +14,8 @@
> #include <linux/videodev2.h>
> #include <linux/v4l2-mediabus.h>
>
> +#include <media/v4l2-mediabus.h>
> +
> /**
> * enum soc_mbus_packing - data packing types on the media-bus
> * @SOC_MBUS_PACKING_NONE: no packing, bit-for-bit transfer to RAM, one
--
Regards,
Laurent Pinchart
--
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 3/8] v4l: async: Add fwnode match support
From: Laurent Pinchart @ 2017-04-07 10:04 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, linux-acpi, devicetree
In-Reply-To: <1491484330-12040-4-git-send-email-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
One more small comment below.
On Thursday 06 Apr 2017 16:12:05 Sakari Ailus wrote:
> Add fwnode matching to complement OF node matching. And fwnode may also be
> an OF node.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 12 ++++++++++++
> include/media/v4l2-async.h | 5 +++++
> include/media/v4l2-subdev.h | 3 +++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 96cc733..384ad5e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -46,6 +46,11 @@ static bool match_of(struct v4l2_subdev *sd, struct
> v4l2_async_subdev *asd) of_node_full_name(asd->match.of.node));
> }
>
> +static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> +{
> + return sd->fwnode == asd->match.fwnode.fwn;
> +}
> +
> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
> if (!asd->match.custom.match)
> @@ -80,6 +85,9 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct
> v4l2_async_notifier * case V4L2_ASYNC_MATCH_OF:
> match = match_of;
> break;
> + case V4L2_ASYNC_MATCH_FWNODE:
> + match = match_fwnode;
> + break;
> default:
> /* Cannot happen, unless someone breaks us */
> WARN_ON(true);
> @@ -158,6 +166,7 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev, case V4L2_ASYNC_MATCH_DEVNAME:
> case V4L2_ASYNC_MATCH_I2C:
> case V4L2_ASYNC_MATCH_OF:
> + case V4L2_ASYNC_MATCH_FWNODE:
> break;
> default:
> dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev :
NULL,
> @@ -282,6 +291,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> */
> if (!sd->of_node && sd->dev)
> sd->of_node = sd->dev->of_node;
> + if (!sd->fwnode && sd->dev)
> + sd->fwnode = sd->dev->of_node ?
> + &sd->dev->of_node->fwnode : sd->dev->fwnode;
>
> mutex_lock(&list_lock);
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8e2a236..8f552d2 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -32,6 +32,7 @@ struct v4l2_async_notifier;
> * @V4L2_ASYNC_MATCH_DEVNAME: Match will use the device name
> * @V4L2_ASYNC_MATCH_I2C: Match will check for I2C adapter ID and address
> * @V4L2_ASYNC_MATCH_OF: Match will use OF node
> + * @V4L2_ASYNC_MATCH_FWNODE: Match will use firmware node
> *
> * This enum is used by the asyncrhronous sub-device logic to define the
> * algorithm that will be used to match an asynchronous device.
> @@ -41,6 +42,7 @@ enum v4l2_async_match_type {
> V4L2_ASYNC_MATCH_DEVNAME,
> V4L2_ASYNC_MATCH_I2C,
> V4L2_ASYNC_MATCH_OF,
> + V4L2_ASYNC_MATCH_FWNODE,
> };
>
> /**
> @@ -58,6 +60,9 @@ struct v4l2_async_subdev {
> const struct device_node *node;
> } of;
> struct {
> + struct fwnode_handle *fwn;
Shouldn't this be const ?
> + } fwnode;
> + struct {
> const char *name;
> } device_name;
> struct {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 0ab1c5d..5f1669c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -788,6 +788,8 @@ struct v4l2_subdev_platform_data {
> * @devnode: subdev device node
> * @dev: pointer to the physical device, if any
> * @of_node: The device_node of the subdev, usually the same as
> dev->of_node.
> + * @fwnode: The fwnode_handle of the subdev, usually the same as
> + * either dev->of_node->fwnode or dev->fwnode (whichever is non-
NULL).
> * @async_list: Links this subdev to a global subdev_list or
> @notifier->done * list.
> * @asd: Pointer to respective &struct v4l2_async_subdev.
> @@ -819,6 +821,7 @@ struct v4l2_subdev {
> struct video_device *devnode;
> struct device *dev;
> struct device_node *of_node;
> + struct fwnode_handle *fwnode;
> struct list_head async_list;
> struct v4l2_async_subdev *asd;
> struct v4l2_async_notifier *notifier;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
From: Linus Walleij @ 2017-04-07 10:07 UTC (permalink / raw)
To: Han, Nandor (GE Healthcare)
Cc: Alexandre Courbot, Rob Herring, Mark Rutland,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Malinen, Semi (GE Healthcare)
In-Reply-To: <HE1P101MB01883B516828F26921267A4FE60A0@HE1P101MB0188.NAMP101.PROD.OUTLOOK.COM>
On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
<nandor.han@ge.com> wrote:
> [Me]
>> > + /* 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.
>
> Can you please be more explicit here.
>
> Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> I don't see how this could be done in remove() :)
If you issue gpiod_put() you release the GPIO hande so something else
can go in and grab the GPIO and assert the reset.
This is not what you want to make possible: you want to hold this gpiod handle
as long as the driver is running. devm_gpiod_get_optional() will do the
trick if you don't want to put the handle under explicit control.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 4/8] v4l: async: Provide interoperability between OF and fwnode matching
From: Laurent Pinchart @ 2017-04-07 10:07 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491484330-12040-5-git-send-email-sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Hi Sakari,
Thank you for the patch.
On Thursday 06 Apr 2017 16:12:06 Sakari Ailus wrote:
> OF and fwnode support are separated in V4L2 and individual drivers may
> implement one of them. Sub-devices do not match with a notifier
> expecting sub-devices with fwnodes, nor the other way around.
Shouldn't we instead convert all drivers to fwnode matching ? What's missing
after the mass conversion in patch 5/8 ?
> Fix this by checking for sub-device's of_node field in fwnode match and
> fwnode field in OF match.
>
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 26 +++++++++++++++++++++++---
> include/media/v4l2-async.h | 2 +-
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 384ad5e..7f5d804 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -14,6 +14,7 @@
> #include <linux/list.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -40,15 +41,34 @@ static bool match_devname(struct v4l2_subdev *sd,
> return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
> }
>
> +static bool fwnode_cmp(struct fwnode_handle *one,
> + struct fwnode_handle *theother)
> +{
> + if (!one || !theother)
> + return false;
> +
> + if (one->type != theother->type)
> + return false;
> +
> + if (is_of_node(one))
> + return !of_node_cmp(of_node_full_name(to_of_node(one)),
> + of_node_full_name(to_of_node(theother)));
> + else
> + return one == theother;
> +}
> +
> static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> {
> - return !of_node_cmp(of_node_full_name(sd->of_node),
> - of_node_full_name(asd->match.of.node));
> + return fwnode_cmp(sd->of_node ?
> + of_fwnode_handle(sd->of_node) : sd->fwnode,
> + of_fwnode_handle(asd->match.of.node));
> }
>
> static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> {
> - return sd->fwnode == asd->match.fwnode.fwn;
> + return fwnode_cmp(sd->of_node ?
> + of_fwnode_handle(sd->of_node) : sd->fwnode,
> + asd->match.fwnode.fwn);
> }
>
> static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8f552d2..df8b682 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -57,7 +57,7 @@ struct v4l2_async_subdev {
> enum v4l2_async_match_type match_type;
> union {
> struct {
> - const struct device_node *node;
> + struct device_node *node;
That seems to be a bit of a hack :-( I'd rather make everything const and cast
to non-const pointers explicitly where the API requires us to. Or, better, add
a to_of_node_const() function.
> } of;
> struct {
> struct fwnode_handle *fwn;
--
Regards,
Laurent Pinchart
--
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: [RFC] Documentation: pinctrl: Add "pinmux" property
From: Linus Walleij @ 2017-04-07 10:11 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux-Renesas,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1491474955-25605-1-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
On Thu, Apr 6, 2017 at 12:35 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
> Document "pinmux" property as part of generic pin controller
> documentation.
> Fix 2 minor typos in documentation while at there.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
Patch applied. This is documenting what is already the case
and is as such very helpful.
> Sending this one as RFC to collect feedback.
I just applied it. It is already much better than what we have.
If more adjustments are needed, people can send additional patches.
> If the change is not rejected is it
> worth adding to pin controller core helper functions to parse the newly
> documented property, as this commit
> <https://patchwork.kernel.org/patch/9411231/>
> did for "pinctrl-pin-array" one?
>
> All drivers using "pinmux" exhibit the same behavior which is fine as long as
> "pinmux" only accepts a list of u32 parameters.
>
> ...
> pins = of_find_property(node, "pinmux", NULL);
> ...
> npins = pins->length / sizeof(u32);
> ...
> of_property_read_u32_index(node, "pinmux",
> i, &pinfunc);
> ...
It makes sense. Just make sure to move users over to use the helpers.
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 1/1] gpio: core: Decouple open drain/source flag with active low/high
From: Laxman Dewangan @ 2017-04-07 10:14 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Rob Herring, Mark Rutland, Frank Rowand,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CACRpkdaK1ut-1=NtqqL15QuQV+P2n8XidNqVspOfu+7x=XMvog-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Friday 07 April 2017 03:55 PM, Linus Walleij wrote:
> On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Currently, the GPIO interface is said to Open Drain if it is Single
>> Ended and active LOW. Similarly, it is said as Open Source if it is
>> Single Ended and active HIGH.
>>
>> The active HIGH/LOW is used in the interface for setting the pin
>> state to HIGH or LOW when enabling/disabling the interface.
>>
>> In Open Drain interface, pin is set to HIGH by putting pin in
>> high impedance and LOW by driving to the LOW.
>>
>> In Open Source interface, pin is set to HIGH by driving pin to
>> HIGH and set to LOW by putting pin in high impedance.
>>
>> With above, the Open Drain/Source is unrelated to the active LOW/HIGH
>> in interface. There is interface where the enable/disable of interface
>> is ether active LOW or HIGH but it is Open Drain type.
>>
>> Hence decouple the Open Drain with Single Ended + Active LOW and
>> Open Source with Single Ended + Active HIGH.
>>
>> Adding different flag for the Open Drain/Open Source which is valid
>> only when Single ended flag is enabled.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Patch applied.
>
> Good that you found this and fixed it before someone git hurt.
Thanks you very much.
Now we are going to use the descriptor based GPIO APIs as this is very
powerful and passing this info from DT is very useful.
--
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 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume
From: Linus Walleij @ 2017-04-07 10:19 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Thierry Reding, Jon Hunter, Rob Herring, Mark Rutland,
linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <58E6405E.5020402-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Thu, Apr 6, 2017 at 3:19 PM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On Thursday 06 April 2017 06:33 PM, Thierry Reding wrote:
>> On Thu, Apr 06, 2017 at 09:57:09AM +0100, Jon Hunter wrote:
>>> On 05/04/17 15:13, Laxman Dewangan wrote:
>>>>
>>>> +state of the system. The configuration of pin is provided via the
>>>> pinctrl
>>>> +DT node as detailed in the pinctrl DT binding document
>>>> + Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>>>> +
>>>> +The PWM node will have following optional properties.
>>>> +pinctrl-names: Pin state names. Must be "suspend" and "resume".
>>>
>>> Why not just use the pre-defined names here? There is a pre-defined name
>>> for "default", "idle" and "sleep" and then you can use the following
>>> APIs and avoid the lookup of the state ...
>>>
>>> pinctrl_pm_select_default_state()
>>> pinctrl_pm_select_idle_state()
>>> pinctrl_pm_select_sleep_state()
>>>
>>> Note for i2c [0][1], I used "default" as the active/on state (which I
>>> know is not that descriptive) and then used 'idle' as the suspended
>>> state. This way we don't need any custom names.
>>
>> Agreed, I think that's how these states are meant to be used.
>
> I did quick grep for the pinctrl_pm_select_* functions in the code tree and
> found usage of these APIs in some of the places.
> I am taking the reference of i2c-st, i2c-nomadic and
> extcon/extcon-usb-gpio.c drivers and from this the interpretation is
>
> default state: When interface active and transfer need to be done in IO
> interface.
> idle state: Active state of the system but interface is not active, put in
> non-active state of the interface.
> sleep state: When system entering into suspend and IO interface is going to
> be inactive.
>
> So in PWM case, we will need the "default" and "sleep" state.
>
> In suspend(), set the "sleep" state and in resume, set the "default" state.
>
> + Linus W as I refereed his st/nomadik driver for reference.
It is actually documented:
include/linux/pinctrl/pinctrl-state.h
/*
* Standard pin control state definitions
*/
/**
* @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put
* into as default, usually this means the pins are up and ready to
* be used by the device driver. This state is commonly used by
* hogs to configure muxing and pins at boot, and also as a state
* to go into when returning from sleep and idle in
* .pm_runtime_resume() or ordinary .resume() for example.
* @PINCTRL_STATE_INIT: normally the pinctrl will be set to "default"
* before the driver's probe() function is called. There are some
* drivers where that is not appropriate becausing doing so would
* glitch the pins. In those cases you can add an "init" pinctrl
* which is the state of the pins before drive probe. After probe
* if the pins are still in "init" state they'll be moved to
* "default".
* @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
* when the pins are idle. This is a state where the system is relaxed
* but not fully sleeping - some power may be on but clocks gated for
* example. Could typically be set from a pm_runtime_suspend() or
* pm_runtime_idle() operation.
* @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into
* when the pins are sleeping. This is a state where the system is in
* its lowest sleep state. Could typically be set from an
* ordinary .suspend() function.
*/
#define PINCTRL_STATE_DEFAULT "default"
#define PINCTRL_STATE_INIT "init"
#define PINCTRL_STATE_IDLE "idle"
#define PINCTRL_STATE_SLEEP "sleep"
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 v2 1/8] v4l: flash led class: Use fwnode_handle instead of device_node in init
From: Sakari Ailus @ 2017-04-07 10:20 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3129720.GhXEqohgpp@avalon>
Hi Laurent,
Thank you for the review!
On Fri, Apr 07, 2017 at 11:49:56AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thursday 06 Apr 2017 16:12:03 Sakari Ailus wrote:
> > Pass the more generic fwnode_handle to the init function than the
> > device_node.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> > drivers/leds/leds-aat1290.c | 5 +++--
> > drivers/leds/leds-max77693.c | 5 +++--
> > drivers/media/v4l2-core/v4l2-flash-led-class.c | 11 ++++++-----
> > include/media/v4l2-flash-led-class.h | 4 ++--
> > 4 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> > index def3cf9..a21e192 100644
> > --- a/drivers/leds/leds-aat1290.c
> > +++ b/drivers/leds/leds-aat1290.c
> > @@ -503,8 +503,9 @@ static int aat1290_led_probe(struct platform_device
> > *pdev) aat1290_init_v4l2_flash_config(led, &led_cfg, &v4l2_sd_cfg);
> >
> > /* Create V4L2 Flash subdev. */
> > - led->v4l2_flash = v4l2_flash_init(dev, sub_node, fled_cdev, NULL,
> > - &v4l2_flash_ops, &v4l2_sd_cfg);
> > + led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> > + fled_cdev, NULL, &v4l2_flash_ops,
> > + &v4l2_sd_cfg);
> > if (IS_ERR(led->v4l2_flash)) {
> > ret = PTR_ERR(led->v4l2_flash);
> > goto error_v4l2_flash_init;
> > diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
> > index 1eb58ef..2d3062d 100644
> > --- a/drivers/leds/leds-max77693.c
> > +++ b/drivers/leds/leds-max77693.c
> > @@ -930,8 +930,9 @@ static int max77693_register_led(struct max77693_sub_led
> > *sub_led, max77693_init_v4l2_flash_config(sub_led, led_cfg, &v4l2_sd_cfg);
> >
> > /* Register in the V4L2 subsystem. */
> > - sub_led->v4l2_flash = v4l2_flash_init(dev, sub_node, fled_cdev, NULL,
> > - &v4l2_flash_ops, &v4l2_sd_cfg);
> > + sub_led->v4l2_flash = v4l2_flash_init(dev, of_fwnode_handle(sub_node),
> > + fled_cdev, NULL,
> &v4l2_flash_ops,
> > + &v4l2_sd_cfg);
> > if (IS_ERR(sub_led->v4l2_flash)) {
> > ret = PTR_ERR(sub_led->v4l2_flash);
> > goto err_v4l2_flash_init;
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > b/drivers/media/v4l2-core/v4l2-flash-led-class.c index 794e563..f430c89
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/of.h>
>
> I think you can drop linux/of.h.
Correct. Will fix.
>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/types.h>
> > #include <media/v4l2-flash-led-class.h>
> > @@ -612,7 +613,7 @@ static const struct v4l2_subdev_internal_ops
> > v4l2_flash_subdev_internal_ops = { static const struct v4l2_subdev_ops
> > v4l2_flash_subdev_ops;
> >
> > struct v4l2_flash *v4l2_flash_init(
> > - struct device *dev, struct device_node *of_node,
> > + struct device *dev, struct fwnode_handle *fwn,
> > struct led_classdev_flash *fled_cdev,
> > struct led_classdev_flash *iled_cdev,
> > const struct v4l2_flash_ops *ops,
> > @@ -638,7 +639,7 @@ struct v4l2_flash *v4l2_flash_init(
> > v4l2_flash->iled_cdev = iled_cdev;
> > v4l2_flash->ops = ops;
> > sd->dev = dev;
> > - sd->of_node = of_node ? of_node : led_cdev->dev->of_node;
> > + sd->fwnode = fwn ? fwn : dev_fwnode(led_cdev->dev);
>
> v4l2_subdev will only have a fwnode field in patch 3/8.
I think I've rearranged the set after writing the patch and missed the
dependency. I'll move it third unless there are other dependencies.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@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 V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume
From: Jon Hunter @ 2017-04-07 10:25 UTC (permalink / raw)
To: Laxman Dewangan, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491557642-15940-4-git-send-email-ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 07/04/17 10:34, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
>
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
>
> Add DT binding details to provide the pin configuration state
> from PWM and pinctrl DT node in suspend and active state of
> the system.
>
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
>
> Changes from V2:
> - Fix the commit message and details
> ---
> .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> index b4e7377..c57e11b 100644
> --- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> @@ -19,6 +19,19 @@ Required properties:
> - reset-names: Must include the following entries:
> - pwm
>
> +Optional properties:
> +============================
> +In some of the interface like PWM based regulator device, it is required
> +to configure the pins differently in different states, especially in suspend
> +state of the system. The configuration of pin is provided via the pinctrl
> +DT node as detailed in the pinctrl DT binding document
> + Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +The PWM node will have following optional properties.
> +pinctrl-names: Pin state names. Must be "default" and "sleep".
> +pinctrl-0: phandle for the default/active state of pin configurations.
> +pinctrl-1: phandle for the sleep state of pin configurations.
> +
> Example:
>
> pwm: pwm@7000a000 {
> @@ -29,3 +42,35 @@ Example:
> resets = <&tegra_car 17>;
> reset-names = "pwm";
> };
> +
> +
> +Example with the pin configuration for suspend and resume:
> +=========================================================
> +Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
> +it requires PWM output to be tristated when system enters suspend.
> +Following will be DT binding to achieve this:
> +
> +#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> + pinmux@700008d4 {
> + pwm_active_state: pwm_active_state {
> + pe7 {
> + nvidia,pins = "pe7";
> + nvidia,tristate = <TEGRA_PIN_DISABLE>;
> + };
> + };
> +
> + pwm_sleep_state: pwm_sleep_state {
> + pe7 {
> + nvidia,pins = "pe7";
> + nvidia,tristate = <TEGRA_PIN_ENABLE>;
> + };
> + };
> + };
> +
> + pwm@7000a000 {
> + /* Mandatory PWM properties */
Maybe these are mandatory for the platform, but given these are
optional, its a bit confusing.
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&pwm_active_state>;
> + pinctrl-1 = <&pwm_sleep_state>;
> + };
However, fine with me so ...
Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cheers
Jon
--
nvpublic
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox