| 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
| ||
|
Message-ID: <20200928083546.gwu7ucx7os5yc5bn@pengutronix.de>
Date: Mon, 28 Sep 2020 10:35:46 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Jonathan Neuschäfer <j.neuschaefer@....net>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
Heiko Stuebner <heiko@...ech.de>, devicetree@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>,
Thierry Reding <thierry.reding@...il.com>,
Sam Ravnborg <sam@...nborg.org>, linux-rtc@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Fabio Estevam <festevam@...il.com>,
Daniel Palmer <daniel@...f.com>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Andreas Kemnade <andreas@...nade.info>,
NXP Linux Team <linux-imx@....com>,
linux-pwm@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>,
allen <allen.chen@....com.tw>,
Sascha Hauer <s.hauer@...gutronix.de>,
Lubomir Rintel <lkundrak@...sk>,
Rob Herring <robh+dt@...nel.org>,
Lee Jones <lee.jones@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
Alessandro Zummo <a.zummo@...ertech.it>,
linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
Josua Mayer <josua.mayer@....eu>,
Shawn Guo <shawnguo@...nel.org>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in
Netronix EC
Hello Jonathan,
On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
> On Fri, Sep 25, 2020 at 08:30:37AM +0200, Uwe Kleine-König wrote:
> > > + if (period > MAX_PERIOD_NS) {
> > > + dev_warn(pwm->dev,
> > > + "Period is not representable in 16 bits after division by %u: %u\n",
> > > + TIME_BASE_NS, period);
> >
> > No error messages in .apply() please; this might spam the kernel log.
> >
> > Also the expectation when a too big period is requested is to configure
> > for the biggest possible period. So just do:
> >
> > if (period > MAX_PERIOD_NS) {
> > period = MAX_PERIOD_NS;
> >
> > if (duty > period)
> > duty = period;
> > }
> >
> > (or something equivalent).
>
> Okay, I'll adjust it.
>
> > > + /*
> > > + * Writing a duty cycle of zone puts the device into a state where
> >
> > What is "zone"? A mixture of zero and one and so approximately 0.5?
>
> Oops, that's a typo. I just meant "zero".
>
> > > + * writing a higher duty cycle doesn't result in the brightness that it
> > > + * usually results in. This can be fixed by cycling the ENABLE register.
> > > + *
> > > + * As a workaround, write ENABLE=0 when the duty cycle is zero.
> > > + */
> > > + if (state->enabled && duty != 0) {
> > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > > + if (res)
> > > + return res;
> > > +
> > > + /* Disable the auto-off timer */
> > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > > + if (res)
> > > + return res;
> > > +
> > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > > + } else {
> > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > > + }
> >
> > This code is wrong for state->enabled = false.
>
> Why?
Hm, I wonder the same. Probably I just misunderstood the code, sorry :-\
> > How does the PWM behave when .apply is called? Does it complete the
> > currently running period? Can it happen that when you switch from say
> >
> > .duty_cycle = 900 * TIME_BASE_NS (0x384)
> > .period = 1800 * TIME_BASE_NS (0x708)
> >
> > to
> >
> > .duty_cycle = 300 * TIME_BASE_NS (0x12c)
> > .period = 600 * TIME_BASE_NS (0x258)
> >
> > that a period with
> >
> > .duty_cycle = 388 * TIME_BASE_NS (0x184)
> > .period = 1800 * TIME_BASE_NS (0x708)
> >
> > (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> > started) or something similar is emitted?
>
> Changes take effect after the low byte is written, so a result like 0x184
> in the above example should not happen.
>
> When the period and duty cycle are both changed, it temporarily results
> in an inconsistent state:
>
> - period = 1800ns, duty cycle = 900ns
> - period = 600ns, duty cycle = 900ns (!)
> - period = 600ns, duty cycle = 300ns
Does this always happen, or only if a new cycle starts at an unlucky
moment?
> The inconsistent state of duty cycle > period is handled gracefully by
> the EC and it outputs a 100% duty cycle, as far as I can tell.
OK.
> I currently don't have a logic analyzer / oscilloscope to measure
> whether we get full PWM periods, or some kind of glitch when the new
> period starts in the middle of the last one.
You can even check this with an LED using something like:
pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
pwm_apply(mypwm, {.enabled = false, ... });
. If the period is completed the LED is on for $big ns, if not the LED
is on for a timespan that is probably hardly noticable with the human
eye.
> > > +}
> > > +
> > > +static struct pwm_ops ntxec_pwm_ops = {
> > > + .apply = ntxec_pwm_apply,
> >
> > Please implement a .get_state() callback. And enable PWM_DEBUG during
> > your tests.
>
> The device doesn't support reading back the PWM state. What should a
> driver do in this case?
Document it as a limitation, please.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists