[PATCH 0/4] PWM changes for rk3288-evb

[PATCH 0/4] PWM changes for rk3288-evb

[Doug Anderson]

These patches enable the pwm backlight for the rk3288-evb board.
There were tested by watching the backlight grow from off to max with
the following instructions:

  cd /sys/class/backlight/backlight*/
  for i in $(seq 255); do echo $i > brightness; sleep .01; done

The first patch enables the proper IP.  I think it could land in
Heiko's tree.

The second patch switches PWM cells from 2 to 3 on rk3288.  I think it
could land in Thierry's tree.  With the PWM subsystem as currently
structured, I believe this will break backward compatibility.  However
the rk3288 PWM driver was added so recently (and the rk3288 is so new)
that this seems OK.  In the worst case if someone yells that they are
broken, I believe we could fix this in Linux by saying that if a PWM
driver specifies 3 cells and the DTS definition of the PWM says 2
cells that we'll just pretend that we have no flags.

The 3rd and 4th patches are DTS ones.  They could land in Heiko's tree
after the second patch has landed.  They are based atop his current
WIP 3.18 dts tree.  Note that instantiating the PWM backlight will
cause the system to hang unless Heiko's (clk: rockchip: protect
critical clocks from getting disabled)
<patchwork.kernel.org/patch/4725391> is landed.

There are no compile time or runtime dependencies between these
patches except that patch #3 needs to come before patch #4.  ...and of
course the PWM won't work without all 4 patches.


Doug Anderson (4):
  ARM: rockchip: rk3288: Switch to use the proper PWM IP
  pwm: rockchip: Allow polarity invert on rk3288
  ARM: dts: Add main PWM info to rk3288
  ARM: dts: Enable pwm backlight on rk3288-EVB

 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  4 +-
 arch/arm/boot/dts/rk3288-evb.dtsi                  | 53 +++++++++++++++++
 arch/arm/boot/dts/rk3288.dtsi                      | 68 ++++++++++++++++++++++
 arch/arm/mach-rockchip/rockchip.c                  | 19 ++++++
 drivers/pwm/pwm-rockchip.c                         | 32 +++++++++-
 5 files changed, 172 insertions(+), 4 deletions(-)

-- 
2.1.0.rc2.206.gedb03e5

[PATCH 2/4] pwm: rockchip: Allow polarity invert on rk3288

[Doug Anderson]

The rk3288 has the ability to invert the polarity of the PWM.  Let's
enable that ability.

To do this we increase the number of pwm_cells to 3 to allow using the
PWM_POLARITY_INVERTED flag.  Since the PWM driver on rk3288 is very
new, I thought this was OK.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  4 +--
 drivers/pwm/pwm-rockchip.c                         | 32 ++++++++++++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index d47d15a..b8be3d0 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -7,8 +7,8 @@ Required properties:
    "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
  - reg: physical base address and length of the controller's registers
  - clocks: phandle and clock specifier of the PWM reference clock
- - #pwm-cells: should be 2. See pwm.txt in this directory for a
-   description of the cell format.
+ - #pwm-cells: must be 2 (rk2928) or 3 (rk3288). See pwm.txt in this directory
+   for a description of the cell format.
 
 Example:
 
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index bdd8644..27f20d6 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -24,7 +24,9 @@
 #define PWM_ENABLE		(1 << 0)
 #define PWM_CONTINUOUS		(1 << 1)
 #define PWM_DUTY_POSITIVE	(1 << 3)
+#define PWM_DUTY_NEGATIVE	(0 << 3)
 #define PWM_INACTIVE_NEGATIVE	(0 << 4)
+#define PWM_INACTIVE_POSITIVE	(1 << 4)
 #define PWM_OUTPUT_LEFT		(0 << 5)
 #define PWM_LP_DISABLE		(0 << 8)
 
@@ -32,6 +34,7 @@ struct rockchip_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
 	const struct rockchip_pwm_data *data;
