From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>, Heiko Stuebner <heiko@sntech.de>, devicetree@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, Thierry Reding <thierry.reding@gmail.com>, Sam Ravnborg <sam@ravnborg.org>, linux-rtc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Fabio Estevam <festevam@gmail.com>, Daniel Palmer <daniel@0x0f.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Andreas Kemnade <andreas@kemnade.info>, NXP Linux Team <linux-imx@nxp.com>, linux-pwm@vger.kernel.org, Stephan Gerhold <stephan@gerhold.net>, allen <allen.chen@ite.com.tw>, Sascha Hauer <s.hauer@pengutronix.de>, Lubomir Rintel <lkundrak@v3.sk>, Rob Herring <robh+dt@kernel.org>, Lee Jones <lee.jones@linaro.org>, linux-arm-kernel@lists.infradead.org, Alessandro Zummo <a.zummo@towertech.it>, linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>, Pengutronix Kernel Team <kernel@pengutronix.de>, Heiko Stuebner <heiko.stuebner@theobroma-systems.com>, Josua Mayer <josua.mayer@jm0.eu>, Shawn Guo <shawnguo@kernel.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC Date: Mon, 28 Sep 2020 10:35:46 +0200 [thread overview] Message-ID: <20200928083546.gwu7ucx7os5yc5bn@pengutronix.de> (raw) In-Reply-To: <20200927211044.GC2510@latitude> [-- Attachment #1: Type: text/plain, Size: 4203 bytes --] 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/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>, Heiko Stuebner <heiko@sntech.de>, Sam Ravnborg <sam@ravnborg.org>, linux-pwm@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, Thierry Reding <thierry.reding@gmail.com>, Fabio Estevam <festevam@gmail.com>, linux-rtc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Lee Jones <lee.jones@linaro.org>, Daniel Palmer <daniel@0x0f.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Andreas Kemnade <andreas@kemnade.info>, NXP Linux Team <linux-imx@nxp.com>, devicetree@vger.kernel.org, Stephan Gerhold <stephan@gerhold.net>, allen <allen.chen@ite.com.tw>, Sascha Hauer <s.hauer@pengutronix.de>, Lubomir Rintel <lkundrak@v3.sk>, Rob Herring <robh+dt@kernel.org>, linux-arm-kernel@lists.infradead.org, Alessandro Zummo <a.zummo@towertech.it>, linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>, Pengutronix Kernel Team <kernel@pengutronix.de>, Heiko Stuebner <heiko.stuebner@theobroma-systems.com>, Josua Mayer <josua.mayer@jm0.eu>, Shawn Guo <shawnguo@kernel.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC Date: Mon, 28 Sep 2020 10:35:46 +0200 [thread overview] Message-ID: <20200928083546.gwu7ucx7os5yc5bn@pengutronix.de> (raw) In-Reply-To: <20200927211044.GC2510@latitude> [-- Attachment #1.1: Type: text/plain, Size: 4203 bytes --] 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/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-28 8:36 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-24 19:24 [PATCH v3 0/7] Netronix embedded controller driver for Kobo and Tolino ebook readers Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 1/7] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-10-12 7:19 ` Uwe Kleine-König 2020-10-12 7:19 ` Uwe Kleine-König 2020-10-12 11:48 ` Lee Jones 2020-10-12 11:48 ` Lee Jones 2020-09-24 19:24 ` [PATCH v3 2/7] dt-bindings: mfd: Add binding for Netronix embedded controller Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-29 18:52 ` Rob Herring 2020-09-29 18:52 ` Rob Herring 2020-09-24 19:24 ` [PATCH v3 3/7] mfd: Add base driver " Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 9:29 ` Andy Shevchenko 2020-09-25 9:29 ` Andy Shevchenko 2020-09-25 21:32 ` Jonathan Neuschäfer 2020-09-25 21:32 ` Jonathan Neuschäfer 2020-09-29 16:37 ` Andy Shevchenko 2020-09-29 16:37 ` Andy Shevchenko 2020-10-02 22:54 ` Jonathan Neuschäfer 2020-10-02 22:54 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 6:30 ` Uwe Kleine-König 2020-09-25 6:30 ` Uwe Kleine-König 2020-09-27 21:10 ` Jonathan Neuschäfer 2020-09-27 21:10 ` Jonathan Neuschäfer 2020-09-28 8:35 ` Uwe Kleine-König [this message] 2020-09-28 8:35 ` Uwe Kleine-König 2020-10-02 23:36 ` Jonathan Neuschäfer 2020-10-02 23:36 ` Jonathan Neuschäfer 2020-10-03 8:17 ` Andy Shevchenko 2020-10-03 8:17 ` Andy Shevchenko 2020-09-24 19:24 ` [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-24 20:40 ` Andreas Kemnade 2020-09-24 20:40 ` Andreas Kemnade 2020-10-02 23:41 ` Jonathan Neuschäfer 2020-10-02 23:41 ` Jonathan Neuschäfer 2020-09-25 5:44 ` Uwe Kleine-König 2020-09-25 5:44 ` Uwe Kleine-König 2020-10-04 1:43 ` Jonathan Neuschäfer 2020-10-04 1:43 ` Jonathan Neuschäfer 2020-10-04 8:42 ` Alexandre Belloni 2020-10-04 8:42 ` Alexandre Belloni 2020-10-11 19:09 ` Jonathan Neuschäfer 2020-10-11 19:09 ` Jonathan Neuschäfer 2020-10-05 7:35 ` Uwe Kleine-König 2020-10-05 7:35 ` Uwe Kleine-König 2020-09-25 9:36 ` Alexandre Belloni 2020-09-25 9:36 ` Alexandre Belloni 2020-09-25 22:00 ` Jonathan Neuschäfer 2020-09-25 22:00 ` Jonathan Neuschäfer 2020-09-24 19:24 ` [PATCH v3 6/7] MAINTAINERS: Add entry for " Jonathan Neuschäfer 2020-09-24 19:24 ` Jonathan Neuschäfer 2020-09-25 5:08 ` [PATCH v3 7/7] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer 2020-09-25 5:08 ` Jonathan Neuschäfer 2020-10-07 7:46 ` Krzysztof Kozlowski 2020-10-07 7:46 ` Krzysztof Kozlowski 2020-10-11 19:42 ` Jonathan Neuschäfer 2020-10-11 19:42 ` Jonathan Neuschäfer
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=20200928083546.gwu7ucx7os5yc5bn@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=a.zummo@towertech.it \ --cc=alexandre.belloni@bootlin.com \ --cc=allen.chen@ite.com.tw \ --cc=andreas@kemnade.info \ --cc=andy.shevchenko@gmail.com \ --cc=arnd@arndb.de \ --cc=broonie@kernel.org \ --cc=daniel@0x0f.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=heiko.stuebner@theobroma-systems.com \ --cc=heiko@sntech.de \ --cc=j.neuschaefer@gmx.net \ --cc=josua.mayer@jm0.eu \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-rtc@vger.kernel.org \ --cc=lkundrak@v3.sk \ --cc=mchehab+huawei@kernel.org \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=sam@ravnborg.org \ --cc=shawnguo@kernel.org \ --cc=stephan@gerhold.net \ --cc=thierry.reding@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.