Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1506781 > unrolled thread

[PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 00:10 +0200
    [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 00:10 +0200
      Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a  separate function Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-24 17:30 +0200
        Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a  separate function Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 23:10 +0200
    [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 00:30 +0200
      Re: [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a  separate function Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-24 17:30 +0200
    Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM  driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-10-24 17:40 +0200
      Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM  driver Lukasz Majewski <l.majewski@majess.pl> - 2016-10-24 23:30 +0200
        Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Stefan Agner <stefan@agner.ch> - 2016-10-25 05:50 +0200
          Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM  driver Lukasz Majewski <l.majewski@majess.pl> - 2016-10-25 09:10 +0200
            Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Fabio Estevam <festevam@gmail.com> - 2016-10-25 19:10 +0200
              Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver Fabio Estevam <festevam@gmail.com> - 2016-10-25 19:10 +0200
                Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM  driver Lukasz Majewski <l.majewski@majess.pl> - 2016-10-27 09:20 +0200

#1506781 — [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

This patch set brings atomic operation to i.MX's PWMv2 driver.

This work has been supported and suggested by Boris Brezillon [1] and 
Stefan Agner, by showing how simple the transition could be :-).

It has been divided into several steps:
- Separate PWMv1 commits from "generic" and non atomic PWM code.

  NOTE: Since I do not have board with PWMv1, I would like to ask somebody
  	for testing

- Move some imx_config_v2 code to separate functions

- Provide PWM atomic implementation (the ->apply() driver) in a single patch
  for better readability.

- Remove redundant PWM code (disable, enable, config callbacks)

- Clean up the driver infrastructure

- Provide "polarity_supported" flag to indicate support for polarity 
  inversion

This work should be applied on top of following commits:

http://patchwork.ozlabs.org/patch/679706/
http://patchwork.ozlabs.org/patch/679707/
http://patchwork.ozlabs.org/patch/679680/


Test HW:
--------
This patch set has been tested on i.MX6q board with vanilla 4.7 kernel.
It applies clearly on 4.9-rcX SHA1: 0c2b6dc4fd4fa13796b319aae969a009f03222c6


The PWM operation has been tested with pwm_bl backlight driver by changing
its brightness.

[1]: http://patchwork.ozlabs.org/patch/685402/


Lukasz Majewski (6):
  pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm
    operation
  pwm: imx: Move PWMv2 software reset code to a separate function
  pwm: imx: Move PWMv2 wait for fifo slot code to a separate function
  pwm: imx: Provide atomic PWM support for IMXv2 PWM
  pwm: imx: Remove redundant IMX PWMv2 code
  pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver

 drivers/pwm/pwm-imx.c | 262 ++++++++++++++++++++++----------------------------
 1 file changed, 114 insertions(+), 148 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1506782 — [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

The software reset code has been extracted from imx_pwm_config_v2 function
and moved to new one - imx_pwm_sw_reset().

This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
 drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 83e43d5..fac5c93 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
 	clk_disable_unprepare(imx->clk_per);
 }
 
+static void imx_pwm_sw_reset(struct pwm_chip *chip)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct device *dev = chip->dev;
+	int wait_count = 0;
+	u32 cr;
+
+	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
+	do {
+		usleep_range(200, 1000);
+		cr = readl(imx->mmio_base + MX3_PWMCR);
+	} while ((cr & MX3_PWMCR_SWR) &&
+		 (wait_count++ < MX3_PWM_SWR_LOOP));
+
+	if (cr & MX3_PWMCR_SWR)
+		dev_warn(dev, "software reset timeout\n");
+}
+
+
 static int imx_pwm_config_v2(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
@@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	unsigned long period_cycles, duty_cycles, prescale;
 	unsigned int period_ms;
 	bool enable = pwm_is_enabled(pwm);
-	int wait_count = 0, fifoav;
+	int fifoav;
 	u32 cr, sr;
 
 	/*
@@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
 				dev_warn(dev, "there is no free FIFO slot\n");
 		}
-	} else {
-		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
-		do {
-			usleep_range(200, 1000);
-			cr = readl(imx->mmio_base + MX3_PWMCR);
-		} while ((cr & MX3_PWMCR_SWR) &&
-			 (wait_count++ < MX3_PWM_SWR_LOOP));
-
-		if (cr & MX3_PWMCR_SWR)
-			dev_warn(dev, "software reset timeout\n");
-	}
+	} else
+		imx_pwm_sw_reset(chip);
 
 	c = clk_get_rate(imx->clk_per);
 	c = c * period_ns;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1507306 — Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

On Sun, 23 Oct 2016 23:45:42 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The software reset code has been extracted from imx_pwm_config_v2 function
> and moved to new one - imx_pwm_sw_reset().
> 
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Just a nit below ;).

BTW, can't you just merge path 2 and 3?

> ---
>  drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 83e43d5..fac5c93 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clk_disable_unprepare(imx->clk_per);
>  }
>  
> +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	struct device *dev = chip->dev;
> +	int wait_count = 0;
> +	u32 cr;
> +
> +	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> +	do {
> +		usleep_range(200, 1000);
> +		cr = readl(imx->mmio_base + MX3_PWMCR);
> +	} while ((cr & MX3_PWMCR_SWR) &&
> +		 (wait_count++ < MX3_PWM_SWR_LOOP));
> +
> +	if (cr & MX3_PWMCR_SWR)
> +		dev_warn(dev, "software reset timeout\n");
> +}
> +
> +
>  static int imx_pwm_config_v2(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> @@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	unsigned long period_cycles, duty_cycles, prescale;
>  	unsigned int period_ms;
>  	bool enable = pwm_is_enabled(pwm);
> -	int wait_count = 0, fifoav;
> +	int fifoav;
>  	u32 cr, sr;
>  
>  	/*
> @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
>  				dev_warn(dev, "there is no free FIFO slot\n");
>  		}
> -	} else {
> -		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> -		do {
> -			usleep_range(200, 1000);
> -			cr = readl(imx->mmio_base + MX3_PWMCR);
> -		} while ((cr & MX3_PWMCR_SWR) &&
> -			 (wait_count++ < MX3_PWM_SWR_LOOP));
> -
> -		if (cr & MX3_PWMCR_SWR)
> -			dev_warn(dev, "software reset timeout\n");
> -	}
> +	} else
> +		imx_pwm_sw_reset(chip);

	} else {
		imx_pwm_sw_reset(chip);
	}

>  
>  	c = clk_get_rate(imx->clk_per);
>  	c = c * period_ns;

[toc] | [prev] | [next] | [standalone]

#1507739 — Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function

[Multipart message — attachments visible in raw view] — view raw

Hi Boris,

> On Sun, 23 Oct 2016 23:45:42 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > The software reset code has been extracted from imx_pwm_config_v2
> > function and moved to new one - imx_pwm_sw_reset().
> > 
> > This change reduces the overall size of imx_pwm_config_v2() and
> > prepares it for atomic PWM operation.
> > 
> > Suggested-by: Stefan Agner <stefan@agner.ch>
> > Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Just a nit below ;).
> 
> BTW, can't you just merge path 2 and 3?

I do prefer to have many small patches touching and changing just one
thing.

> 
> > ---
> >  drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 83e43d5..fac5c93 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip
> > *chip, struct pwm_device *pwm) clk_disable_unprepare(imx->clk_per);
> >  }
> >  
> > +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> > +{
> > +	struct imx_chip *imx = to_imx_chip(chip);
> > +	struct device *dev = chip->dev;
> > +	int wait_count = 0;
> > +	u32 cr;
> > +
> > +	writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> > +	do {
> > +		usleep_range(200, 1000);
> > +		cr = readl(imx->mmio_base + MX3_PWMCR);
> > +	} while ((cr & MX3_PWMCR_SWR) &&
> > +		 (wait_count++ < MX3_PWM_SWR_LOOP));
> > +
> > +	if (cr & MX3_PWMCR_SWR)
> > +		dev_warn(dev, "software reset timeout\n");
> > +}
> > +
> > +
> >  static int imx_pwm_config_v2(struct pwm_chip *chip,
> >  		struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> > @@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, unsigned long period_cycles, duty_cycles, prescale;
> >  	unsigned int period_ms;
> >  	bool enable = pwm_is_enabled(pwm);
> > -	int wait_count = 0, fifoav;
> > +	int fifoav;
> >  	u32 cr, sr;
> >  
> >  	/*
> > @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> >  				dev_warn(dev, "there is no free
> > FIFO slot\n"); }
> > -	} else {
> > -		writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> > -		do {
> > -			usleep_range(200, 1000);
> > -			cr = readl(imx->mmio_base + MX3_PWMCR);
> > -		} while ((cr & MX3_PWMCR_SWR) &&
> > -			 (wait_count++ < MX3_PWM_SWR_LOOP));
> > -
> > -		if (cr & MX3_PWMCR_SWR)
> > -			dev_warn(dev, "software reset timeout\n");
> > -	}
> > +	} else
> > +		imx_pwm_sw_reset(chip);
> 
> 	} else {
> 		imx_pwm_sw_reset(chip);
> 	}

OK.

> 
> >  
> >  	c = clk_get_rate(imx->clk_per);
> >  	c = c * period_ns;
> 

Best regards,

Łukasz Majewski

[toc] | [prev] | [next] | [standalone]

#1506786 — [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

The code, which waits for fifo slot, has been extracted from
imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().

This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.

Suggested-by: Stefan Agner <stefan@agner.ch>
Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
 drivers/pwm/pwm-imx.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index fac5c93..f3577c5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -149,18 +149,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
 		dev_warn(dev, "software reset timeout\n");
 }
 
+static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
+				   struct pwm_device *pwm)
+{
+	struct imx_chip *imx = to_imx_chip(chip);
+	struct device *dev = chip->dev;
+	unsigned int period_ms;
+	int fifoav;
+	u32 sr;
+
+	sr = readl(imx->mmio_base + MX3_PWMSR);
+	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+					 NSEC_PER_MSEC);
+		msleep(period_ms);
+
+		sr = readl(imx->mmio_base + MX3_PWMSR);
+		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+			dev_warn(dev, "there is no free FIFO slot\n");
+	}
+}
 
 static int imx_pwm_config_v2(struct pwm_chip *chip,
 		struct pwm_device *pwm, int duty_ns, int period_ns)
 {
 	struct imx_chip *imx = to_imx_chip(chip);
-	struct device *dev = chip->dev;
 	unsigned long long c;
 	unsigned long period_cycles, duty_cycles, prescale;
-	unsigned int period_ms;
 	bool enable = pwm_is_enabled(pwm);
-	int fifoav;
-	u32 cr, sr;
+	u32 cr;
 
 	/*
 	 * i.MX PWMv2 has a 4-word sample FIFO.
@@ -169,19 +187,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
 	 * wait for a full PWM cycle to get a relinquished FIFO slot
 	 * when the controller is enabled and the FIFO is fully loaded.
 	 */
-	if (enable) {
-		sr = readl(imx->mmio_base + MX3_PWMSR);
-		fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
-		if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
-			period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
-						 NSEC_PER_MSEC);
-			msleep(period_ms);
-
-			sr = readl(imx->mmio_base + MX3_PWMSR);
-			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
-				dev_warn(dev, "there is no free FIFO slot\n");
-		}
-	} else
+	if (enable)
+		imx_pwm_wait_fifo_slot(chip, pwm);
+	else
 		imx_pwm_sw_reset(chip);
 
 	c = clk_get_rate(imx->clk_per);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1507311 — Re: [PATCH 3/6] pwm: imx: Move PWMv2 wait for fifo slot code to a separate function

On Sun, 23 Oct 2016 23:45:43 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> The code, which waits for fifo slot, has been extracted from
> imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().
> 
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
> 
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/pwm/pwm-imx.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index fac5c93..f3577c5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -149,18 +149,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
>  		dev_warn(dev, "software reset timeout\n");
>  }
>  
> +static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> +				   struct pwm_device *pwm)
> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	struct device *dev = chip->dev;
> +	unsigned int period_ms;
> +	int fifoav;
> +	u32 sr;
> +
> +	sr = readl(imx->mmio_base + MX3_PWMSR);
> +	fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> +	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> +		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
> +					 NSEC_PER_MSEC);
> +		msleep(period_ms);
> +
> +		sr = readl(imx->mmio_base + MX3_PWMSR);
> +		if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> +			dev_warn(dev, "there is no free FIFO slot\n");
> +	}
> +}
>  
>  static int imx_pwm_config_v2(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
>  	struct imx_chip *imx = to_imx_chip(chip);
> -	struct device *dev = chip->dev;
>  	unsigned long long c;
>  	unsigned long period_cycles, duty_cycles, prescale;
> -	unsigned int period_ms;
>  	bool enable = pwm_is_enabled(pwm);
> -	int fifoav;
> -	u32 cr, sr;
> +	u32 cr;
>  
>  	/*
>  	 * i.MX PWMv2 has a 4-word sample FIFO.
> @@ -169,19 +187,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>  	 * wait for a full PWM cycle to get a relinquished FIFO slot
>  	 * when the controller is enabled and the FIFO is fully loaded.
>  	 */
> -	if (enable) {
> -		sr = readl(imx->mmio_base + MX3_PWMSR);
> -		fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> -		if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> -			period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
> -						 NSEC_PER_MSEC);
> -			msleep(period_ms);
> -
> -			sr = readl(imx->mmio_base + MX3_PWMSR);
> -			if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> -				dev_warn(dev, "there is no free FIFO slot\n");
> -		}
> -	} else
> +	if (enable)
> +		imx_pwm_wait_fifo_slot(chip, pwm);
> +	else
>  		imx_pwm_sw_reset(chip);
>  
>  	c = clk_get_rate(imx->clk_per);

