RE: [PATCH] pwm: tegra: dynamic clk freq configuration by PWM driver

[Sandipan Patra]

Hi Uwe,

Thanks for reviewing and providing your inputs to the changes.

The review comments are addressed in the new patch.
Patch (V2) is ready to be reviewed at:
https://patchwork.ozlabs.org/project/linux-pwm/patch/1587398043-18767-1-git-send-email-spatra@xxxxxxxxxx/
Please help reviewing further.


Thanks & Regards,
Sandipan


> -----Original Message-----
> From: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Saturday, April 18, 2020 1:53 PM
> To: Sandipan Patra <spatra@xxxxxxxxxx>
> Cc: Thierry Reding <treding@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; Jonathan
> Hunter <jonathanh@xxxxxxxxxx>; Bibek Basu <bbasu@xxxxxxxxxx>; Laxman
> Dewangan <ldewangan@xxxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] pwm: tegra: dynamic clk freq configuration by PWM driver
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello,
> 
> On Fri, Apr 17, 2020 at 02:53:22PM +0000, Sandipan Patra wrote:
> > > To put my expression in words: pick the maximum of the possible
> > > periods that are less or equal to the requested value.  Maybe this
> > > is better
> > > understandable:
> > >
> > >         max { x â implementablePeriods | x <= requestedPeriod }
> > >
> > > ?
> >
> > I think I got your question.
> > Should tegra_pwm_config() not return error (EINVAL) when the requested
> > period is invalid but it should configure to a nearest possible value?
> 
> If you cannot configure according to the above rule, yes, return an error code.
> EINVAL is the usual one I think (some also return ERANGE).
> 
> > > > Yes, the output stops as soon as the PWM_ENABLE bit is cleared in
> > > > hardware. Then The output is set to 0 (which is inactive).
> > > > Once .disable() => tegra_pwm_disable() gets invoked, enable bit is
> > > > cleared and hence PWM will possess no output signal.
> > > > tegra_pwm_config() will be invoked for any new configuration request.
> > >
> > > Some drivers already have a "Limitations" section in their header.
> > > Please take a look at the existing examples and provide something
> > > similar. (Note you still didn't answer "How does a running PWM
> > > behave when the register is updated? Does it complete the currently
> > > running period?". I assume the answer to the second question is "No"
> > > (and the first is only there for rhetoric reasons).)
> > >
> >
> > 1. I will add the below comments as Limitations:
> > -     When PWM is disabled, the output is driven to 0 and
> 
> In fact, this is a good property. So the only problem is, that for both stop and
> reconfiguration the currently running period isn't completed.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-KÃnig            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |