Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Boris, > > > > Could you be more specific here? > > > > As I mentioned before, the problem is not with the lack of > > "atomic" API. > > Below is a quick and dirty I made on top of this patch to show you how > atomic update can be implemented in this driver. Thank you for example patch. I will implement the ->apply() callback and post patches very soon :-). I had two issues with the ->apply() implementation: - Do my work on top of this patch (https://lkml.org/lkml/2016/10/7/454 as you did) to avoid rewriting work already done. - In the example ->apply() implementation for rockchip (https://patchwork.kernel.org/patch/7228221/) the ->config() callback was not removed when ->apply() was implemented. I was confused with such approach, but as you explained in this mail, the solely ->apply() is enough. > It's not tested, and > probably not working, but it should give you a better idea of what is > expected. Thanks for explanation, Łukasz Majewski pgplQhSQ8CesH.pgp Description: OpenPGP digital signature
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On Fri, 21 Oct 2016 23:49:39 +0200
Lukasz Majewski wrote:
> Hi Stefan,
>
> > On 2016-10-20 01:30, Lukasz Majewski wrote:
> > > Hi Stefan,
> > >
> > >> Hi Stefan,
> > >>
> > >> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> > >> > > Hi Stefan,
> > >> > >
> > >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > >> > >> > From: Lothar Wassmann
> > >> > >> >
> > >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > >> > configurable output polarity. This patch adds support to
> > >> > >> > utilize this feature where available.
> > >> > >> >
> > >> > >> > Signed-off-by: Lothar Waßmann
> > >> > >> > Signed-off-by: Lukasz Majewski
> > >> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > >> > Acked-by: Shawn Guo
> > >> > >> > Reviewed-by: Sascha Hauer
> > >> > >> > ---
> > >> > >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> > >> > >> > drivers/pwm/pwm-imx.c | 51
> > >> > >> > +-- 2 files changed, 51 insertions(+), 6
> > >> > >> > deletions(-)
> > >> > >> >
> > >> > >> > diff --git
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> > >> > >> > e00c2e9..c61bdf8 100644 ---
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> > >> > >> > +6,8 @@ Required properties:
> > >> > >> >- "fsl,imx1-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX1
> > >> > >> >- "fsl,imx27-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX27
> > >> > >> > - reg: physical base address and length of the controller's
> > >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > >> > directory for a description of
> > >> > >> > - the cells format.
> > >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> > >> > >> > See pwm.txt
> > >> > >> > + in this directory for a description of the cells format.
> > >> > >> > - clocks : Clock specifiers for both ipg and per clocks.
> > >> > >> > - clock-names : Clock names should include both "ipg" and
> > >> > >> > "per" See the clock consumer binding,
> > >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > >> > Example:
> > >> > >> >
> > >> > >> > pwm1: pwm@53fb4000 {
> > >> > >> > - #pwm-cells = <2>;
> > >> > >> > + #pwm-cells = <3>;
> > >> > >> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> > >> > reg = <0x53fb4000 0x4000>;
> > >> > >> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > >> > index d600fd5..c37d223 100644
> > >> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > >> > @@ -38,6 +38,7 @@
> > >> > >> > #define MX3_PWMCR_DOZEEN (1 << 24)
> > >> > >> > #define MX3_PWMCR_WAITEN (1 << 23)
> > >> > >> > #define MX3_PWMCR_DBGEN (1 << 22)
> > >> > >> > +#define MX3_PWMCR_POUTC (1 << 18)
> > >> > >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
> > >> > >> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> > >> > >> > #define MX3_PWMCR_SWR(1 << 3)
> > >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > >> > pwm_chip *chip, if (enable)
> > >> > >> > cr |= MX3_PWMCR_EN;
> > >> > >> >
> > >> > >> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > >> > + cr |= MX3_PWMCR_POUTC;
> > >> > >> > +
> > >> > >>
> > >> > >> This seems wrong to me, the config callback is meant for
> > >> > >> period/duty cycle only.
> > >
> > > Unfortunately, it also resets the PWM IP block and setups it again
> > > (by writing to PWMCR register). In that function we setup for
> > > example MX3_PWMCR_DOZEEN
> > > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> > >
> > >
> > > I've double checked the backlight and pwm code flow.
> > >
> > > Please find following snippet:
> > >
> > > [0.135545] # imx_pwm_probe
> > > [0.135581] PWM supports output inversion
> > > [0.136864] # pwm_backlight_probe
> > > [0.136913] backlight supply power not found, using dummy
> > > regulator [0.136984] # imx_pwm_set_polarity 1
> > > [0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > > 0x4 0xf08f8000
> > > [0.137005] #0 imx_pwm_config_v2 cr: 0x4
> > > [0.137683] #1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > > [0.137693] #2 imx_pwm_config_v2 cr: 0x1c20050
> > > [0.137702] #3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > > [0.137711] @@ pwm_apply_state
>
> Maybe a bit more logs:
>
> [0.135451] # imx_pwm_probe
> [0.135488] PWM supports output inversion
> [0.136777] # pwm_backlight_probe
> [0.136826] backlight supply p
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Lukasz,
On Fri, 21 Oct 2016 23:49:39 +0200
Lukasz Majewski wrote:
> Hi Stefan,
>
> > On 2016-10-20 01:30, Lukasz Majewski wrote:
> > > Hi Stefan,
> > >
> > >> Hi Stefan,
> > >>
> > >> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> > >> > > Hi Stefan,
> > >> > >
> > >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > >> > >> > From: Lothar Wassmann
> > >> > >> >
> > >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > >> > configurable output polarity. This patch adds support to
> > >> > >> > utilize this feature where available.
> > >> > >> >
> > >> > >> > Signed-off-by: Lothar Waßmann
> > >> > >> > Signed-off-by: Lukasz Majewski
> > >> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > >> > Acked-by: Shawn Guo
> > >> > >> > Reviewed-by: Sascha Hauer
> > >> > >> > ---
> > >> > >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> > >> > >> > drivers/pwm/pwm-imx.c | 51
> > >> > >> > +-- 2 files changed, 51 insertions(+), 6
> > >> > >> > deletions(-)
> > >> > >> >
> > >> > >> > diff --git
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> > >> > >> > e00c2e9..c61bdf8 100644 ---
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> > >> > >> > +6,8 @@ Required properties:
> > >> > >> >- "fsl,imx1-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX1
> > >> > >> >- "fsl,imx27-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX27
> > >> > >> > - reg: physical base address and length of the controller's
> > >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > >> > directory for a description of
> > >> > >> > - the cells format.
> > >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> > >> > >> > See pwm.txt
> > >> > >> > + in this directory for a description of the cells format.
> > >> > >> > - clocks : Clock specifiers for both ipg and per clocks.
> > >> > >> > - clock-names : Clock names should include both "ipg" and
> > >> > >> > "per" See the clock consumer binding,
> > >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > >> > Example:
> > >> > >> >
> > >> > >> > pwm1: pwm@53fb4000 {
> > >> > >> > - #pwm-cells = <2>;
> > >> > >> > + #pwm-cells = <3>;
> > >> > >> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> > >> > reg = <0x53fb4000 0x4000>;
> > >> > >> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > >> > index d600fd5..c37d223 100644
> > >> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > >> > @@ -38,6 +38,7 @@
> > >> > >> > #define MX3_PWMCR_DOZEEN (1 << 24)
> > >> > >> > #define MX3_PWMCR_WAITEN (1 << 23)
> > >> > >> > #define MX3_PWMCR_DBGEN (1 << 22)
> > >> > >> > +#define MX3_PWMCR_POUTC (1 << 18)
> > >> > >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
> > >> > >> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> > >> > >> > #define MX3_PWMCR_SWR(1 << 3)
> > >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > >> > pwm_chip *chip, if (enable)
> > >> > >> > cr |= MX3_PWMCR_EN;
> > >> > >> >
> > >> > >> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > >> > + cr |= MX3_PWMCR_POUTC;
> > >> > >> > +
> > >> > >>
> > >> > >> This seems wrong to me, the config callback is meant for
> > >> > >> period/duty cycle only.
> > >
> > > Unfortunately, it also resets the PWM IP block and setups it again
> > > (by writing to PWMCR register).
Well, this is an implementation problem. If you were reading the
MX3_PWMCR register and masking it with MX3_PWMCR_POUTC to keep the
value set by ->set_polarity() that wouldn't be a problem.
> > > In that function we setup for
> > > example MX3_PWMCR_DOZEEN
> > > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
I think there's no real problem in setting the polarity again in
->config(), it's just that it feels weird to have it set twice (in
->set_polarity() and in ->config()).
> > >
> > >
> > > I've double checked the backlight and pwm code flow.
> > >
> > > Please find following snippet:
> > >
> > > [0.135545] # imx_pwm_probe
> > > [0.135581] PWM supports output inversion
> > > [0.136864] # pwm_backlight_probe
> > > [0.136913] backlight supply power not found, using dummy
> > > regulator [0.136984] # imx_pwm_set_polarity 1
> > > [0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > > 0x4 0xf08f8000
> > > [0.137005] #0 imx_pwm_config_v2 cr: 0x4
> > > [0.137683] #1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > > [
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On Fri, 7 Oct 2016 20:41:26 +0530
Bhuvanchandra DV wrote:
> From: Lothar Wassmann
>
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
> polarity. This patch adds support to utilize this feature where available.
>
> Signed-off-by: Lothar Waßmann
> Signed-off-by: Lukasz Majewski
> Signed-off-by: Bhuvanchandra DV
> Acked-by: Shawn Guo
> Reviewed-by: Sascha Hauer
> ---
> Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> drivers/pwm/pwm-imx.c | 51
> +--
> 2 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> index e00c2e9..c61bdf8 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> @@ -6,8 +6,8 @@ Required properties:
>- "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
>- "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
> - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> - the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
> + in this directory for a description of the cells format.
> - clocks : Clock specifiers for both ipg and per clocks.
> - clock-names : Clock names should include both "ipg" and "per"
> See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
> Example:
>
> pwm1: pwm@53fb4000 {
> - #pwm-cells = <2>;
> + #pwm-cells = <3>;
> compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> reg = <0x53fb4000 0x4000>;
> clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
> #define MX3_PWMCR_DOZEEN (1 << 24)
> #define MX3_PWMCR_WAITEN (1 << 23)
> #define MX3_PWMCR_DBGEN (1 << 22)
> +#define MX3_PWMCR_POUTC (1 << 18)
> #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
> #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> #define MX3_PWMCR_SWR(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> if (enable)
> cr |= MX3_PWMCR_EN;
>
> + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> + cr |= MX3_PWMCR_POUTC;
> +
> writel(cr, imx->mmio_base + MX3_PWMCR);
>
> return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip *chip,
> struct pwm_device *pwm)
> clk_disable_unprepare(imx->clk_per);
> }
>
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + enum pwm_polarity polarity)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + u32 val;
> +
> + if (polarity == pwm->args.polarity)
> + return 0;
> +
> + val = readl(imx->mmio_base + MX3_PWMCR);
> +
> + if (polarity == PWM_POLARITY_INVERSED)
> + val |= MX3_PWMCR_POUTC;
> + else
> + val &= ~MX3_PWMCR_POUTC;
> +
> + writel(val, imx->mmio_base + MX3_PWMCR);
> +
> + dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> + polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> +
> + return 0;
> +}
I'll read the discussion Lukasz and Stefan had before reviewing this
part ;-).
> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
> .enable = imx_pwm_enable,
> .disable = imx_pwm_disable,
> .config = imx_pwm_config,
> .owner = THIS_MODULE,
> };
>
> +static struct pwm_ops imx_pwm_ops_v2 = {
> + .enable = imx_pwm_enable,
> + .disable = imx_pwm_disable,
> + .set_polarity = imx_pwm_set_polarity,
> + .config = imx_pwm_config,
> + .owner = THIS_MODULE,
> +};
> +
> struct imx_pwm_data {
> int (*config)(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns);
> void (*set_enable)(struct pwm_chip *chip, bool enable);
> + struct pwm_ops *pwm_ops;
Probably better to make that consistent: drop the ->config() and
->set_enable() hooks, and make both v1 and v2 define their own pwm_ops.
If you have common logic that is shared between v1 and v2, you can just
create helper functions implementing this common logic and call them
from your version-specific implementation.
> };
>
> static struct imx_pwm_data imx_pwm_data_v1 = {
> .config = imx_pwm_config_v1,
> .set_enable = imx_pwm_set_enable_v1,
> + .pwm_ops = &imx_pwm_ops_v1,
> };
>
> static struct imx_pwm_data imx_pwm_data_v2 = {
> .config = imx_pwm_config_v2,
> .set_enable = imx_pwm_set_enable_v2,
> + .pwm_ops = &imx_pw
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Stefan,
> On 2016-10-20 01:30, Lukasz Majewski wrote:
> > Hi Stefan,
> >
> >> Hi Stefan,
> >>
> >> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> >> > > Hi Stefan,
> >> > >
> >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> >> > >> > From: Lothar Wassmann
> >> > >> >
> >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> >> > >> > configurable output polarity. This patch adds support to
> >> > >> > utilize this feature where available.
> >> > >> >
> >> > >> > Signed-off-by: Lothar Waßmann
> >> > >> > Signed-off-by: Lukasz Majewski
> >> > >> > Signed-off-by: Bhuvanchandra DV
> >> > >> > Acked-by: Shawn Guo
> >> > >> > Reviewed-by: Sascha Hauer
> >> > >> > ---
> >> > >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> >> > >> > drivers/pwm/pwm-imx.c | 51
> >> > >> > +-- 2 files changed, 51 insertions(+), 6
> >> > >> > deletions(-)
> >> > >> >
> >> > >> > diff --git
> >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> >> > >> > e00c2e9..c61bdf8 100644 ---
> >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> >> > >> > +6,8 @@ Required properties:
> >> > >> >- "fsl,imx1-pwm" for PWM compatible with the one
> >> > >> > integrated on i.MX1
> >> > >> >- "fsl,imx27-pwm" for PWM compatible with the one
> >> > >> > integrated on i.MX27
> >> > >> > - reg: physical base address and length of the controller's
> >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> >> > >> > directory for a description of
> >> > >> > - the cells format.
> >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> >> > >> > See pwm.txt
> >> > >> > + in this directory for a description of the cells format.
> >> > >> > - clocks : Clock specifiers for both ipg and per clocks.
> >> > >> > - clock-names : Clock names should include both "ipg" and
> >> > >> > "per" See the clock consumer binding,
> >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> >> > >> > Example:
> >> > >> >
> >> > >> > pwm1: pwm@53fb4000 {
> >> > >> > - #pwm-cells = <2>;
> >> > >> > + #pwm-cells = <3>;
> >> > >> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> >> > >> > reg = <0x53fb4000 0x4000>;
> >> > >> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > >> > index d600fd5..c37d223 100644
> >> > >> > --- a/drivers/pwm/pwm-imx.c
> >> > >> > +++ b/drivers/pwm/pwm-imx.c
> >> > >> > @@ -38,6 +38,7 @@
> >> > >> > #define MX3_PWMCR_DOZEEN (1 << 24)
> >> > >> > #define MX3_PWMCR_WAITEN (1 << 23)
> >> > >> > #define MX3_PWMCR_DBGEN(1 << 22)
> >> > >> > +#define MX3_PWMCR_POUTC(1 << 18)
> >> > >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> >> > >> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> >> > >> > #define MX3_PWMCR_SWR (1 << 3)
> >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> >> > >> > pwm_chip *chip, if (enable)
> >> > >> > cr |= MX3_PWMCR_EN;
> >> > >> >
> >> > >> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> >> > >> > + cr |= MX3_PWMCR_POUTC;
> >> > >> > +
> >> > >>
> >> > >> This seems wrong to me, the config callback is meant for
> >> > >> period/duty cycle only.
> >
> > Unfortunately, it also resets the PWM IP block and setups it again
> > (by writing to PWMCR register). In that function we setup for
> > example MX3_PWMCR_DOZEEN
> > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> >
> >
> > I've double checked the backlight and pwm code flow.
> >
> > Please find following snippet:
> >
> > [0.135545] # imx_pwm_probe
> > [0.135581] PWM supports output inversion
> > [0.136864] # pwm_backlight_probe
> > [0.136913] backlight supply power not found, using dummy
> > regulator [0.136984] # imx_pwm_set_polarity 1
> > [0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > 0x4 0xf08f8000
> > [0.137005] #0 imx_pwm_config_v2 cr: 0x4
> > [0.137683] #1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > [0.137693] #2 imx_pwm_config_v2 cr: 0x1c20050
> > [0.137702] #3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > [0.137711] @@ pwm_apply_state
Maybe a bit more logs:
[0.135451] # imx_pwm_probe
[0.135488] PWM supports output inversion
[0.136777] # pwm_backlight_probe
[0.136826] backlight supply power not found, using dummy regulator
[0.136893] * pwm_apply_state state->enabled: 0
[0.136902] # imx_pwm_set_polarity 1
[0.136913] imx_pwm_set_polarity: polarity set to inverted cr: 0x4
0xf08f8000
[0.136923] #0 imx_pwm_config_v2 c
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On 2016-10-20 01:30, Lukasz Majewski wrote:
> Hi Stefan,
>
>> Hi Stefan,
>>
>> > On 2016-10-12 15:15, Lukasz Majewski wrote:
>> > > Hi Stefan,
>> > >
>> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
>> > >> > From: Lothar Wassmann
>> > >> >
>> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
>> > >> > configurable output polarity. This patch adds support to
>> > >> > utilize this feature where available.
>> > >> >
>> > >> > Signed-off-by: Lothar Waßmann
>> > >> > Signed-off-by: Lukasz Majewski
>> > >> > Signed-off-by: Bhuvanchandra DV
>> > >> > Acked-by: Shawn Guo
>> > >> > Reviewed-by: Sascha Hauer
>> > >> > ---
>> > >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
>> > >> > drivers/pwm/pwm-imx.c | 51
>> > >> > +-- 2 files changed, 51 insertions(+), 6
>> > >> > deletions(-)
>> > >> >
>> > >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > index e00c2e9..c61bdf8 100644
>> > >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > >> > @@ -6,8 +6,8 @@ Required properties:
>> > >> >- "fsl,imx1-pwm" for PWM compatible with the one integrated
>> > >> > on i.MX1
>> > >> >- "fsl,imx27-pwm" for PWM compatible with the one integrated
>> > >> > on i.MX27
>> > >> > - reg: physical base address and length of the controller's
>> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
>> > >> > directory for a description of
>> > >> > - the cells format.
>> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
>> > >> > pwm.txt
>> > >> > + in this directory for a description of the cells format.
>> > >> > - clocks : Clock specifiers for both ipg and per clocks.
>> > >> > - clock-names : Clock names should include both "ipg" and
>> > >> > "per" See the clock consumer binding,
>> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
>> > >> > Example:
>> > >> >
>> > >> > pwm1: pwm@53fb4000 {
>> > >> > - #pwm-cells = <2>;
>> > >> > + #pwm-cells = <3>;
>> > >> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>> > >> > reg = <0x53fb4000 0x4000>;
>> > >> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
>> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > >> > index d600fd5..c37d223 100644
>> > >> > --- a/drivers/pwm/pwm-imx.c
>> > >> > +++ b/drivers/pwm/pwm-imx.c
>> > >> > @@ -38,6 +38,7 @@
>> > >> > #define MX3_PWMCR_DOZEEN (1 << 24)
>> > >> > #define MX3_PWMCR_WAITEN (1 << 23)
>> > >> > #define MX3_PWMCR_DBGEN (1 << 22)
>> > >> > +#define MX3_PWMCR_POUTC (1 << 18)
>> > >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
>> > >> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
>> > >> > #define MX3_PWMCR_SWR(1 << 3)
>> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
>> > >> > pwm_chip *chip, if (enable)
>> > >> > cr |= MX3_PWMCR_EN;
>> > >> >
>> > >> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
>> > >> > + cr |= MX3_PWMCR_POUTC;
>> > >> > +
>> > >>
>> > >> This seems wrong to me, the config callback is meant for
>> > >> period/duty cycle only.
>
> Unfortunately, it also resets the PWM IP block and setups it again (by
> writing to PWMCR register). In that function we setup for example
> MX3_PWMCR_DOZEEN
> and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
>
>
> I've double checked the backlight and pwm code flow.
>
> Please find following snippet:
>
> [0.135545] # imx_pwm_probe
> [0.135581] PWM supports output inversion
> [0.136864] # pwm_backlight_probe
> [0.136913] backlight supply power not found, using dummy regulator
> [0.136984] # imx_pwm_set_polarity 1
> [0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> 0x4 0xf08f8000
> [0.137005] #0 imx_pwm_config_v2 cr: 0x4
> [0.137683] #1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [0.137693] #2 imx_pwm_config_v2 cr: 0x1c20050
> [0.137702] #3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [0.137711] @@ pwm_apply_state
>
> Here the pwm_backlight_probe calls set_polarity callback available in
> pwm - the polarity is set (the 0x4 value).
>
> The above operation is performed in pwm_apply_state (@ drivers/pwm/core.c).
> In the same function, latter we call the pwm->chip->ops->config(), which is
> the
> pointer to config_v2.
> Since the PWM is not yet enabled, this function performs SW reset and
> PWM inversion setting is cleared.
That function should not do that. It was probably already problematic in
the old times, it is definitely now with the atomic PWM stuff.
>
> Possible solutions:
>
> 1. Leave the original patch from Bhuvanchandra as it was (I'm for this
>
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Stefan,
> Hi Stefan,
>
> > On 2016-10-12 15:15, Lukasz Majewski wrote:
> > > Hi Stefan,
> > >
> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > >> > From: Lothar Wassmann
> > >> >
> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > configurable output polarity. This patch adds support to
> > >> > utilize this feature where available.
> > >> >
> > >> > Signed-off-by: Lothar Waßmann
> > >> > Signed-off-by: Lukasz Majewski
> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > Acked-by: Shawn Guo
> > >> > Reviewed-by: Sascha Hauer
> > >> > ---
> > >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> > >> > drivers/pwm/pwm-imx.c | 51
> > >> > +-- 2 files changed, 51 insertions(+), 6
> > >> > deletions(-)
> > >> >
> > >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > index e00c2e9..c61bdf8 100644
> > >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > @@ -6,8 +6,8 @@ Required properties:
> > >> >- "fsl,imx1-pwm" for PWM compatible with the one integrated
> > >> > on i.MX1
> > >> >- "fsl,imx27-pwm" for PWM compatible with the one integrated
> > >> > on i.MX27
> > >> > - reg: physical base address and length of the controller's
> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > directory for a description of
> > >> > - the cells format.
> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> > >> > pwm.txt
> > >> > + in this directory for a description of the cells format.
> > >> > - clocks : Clock specifiers for both ipg and per clocks.
> > >> > - clock-names : Clock names should include both "ipg" and
> > >> > "per" See the clock consumer binding,
> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > Example:
> > >> >
> > >> > pwm1: pwm@53fb4000 {
> > >> > - #pwm-cells = <2>;
> > >> > + #pwm-cells = <3>;
> > >> >compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> >reg = <0x53fb4000 0x4000>;
> > >> >clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > index d600fd5..c37d223 100644
> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > @@ -38,6 +38,7 @@
> > >> > #define MX3_PWMCR_DOZEEN (1 << 24)
> > >> > #define MX3_PWMCR_WAITEN (1 << 23)
> > >> > #define MX3_PWMCR_DBGEN (1 << 22)
> > >> > +#define MX3_PWMCR_POUTC (1 << 18)
> > >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> > >> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> > >> > #define MX3_PWMCR_SWR (1 << 3)
> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > pwm_chip *chip, if (enable)
> > >> >cr |= MX3_PWMCR_EN;
> > >> >
> > >> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > + cr |= MX3_PWMCR_POUTC;
> > >> > +
> > >>
> > >> This seems wrong to me, the config callback is meant for
> > >> period/duty cycle only.
Unfortunately, it also resets the PWM IP block and setups it again (by
writing to PWMCR register). In that function we setup for example
MX3_PWMCR_DOZEEN
and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
I've double checked the backlight and pwm code flow.
Please find following snippet:
[0.135545] # imx_pwm_probe
[0.135581] PWM supports output inversion
[0.136864] # pwm_backlight_probe
[0.136913] backlight supply power not found, using dummy regulator
[0.136984] # imx_pwm_set_polarity 1
[0.136995] imx_pwm_set_polarity: polarity set to inverted cr: 0x4
0xf08f8000
[0.137005] #0 imx_pwm_config_v2 cr: 0x4
[0.137683] #1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
[0.137693] #2 imx_pwm_config_v2 cr: 0x1c20050
[0.137702] #3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
[0.137711] @@ pwm_apply_state
Here the pwm_backlight_probe calls set_polarity callback available in
pwm - the polarity is set (the 0x4 value).
The above operation is performed in pwm_apply_state (@ drivers/pwm/core.c).
In the same function, latter we call the pwm->chip->ops->config(), which is the
pointer to config_v2.
Since the PWM is not yet enabled, this function performs SW reset and
PWM inversion setting is cleared.
Possible solutions:
1. Leave the original patch from Bhuvanchandra as it was (I'm for this
option)
2. Enable early PWM (in core, or in bl driver) so the config_v2 is not
calling SW reset on the PWM. (but this solutions seems _really_ bad to me)
3. Perform defer probe of pwm backlight driver (pwm_bl.c) until the pwm
is fully configured (it might be a bit tricky).
Best regards,
Łukasz Majewski
> > >
> > > If
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Stefan,
> On 2016-10-12 15:15, Lukasz Majewski wrote:
> > Hi Stefan,
> >
> >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> >> > From: Lothar Wassmann
> >> >
> >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> >> > configurable output polarity. This patch adds support to utilize
> >> > this feature where available.
> >> >
> >> > Signed-off-by: Lothar Waßmann
> >> > Signed-off-by: Lukasz Majewski
> >> > Signed-off-by: Bhuvanchandra DV
> >> > Acked-by: Shawn Guo
> >> > Reviewed-by: Sascha Hauer
> >> > ---
> >> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> >> > drivers/pwm/pwm-imx.c | 51
> >> > +-- 2 files changed, 51 insertions(+), 6
> >> > deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > index e00c2e9..c61bdf8 100644
> >> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> >> > @@ -6,8 +6,8 @@ Required properties:
> >> >- "fsl,imx1-pwm" for PWM compatible with the one integrated on
> >> > i.MX1
> >> >- "fsl,imx27-pwm" for PWM compatible with the one integrated
> >> > on i.MX27
> >> > - reg: physical base address and length of the controller's
> >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> >> > directory for a description of
> >> > - the cells format.
> >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> >> > pwm.txt
> >> > + in this directory for a description of the cells format.
> >> > - clocks : Clock specifiers for both ipg and per clocks.
> >> > - clock-names : Clock names should include both "ipg" and "per"
> >> > See the clock consumer binding,
> >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> >> > Example:
> >> >
> >> > pwm1: pwm@53fb4000 {
> >> > -#pwm-cells = <2>;
> >> > +#pwm-cells = <3>;
> >> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> >> > reg = <0x53fb4000 0x4000>;
> >> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > index d600fd5..c37d223 100644
> >> > --- a/drivers/pwm/pwm-imx.c
> >> > +++ b/drivers/pwm/pwm-imx.c
> >> > @@ -38,6 +38,7 @@
> >> > #define MX3_PWMCR_DOZEEN(1 << 24)
> >> > #define MX3_PWMCR_WAITEN(1 << 23)
> >> > #define MX3_PWMCR_DBGEN (1 << 22)
> >> > +#define MX3_PWMCR_POUTC (1 << 18)
> >> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> >> > #define MX3_PWMCR_CLKSRC_IPG(1 << 16)
> >> > #define MX3_PWMCR_SWR (1 << 3)
> >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> >> > *chip, if (enable)
> >> > cr |= MX3_PWMCR_EN;
> >> >
> >> > +if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> >> > +cr |= MX3_PWMCR_POUTC;
> >> > +
> >>
> >> This seems wrong to me, the config callback is meant for
> >> period/duty cycle only.
> >
> > If it is meant only for that, then the polarity should be removed
> > from it.
> >
> > However after very quick testing, at least on my setup, it turns out
> > that removing this lines causes polarity to _not_ being set (and the
> > polarity is not inverted).
> >
> > I will investigate this further on my setup and hopefully sent
> > proper patch.
> >
> >> The set_polarity callback should get called in case a
> >> different polarity is requested.
> >
> > On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
> > pwm_apply_args(), so everything should work. However, as I mentioned
> > above there still is some problem with inversion setting.
> >
> >>
> >>
> >> > writel(cr, imx->mmio_base + MX3_PWMCR);
> >> >
> >> > return 0;
> >> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> >> > *chip, struct pwm_device *pwm)
> >> > clk_disable_unprepare(imx->clk_per);
> >> > }
> >> >
> >> > -static struct pwm_ops imx_pwm_ops = {
> >> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> >> > pwm_device *pwm,
> >> > +enum pwm_polarity polarity)
> >> > +{
> >> > +struct imx_chip *imx = to_imx_chip(chip);
> >> > +u32 val;
> >> > +
> >> > +if (polarity == pwm->args.polarity)
> >> > +return 0;
> >>
> >> I don't think that this is right. Today, pwm_apply_args (in
> >> include/linux/pwm.h) copies the polarity from args to
> >> state.polarity, which is then passed as polarity argument to this
> >> function. So this will always return 0 afaict.
> >
> > Yes, I've overlooked it (that the state is copied).
> >
> > It can be dropped.
>
> Did you do the above test with that line dropped?
Yes. The above code has been also removed.
Best regards,
Łukasz Majewski
>
> >
> >>
> >> I would just drop that.
> >>
> >> There is proba
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On 2016-10-12 15:15, Lukasz Majewski wrote:
> Hi Stefan,
>
>> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
>> > From: Lothar Wassmann
>> >
>> > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
>> > output polarity. This patch adds support to utilize this feature
>> > where available.
>> >
>> > Signed-off-by: Lothar Waßmann
>> > Signed-off-by: Lukasz Majewski
>> > Signed-off-by: Bhuvanchandra DV
>> > Acked-by: Shawn Guo
>> > Reviewed-by: Sascha Hauer
>> > ---
>> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
>> > drivers/pwm/pwm-imx.c | 51
>> > +-- 2 files changed, 51 insertions(+), 6
>> > deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > index e00c2e9..c61bdf8 100644
>> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>> > @@ -6,8 +6,8 @@ Required properties:
>> >- "fsl,imx1-pwm" for PWM compatible with the one integrated on
>> > i.MX1
>> >- "fsl,imx27-pwm" for PWM compatible with the one integrated on
>> > i.MX27
>> > - reg: physical base address and length of the controller's
>> > registers -- #pwm-cells: should be 2. See pwm.txt in this directory
>> > for a description of
>> > - the cells format.
>> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
>> > pwm.txt
>> > + in this directory for a description of the cells format.
>> > - clocks : Clock specifiers for both ipg and per clocks.
>> > - clock-names : Clock names should include both "ipg" and "per"
>> > See the clock consumer binding,
>> > @@ -17,7 +17,7 @@ See the clock consumer binding,
>> > Example:
>> >
>> > pwm1: pwm@53fb4000 {
>> > - #pwm-cells = <2>;
>> > + #pwm-cells = <3>;
>> >compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
>> >reg = <0x53fb4000 0x4000>;
>> >clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
>> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > index d600fd5..c37d223 100644
>> > --- a/drivers/pwm/pwm-imx.c
>> > +++ b/drivers/pwm/pwm-imx.c
>> > @@ -38,6 +38,7 @@
>> > #define MX3_PWMCR_DOZEEN (1 << 24)
>> > #define MX3_PWMCR_WAITEN (1 << 23)
>> > #define MX3_PWMCR_DBGEN (1 << 22)
>> > +#define MX3_PWMCR_POUTC (1 << 18)
>> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
>> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
>> > #define MX3_PWMCR_SWR (1 << 3)
>> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
>> > *chip, if (enable)
>> >cr |= MX3_PWMCR_EN;
>> >
>> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
>> > + cr |= MX3_PWMCR_POUTC;
>> > +
>>
>> This seems wrong to me, the config callback is meant for period/duty
>> cycle only.
>
> If it is meant only for that, then the polarity should be removed from
> it.
>
> However after very quick testing, at least on my setup, it turns out
> that removing this lines causes polarity to _not_ being set (and the
> polarity is not inverted).
>
> I will investigate this further on my setup and hopefully sent proper
> patch.
>
>> The set_polarity callback should get called in case a
>> different polarity is requested.
>
> On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
> pwm_apply_args(), so everything should work. However, as I mentioned
> above there still is some problem with inversion setting.
>
>>
>>
>> >writel(cr, imx->mmio_base + MX3_PWMCR);
>> >
>> >return 0;
>> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
>> > *chip, struct pwm_device *pwm)
>> >clk_disable_unprepare(imx->clk_per);
>> > }
>> >
>> > -static struct pwm_ops imx_pwm_ops = {
>> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
>> > pwm_device *pwm,
>> > + enum pwm_polarity polarity)
>> > +{
>> > + struct imx_chip *imx = to_imx_chip(chip);
>> > + u32 val;
>> > +
>> > + if (polarity == pwm->args.polarity)
>> > + return 0;
>>
>> I don't think that this is right. Today, pwm_apply_args (in
>> include/linux/pwm.h) copies the polarity from args to state.polarity,
>> which is then passed as polarity argument to this function. So this
>> will always return 0 afaict.
>
> Yes, I've overlooked it (that the state is copied).
>
> It can be dropped.
Did you do the above test with that line dropped?
>
>>
>> I would just drop that.
>>
>> There is probably one little problem in the current state of affairs:
>> If the bootloader makes use of a PWM channel with inverted state,
>> then the kernel would not know about that and currently assume a
>> wrong initial state... I guess at one point in time we should
>> implement the state retrieval callback and move to the new atomic PWM
>> API, which would mean to implement apply callback.
>
> Are there any patches on the horizon?
>
Not that I know
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Stefan,
> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > From: Lothar Wassmann
> >
> > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
> > output polarity. This patch adds support to utilize this feature
> > where available.
> >
> > Signed-off-by: Lothar Waßmann
> > Signed-off-by: Lukasz Majewski
> > Signed-off-by: Bhuvanchandra DV
> > Acked-by: Shawn Guo
> > Reviewed-by: Sascha Hauer
> > ---
> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> > drivers/pwm/pwm-imx.c | 51
> > +-- 2 files changed, 51 insertions(+), 6
> > deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > index e00c2e9..c61bdf8 100644
> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > @@ -6,8 +6,8 @@ Required properties:
> >- "fsl,imx1-pwm" for PWM compatible with the one integrated on
> > i.MX1
> >- "fsl,imx27-pwm" for PWM compatible with the one integrated on
> > i.MX27
> > - reg: physical base address and length of the controller's
> > registers -- #pwm-cells: should be 2. See pwm.txt in this directory
> > for a description of
> > - the cells format.
> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> > pwm.txt
> > + in this directory for a description of the cells format.
> > - clocks : Clock specifiers for both ipg and per clocks.
> > - clock-names : Clock names should include both "ipg" and "per"
> > See the clock consumer binding,
> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > Example:
> >
> > pwm1: pwm@53fb4000 {
> > - #pwm-cells = <2>;
> > + #pwm-cells = <3>;
> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > reg = <0x53fb4000 0x4000>;
> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..c37d223 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -38,6 +38,7 @@
> > #define MX3_PWMCR_DOZEEN (1 << 24)
> > #define MX3_PWMCR_WAITEN (1 << 23)
> > #define MX3_PWMCR_DBGEN(1 << 22)
> > +#define MX3_PWMCR_POUTC(1 << 18)
> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> > #define MX3_PWMCR_SWR (1 << 3)
> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (enable)
> > cr |= MX3_PWMCR_EN;
> >
> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > + cr |= MX3_PWMCR_POUTC;
> > +
>
> This seems wrong to me, the config callback is meant for period/duty
> cycle only.
If it is meant only for that, then the polarity should be removed from
it.
However after very quick testing, at least on my setup, it turns out
that removing this lines causes polarity to _not_ being set (and the
polarity is not inverted).
I will investigate this further on my setup and hopefully sent proper
patch.
> The set_polarity callback should get called in case a
> different polarity is requested.
On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
pwm_apply_args(), so everything should work. However, as I mentioned
above there still is some problem with inversion setting.
>
>
> > writel(cr, imx->mmio_base + MX3_PWMCR);
> >
> > return 0;
> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm)
> > clk_disable_unprepare(imx->clk_per);
> > }
> >
> > -static struct pwm_ops imx_pwm_ops = {
> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + u32 val;
> > +
> > + if (polarity == pwm->args.polarity)
> > + return 0;
>
> I don't think that this is right. Today, pwm_apply_args (in
> include/linux/pwm.h) copies the polarity from args to state.polarity,
> which is then passed as polarity argument to this function. So this
> will always return 0 afaict.
Yes, I've overlooked it (that the state is copied).
It can be dropped.
>
> I would just drop that.
>
> There is probably one little problem in the current state of affairs:
> If the bootloader makes use of a PWM channel with inverted state,
> then the kernel would not know about that and currently assume a
> wrong initial state... I guess at one point in time we should
> implement the state retrieval callback and move to the new atomic PWM
> API, which would mean to implement apply callback.
Are there any patches on the horizon?
>
> --
> Stefan
>
>
> > +
> > + val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > + if (polarity == PWM_POLARITY_INVERSED)
> > + val |= MX3_PWMCR_POUTC;
> > + else
> > + val &= ~MX3_PWMCR_POUTC;
> > +
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> From: Lothar Wassmann
>
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output
> polarity. This patch adds support to utilize this feature where available.
>
> Signed-off-by: Lothar Waßmann
> Signed-off-by: Lukasz Majewski
> Signed-off-by: Bhuvanchandra DV
> Acked-by: Shawn Guo
> Reviewed-by: Sascha Hauer
> ---
> Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> drivers/pwm/pwm-imx.c | 51
> +--
> 2 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> index e00c2e9..c61bdf8 100644
> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> @@ -6,8 +6,8 @@ Required properties:
>- "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
>- "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
> - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> - the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
> + in this directory for a description of the cells format.
> - clocks : Clock specifiers for both ipg and per clocks.
> - clock-names : Clock names should include both "ipg" and "per"
> See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
> Example:
>
> pwm1: pwm@53fb4000 {
> - #pwm-cells = <2>;
> + #pwm-cells = <3>;
> compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> reg = <0x53fb4000 0x4000>;
> clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
> #define MX3_PWMCR_DOZEEN (1 << 24)
> #define MX3_PWMCR_WAITEN (1 << 23)
> #define MX3_PWMCR_DBGEN (1 << 22)
> +#define MX3_PWMCR_POUTC (1 << 18)
> #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
> #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> #define MX3_PWMCR_SWR(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> if (enable)
> cr |= MX3_PWMCR_EN;
>
> + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> + cr |= MX3_PWMCR_POUTC;
> +
This seems wrong to me, the config callback is meant for period/duty
cycle only. The set_polarity callback should get called in case a
different polarity is requested.
> writel(cr, imx->mmio_base + MX3_PWMCR);
>
> return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> *chip, struct pwm_device *pwm)
> clk_disable_unprepare(imx->clk_per);
> }
>
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + enum pwm_polarity polarity)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + u32 val;
> +
> + if (polarity == pwm->args.polarity)
> + return 0;
I don't think that this is right. Today, pwm_apply_args (in
include/linux/pwm.h) copies the polarity from args to state.polarity,
which is then passed as polarity argument to this function. So this will
always return 0 afaict.
I would just drop that.
There is probably one little problem in the current state of affairs: If
the bootloader makes use of a PWM channel with inverted state, then the
kernel would not know about that and currently assume a wrong initial
state... I guess at one point in time we should implement the state
retrieval callback and move to the new atomic PWM API, which would mean
to implement apply callback.
--
Stefan
> +
> + val = readl(imx->mmio_base + MX3_PWMCR);
> +
> + if (polarity == PWM_POLARITY_INVERSED)
> + val |= MX3_PWMCR_POUTC;
> + else
> + val &= ~MX3_PWMCR_POUTC;
> +
> + writel(val, imx->mmio_base + MX3_PWMCR);
> +
> + dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> + polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> +
> + return 0;
> +}
> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
> .enable = imx_pwm_enable,
> .disable = imx_pwm_disable,
> .config = imx_pwm_config,
> .owner = THIS_MODULE,
> };
>
> +static struct pwm_ops imx_pwm_ops_v2 = {
> + .enable = imx_pwm_enable,
> + .disable = imx_pwm_disable,
> + .set_polarity = imx_pwm_set_polarity,
> + .config = imx_pwm_config,
> + .owner = THIS_MODULE,
> +};
> +
> struct imx_pwm_data {
> int (*config)(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns);
> void (*set_enable)(struct pwm_chip
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
On Fri, Oct 07, 2016 at 08:41:26PM +0530, Bhuvanchandra DV wrote: > From: Lothar Wassmann > > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable output > polarity. This patch adds support to utilize this feature where available. > > Signed-off-by: Lothar Waßmann > Signed-off-by: Lukasz Majewski > Signed-off-by: Bhuvanchandra DV > Acked-by: Shawn Guo > Reviewed-by: Sascha Hauer > --- > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +-- Acked-by: Rob Herring > drivers/pwm/pwm-imx.c | 51 > +-- > 2 files changed, 51 insertions(+), 6 deletions(-)
Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion
Hi Bhuvanchandra,
> From: Lothar Wassmann
>
> The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
> output polarity. This patch adds support to utilize this feature
> where available.
>
> Signed-off-by: Lothar Waßmann
> Signed-off-by: Lukasz Majewski
> Signed-off-by: Bhuvanchandra DV
> Acked-by: Shawn Guo
> Reviewed-by: Sascha Hauer
I've tested those patches on my iMX6q board on top of v4.7 linux kernel.
Tested-by: Lukasz Majewski
Best regards,
Łukasz Majewski
> ---
> Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> drivers/pwm/pwm-imx.c | 51
> +-- 2 files changed, 51 insertions(+), 6
> deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> e00c2e9..c61bdf8 100644 ---
> a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8 +6,8 @@
> Required properties:
>- "fsl,imx1-pwm" for PWM compatible with the one integrated on
> i.MX1
>- "fsl,imx27-pwm" for PWM compatible with the one integrated on
> i.MX27
> - reg: physical base address and length of the controller's registers
> -- #pwm-cells: should be 2. See pwm.txt in this directory for a
> description of
> - the cells format.
> +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> pwm.txt
> + in this directory for a description of the cells format.
> - clocks : Clock specifiers for both ipg and per clocks.
> - clock-names : Clock names should include both "ipg" and "per"
> See the clock consumer binding,
> @@ -17,7 +17,7 @@ See the clock consumer binding,
> Example:
>
> pwm1: pwm@53fb4000 {
> - #pwm-cells = <2>;
> + #pwm-cells = <3>;
> compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> reg = <0x53fb4000 0x4000>;
> clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..c37d223 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -38,6 +38,7 @@
> #define MX3_PWMCR_DOZEEN (1 << 24)
> #define MX3_PWMCR_WAITEN (1 << 23)
> #define MX3_PWMCR_DBGEN (1 << 22)
> +#define MX3_PWMCR_POUTC (1 << 18)
> #define MX3_PWMCR_CLKSRC_IPG_HIGH(2 << 16)
> #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> #define MX3_PWMCR_SWR(1 << 3)
> @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> *chip, if (enable)
> cr |= MX3_PWMCR_EN;
>
> + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> + cr |= MX3_PWMCR_POUTC;
> +
> writel(cr, imx->mmio_base + MX3_PWMCR);
>
> return 0;
> @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> *chip, struct pwm_device *pwm) clk_disable_unprepare(imx->clk_per);
> }
>
> -static struct pwm_ops imx_pwm_ops = {
> +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + u32 val;
> +
> + if (polarity == pwm->args.polarity)
> + return 0;
> +
> + val = readl(imx->mmio_base + MX3_PWMCR);
> +
> + if (polarity == PWM_POLARITY_INVERSED)
> + val |= MX3_PWMCR_POUTC;
> + else
> + val &= ~MX3_PWMCR_POUTC;
> +
> + writel(val, imx->mmio_base + MX3_PWMCR);
> +
> + dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> + polarity == PWM_POLARITY_INVERSED ? "inverted" :
> "normal"); +
> + return 0;
> +}
> +
> +static struct pwm_ops imx_pwm_ops_v1 = {
> .enable = imx_pwm_enable,
> .disable = imx_pwm_disable,
> .config = imx_pwm_config,
> .owner = THIS_MODULE,
> };
>
> +static struct pwm_ops imx_pwm_ops_v2 = {
> + .enable = imx_pwm_enable,
> + .disable = imx_pwm_disable,
> + .set_polarity = imx_pwm_set_polarity,
> + .config = imx_pwm_config,
> + .owner = THIS_MODULE,
> +};
> +
> struct imx_pwm_data {
> int (*config)(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns);
> void (*set_enable)(struct pwm_chip *chip, bool enable);
> + struct pwm_ops *pwm_ops;
> };
>
> static struct imx_pwm_data imx_pwm_data_v1 = {
> .config = imx_pwm_config_v1,
> .set_enable = imx_pwm_set_enable_v1,
> + .pwm_ops = &imx_pwm_ops_v1,
> };
>
> static struct imx_pwm_data imx_pwm_data_v2 = {
> .config = imx_pwm_config_v2,
> .set_enable = imx_pwm_set_enable_v2,
> + .pwm_ops = &imx_pwm_ops_v2,
> };
>
> static const struct of_device_id imx_pwm_dt_ids[] = {
> @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device
> *pdev) if (!of_id)
> return -ENODEV;
>
> + data = of_id->data;
> +
> imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> if (imx =