[toc] | [prev] | [next] | [standalone]

#1507319 — Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

On Sun, 23 Oct 2016 23:45:40 +0200
Lukasz Majewski <l.majewski@majess.pl> wrote:

> This patch set brings atomic operation to i.MX's PWMv2 driver.
> 
> This work has been supported and suggested by Boris Brezillon [1] and 
> Stefan Agner, by showing how simple the transition could be :-).
> 
> It has been divided into several steps:
> - Separate PWMv1 commits from "generic" and non atomic PWM code.
> 
>   NOTE: Since I do not have board with PWMv1, I would like to ask somebody
>   	for testing
> 
> - Move some imx_config_v2 code to separate functions
> 
> - Provide PWM atomic implementation (the ->apply() driver) in a single patch
>   for better readability.
> 
> - Remove redundant PWM code (disable, enable, config callbacks)
> 
> - Clean up the driver infrastructure
> 
> - Provide "polarity_supported" flag to indicate support for polarity 
>   inversion
> 
> This work should be applied on top of following commits:
> 
> http://patchwork.ozlabs.org/patch/679706/
> http://patchwork.ozlabs.org/patch/679707/
> http://patchwork.ozlabs.org/patch/679680/

I'm not sure I follow the logic here. Has patch [1] already been
applied? If that's not the case, then you should just drop it and put
your changes on top of mainline.

