Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski <l.majewski@majess.pl>]

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

Hi Stefan,

> On 2016-12-26 23:55, Lukasz Majewski wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > The use of the ipg clock was introduced with commit 7b27c160c681
> > ("pwm: i.MX: fix clock lookup").
> > In the commit message it was claimed that the ipg clock is enabled
> > for register accesses. This is true for the ->config() callback,
> > but not for the ->set_enable() callback. Given that the ipg clock
> > is not consistently enabled for all register accesses we can assume
> > that either it is not required at all or that the current code does
> > not work. Remove the ipg clock code for now so that it's no longer
> > in the way of refactoring the driver.
> 
> Hi Lukasz,
> 
> Has my concern addressed in any way with this resend?
> https://lkml.org/lkml/2016/11/22/729

Unfortunately not, since I don't have iMX7 for testing.

> 
> Breaking hardware is usually not an option :-)

Yes, I know, but

Please look on the patch set from my perspective:

I originally wanted to add polarity inversion to PWM. Then, there was
the request from you and Boris to go with "atomicity" support, so I
converted the driver to support it.

This patch set has been resent on purpose at the end of merge window,
so we do have some time to fix it if it would be accepted to -next
tree (or any other PWM related one). Moreover, the burden for preparing
patches would be smaller - since we all have agreed that "atomicity" is
a more than welcome feature.


> 
> I checked the i.MX 7 reference manual again, and in this case the
> peripheral access clock is a clock line named "ipg_clk_s" (Table
> 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 all
> clocks are behind a single gate, so in fact it does not matter which
> clock we take. Given that others have peripheral access behind the
> "pwm" gate, I guess we should take the "pwm" gate...


If possible please prepare a patch. It would be the best solution.

Thanks in advance,
Łukasz Majewski

> 
> --
> Stefan
> 
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > [commit message text refactored by Lukasz Majewski
> > <l.majewski@majess.pl>] ---
> > Changes for v3:
> > - New patch
> > ---
> >  drivers/pwm/pwm-imx.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..70609ef2 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -49,7 +49,6 @@
> >  
> >  struct imx_chip {
> >  	struct clk	*clk_per;
> > -	struct clk	*clk_ipg;
> >  
> >  	void __iomem	*mmio_base;
> >  
> > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> >  {
> >  	struct imx_chip *imx = to_imx_chip(chip);
> > -	int ret;
> > -
> > -	ret = clk_prepare_enable(imx->clk_ipg);
> > -	if (ret)
> > -		return ret;
> >  
> > -	ret = imx->config(chip, pwm, duty_ns, period_ns);
> > -
> > -	clk_disable_unprepare(imx->clk_ipg);
> > -
> > -	return ret;
> > +	return imx->config(chip, pwm, duty_ns, period_ns);
> >  }
> >  
> >  static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > *pwm) @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_per);
> >  	}
> >  
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > -	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with
> > %ld\n",
> > -				PTR_ERR(imx->clk_ipg));
> > -		return PTR_ERR(imx->clk_ipg);
> > -	}
> > -
> >  	imx->chip.ops = &imx_pwm_ops;
> >  	imx->chip.dev = &pdev->dev;
> >  	imx->chip.base = -1;


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