+	enum pwm_polarity polarity;
 	void __iomem *base;
 };
 
@@ -45,6 +48,7 @@ struct rockchip_pwm_regs {
 struct rockchip_pwm_data {
 	struct rockchip_pwm_regs regs;
 	unsigned int prescaler;
+	bool has_invert;
 
 	void (*set_enable)(struct pwm_chip *chip, bool enable);
 };
@@ -74,10 +78,14 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
-			  PWM_CONTINUOUS | PWM_DUTY_POSITIVE |
-			  PWM_INACTIVE_NEGATIVE;
+			  PWM_CONTINUOUS;
 	u32 val;
 
+	if (pc->polarity == PWM_POLARITY_INVERSED)
+		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+	else
+		enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
+
 	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
 
 	if (enable)
@@ -124,6 +132,19 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+			      enum pwm_polarity polarity)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+
+	if (!pc->data->has_invert)
+		return -ENOSYS;
+
+	pc->polarity = polarity;
+
+	return 0;
+}
+
 static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
@@ -149,6 +170,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static const struct pwm_ops rockchip_pwm_ops = {
 	.config = rockchip_pwm_config,
+	.set_polarity = rockchip_pwm_set_polarity,
 	.enable = rockchip_pwm_enable,
 	.disable = rockchip_pwm_disable,
 	.owner = THIS_MODULE,
@@ -173,6 +195,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
 		.ctrl = 0x0c,
 	},
 	.prescaler = 1,
+	.has_invert = 1,
 	.set_enable = rockchip_pwm_set_enable_v2,
 };
 
@@ -184,6 +207,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
 		.ctrl = 0x00,
 	},
 	.prescaler = 1,
+	.has_invert = 1,
 	.set_enable = rockchip_pwm_set_enable_v2,
 };
 
@@ -228,6 +252,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	pc->data = id->data;
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &rockchip_pwm_ops;
+	if (pc->data->has_invert) {
+		pc->chip.of_xlate = of_pwm_xlate_with_flags;
+		pc->chip.of_pwm_n_cells = 3;
+	}
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 3/4] ARM: dts: Add main PWM info to rk3288

[Doug Anderson]

This adds the PWM info (other than the VOP PWM) to the main rk3288
dtsi file.

Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 arch/arm/boot/dts/rk3288.dtsi | 68 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 36be7bb..9c9d9c5 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -261,6 +261,50 @@
 		status = "disabled";
 	};
 
+	pwm0: pwm@ff680000 {
+		compatible = "rockchip,rk3288-pwm";
+		reg = <0xff680000 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm0_pin>;
+		clocks = <&cru PCLK_PWM>;
+		clock-names = "pwm";
+		status = "disabled";
+	};
+
+	pwm1: pwm@ff680010 {
+		compatible = "rockchip,rk3288-pwm";
+		reg = <0xff680010 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm1_pin>;
+		clocks = <&cru PCLK_PWM>;
+		clock-names = "pwm";
+		status = "disabled";
+	};
+
+	pwm2: pwm@ff680020 {
+		compatible = "rockchip,rk3288-pwm";
+		reg = <0xff680020 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm2_pin>;
+		clocks = <&cru PCLK_PWM>;
+		clock-names = "pwm";
+		status = "disabled";
+	};
+
+	pwm3: pwm@ff680030 {
+		compatible = "rockchip,rk3288-pwm";
+		reg = <0xff680030 0x10>;
+		#pwm-cells = <2>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm3_pin>;
+		clocks = <&cru PCLK_PWM>;
+		clock-names = "pwm";
+		status = "disabled";
+	};
+
 	pmu: power-management@ff730000 {
 		compatible = "rockchip,rk3288-pmu", "syscon";
 		reg = <0xff730000 0x100>;
@@ -611,5 +655,29 @@
 				rockchip,pins = <5 15 3 &pcfg_pull_none>;
 			};
 		};