[1]http://patchwork.ozlabs.org/patch/679680/

[toc] | [prev] | [next] | [standalone]

#1507747 — Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Multipart message — attachments visible in raw view] — view raw

Hi Boris,

> On Sun, 23 Oct 2016 23:45:40 +0200
> Lukasz Majewski <l.majewski@majess.pl> wrote:
> 
> > This patch set brings atomic operation to i.MX's PWMv2 driver.
> > 
> > This work has been supported and suggested by Boris Brezillon [1]
> > and Stefan Agner, by showing how simple the transition could be :-).
> > 
> > It has been divided into several steps:
> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
> > 
> >   NOTE: Since I do not have board with PWMv1, I would like to ask
> > somebody for testing
> > 
> > - Move some imx_config_v2 code to separate functions
> > 
> > - Provide PWM atomic implementation (the ->apply() driver) in a
> > single patch for better readability.
> > 
> > - Remove redundant PWM code (disable, enable, config callbacks)
> > 
> > - Clean up the driver infrastructure
> > 
> > - Provide "polarity_supported" flag to indicate support for
> > polarity inversion
> > 
> > This work should be applied on top of following commits:
> > 
> > http://patchwork.ozlabs.org/patch/679706/ 
[2]

> > http://patchwork.ozlabs.org/patch/679707/
[3]

