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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161227085401.0115add5@jawa>
Date:   Tue, 27 Dec 2016 08:54:01 +0100
From:   Lukasz Majewski <l.majewski@...ess.pl>
To:     Stefan Agner <stefan@...er.ch>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        linux-pwm@...r.kernel.org,
        Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
        linux-kernel@...r.kernel.org,
        Fabio Estevam <fabio.estevam@....com>,
        Fabio Estevam <festevam@...il.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Lothar Wassmann <LW@...o-electronics.de>,
        kernel@...gutronix.de, Philipp Zabel <p.zabel@...gutronix.de>
Subject: Re: [PATCH v3 RESEND 02/11] pwm: imx: remove ipg clock

Hi Stefan,

> On 2016-12-26 23:55, Lukasz Majewski wrote:
> > From: Sascha Hauer <s.hauer@...gutronix.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@...gutronix.de>
> > Cc: Philipp Zabel <p.zabel@...gutronix.de>
> > ---
> > [commit message text refactored by Lukasz Majewski
> > <l.majewski@...ess.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;


Content of type "application/pgp-signature" skipped