+
+		pwm0 {
+			pwm0_pin: pwm0-pin {
+				rockchip,pins = <7 0 RK_FUNC_1 &pcfg_pull_none>;
+			};
+		};
+
+		pwm1 {
+			pwm1_pin: pwm1-pin {
+				rockchip,pins = <7 1 RK_FUNC_1 &pcfg_pull_none>;
+			};
+		};
+
+		pwm2 {
+			pwm2_pin: pwm2-pin {
+				rockchip,pins = <7 22 RK_FUNC_3 &pcfg_pull_none>;
+			};
+		};
+
+		pwm3 {
+			pwm3_pin: pwm3-pin {
+				rockchip,pins = <7 23 RK_FUNC_3 &pcfg_pull_none>;
+			};
+		};
 	};
 };
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 4/4] ARM: dts: Enable pwm backlight on rk3288-EVB

[Doug Anderson]

The pwm0 is the PWM associated with the LCD backlight.  Enable it.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 2964370..242e1a9 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -10,6 +10,7 @@
  * GNU General Public License for more details.
  */
 
+#include <dt-bindings/pwm/pwm.h>
 #include "rk3288.dtsi"
 
 / {
@@ -17,6 +18,48 @@
 		reg = <0x0 0x80000000>;
 	};
 
+        backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <
+			  0   1   2   3   4   5   6   7
+			  8   9  10  11  12  13  14  15
+			 16  17  18  19  20  21  22  23
+			 24  25  26  27  28  29  30  31
+			 32  33  34  35  36  37  38  39
+			 40  41  42  43  44  45  46  47
+			 48  49  50  51  52  53  54  55
+			 56  57  58  59  60  61  62  63
+			 64  65  66  67  68  69  70  71
+			 72  73  74  75  76  77  78  79
+			 80  81  82  83  84  85  86  87
+			 88  89  90  91  92  93  94  95
+			 96  97  98  99 100 101 102 103
+			104 105 106 107 108 109 110 111
+			112 113 114 115 116 117 118 119
+			120 121 122 123 124 125 126 127
+			128 129 130 131 132 133 134 135
+			136 137 138 139 140 141 142 143
+			144 145 146 147 148 149 150 151
+			152 153 154 155 156 157 158 159
+			160 161 162 163 164 165 166 167
+			168 169 170 171 172 173 174 175
+			176 177 178 179 180 181 182 183
+			184 185 186 187 188 189 190 191
+			192 193 194 195 196 197 198 199
+			200 201 202 203 204 205 206 207
+			208 209 210 211 212 213 214 215
+			216 217 218 219 220 221 222 223
+			224 225 226 227 228 229 230 231
+			232 233 234 235 236 237 238 239
+			240 241 242 243 244 245 246 247
+			248 249 250 251 252 253 254 255>;
+		default-brightness-level = <128>;
+		enable-gpios = <&gpio7 2 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&bl_en>;
+		pwms = <&pwm0 0 1000000 PWM_POLARITY_INVERTED>;
+        };
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		#address-cells = <1>;
@@ -81,6 +124,10 @@
 	status = "okay";
 };
 
+&pwm0 {
+	status = "okay";
+};
+
 &uart0 {
 	status = "okay";
 };