> > http://patchwork.ozlabs.org/patch/679680/
> 
> I'm not sure I follow the logic here. Has patch [1] already been
> applied? If that's not the case, then you should just drop it and put
> your changes on top of mainline.
> 
> [1]http://patchwork.ozlabs.org/patch/679680/

Patches [2] and [3] have been developed initially by Lothar and
subsequently picked up by Bhuvanchandra. There is no issue with them.

The patch [1] is a bit more tricky. The work has been done by
Bhuvanchandra, which adds DTS and core support for polarity inversion.

This code works and utilizes the "old" PWM API with enable, disable and
config. However, Stefan had some comments about the placement for the
polarity setting (in the .config_v2()) and proposed switch to atomic
API.

To make things easier and cleaner, I decided to put my atomic API
rework on top of those patches. In this way I can credit the previous
work and avoid rewriting DTS polarity inversion code already developed
and validated by Bhuvanchandra.



Best regards,
Łukasz Majewski

[toc] | [prev] | [next] | [standalone]

#1507956

Hi Lukasz,

Thanks for your work, great to see this coming along! :-)

On 2016-10-24 23:26, Lukasz Majewski wrote:
> Hi Boris,
> 
>> On Sun, 23 Oct 2016 23:45:40 +0200
>> Lukasz Majewski <l.majewski@majess.pl> wrote:
>>
>> > This patch set brings atomic operation to i.MX's PWMv2 driver.
>> >
>> > This work has been supported and suggested by Boris Brezillon [1]
>> > and Stefan Agner, by showing how simple the transition could be :-).
>> >
>> > It has been divided into several steps:
>> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
>> >
>> >   NOTE: Since I do not have board with PWMv1, I would like to ask
>> > somebody for testing
>> >
>> > - Move some imx_config_v2 code to separate functions
>> >
>> > - Provide PWM atomic implementation (the ->apply() driver) in a
>> > single patch for better readability.
>> >
>> > - Remove redundant PWM code (disable, enable, config callbacks)
>> >
>> > - Clean up the driver infrastructure
>> >
>> > - Provide "polarity_supported" flag to indicate support for
>> > polarity inversion
>> >
>> > This work should be applied on top of following commits:
>> >
>> > http://patchwork.ozlabs.org/patch/679706/
> [2]
> 
>> > http://patchwork.ozlabs.org/patch/679707/
> [3]
> 
>> > http://patchwork.ozlabs.org/patch/679680/
>>
>> I'm not sure I follow the logic here. Has patch [1] already been
>> applied? If that's not the case, then you should just drop it and put
>> your changes on top of mainline.
>>
>> [1]http://patchwork.ozlabs.org/patch/679680/
> 
> Patches [2] and [3] have been developed initially by Lothar and
> subsequently picked up by Bhuvanchandra. There is no issue with them.

