* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-19 16:07 ` Doug Anderson
0 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-19 16:07 UTC (permalink / raw)
To: Heiko Stuebner, Thierry Reding, Caesar Wang
Cc: Sonny Rao, olof, Eddie Cai, Dmitry Torokhov, Doug Anderson,
robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
linux-pwm, devicetree, linux-kernel, linux-arm-kernel
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>
---
Changes in v3:
- Don't store a private copy of polarity.
- Use true instead of 1.
- Cleanup init order with "has_invert".
Changes in v2: None
.../devicetree/bindings/pwm/pwm-rockchip.txt | 4 +-
drivers/pwm/pwm-rockchip.c | 48 ++++++++++++++++++----
2 files changed, 43 insertions(+), 9 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..646aed2 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)
@@ -45,8 +47,10 @@ 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);
+ void (*set_enable)(struct pwm_chip *chip,
+ struct pwm_device *pwm, bool enable);
};
static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -54,7 +58,8 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
return container_of(c, struct rockchip_pwm_chip, chip);
}
-static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
+ struct pwm_device *pwm, bool enable)
{
struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
@@ -70,14 +75,19 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
writel_relaxed(val, pc->base + pc->data->regs.ctrl);
}
-static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
+static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
+ struct pwm_device *pwm, 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 (pwm->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 +134,23 @@ 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;
+
+ /*
+ * No action needed here because pwm->polarity will be set by the core
+ * and the core will only change polarity when the PWM is not enabled.
+ * We'll handle things in set_enable().
+ */
+
+ 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);
@@ -133,7 +160,7 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
if (ret)
return ret;
- pc->data->set_enable(chip, true);
+ pc->data->set_enable(chip, pwm, true);
return 0;
}
@@ -142,13 +169,14 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
- pc->data->set_enable(chip, false);
+ pc->data->set_enable(chip, pwm, false);
clk_disable(pc->clk);
}
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 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
.ctrl = 0x0c,
},
.prescaler = 1,
+ .has_invert = true,
.set_enable = rockchip_pwm_set_enable_v2,
};
@@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
.ctrl = 0x00,
},
.prescaler = 1,
+ .has_invert = true,
.set_enable = rockchip_pwm_set_enable_v2,
};
@@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
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;
+ }
ret = pwmchip_add(&pc->chip);
if (ret < 0) {
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-19 16:07 ` Doug Anderson
0 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-19 16:07 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
Changes in v3:
- Don't store a private copy of polarity.
- Use true instead of 1.
- Cleanup init order with "has_invert".
Changes in v2: None
.../devicetree/bindings/pwm/pwm-rockchip.txt | 4 +-
drivers/pwm/pwm-rockchip.c | 48 ++++++++++++++++++----
2 files changed, 43 insertions(+), 9 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..646aed2 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)
@@ -45,8 +47,10 @@ 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);
+ void (*set_enable)(struct pwm_chip *chip,
+ struct pwm_device *pwm, bool enable);
};
static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -54,7 +58,8 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
return container_of(c, struct rockchip_pwm_chip, chip);
}
-static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
+ struct pwm_device *pwm, bool enable)
{
struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
@@ -70,14 +75,19 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
writel_relaxed(val, pc->base + pc->data->regs.ctrl);
}
-static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
+static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
+ struct pwm_device *pwm, 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 (pwm->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 +134,23 @@ 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;
+
+ /*
+ * No action needed here because pwm->polarity will be set by the core
+ * and the core will only change polarity when the PWM is not enabled.
+ * We'll handle things in set_enable().
+ */
+
+ 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);
@@ -133,7 +160,7 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
if (ret)
return ret;
- pc->data->set_enable(chip, true);
+ pc->data->set_enable(chip, pwm, true);
return 0;
}
@@ -142,13 +169,14 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
- pc->data->set_enable(chip, false);
+ pc->data->set_enable(chip, pwm, false);
clk_disable(pc->clk);
}
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 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
.ctrl = 0x0c,
},
.prescaler = 1,
+ .has_invert = true,
.set_enable = rockchip_pwm_set_enable_v2,
};
@@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
.ctrl = 0x00,
},
.prescaler = 1,
+ .has_invert = true,
.set_enable = rockchip_pwm_set_enable_v2,
};
@@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
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;
+ }
ret = pwmchip_add(&pc->chip);
if (ret < 0) {
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-19 16:07 ` Doug Anderson
@ 2014-08-19 22:15 ` Caesar Wang
-1 siblings, 0 replies; 25+ messages in thread
From: Caesar Wang @ 2014-08-19 22:15 UTC (permalink / raw)
To: Doug Anderson, Heiko Stuebner, Thierry Reding
Cc: Sonny Rao, olof, Eddie Cai, Dmitry Torokhov, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-pwm, devicetree,
linux-kernel, linux-arm-kernel
Hi Doug,
I reviewed it, the change is need.
Reviewed-on: https://github.com/rkchrome/kernel.git
Reviewed-by: Caesar Wang <caesar.wang@rock-chips.com>
在 2014年08月20日 00:07, 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>
> ---
> Changes in v3:
> - Don't store a private copy of polarity.
> - Use true instead of 1.
> - Cleanup init order with "has_invert".
>
> Changes in v2: None
>
> .../devicetree/bindings/pwm/pwm-rockchip.txt | 4 +-
> drivers/pwm/pwm-rockchip.c | 48 ++++++++++++++++++----
> 2 files changed, 43 insertions(+), 9 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..646aed2 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)
>
> @@ -45,8 +47,10 @@ 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);
> + void (*set_enable)(struct pwm_chip *chip,
> + struct pwm_device *pwm, bool enable);
> };
>
> static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
> @@ -54,7 +58,8 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
> return container_of(c, struct rockchip_pwm_chip, chip);
> }
>
> -static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
> + struct pwm_device *pwm, bool enable)
> {
> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
> @@ -70,14 +75,19 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> -static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
> + struct pwm_device *pwm, 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 (pwm->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 +134,23 @@ 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;
> +
> + /*
> + * No action needed here because pwm->polarity will be set by the core
> + * and the core will only change polarity when the PWM is not enabled.
> + * We'll handle things in set_enable().
> + */
> +
> + 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);
> @@ -133,7 +160,7 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> if (ret)
> return ret;
>
> - pc->data->set_enable(chip, true);
> + pc->data->set_enable(chip, pwm, true);
>
> return 0;
> }
> @@ -142,13 +169,14 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>
> - pc->data->set_enable(chip, false);
> + pc->data->set_enable(chip, pwm, false);
>
> clk_disable(pc->clk);
> }
>
> 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 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .ctrl = 0x0c,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .ctrl = 0x00,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> 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;
> + }
>
> ret = pwmchip_add(&pc->chip);
> if (ret < 0) {
--
Best regards,
Caesar
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-19 22:15 ` Caesar Wang
0 siblings, 0 replies; 25+ messages in thread
From: Caesar Wang @ 2014-08-19 22:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi Doug,
I reviewed it, the change is need.
Reviewed-on: https://github.com/rkchrome/kernel.git
Reviewed-by: Caesar Wang <caesar.wang@rock-chips.com>
? 2014?08?20? 00:07, 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>
> ---
> Changes in v3:
> - Don't store a private copy of polarity.
> - Use true instead of 1.
> - Cleanup init order with "has_invert".
>
> Changes in v2: None
>
> .../devicetree/bindings/pwm/pwm-rockchip.txt | 4 +-
> drivers/pwm/pwm-rockchip.c | 48 ++++++++++++++++++----
> 2 files changed, 43 insertions(+), 9 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..646aed2 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)
>
> @@ -45,8 +47,10 @@ 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);
> + void (*set_enable)(struct pwm_chip *chip,
> + struct pwm_device *pwm, bool enable);
> };
>
> static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
> @@ -54,7 +58,8 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
> return container_of(c, struct rockchip_pwm_chip, chip);
> }
>
> -static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
> + struct pwm_device *pwm, bool enable)
> {
> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
> @@ -70,14 +75,19 @@ static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> -static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
> + struct pwm_device *pwm, 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 (pwm->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 +134,23 @@ 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;
> +
> + /*
> + * No action needed here because pwm->polarity will be set by the core
> + * and the core will only change polarity when the PWM is not enabled.
> + * We'll handle things in set_enable().
> + */
> +
> + 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);
> @@ -133,7 +160,7 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> if (ret)
> return ret;
>
> - pc->data->set_enable(chip, true);
> + pc->data->set_enable(chip, pwm, true);
>
> return 0;
> }
> @@ -142,13 +169,14 @@ static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>
> - pc->data->set_enable(chip, false);
> + pc->data->set_enable(chip, pwm, false);
>
> clk_disable(pc->clk);
> }
>
> 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 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .ctrl = 0x0c,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .ctrl = 0x00,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> 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;
> + }
>
> ret = pwmchip_add(&pc->chip);
> if (ret < 0) {
--
Best regards,
Caesar
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-19 16:07 ` Doug Anderson
@ 2014-08-20 10:04 ` Thierry Reding
-1 siblings, 0 replies; 25+ messages in thread
From: Thierry Reding @ 2014-08-20 10:04 UTC (permalink / raw)
To: Doug Anderson
Cc: Heiko Stuebner, Caesar Wang, Sonny Rao, olof, Eddie Cai,
Dmitry Torokhov, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, linux-pwm, devicetree, linux-kernel,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 2438 bytes --]
On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
> 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.
I don't see any files in arch/arm/boot/dts using either of the
rockchip,vop-pwm or rockchip,rk3288-pwm compatible strings, so there's
no reason to consider this stable ABI yet. As far as I'm concerned the
last sentence can just as well be dropped.
Besides, patches have been posted to support #pwm-cells = <2> and
#pwm-cells = <3> at the same time which should give you backwards-
compatibility for free.
A couple more comments inline.
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> +int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> + enum pwm_polarity polarity)
This should be static.
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> +
> + if (!pc->data->has_invert)
> + return -ENOSYS;
> +
> + /*
> + * No action needed here because pwm->polarity will be set by the core
> + * and the core will only change polarity when the PWM is not enabled.
> + * We'll handle things in set_enable().
> + */
> +
> + return 0;
> +}
An alternative here would be to provide a separate pwm_ops with
.set_polarity = NULL for the versions of the IP block that don't support
polarity inversion yet, but this works for me too.
> @@ -173,6 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .ctrl = 0x0c,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .ctrl = 0x00,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
Can you please add a '.has_invert = false,' line to pwm_data_v1? I know
it's not strictly necessary but I like it when things are explicitly
stated.
>
> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> pc->chip.ops = &rockchip_pwm_ops;
> pc->chip.base = -1;
> pc->chip.npwm = 1;
> + if (pc->data->has_invert) {
There should be a blank line between the above two.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-20 10:04 ` Thierry Reding
0 siblings, 0 replies; 25+ messages in thread
From: Thierry Reding @ 2014-08-20 10:04 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
> 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.
I don't see any files in arch/arm/boot/dts using either of the
rockchip,vop-pwm or rockchip,rk3288-pwm compatible strings, so there's
no reason to consider this stable ABI yet. As far as I'm concerned the
last sentence can just as well be dropped.
Besides, patches have been posted to support #pwm-cells = <2> and
#pwm-cells = <3> at the same time which should give you backwards-
compatibility for free.
A couple more comments inline.
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> +int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> + enum pwm_polarity polarity)
This should be static.
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> +
> + if (!pc->data->has_invert)
> + return -ENOSYS;
> +
> + /*
> + * No action needed here because pwm->polarity will be set by the core
> + * and the core will only change polarity when the PWM is not enabled.
> + * We'll handle things in set_enable().
> + */
> +
> + return 0;
> +}
An alternative here would be to provide a separate pwm_ops with
.set_polarity = NULL for the versions of the IP block that don't support
polarity inversion yet, but this works for me too.
> @@ -173,6 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .ctrl = 0x0c,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
>
> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .ctrl = 0x00,
> },
> .prescaler = 1,
> + .has_invert = true,
> .set_enable = rockchip_pwm_set_enable_v2,
> };
Can you please add a '.has_invert = false,' line to pwm_data_v1? I know
it's not strictly necessary but I like it when things are explicitly
stated.
>
> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
> pc->chip.ops = &rockchip_pwm_ops;
> pc->chip.base = -1;
> pc->chip.npwm = 1;
> + if (pc->data->has_invert) {
There should be a blank line between the above two.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/ee85c7ef/attachment.sig>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-20 10:04 ` Thierry Reding
@ 2014-08-20 18:54 ` Doug Anderson
-1 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-20 18:54 UTC (permalink / raw)
To: Thierry Reding
Cc: Heiko Stuebner, Caesar Wang, Sonny Rao, Olof Johansson, Eddie Cai,
Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, linux-pwm, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Thierry,
On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>> 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.
>
> I don't see any files in arch/arm/boot/dts using either of the
> rockchip,vop-pwm or rockchip,rk3288-pwm compatible strings, so there's
> no reason to consider this stable ABI yet. As far as I'm concerned the
> last sentence can just as well be dropped.
>
> Besides, patches have been posted to support #pwm-cells = <2> and
> #pwm-cells = <3> at the same time which should give you backwards-
> compatibility for free.
Done. I'm happy that the subsystem is being improved to handle
pwm-cells more dynamically, too! That's a nice improvement.
> A couple more comments inline.
>
>> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> [...]
>> +int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>
> This should be static.
Done.
>> +{
>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>> +
>> + if (!pc->data->has_invert)
>> + return -ENOSYS;
>> +
>> + /*
>> + * No action needed here because pwm->polarity will be set by the core
>> + * and the core will only change polarity when the PWM is not enabled.
>> + * We'll handle things in set_enable().
>> + */
>> +
>> + return 0;
>> +}
>
> An alternative here would be to provide a separate pwm_ops with
> .set_polarity = NULL for the versions of the IP block that don't support
> polarity inversion yet, but this works for me too.
Good point. I'll make the change since it paves the way for other
pwm_ops that are different.
>> @@ -173,6 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>> .ctrl = 0x0c,
>> },
>> .prescaler = 1,
>> + .has_invert = true,
>> .set_enable = rockchip_pwm_set_enable_v2,
>> };
>>
>> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
>> .ctrl = 0x00,
>> },
>> .prescaler = 1,
>> + .has_invert = true,
>> .set_enable = rockchip_pwm_set_enable_v2,
>> };
>
> Can you please add a '.has_invert = false,' line to pwm_data_v1? I know
> it's not strictly necessary but I like it when things are explicitly
> stated.
No longer relevant with different pwm_ops.
>> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>> pc->chip.ops = &rockchip_pwm_ops;
>> pc->chip.base = -1;
>> pc->chip.npwm = 1;
>> + if (pc->data->has_invert) {
>
> There should be a blank line between the above two.
Done.
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-20 18:54 ` Doug Anderson
0 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-20 18:54 UTC (permalink / raw)
To: linux-arm-kernel
Thierry,
On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>> 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.
>
> I don't see any files in arch/arm/boot/dts using either of the
> rockchip,vop-pwm or rockchip,rk3288-pwm compatible strings, so there's
> no reason to consider this stable ABI yet. As far as I'm concerned the
> last sentence can just as well be dropped.
>
> Besides, patches have been posted to support #pwm-cells = <2> and
> #pwm-cells = <3> at the same time which should give you backwards-
> compatibility for free.
Done. I'm happy that the subsystem is being improved to handle
pwm-cells more dynamically, too! That's a nice improvement.
> A couple more comments inline.
>
>> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> [...]
>> +int rockchip_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>
> This should be static.
Done.
>> +{
>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>> +
>> + if (!pc->data->has_invert)
>> + return -ENOSYS;
>> +
>> + /*
>> + * No action needed here because pwm->polarity will be set by the core
>> + * and the core will only change polarity when the PWM is not enabled.
>> + * We'll handle things in set_enable().
>> + */
>> +
>> + return 0;
>> +}
>
> An alternative here would be to provide a separate pwm_ops with
> .set_polarity = NULL for the versions of the IP block that don't support
> polarity inversion yet, but this works for me too.
Good point. I'll make the change since it paves the way for other
pwm_ops that are different.
>> @@ -173,6 +201,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
>> .ctrl = 0x0c,
>> },
>> .prescaler = 1,
>> + .has_invert = true,
>> .set_enable = rockchip_pwm_set_enable_v2,
>> };
>>
>> @@ -184,6 +213,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
>> .ctrl = 0x00,
>> },
>> .prescaler = 1,
>> + .has_invert = true,
>> .set_enable = rockchip_pwm_set_enable_v2,
>> };
>
> Can you please add a '.has_invert = false,' line to pwm_data_v1? I know
> it's not strictly necessary but I like it when things are explicitly
> stated.
No longer relevant with different pwm_ops.
>> @@ -230,6 +260,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>> pc->chip.ops = &rockchip_pwm_ops;
>> pc->chip.base = -1;
>> pc->chip.npwm = 1;
>> + if (pc->data->has_invert) {
>
> There should be a blank line between the above two.
Done.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-20 18:54 ` Doug Anderson
@ 2014-08-20 19:29 ` Dmitry Torokhov
-1 siblings, 0 replies; 25+ messages in thread
From: Dmitry Torokhov @ 2014-08-20 19:29 UTC (permalink / raw)
To: Doug Anderson, Thierry Reding
Cc: Heiko Stuebner, Caesar Wang, Sonny Rao, Olof Johansson, Eddie Cai,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, hpa
On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
>On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
><thierry.reding@gmail.com> wrote:
>> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>>> The rk3288 has the ability to invert
>>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>>> +
>>> + if (!pc->data->has_invert)
>>> + return -ENOSYS;
>>> +
At the kernel summit hpa also mentioned that ENOSYS should only be used for missing syscalls. Not sure what error code would suit better here though...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-20 19:29 ` Dmitry Torokhov
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Torokhov @ 2014-08-20 19:29 UTC (permalink / raw)
To: linux-arm-kernel
On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
>On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
><thierry.reding@gmail.com> wrote:
>> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>>> The rk3288 has the ability to invert
>>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>>> +
>>> + if (!pc->data->has_invert)
>>> + return -ENOSYS;
>>> +
At the kernel summit hpa also mentioned that ENOSYS should only be used for missing syscalls. Not sure what error code would suit better here though...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-20 19:29 ` Dmitry Torokhov
@ 2014-08-20 19:31 ` Doug Anderson
-1 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-20 19:31 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Thierry Reding, Heiko Stuebner, Caesar Wang, Sonny Rao,
Olof Johansson, Eddie Cai, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, linux-pwm, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, hpa
Dmitry,
On Wed, Aug 20, 2014 at 12:29 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
>>On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
>><thierry.reding@gmail.com> wrote:
>>> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>>>> The rk3288 has the ability to invert
>>>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>>>> +
>>>> + if (!pc->data->has_invert)
>>>> + return -ENOSYS;
>>>> +
>
> At the kernel summit hpa also mentioned that ENOSYS should only be used for missing syscalls. Not sure what error code would suit better here though...
Luckily this code is gone now. See v4 of the series. ;)
-Doug
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-20 19:31 ` Doug Anderson
0 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2014-08-20 19:31 UTC (permalink / raw)
To: linux-arm-kernel
Dmitry,
On Wed, Aug 20, 2014 at 12:29 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
>>On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
>><thierry.reding@gmail.com> wrote:
>>> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
>>>> The rk3288 has the ability to invert
>>>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>>>> +
>>>> + if (!pc->data->has_invert)
>>>> + return -ENOSYS;
>>>> +
>
> At the kernel summit hpa also mentioned that ENOSYS should only be used for missing syscalls. Not sure what error code would suit better here though...
Luckily this code is gone now. See v4 of the series. ;)
-Doug
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
2014-08-20 19:29 ` Dmitry Torokhov
@ 2014-08-21 6:40 ` Thierry Reding
-1 siblings, 0 replies; 25+ messages in thread
From: Thierry Reding @ 2014-08-21 6:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Doug Anderson, Heiko Stuebner, Caesar Wang, Sonny Rao,
Olof Johansson, Eddie Cai, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, linux-pwm, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, hpa
[-- Attachment #1: Type: text/plain, Size: 903 bytes --]
On Wed, Aug 20, 2014 at 02:29:17PM -0500, Dmitry Torokhov wrote:
> On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
> >On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
> ><thierry.reding@gmail.com> wrote:
> >> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
> >>> The rk3288 has the ability to invert
> >>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> >>> +
> >>> + if (!pc->data->has_invert)
> >>> + return -ENOSYS;
> >>> +
>
> At the kernel summit hpa also mentioned that ENOSYS should only be
> used for missing syscalls. Not sure what error code would suit better
> here though...
I'd be interested in the rationale why ENOSYS shouldn't be used within
the kernel. As long as it doesn't leak to userspace where it could
possibly confuse applications I don't see any harm in using it.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/4] pwm: rockchip: Allow polarity invert on rk3288
@ 2014-08-21 6:40 ` Thierry Reding
0 siblings, 0 replies; 25+ messages in thread
From: Thierry Reding @ 2014-08-21 6:40 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 20, 2014 at 02:29:17PM -0500, Dmitry Torokhov wrote:
> On August 20, 2014 1:54:11 PM CDT, Doug Anderson <dianders@chromium.org> wrote:
> >On Wed, Aug 20, 2014 at 3:04 AM, Thierry Reding
> ><thierry.reding@gmail.com> wrote:
> >> On Tue, Aug 19, 2014 at 09:07:54AM -0700, Doug Anderson wrote:
> >>> The rk3288 has the ability to invert
> >>> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> >>> +
> >>> + if (!pc->data->has_invert)
> >>> + return -ENOSYS;
> >>> +
>
> At the kernel summit hpa also mentioned that ENOSYS should only be
> used for missing syscalls. Not sure what error code would suit better
> here though...
I'd be interested in the rationale why ENOSYS shouldn't be used within
the kernel. As long as it doesn't leak to userspace where it could
possibly confuse applications I don't see any harm in using it.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140821/44ef4883/attachment.sig>
^ permalink raw reply [flat|nested] 25+ messages in thread