@@ -102,6 +149,12 @@
 };
 
 &pinctrl {
+	backlight {
+		bl_en: bl-en {
+			rockchip,pins = <7 2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
 	buttons {
 		pwrbtn: pwrbtn {
 			rockchip,pins = <0 5 RK_FUNC_GPIO &pcfg_pull_up>;
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH 2/4] pwm: rockchip: Allow polarity invert on rk3288

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

On Mon, Aug 18, 2014 at 10:09:07AM -0700, Doug Anderson wrote:
[...]
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
>  #define PWM_LP_DISABLE		(0 << 8)
>  
> @@ -32,6 +34,7 @@ struct rockchip_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	const struct rockchip_pwm_data *data;
> +	enum pwm_polarity polarity;

Why do you need this field? struct pwm_device already has a copy of it.

> @@ -74,10 +78,14 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>  {
>  	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>  	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> -			  PWM_CONTINUOUS | PWM_DUTY_POSITIVE |
> -			  PWM_INACTIVE_NEGATIVE;
> +			  PWM_CONTINUOUS;
>  	u32 val;
>  
> +	if (pc->polarity == PWM_POLARITY_INVERSED)
> +		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> +	else
> +		enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;

I have a feeling you're going to answer the above question with: "Because
it's needed here". If so, my reply would be: "Then this function should
take a struct pwm_device instead of struct pwm_chip."

> @@ -173,6 +195,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>  		.ctrl = 0x0c,
>  	},
>  	.prescaler = 1,
> +	.has_invert = 1,

Since has_invert is a boolean, the proper value here would be "true".

> @@ -228,6 +252,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	pc->data = id->data;
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &rockchip_pwm_ops;
> +	if (pc->data->has_invert) {
> +		pc->chip.of_xlate = of_pwm_xlate_with_flags;
> +		pc->chip.of_pwm_n_cells = 3;
> +	}
>  	pc->chip.base = -1;
>  	pc->chip.npwm = 1;

I suggest to rewrite the above as follows for readability:

 	pc->data = id->data;
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &rockchip_pwm_ops;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
+
+	if (pc->data->has_invert) {
+		pc->chip.of_xlate = of_pwm_xlate_with_flags;
+		pc->chip.of_pwm_n_cells = 3;
+	}

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] ARM: dts: Enable pwm backlight on rk3288-EVB

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]

On Mon, Aug 18, 2014 at 10:09:09AM -0700, Doug Anderson wrote:
> The pwm0 is the PWM associated with the LCD backlight.  Enable it.

In the subject, s/pwm/PWM/.

> 
> Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  arch/arm/boot/dts/rk3288-evb.dtsi | 53 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
> index 2964370..242e1a9 100644
> --- a/arch/arm/boot/dts/rk3288-evb.dtsi
> +++ b/arch/arm/boot/dts/rk3288-evb.dtsi
> @@ -10,6 +10,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <dt-bindings/pwm/pwm.h>
>  #include "rk3288.dtsi"
>  
>  / {
> @@ -17,6 +18,48 @@
>  		reg = <0x0 0x80000000>;
>  	};
>  
> +        backlight {

Indentation seems to be off here.

> +		compatible = "pwm-backlight";
> +		brightness-levels = <
> +			  0   1   2   3   4   5   6   7
> +			  8   9  10  11  12  13  14  15
> +			 16  17  18  19  20  21  22  23
> +			 24  25  26  27  28  29  30  31
> +			 32  33  34  35  36  37  38  39
> +			 40  41  42  43  44  45  46  47
> +			 48  49  50  51  52  53  54  55
> +			 56  57  58  59  60  61  62  63
> +			 64  65  66  67  68  69  70  71
> +			 72  73  74  75  76  77  78  79
> +			 80  81  82  83  84  85  86  87
> +			 88  89  90  91  92  93  94  95
> +			 96  97  98  99 100 101 102 103
> +			104 105 106 107 108 109 110 111
> +			112 113 114 115 116 117 118 119
> +			120 121 122 123 124 125 126 127
> +			128 129 130 131 132 133 134 135
> +			136 137 138 139 140 141 142 143
> +			144 145 146 147 148 149 150 151
> +			152 153 154 155 156 157 158 159
> +			160 161 162 163 164 165 166 167
> +			168 169 170 171 172 173 174 175
> +			176 177 178 179 180 181 182 183
> +			184 185 186 187 188 189 190 191
> +			192 193 194 195 196 197 198 199
> +			200 201 202 203 204 205 206 207
> +			208 209 210 211 212 213 214 215
> +			216 217 218 219 220 221 222 223
> +			224 225 226 227 228 229 230 231
> +			232 233 234 235 236 237 238 239
> +			240 241 242 243 244 245 246 247
> +			248 249 250 251 252 253 254 255>;
> +		default-brightness-level = <128>;
> +		enable-gpios = <&gpio7 2 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bl_en>;
> +		pwms = <&pwm0 0 1000000 PWM_POLARITY_INVERTED>;
> +        };

And here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/4] pwm: rockchip: Allow polarity invert on rk3288

[Doug Anderson]

Thierry

On Tue, Aug 19, 2014 at 12:18 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 10:09:07AM -0700, Doug Anderson wrote:
> [...]
>> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
>>  #define PWM_LP_DISABLE               (0 << 8)
>>
>> @@ -32,6 +34,7 @@ struct rockchip_pwm_chip {
>>       struct pwm_chip chip;
>>       struct clk *clk;
>>       const struct rockchip_pwm_data *data;
>> +     enum pwm_polarity polarity;
>
> Why do you need this field? struct pwm_device already has a copy of it.

OK, good point.


>> @@ -74,10 +78,14 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>>  {
>>       struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>>       u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
>> -                       PWM_CONTINUOUS | PWM_DUTY_POSITIVE |
>> -                       PWM_INACTIVE_NEGATIVE;
>> +                       PWM_CONTINUOUS;
>>       u32 val;
>>
>> +     if (pc->polarity == PWM_POLARITY_INVERSED)
>> +             enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
>> +     else
>> +             enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
>
> I have a feeling you're going to answer the above question with: "Because
> it's needed here". If so, my reply would be: "Then this function should
> take a struct pwm_device instead of struct pwm_chip."

OK.  I've chosen to have it take a pwm_device AND a pwm_chip.  It is a
little redundant because a pwm_device has a pointer to its pwm_chip,
but it follows the lead of all of the callbacks in "struct pwm_ops".
If you'd like me to spin it to take only a pwm_device I'm happy to.


>
>> @@ -173,6 +195,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>>               .ctrl = 0x0c,
>>       },
>>       .prescaler = 1,
>> +     .has_invert = 1,
>
> Since has_invert is a boolean, the proper value here would be "true".

Done.


>> @@ -228,6 +252,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>>       pc->data = id->data;
>>       pc->chip.dev = &pdev->dev;
>>       pc->chip.ops = &rockchip_pwm_ops;
>> +     if (pc->data->has_invert) {
>> +             pc->chip.of_xlate = of_pwm_xlate_with_flags;
>> +             pc->chip.of_pwm_n_cells = 3;
>> +     }
>>       pc->chip.base = -1;
>>       pc->chip.npwm = 1;
>
> I suggest to rewrite the above as follows for readability:
>
>         pc->data = id->data;
>         pc->chip.dev = &pdev->dev;
>         pc->chip.ops = &rockchip_pwm_ops;
>         pc->chip.base = -1;
>         pc->chip.npwm = 1;

Done.


> +       if (pc->data->has_invert) {
> +               pc->chip.of_xlate = of_pwm_xlate_with_flags;
> +               pc->chip.of_pwm_n_cells = 3;
> +       }
>
> Thierry

Re: [PATCH 4/4] ARM: dts: Enable pwm backlight on rk3288-EVB

[Doug Anderson]

Thierry,

On Tue, Aug 19, 2014 at 12:22 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 10:09:09AM -0700, Doug Anderson wrote:
>> The pwm0 is the PWM associated with the LCD backlight.  Enable it.
>
> In the subject, s/pwm/PWM/.

Done.


>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  arch/arm/boot/dts/rk3288-evb.dtsi | 53 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
>> index 2964370..242e1a9 100644
>> --- a/arch/arm/boot/dts/rk3288-evb.dtsi
>> +++ b/arch/arm/boot/dts/rk3288-evb.dtsi
>> @@ -10,6 +10,7 @@
>>   * GNU General Public License for more details.
>>   */
>>
>> +#include <dt-bindings/pwm/pwm.h>
>>  #include "rk3288.dtsi"
>>
>>  / {
>> @@ -17,6 +18,48 @@
>>               reg = <0x0 0x80000000>;
>>       };
>>
>> +        backlight {
>
> Indentation seems to be off here.

Good catch.  Done.


>> +             compatible = "pwm-backlight";
>> +             brightness-levels = <
>> +                       0   1   2   3   4   5   6   7
>> +                       8   9  10  11  12  13  14  15
>> +                      16  17  18  19  20  21  22  23
>> +                      24  25  26  27  28  29  30  31
>> +                      32  33  34  35  36  37  38  39
>> +                      40  41  42  43  44  45  46  47
>> +                      48  49  50  51  52  53  54  55
>> +                      56  57  58  59  60  61  62  63
>> +                      64  65  66  67  68  69  70  71
>> +                      72  73  74  75  76  77  78  79
>> +                      80  81  82  83  84  85  86  87
>> +                      88  89  90  91  92  93  94  95
>> +                      96  97  98  99 100 101 102 103
>> +                     104 105 106 107 108 109 110 111
>> +                     112 113 114 115 116 117 118 119
>> +                     120 121 122 123 124 125 126 127
>> +                     128 129 130 131 132 133 134 135
>> +                     136 137 138 139 140 141 142 143
>> +                     144 145 146 147 148 149 150 151
>> +                     152 153 154 155 156 157 158 159
>> +                     160 161 162 163 164 165 166 167
>> +                     168 169 170 171 172 173 174 175
>> +                     176 177 178 179 180 181 182 183
>> +                     184 185 186 187 188 189 190 191
>> +                     192 193 194 195 196 197 198 199
>> +                     200 201 202 203 204 205 206 207
>> +                     208 209 210 211 212 213 214 215
>> +                     216 217 218 219 220 221 222 223
>> +                     224 225 226 227 228 229 230 231
>> +                     232 233 234 235 236 237 238 239
>> +                     240 241 242 243 244 245 246 247
>> +                     248 249 250 251 252 253 254 255>;
>> +             default-brightness-level = <128>;
>> +             enable-gpios = <&gpio7 2 GPIO_ACTIVE_HIGH>;
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&bl_en>;
>> +             pwms = <&pwm0 0 1000000 PWM_POLARITY_INVERTED>;
>> +        };
>
> And here.

Done.

Re: [PATCH 2/4] pwm: rockchip: Allow polarity invert on rk3288

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Tue, Aug 19, 2014 at 09:05:20AM -0700, Doug Anderson wrote:
> On Tue, Aug 19, 2014 at 12:18 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Aug 18, 2014 at 10:09:07AM -0700, Doug Anderson wrote:
[...]
> >> @@ -74,10 +78,14 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> >>  {
> >>       struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> >>       u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> >> -                       PWM_CONTINUOUS | PWM_DUTY_POSITIVE |
> >> -                       PWM_INACTIVE_NEGATIVE;
> >> +                       PWM_CONTINUOUS;
> >>       u32 val;
> >>
> >> +     if (pc->polarity == PWM_POLARITY_INVERSED)
> >> +             enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> >> +     else
> >> +             enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
> >
> > I have a feeling you're going to answer the above question with: "Because
> > it's needed here". If so, my reply would be: "Then this function should
> > take a struct pwm_device instead of struct pwm_chip."
> 
> OK.  I've chosen to have it take a pwm_device AND a pwm_chip.  It is a
> little redundant because a pwm_device has a pointer to its pwm_chip,
> but it follows the lead of all of the callbacks in "struct pwm_ops".
> If you'd like me to spin it to take only a pwm_device I'm happy to.

No, that's fine.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]