As such none of this will get merged since all patchset have known
flaws...

Generally, it is ok to refer to other patchset being a prerequisite, but
that only makes sense if those patch set are still actively worked on
(by somebody other than you).

In this case I really recommend to create a new, complete patchset.

> 
> The patch [1] is a bit more tricky. The work has been done by
> Bhuvanchandra, which adds DTS and core support for polarity inversion.
> 
> This code works and utilizes the "old" PWM API with enable, disable and
> config. However, Stefan had some comments about the placement for the
> polarity setting (in the .config_v2()) and proposed switch to atomic
> API.

Part of the reason I advocated for the atomic API is to make adding the
polarity functionality easier. It does not archive this goal if we add
the "flawed" code first and then transition to the atomic API.

> 
> To make things easier and cleaner, I decided to put my atomic API
> rework on top of those patches. In this way I can credit the previous
> work and avoid rewriting DTS polarity inversion code already developed
> and validated by Bhuvanchandra.

When you apply the patches using git apply, the authorship and signoffs
will stay. There is no problem in including other peoples work into your
patchset, credit will still be given. If you have to change another
persons patch, you typically also add your signoff to show that you
worked on it too.

Here is how I would do it:

1. Start a new branch from mainline (or even -next).
2. Implement the transition to the new atomic API and test it as such
alone (this way we have no polarity support influence yet, just clean
transition to a new API)
3. Cherry pick the PWM core changes for the optional 2/3 args driver
support (they should apply cleanly)
4. Cherry pick (they likely will fail to merge) or reimplement the PWM
polarity driver changes on top of atomic API
5. Cherry pick device tree changes

