From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Baolin Wang <baolin.wang@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Orson Zhai <orsonzhai@gmail.com>,
Chunyan Zhang <zhang.lyra@gmail.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
linux-pwm@vger.kernel.org, DTML <devicetree@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
kernel@pengutronix.de
Subject: Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support
Date: Wed, 14 Aug 2019 12:55:35 +0200 [thread overview]
Message-ID: <20190814105535.svslc57qp3wx5lub@pengutronix.de> (raw)
In-Reply-To: <CAMz4ku+3txx5kO-u_+_pxFwoovnX81WFF-moNBasUUgEpvQb+Q@mail.gmail.com>
Hello Baolin,
On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > [...]
> > > Not really, our hardware's method is, when you changed a new
> > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > while to complete current period, then change to the new period.
> >
> > Can you describe that in more detail? This doesn't explain why MOD must be
> > configured before DUTY. Is there another reason for that?
>
> Sorry, I did not explain this explicitly. When we change a new PWM
> configuration, the PWM controller will make sure the current period is
> completed before changing to a new period. Once setting the MOD
> register (since we always set MOD firstly), that will tell the
> hardware that a new period need to change.
So if the current period just ended after you reconfigured MOD but
before you wrote to DUTY we'll see a bogus period, right? I assume the
same holds true for writing the prescale value?
> The reason MOD must be configured before DUTY is that, if we
> configured DUTY firstly, the PWM can work abnormally if the current
> DUTY is larger than previous MOD. That is also our hardware's
> limitation.
OK, so you must not get into a situation where DUTY > MOD, right?
Now if the hardware was configured for
period = 8s, duty = 4s
and now you are supposed to change to
period = 2s, duty = 1s
you'd need to write DUTY first, don't you?
> > > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > > + int ret, i;
> > > > > +
> > > > > + ret = pwmchip_remove(&spc->chip);
> > > > > +
> > > > > + for (i = 0; i < spc->num_pwms; i++) {
> > > > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > > +
> > > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > >
> > > > If a PWM was still running you're effectively stopping it here, right?
> > > > Are you sure you don't disable once more than you enabled?
> > >
> > > Yes, you are right. I should check current enable status of the PWM channel.
> > > Thanks for your comments.
> >
> > I didn't recheck, but I think the right approach is to not fiddle with
> > the clocks at all and rely on the PWM framework to not let someone call
> > sprd_pwm_remove when a PWM is still in use.
>
> So you mean just return pwmchip_remove(&spc->chip); ?
right.
I just rechecked: If there is still a pwm in use, pwmchip_remove returns
-EBUSY. So this should be safe.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2019-08-14 10:55 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-13 13:46 [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation Baolin Wang
2019-08-13 13:46 ` [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support Baolin Wang
2019-08-13 15:16 ` Uwe Kleine-König
2019-08-14 8:42 ` Baolin Wang
2019-08-14 9:23 ` Uwe Kleine-König
2019-08-14 10:01 ` Baolin Wang
2019-08-14 10:55 ` Uwe Kleine-König [this message]
2019-08-14 12:23 ` Baolin Wang
2019-08-14 13:30 ` Uwe Kleine-König
2019-08-14 14:07 ` Michal Vokáč
2019-08-14 16:17 ` Are the requirements of the PWM API sensible? Uwe Kleine-König
2019-08-13 14:12 ` [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation Uwe Kleine-König
2019-08-14 1:51 ` Baolin Wang
2019-08-14 7:01 ` Uwe Kleine-König
2019-08-14 7:25 ` Baolin Wang
2019-08-14 7:39 ` Uwe Kleine-König
2019-08-14 7:52 ` Baolin Wang
2019-08-14 8:49 ` Uwe Kleine-König
2019-08-14 9:33 ` Baolin Wang
2019-08-14 9:46 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190814105535.svslc57qp3wx5lub@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=baolin.wang@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=orsonzhai@gmail.com \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=vincent.guittot@linaro.org \
--cc=zhang.lyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).