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

From: Lukasz Majewski <l.majewski@majess.pl>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Stefan Agner <stefan@agner.ch>,
	linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Fabio Estevam <fabio.estevam@nxp.com>,
	Fabio Estevam <festevam@gmail.com>,
	Lothar Wassmann <LW@karo-electronics.de>,
	Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>,
	kernel@pengutronix.de
Subject: Re: [PATCH 2/6] pwm: imx: Move PWMv2 software reset code to a separate function
Date: Mon, 24 Oct 2016 23:02:32 +0200	[thread overview]
Message-ID: <20161024230232.3527fd42@jawa> (raw)
In-Reply-To: <20161024172322.3924eb8a@bbrezillon>

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

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]