With this approach we'll end up with a nice history where we should end
up with a fully functional PWM system between every patch.

Btw, past perfect tense is not really usual in commit messages.
SubmittingPatches chapter 2 has some tips on writing good commit
messages:
https://www.kernel.org/doc/Documentation/SubmittingPatches

--
Stefan

[toc] | [prev] | [next] | [standalone]

#1508019 — Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Multipart message — attachments visible in raw view] — view raw

Hi Stefan,

> Hi Lukasz,
> 
> Thanks for your work, great to see this coming along! :-)
> 
> On 2016-10-24 23:26, Lukasz Majewski wrote:
> > Hi Boris,
> > 
> >> On Sun, 23 Oct 2016 23:45:40 +0200
> >> Lukasz Majewski <l.majewski@majess.pl> wrote:
> >>
> >> > This patch set brings atomic operation to i.MX's PWMv2 driver.
> >> >
> >> > This work has been supported and suggested by Boris Brezillon [1]
> >> > and Stefan Agner, by showing how simple the transition could
> >> > be :-).
> >> >
> >> > It has been divided into several steps:
> >> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
> >> >
> >> >   NOTE: Since I do not have board with PWMv1, I would like to ask
> >> > somebody for testing
> >> >
> >> > - Move some imx_config_v2 code to separate functions
> >> >
> >> > - Provide PWM atomic implementation (the ->apply() driver) in a
> >> > single patch for better readability.
> >> >
> >> > - Remove redundant PWM code (disable, enable, config callbacks)
> >> >
> >> > - Clean up the driver infrastructure
> >> >
> >> > - Provide "polarity_supported" flag to indicate support for
> >> > polarity inversion
> >> >
> >> > This work should be applied on top of following commits:
> >> >
> >> > http://patchwork.ozlabs.org/patch/679706/
> > [2]
> > 
> >> > http://patchwork.ozlabs.org/patch/679707/
> > [3]
> > 
> >> > http://patchwork.ozlabs.org/patch/679680/
> >>
> >> I'm not sure I follow the logic here. Has patch [1] already been
> >> applied? If that's not the case, then you should just drop it and
> >> put your changes on top of mainline.
> >>
> >> [1]http://patchwork.ozlabs.org/patch/679680/
> > 
> > Patches [2] and [3] have been developed initially by Lothar and
> > subsequently picked up by Bhuvanchandra. There is no issue with
> > them.
> 
> As such none of this will get merged since all patchset have known
> flaws...
> 
> Generally, it is ok to refer to other patchset being a prerequisite,
> but that only makes sense if those patch set are still actively
> worked on (by somebody other than you).
> 
> In this case I really recommend to create a new, complete patchset.
> 
> > 
> > The patch [1] is a bit more tricky. The work has been done by
> > Bhuvanchandra, which adds DTS and core support for polarity
> > inversion.
> > 
> > This code works and utilizes the "old" PWM API with enable, disable
> > and config. However, Stefan had some comments about the placement
> > for the polarity setting (in the .config_v2()) and proposed switch
> > to atomic API.
> 
> Part of the reason I advocated for the atomic API is to make adding
> the polarity functionality easier. It does not archive this goal if
> we add the "flawed" code first and then transition to the atomic API.
> 
> > 
> > To make things easier and cleaner, I decided to put my atomic API
> > rework on top of those patches. In this way I can credit the
> > previous work and avoid rewriting DTS polarity inversion code
> > already developed and validated by Bhuvanchandra.
> 
> When you apply the patches using git apply, the authorship and
> signoffs will stay. There is no problem in including other peoples
> work into your patchset, credit will still be given. If you have to
> change another persons patch, you typically also add your signoff to
> show that you worked on it too.

I do wanted to reuse as much work as possible (especially that the code
was working).

> 
> Here is how I would do it:
> 
> 1. Start a new branch from mainline (or even -next).

With the newest mainline 4.9-rcX
SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6

the i.MX6q is not booting. Apparently I do need to wait for things to
calm down.

The last working version is v4.8, which PWM's code is the same as v4.7.

> 2. Implement the transition to the new atomic API and test it as such
> alone (this way we have no polarity support influence yet, just clean
> transition to a new API)

I _just_ needed to add polarity support (by setting one bit) to the
driver, so I ended up with rewriting the whole PWM i.MX driver :-).

> 3. Cherry pick the PWM core changes for the optional 2/3 args driver
> support (they should apply cleanly)
> 4. Cherry pick (they likely will fail to merge) or reimplement the PWM
> polarity driver changes on top of atomic API
> 5. Cherry pick device tree changes
> 
> With this approach we'll end up with a nice history where we should
> end up with a fully functional PWM system between every patch.

I'm fine with proposed approach. I will prepare v2 of patches soon.

> 
> Btw, past perfect tense is not really usual in commit messages.
> SubmittingPatches chapter 2 has some tips on writing good commit
> messages:
> https://www.kernel.org/doc/Documentation/SubmittingPatches

OK.

Thanks you for your support,

Best regards,

Łukasz Majewski

> 
> --
> Stefan

[toc] | [prev] | [next] | [standalone]

#1508486

Hi Lukasz,

On Tue, Oct 25, 2016 at 5:07 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:

> With the newest mainline 4.9-rcX
> SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6
>
> the i.MX6q is not booting. Apparently I do need to wait for things to
> calm down.

Does this commit help with the boot issue you observed?

[toc] | [prev] | [next] | [standalone]

#1508489

On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <festevam@gmail.com> wrote:

>> the i.MX6q is not booting. Apparently I do need to wait for things to
>> calm down.
>
> Does this commit help with the boot issue you observed?

Sorry, missed to put the commit. Here it goes:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804

[toc] | [prev] | [next] | [standalone]

#1510108 — Re: [PATCH 0/6] pwm: imx: Provide atomic operation for IMX PWM driver

[Multipart message — attachments visible in raw view] — view raw

On Tue, 25 Oct 2016 15:09:03 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <festevam@gmail.com>
> wrote:
> 
> >> the i.MX6q is not booting. Apparently I do need to wait for things
> >> to calm down.
> >
> > Does this commit help with the boot issue you observed?
> 
> Sorry, missed to put the commit. Here it goes:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804

I can confirm that the above patch fixed the problem.

My iMX6q board boots on 4.9.0-rc2

Thanks for support,

Łukasz Majewski

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web