The Netronix EC provides a PWM output which is used for the backlight
on some ebook readers. This patches adds a driver for the PWM output.
The .get_state callback is not implemented, because the PWM state can't
be read back from the hardware.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v4:
- Document hardware/driver limitations
- Only accept normal polarity
- Fix a typo ("zone" -> "zero")
- change MAX_PERIOD_NS to 0xffff * 125
- Clamp period to the maximum rather than returning an error
- Rename private struct pointer to priv
- Rearrage control flow in _probe to save a few lines and a temporary variable
- Add missing MODULE_ALIAS line
- Spell out ODM
v3:
- https://lore.kernel.org/lkml/[email protected]/
- Relicense as GPLv2 or later
- Add email address to copyright line
- Remove OF compatible string and don't include linux/of_device.h
- Fix bogus ?: in return line
- Don't use a comma after sentinels
- Avoid ret |= ... pattern
- Move 8-bit register conversion to ntxec.h
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Various grammar and style improvements, as suggested by Uwe Kleine-König,
Lee Jones, and Alexandre Belloni
- Switch to regmap
- Prefix registers with NTXEC_REG_
- Add help text to the Kconfig option
- Use the .apply callback instead of the old API
- Add a #define for the time base (125ns)
- Don't change device state in .probe; this avoids multiple problems
- Rework division and overflow check logic to perform divisions in 32 bits
- Avoid setting duty cycle to zero, to work around a hardware quirk
---
drivers/pwm/Kconfig | 8 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-ntxec.c | 166 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 175 insertions(+)
create mode 100644 drivers/pwm/pwm-ntxec.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 63be5362fd3a5..815f329ed5b46 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -350,6 +350,14 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_NTXEC
+ tristate "Netronix embedded controller PWM support"
+ depends on MFD_NTXEC
+ help
+ Say yes here if you want to support the PWM output of the embedded
+ controller found in certain e-book readers designed by the original
+ design manufacturer Netronix.
+
config PWM_OMAP_DMTIMER
tristate "OMAP Dual-Mode Timer PWM support"
depends on OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index cbdcd55d69eef..1deb29e6ae8e5 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON) += pwm-meson.o
obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o
obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
new file mode 100644
index 0000000000000..4f4f736d71aba
--- /dev/null
+++ b/drivers/pwm/pwm-ntxec.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the original design manufacturer Netronix, Inc.
+ * It contains RTC, battery monitoring, system power management, and PWM
+ * functionality.
+ *
+ * This driver implements PWM output.
+ *
+ * Copyright 2020 Jonathan Neuschäfer <[email protected]>
+ *
+ * Limitations:
+ * - The get_state callback is not implemented, because the current state of
+ * the PWM output can't be read back from the hardware.
+ * - The hardware can only generate normal polarity output.
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct ntxec_pwm {
+ struct device *dev;
+ struct ntxec *ec;
+ struct pwm_chip chip;
+};
+
+static struct ntxec_pwm *pwmchip_to_priv(struct pwm_chip *chip)
+{
+ return container_of(chip, struct ntxec_pwm, chip);
+}
+
+#define NTXEC_REG_AUTO_OFF_HI 0xa1
+#define NTXEC_REG_AUTO_OFF_LO 0xa2
+#define NTXEC_REG_ENABLE 0xa3
+#define NTXEC_REG_PERIOD_LOW 0xa4
+#define NTXEC_REG_PERIOD_HIGH 0xa5
+#define NTXEC_REG_DUTY_LOW 0xa6
+#define NTXEC_REG_DUTY_HIGH 0xa7
+
+/*
+ * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
+ * measured in this unit.
+ */
+#define TIME_BASE_NS 125
+
+/*
+ * The maximum input value (in nanoseconds) is determined by the time base and
+ * the range of the hardware registers that hold the converted value.
+ * It fits into 32 bits, so we can do our calculations in 32 bits as well.
+ */
+#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
+
+static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+ const struct pwm_state *state)
+{
+ struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
+ unsigned int duty = state->duty_cycle;
+ unsigned int period = state->period;
+ int res = 0;
+
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EINVAL;
+
+ if (period > MAX_PERIOD_NS) {
+ period = MAX_PERIOD_NS;
+
+ if (duty > period)
+ duty = period;
+ }
+
+ period /= TIME_BASE_NS;
+ duty /= TIME_BASE_NS;
+
+ res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
+ if (res)
+ return res;
+
+ res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
+ if (res)
+ return res;
+
+ res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
+ if (res)
+ return res;
+
+ res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
+ if (res)
+ return res;
+
+ /*
+ * Writing a duty cycle of zero puts the device into a state where
+ * 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
+ if (res)
+ return res;
+
+ /* Disable the auto-off timer */
+ res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
+ if (res)
+ return res;
+
+ return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
+ } else {
+ return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
+ }
+}
+
+static struct pwm_ops ntxec_pwm_ops = {
+ .apply = ntxec_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static int ntxec_pwm_probe(struct platform_device *pdev)
+{
+ struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
+ struct ntxec_pwm *priv;
+ struct pwm_chip *chip;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->ec = ec;
+ priv->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, priv);
+
+ chip = &priv->chip;
+ chip->dev = &pdev->dev;
+ chip->ops = &ntxec_pwm_ops;
+ chip->base = -1;
+ chip->npwm = 1;
+
+ return pwmchip_add(chip);
+}
+
+static int ntxec_pwm_remove(struct platform_device *pdev)
+{
+ struct ntxec_pwm *priv = platform_get_drvdata(pdev);
+ struct pwm_chip *chip = &priv->chip;
+
+ return pwmchip_remove(chip);
+}
+
+static struct platform_driver ntxec_pwm_driver = {
+ .driver = {
+ .name = "ntxec-pwm",
+ },
+ .probe = ntxec_pwm_probe,
+ .remove = ntxec_pwm_remove,
+};
+module_platform_driver(ntxec_pwm_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <[email protected]>");
+MODULE_DESCRIPTION("PWM driver for Netronix EC");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ntxec-pwm");
--
2.29.2
Hello,
On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neusch?fer wrote:
> The Netronix EC provides a PWM output which is used for the backlight
> on some ebook readers. This patches adds a driver for the PWM output.
>
> The .get_state callback is not implemented, because the PWM state can't
> be read back from the hardware.
>
> Signed-off-by: Jonathan Neusch?fer <[email protected]>
> ---
>
> v4:
> - Document hardware/driver limitations
> - Only accept normal polarity
> - Fix a typo ("zone" -> "zero")
> - change MAX_PERIOD_NS to 0xffff * 125
> - Clamp period to the maximum rather than returning an error
> - Rename private struct pointer to priv
> - Rearrage control flow in _probe to save a few lines and a temporary variable
> - Add missing MODULE_ALIAS line
> - Spell out ODM
>
> v3:
> - https://lore.kernel.org/lkml/[email protected]/
> - Relicense as GPLv2 or later
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Fix bogus ?: in return line
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Various grammar and style improvements, as suggested by Uwe Kleine-K?nig,
> Lee Jones, and Alexandre Belloni
> - Switch to regmap
> - Prefix registers with NTXEC_REG_
> - Add help text to the Kconfig option
> - Use the .apply callback instead of the old API
> - Add a #define for the time base (125ns)
> - Don't change device state in .probe; this avoids multiple problems
> - Rework division and overflow check logic to perform divisions in 32 bits
> - Avoid setting duty cycle to zero, to work around a hardware quirk
> ---
> drivers/pwm/Kconfig | 8 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-ntxec.c | 166 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 175 insertions(+)
> create mode 100644 drivers/pwm/pwm-ntxec.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 63be5362fd3a5..815f329ed5b46 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -350,6 +350,14 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_NTXEC
> + tristate "Netronix embedded controller PWM support"
> + depends on MFD_NTXEC
> + help
> + Say yes here if you want to support the PWM output of the embedded
> + controller found in certain e-book readers designed by the original
> + design manufacturer Netronix.
> +
> config PWM_OMAP_DMTIMER
> tristate "OMAP Dual-Mode Timer PWM support"
> depends on OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index cbdcd55d69eef..1deb29e6ae8e5 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON) += pwm-meson.o
> obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
> obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o
> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
> +obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
> obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o
> obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
> new file mode 100644
> index 0000000000000..4f4f736d71aba
> --- /dev/null
> +++ b/drivers/pwm/pwm-ntxec.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the original design manufacturer Netronix, Inc.
> + * It contains RTC, battery monitoring, system power management, and PWM
> + * functionality.
> + *
> + * This driver implements PWM output.
> + *
> + * Copyright 2020 Jonathan Neusch?fer <[email protected]>
> + *
> + * Limitations:
> + * - The get_state callback is not implemented, because the current state of
> + * the PWM output can't be read back from the hardware.
> + * - The hardware can only generate normal polarity output.
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct ntxec_pwm {
> + struct device *dev;
> + struct ntxec *ec;
> + struct pwm_chip chip;
> +};
> +
> +static struct ntxec_pwm *pwmchip_to_priv(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct ntxec_pwm, chip);
> +}
> +
> +#define NTXEC_REG_AUTO_OFF_HI 0xa1
> +#define NTXEC_REG_AUTO_OFF_LO 0xa2
> +#define NTXEC_REG_ENABLE 0xa3
> +#define NTXEC_REG_PERIOD_LOW 0xa4
> +#define NTXEC_REG_PERIOD_HIGH 0xa5
> +#define NTXEC_REG_DUTY_LOW 0xa6
> +#define NTXEC_REG_DUTY_HIGH 0xa7
> +
> +/*
> + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> + * measured in this unit.
> + */
> +#define TIME_BASE_NS 125
> +
> +/*
> + * The maximum input value (in nanoseconds) is determined by the time base and
> + * the range of the hardware registers that hold the converted value.
> + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> + */
> +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> +
> +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> + const struct pwm_state *state)
> +{
> + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> + unsigned int duty = state->duty_cycle;
> + unsigned int period = state->period;
state->duty_cycle and state->period are u64, so you're losing
information here. Consider state->duty_cycle = 0x100000001 and
state->period = 0x200000001.
> + int res = 0;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (period > MAX_PERIOD_NS) {
> + period = MAX_PERIOD_NS;
> +
> + if (duty > period)
> + duty = period;
> + }
> +
> + period /= TIME_BASE_NS;
> + duty /= TIME_BASE_NS;
> +
> + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> + if (res)
> + return res;
I wonder if you can add some logic to the regmap in the mfd driver such
that ntxec_reg8 isn't necessary for all users.
> + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> + if (res)
> + return res;
> +
> + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> + if (res)
> + return res;
> +
> + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> + if (res)
> + return res;
I think I already asked, but I don't remember the reply: What happens to
the output between these writes? A comment here about this would be
suitable.
> +
> + /*
> + * Writing a duty cycle of zero puts the device into a state where
> + * 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> + if (res)
> + return res;
> +
> + /* Disable the auto-off timer */
> + res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> + if (res)
> + return res;
> +
> + return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> + } else {
> + return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> + }
> +}
> +
> +static struct pwm_ops ntxec_pwm_ops = {
This can be const.
> + .apply = ntxec_pwm_apply,
/*
* The current state cannot be read out, so there is no .get_state
* callback.
*/
Hmm, at least you could provice a .get_state() callback that reports the
setting that was actually implemented for in the last call to .apply()?
@Thierry: Do you have concerns here? Actually it would be more effective
to have a callback (like .apply()) that modfies its pwm_state
accordingly. (Some drivers did that in the past, but I changed that to
get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
The downside is that people have to understand that concept to properly
use it. I'm torn about the right approach.
> + .owner = THIS_MODULE,
> +};
> +
> +static int ntxec_pwm_probe(struct platform_device *pdev)
> +{
> + struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
> + struct ntxec_pwm *priv;
> + struct pwm_chip *chip;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ec = ec;
> + priv->dev = &pdev->dev;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + chip = &priv->chip;
> + chip->dev = &pdev->dev;
> + chip->ops = &ntxec_pwm_ops;
> + chip->base = -1;
> + chip->npwm = 1;
> +
> + return pwmchip_add(chip);
> +}
> +
> +static int ntxec_pwm_remove(struct platform_device *pdev)
> +{
> + struct ntxec_pwm *priv = platform_get_drvdata(pdev);
> + struct pwm_chip *chip = &priv->chip;
> +
> + return pwmchip_remove(chip);
> +}
> +
> +static struct platform_driver ntxec_pwm_driver = {
> + .driver = {
> + .name = "ntxec-pwm",
> + },
> + .probe = ntxec_pwm_probe,
> + .remove = ntxec_pwm_remove,
> +};
> +module_platform_driver(ntxec_pwm_driver);
> +
> +MODULE_AUTHOR("Jonathan Neusch?fer <[email protected]>");
> +MODULE_DESCRIPTION("PWM driver for Netronix EC");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ntxec-pwm");
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
[...]
> > +/*
> > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > + * measured in this unit.
> > + */
> > +#define TIME_BASE_NS 125
> > +
> > +/*
> > + * The maximum input value (in nanoseconds) is determined by the time base and
> > + * the range of the hardware registers that hold the converted value.
> > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > + */
> > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> > +
> > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > + const struct pwm_state *state)
> > +{
> > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > + unsigned int duty = state->duty_cycle;
> > + unsigned int period = state->period;
>
> state->duty_cycle and state->period are u64, so you're losing
> information here. Consider state->duty_cycle = 0x100000001 and
> state->period = 0x200000001.
Oh, good point, I didn't notice the truncation.
The reason I picked unsigned int was to avoid a 64-bit division;
I suppose I can do something like this:
period = (u32)period / TIME_BASE_NS;
duty = (u32)duty / TIME_BASE_NS;
> > + int res = 0;
> > +
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EINVAL;
> > +
> > + if (period > MAX_PERIOD_NS) {
> > + period = MAX_PERIOD_NS;
> > +
> > + if (duty > period)
> > + duty = period;
> > + }
> > +
> > + period /= TIME_BASE_NS;
> > + duty /= TIME_BASE_NS;
> > +
> > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > + if (res)
> > + return res;
>
> I wonder if you can add some logic to the regmap in the mfd driver such
> that ntxec_reg8 isn't necessary for all users.
I think that would involve:
1. adding custom register access functions to the regmap, which decide
based on the register number whether a register needs 8-bit or 16-bit
access. So far I have avoided information about registers into the
main driver, when the registers are only used in the sub-drivers.
or
2. switching the regmap configuration to little endian, which would be
advantageous for 8-bit registers, inconsequential for 16-bit
registers that consist of independent high and low halves, and wrong
for the 16-bit registers 0x41, which reads the battery voltage ADC
value. It is also different from how the vendor kernel treats 16-bit
registers.
Perhaps there is another option that I haven't considered yet.
>
> > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > + if (res)
> > + return res;
>
> I think I already asked, but I don't remember the reply: What happens to
> the output between these writes? A comment here about this would be
> suitable.
I will add something like the following:
/*
* Changes to the period and duty cycle take effect as soon as the
* corresponding low byte is written, so the hardware may be configured
* to an inconsistent state after the period is written and before the
* duty cycle is fully written. If, in such a case, the old duty cycle
* is longer than the new period, the EC will output 100% for a moment.
*/
>
> > +
> > + /*
> > + * Writing a duty cycle of zero puts the device into a state where
> > + * 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > + if (res)
> > + return res;
> > +
> > + /* Disable the auto-off timer */
> > + res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > + if (res)
> > + return res;
> > +
> > + return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > + } else {
> > + return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > + }
> > +}
> > +
> > +static struct pwm_ops ntxec_pwm_ops = {
>
> This can be const.
Indeed, I'll change it.
> > + .apply = ntxec_pwm_apply,
>
> /*
> * The current state cannot be read out, so there is no .get_state
> * callback.
> */
>
> Hmm, at least you could provice a .get_state() callback that reports the
> setting that was actually implemented for in the last call to .apply()?
Yes... I see two options:
1. Caching the state in the driver's private struct. I'm not completely
convinced of the value, given that the information is mostly
available in the PWM core already (except for the adjustments that
the driver makes).
2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
This seems a bit dirty.
> @Thierry: Do you have concerns here? Actually it would be more effective
> to have a callback (like .apply()) that modfies its pwm_state
> accordingly. (Some drivers did that in the past, but I changed that to
> get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> The downside is that people have to understand that concept to properly
> use it. I'm torn about the right approach.
General guidance for such cases when the state can't be read back from
the hardware would be appreciated.
Thanks,
Jonathan Neuschäfer
Hello Jonathan,
On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neusch?fer wrote:
> On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-K?nig wrote:
> > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neusch?fer wrote:
> [...]
> > > +/*
> > > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > > + * measured in this unit.
> > > + */
> > > +#define TIME_BASE_NS 125
> > > +
> > > +/*
> > > + * The maximum input value (in nanoseconds) is determined by the time base and
> > > + * the range of the hardware registers that hold the converted value.
> > > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > > + */
> > > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> > > +
> > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > + unsigned int duty = state->duty_cycle;
> > > + unsigned int period = state->period;
> >
> > state->duty_cycle and state->period are u64, so you're losing
> > information here. Consider state->duty_cycle = 0x100000001 and
> > state->period = 0x200000001.
>
> Oh, good point, I didn't notice the truncation.
>
> The reason I picked unsigned int was to avoid a 64-bit division;
> I suppose I can do something like this:
>
> period = (u32)period / TIME_BASE_NS;
> duty = (u32)duty / TIME_BASE_NS;
You can do that after you checked period > MAX_PERIOD_NS below, yes.
Something like:
if (state->polarity != PWM_POLARITY_NORMAL)
return -EINVAL;
if (state->period > MAX_PERIOD_NS) {
period = MAX_PERIOD_NS;
else
period = state->period;
if (state->duty_cycle > period)
duty_cycle = period;
else
duty_cycle = state->duty_cycle;
should work with even keeping the local variables as unsigned int.
> > > + int res = 0;
> > > +
> > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > + return -EINVAL;
> > > +
> > > + if (period > MAX_PERIOD_NS) {
> > > + period = MAX_PERIOD_NS;
> > > +
> > > + if (duty > period)
> > > + duty = period;
> > > + }
> > > +
> > > + period /= TIME_BASE_NS;
> > > + duty /= TIME_BASE_NS;
> > > +
> > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > + if (res)
> > > + return res;
> >
> > I wonder if you can add some logic to the regmap in the mfd driver such
> > that ntxec_reg8 isn't necessary for all users.
>
> I think that would involve:
>
> 1. adding custom register access functions to the regmap, which decide
> based on the register number whether a register needs 8-bit or 16-bit
> access. So far I have avoided information about registers into the
> main driver, when the registers are only used in the sub-drivers.
>
> or
>
> 2. switching the regmap configuration to little endian, which would be
> advantageous for 8-bit registers, inconsequential for 16-bit
> registers that consist of independent high and low halves, and wrong
> for the 16-bit registers 0x41, which reads the battery voltage ADC
> value. It is also different from how the vendor kernel treats 16-bit
> registers.
>
> Perhaps there is another option that I haven't considered yet.
I don't know enough about regmap to teach you something here. But maybe
Mark has an idea. (I promoted him from Cc: to To:, maybe he will
notice.)
> > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > + if (res)
> > > + return res;
> >
> > I think I already asked, but I don't remember the reply: What happens to
> > the output between these writes? A comment here about this would be
> > suitable.
>
> I will add something like the following:
>
> /*
> * Changes to the period and duty cycle take effect as soon as the
> * corresponding low byte is written, so the hardware may be configured
> * to an inconsistent state after the period is written and before the
> * duty cycle is fully written. If, in such a case, the old duty cycle
> * is longer than the new period, the EC will output 100% for a moment.
> */
Is the value pair taken over by hardware atomically? That is, is it
really "will" in your last line, or only "might". (E.g. when changing
from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
after reducing period, the new duty_cycle is probably written before the
counter reaches 500. Do we get a 100% cycle here?)
Other than that the info is fine. Make sure to point this out in the
Limitations paragraph at the top of the driver please, too.
> > > + .apply = ntxec_pwm_apply,
> >
> > /*
> > * The current state cannot be read out, so there is no .get_state
> > * callback.
> > */
> >
> > Hmm, at least you could provice a .get_state() callback that reports the
> > setting that was actually implemented for in the last call to .apply()?
>
> Yes... I see two options:
>
> 1. Caching the state in the driver's private struct. I'm not completely
> convinced of the value, given that the information is mostly
> available in the PWM core already (except for the adjustments that
> the driver makes).
>
> 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> This seems a bit dirty.
2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
can be made doing that)?
> > @Thierry: Do you have concerns here? Actually it would be more effective
> > to have a callback (like .apply()) that modfies its pwm_state
> > accordingly. (Some drivers did that in the past, but I changed that to
> > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > The downside is that people have to understand that concept to properly
> > use it. I'm torn about the right approach.
>
> General guidance for such cases when the state can't be read back from
> the hardware would be appreciated.
Yes, improving the documentation would be great here. Thierry, can you
please comment on
https://lore.kernel.org/r/[email protected]
which I'm waiting on before describing our understanding in more detail.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
> > [...]
> > > > +/*
> > > > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > > > + * measured in this unit.
> > > > + */
> > > > +#define TIME_BASE_NS 125
> > > > +
> > > > +/*
> > > > + * The maximum input value (in nanoseconds) is determined by the time base and
> > > > + * the range of the hardware registers that hold the converted value.
> > > > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > > > + */
> > > > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> > > > +
> > > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > > + const struct pwm_state *state)
> > > > +{
> > > > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > > + unsigned int duty = state->duty_cycle;
> > > > + unsigned int period = state->period;
> > >
> > > state->duty_cycle and state->period are u64, so you're losing
> > > information here. Consider state->duty_cycle = 0x100000001 and
> > > state->period = 0x200000001.
> >
> > Oh, good point, I didn't notice the truncation.
> >
> > The reason I picked unsigned int was to avoid a 64-bit division;
> > I suppose I can do something like this:
> >
> > period = (u32)period / TIME_BASE_NS;
> > duty = (u32)duty / TIME_BASE_NS;
>
> You can do that after you checked period > MAX_PERIOD_NS below, yes.
> Something like:
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> return -EINVAL;
>
> if (state->period > MAX_PERIOD_NS) {
> period = MAX_PERIOD_NS;
> else
> period = state->period;
>
> if (state->duty_cycle > period)
> duty_cycle = period;
> else
> duty_cycle = state->duty_cycle;
>
> should work with even keeping the local variables as unsigned int.
>
> > > > + int res = 0;
> > > > +
> > > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > > + return -EINVAL;
> > > > +
> > > > + if (period > MAX_PERIOD_NS) {
> > > > + period = MAX_PERIOD_NS;
> > > > +
> > > > + if (duty > period)
> > > > + duty = period;
> > > > + }
> > > > +
> > > > + period /= TIME_BASE_NS;
> > > > + duty /= TIME_BASE_NS;
> > > > +
> > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > > + if (res)
> > > > + return res;
> > >
> > > I wonder if you can add some logic to the regmap in the mfd driver such
> > > that ntxec_reg8 isn't necessary for all users.
> >
> > I think that would involve:
> >
> > 1. adding custom register access functions to the regmap, which decide
> > based on the register number whether a register needs 8-bit or 16-bit
> > access. So far I have avoided information about registers into the
> > main driver, when the registers are only used in the sub-drivers.
> >
> > or
> >
> > 2. switching the regmap configuration to little endian, which would be
> > advantageous for 8-bit registers, inconsequential for 16-bit
> > registers that consist of independent high and low halves, and wrong
> > for the 16-bit registers 0x41, which reads the battery voltage ADC
> > value. It is also different from how the vendor kernel treats 16-bit
> > registers.
> >
> > Perhaps there is another option that I haven't considered yet.
>
> I don't know enough about regmap to teach you something here. But maybe
> Mark has an idea. (I promoted him from Cc: to To:, maybe he will
> notice.)
>
> > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > > + if (res)
> > > > + return res;
> > >
> > > I think I already asked, but I don't remember the reply: What happens to
> > > the output between these writes? A comment here about this would be
> > > suitable.
> >
> > I will add something like the following:
> >
> > /*
> > * Changes to the period and duty cycle take effect as soon as the
> > * corresponding low byte is written, so the hardware may be configured
> > * to an inconsistent state after the period is written and before the
> > * duty cycle is fully written. If, in such a case, the old duty cycle
> > * is longer than the new period, the EC will output 100% for a moment.
> > */
>
> Is the value pair taken over by hardware atomically? That is, is it
> really "will" in your last line, or only "might". (E.g. when changing
> from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> after reducing period, the new duty_cycle is probably written before the
> counter reaches 500. Do we get a 100% cycle here?)
>
> Other than that the info is fine. Make sure to point this out in the
> Limitations paragraph at the top of the driver please, too.
Perhaps also use something like regmap_bulk_write() to make sure the
time between these writes is a short as possible.
>
> > > > + .apply = ntxec_pwm_apply,
> > >
> > > /*
> > > * The current state cannot be read out, so there is no .get_state
> > > * callback.
> > > */
> > >
> > > Hmm, at least you could provice a .get_state() callback that reports the
> > > setting that was actually implemented for in the last call to .apply()?
> >
> > Yes... I see two options:
> >
> > 1. Caching the state in the driver's private struct. I'm not completely
> > convinced of the value, given that the information is mostly
> > available in the PWM core already (except for the adjustments that
> > the driver makes).
> >
> > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> > This seems a bit dirty.
>
> 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> can be made doing that)?
>
> > > @Thierry: Do you have concerns here? Actually it would be more effective
> > > to have a callback (like .apply()) that modfies its pwm_state
> > > accordingly. (Some drivers did that in the past, but I changed that to
> > > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > > The downside is that people have to understand that concept to properly
> > > use it. I'm torn about the right approach.
> >
> > General guidance for such cases when the state can't be read back from
> > the hardware would be appreciated.
>
> Yes, improving the documentation would be great here. Thierry, can you
> please comment on
> https://lore.kernel.org/r/[email protected]
> which I'm waiting on before describing our understanding in more detail.
Hm... that link gives me a "Not Found" message. Anyway, I think perhaps
the best compromise would be for the core to provide an implementation
of ->get_state() that drivers can use if they can't read out hardware
state. This generic implementation would then just copy over the
internal state and we have to trust that that's really what was applied.
One drawback of that is that we don't factor in things like rounding
errors and other limitations. So a better alternative may be to require
drivers to store a cached version of the state and return that in their
->get_state() implementation.
Or perhaps a hybrid of the above would work where the core provides the
helper that copies cached state and a cached state structure for storage
and then the drivers that can't properly read back hardware state just
need to update the cached state during ->apply().
I slightly prefer variant 2 because it's not clear to me how often we'll
need this and we can always easily convert to variant 3 if this becomes
a more common thing to do.
Thierry
On Fri, Nov 27, 2020 at 12:08:51PM +0100, Thierry Reding wrote:
> On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-K?nig wrote:
> > Hello Jonathan,
> >
> > On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neusch?fer wrote:
> > > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-K?nig wrote:
> > > > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neusch?fer wrote:
> > > [...]
> > > > > +/*
> > > > > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > > > > + * measured in this unit.
> > > > > + */
> > > > > +#define TIME_BASE_NS 125
> > > > > +
> > > > > +/*
> > > > > + * The maximum input value (in nanoseconds) is determined by the time base and
> > > > > + * the range of the hardware registers that hold the converted value.
> > > > > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > > > > + */
> > > > > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> > > > > +
> > > > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > > > + const struct pwm_state *state)
> > > > > +{
> > > > > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > > > + unsigned int duty = state->duty_cycle;
> > > > > + unsigned int period = state->period;
> > > >
> > > > state->duty_cycle and state->period are u64, so you're losing
> > > > information here. Consider state->duty_cycle = 0x100000001 and
> > > > state->period = 0x200000001.
> > >
> > > Oh, good point, I didn't notice the truncation.
> > >
> > > The reason I picked unsigned int was to avoid a 64-bit division;
> > > I suppose I can do something like this:
> > >
> > > period = (u32)period / TIME_BASE_NS;
> > > duty = (u32)duty / TIME_BASE_NS;
> >
> > You can do that after you checked period > MAX_PERIOD_NS below, yes.
> > Something like:
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > return -EINVAL;
> >
> > if (state->period > MAX_PERIOD_NS) {
> > period = MAX_PERIOD_NS;
> > else
> > period = state->period;
> >
> > if (state->duty_cycle > period)
> > duty_cycle = period;
> > else
> > duty_cycle = state->duty_cycle;
> >
> > should work with even keeping the local variables as unsigned int.
> >
> > > > > + int res = 0;
> > > > > +
> > > > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (period > MAX_PERIOD_NS) {
> > > > > + period = MAX_PERIOD_NS;
> > > > > +
> > > > > + if (duty > period)
> > > > > + duty = period;
> > > > > + }
> > > > > +
> > > > > + period /= TIME_BASE_NS;
> > > > > + duty /= TIME_BASE_NS;
> > > > > +
> > > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > > > + if (res)
> > > > > + return res;
> > > >
> > > > I wonder if you can add some logic to the regmap in the mfd driver such
> > > > that ntxec_reg8 isn't necessary for all users.
> > >
> > > I think that would involve:
> > >
> > > 1. adding custom register access functions to the regmap, which decide
> > > based on the register number whether a register needs 8-bit or 16-bit
> > > access. So far I have avoided information about registers into the
> > > main driver, when the registers are only used in the sub-drivers.
> > >
> > > or
> > >
> > > 2. switching the regmap configuration to little endian, which would be
> > > advantageous for 8-bit registers, inconsequential for 16-bit
> > > registers that consist of independent high and low halves, and wrong
> > > for the 16-bit registers 0x41, which reads the battery voltage ADC
> > > value. It is also different from how the vendor kernel treats 16-bit
> > > registers.
> > >
> > > Perhaps there is another option that I haven't considered yet.
> >
> > I don't know enough about regmap to teach you something here. But maybe
> > Mark has an idea. (I promoted him from Cc: to To:, maybe he will
> > notice.)
> >
> > > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > > > + if (res)
> > > > > + return res;
> > > > > +
> > > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > > > + if (res)
> > > > > + return res;
> > > > > +
> > > > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > > > + if (res)
> > > > > + return res;
> > > >
> > > > I think I already asked, but I don't remember the reply: What happens to
> > > > the output between these writes? A comment here about this would be
> > > > suitable.
> > >
> > > I will add something like the following:
> > >
> > > /*
> > > * Changes to the period and duty cycle take effect as soon as the
> > > * corresponding low byte is written, so the hardware may be configured
> > > * to an inconsistent state after the period is written and before the
> > > * duty cycle is fully written. If, in such a case, the old duty cycle
> > > * is longer than the new period, the EC will output 100% for a moment.
> > > */
> >
> > Is the value pair taken over by hardware atomically? That is, is it
> > really "will" in your last line, or only "might". (E.g. when changing
> > from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> > after reducing period, the new duty_cycle is probably written before the
> > counter reaches 500. Do we get a 100% cycle here?)
> >
> > Other than that the info is fine. Make sure to point this out in the
> > Limitations paragraph at the top of the driver please, too.
>
> Perhaps also use something like regmap_bulk_write() to make sure the
> time between these writes is a short as possible.
>
> >
> > > > > + .apply = ntxec_pwm_apply,
> > > >
> > > > /*
> > > > * The current state cannot be read out, so there is no .get_state
> > > > * callback.
> > > > */
> > > >
> > > > Hmm, at least you could provice a .get_state() callback that reports the
> > > > setting that was actually implemented for in the last call to .apply()?
> > >
> > > Yes... I see two options:
> > >
> > > 1. Caching the state in the driver's private struct. I'm not completely
> > > convinced of the value, given that the information is mostly
> > > available in the PWM core already (except for the adjustments that
> > > the driver makes).
> > >
> > > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> > > This seems a bit dirty.
> >
> > 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> > can be made doing that)?
> >
> > > > @Thierry: Do you have concerns here? Actually it would be more effective
> > > > to have a callback (like .apply()) that modfies its pwm_state
> > > > accordingly. (Some drivers did that in the past, but I changed that to
> > > > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > > > The downside is that people have to understand that concept to properly
> > > > use it. I'm torn about the right approach.
> > >
> > > General guidance for such cases when the state can't be read back from
> > > the hardware would be appreciated.
> >
> > Yes, improving the documentation would be great here. Thierry, can you
> > please comment on
> > https://lore.kernel.org/r/[email protected]
> > which I'm waiting on before describing our understanding in more detail.
>
> Hm... that link gives me a "Not Found" message. Anyway, I think perhaps
I thought I tested that before sending the link, but it gives Not Found
for me, too.
https://patchwork.ozlabs.org/project/linux-pwm/patch/[email protected]/
is the patchwork link.
> the best compromise would be for the core to provide an implementation
> of ->get_state() that drivers can use if they can't read out hardware
> state. This generic implementation would then just copy over the
> internal state and we have to trust that that's really what was applied.
>
> One drawback of that is that we don't factor in things like rounding
> errors and other limitations. So a better alternative may be to require
> drivers to store a cached version of the state and return that in their
> ->get_state() implementation.
Yes. That's what Jonathan called 1.
> Or perhaps a hybrid of the above would work where the core provides the
> helper that copies cached state and a cached state structure for storage
> and then the drivers that can't properly read back hardware state just
> need to update the cached state during ->apply().
I thought about doing
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index a13ff383fa1d..c60a638ebfad 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -261,7 +261,7 @@ struct pwm_ops {
int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_capture *result, unsigned long timeout);
int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
- const struct pwm_state *state);
+ struct pwm_state *state);
void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state);
struct module *owner;
and adapting the callbacks to provide the actually implemented setting
in *state. But I don't like this because providing this is (for probably
most drivers) extra effort. So the drivers should only cache the written
register values and calculate the actual timing from these values if and
when required. Providing help from the framework for this is more
complicated because different drivers have different needs here. (Two
u64 for duty_cycle and period is probably enough for everybody?)
> I slightly prefer variant 2 because it's not clear to me how often we'll
> need this and we can always easily convert to variant 3 if this becomes
> a more common thing to do.
I cannot follow because I don't know what variant is "variant 2" and
"variant 3" for you.
I suggest for now we go for "don't provide a .get_state() callback"
because in the only case where it is currently called there is no cached
data to rely on anyhow.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
[...]
> > > state->duty_cycle and state->period are u64, so you're losing
> > > information here. Consider state->duty_cycle = 0x100000001 and
> > > state->period = 0x200000001.
> >
> > Oh, good point, I didn't notice the truncation.
> >
> > The reason I picked unsigned int was to avoid a 64-bit division;
> > I suppose I can do something like this:
> >
> > period = (u32)period / TIME_BASE_NS;
> > duty = (u32)duty / TIME_BASE_NS;
>
> You can do that after you checked period > MAX_PERIOD_NS below, yes.
> Something like:
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> return -EINVAL;
>
> if (state->period > MAX_PERIOD_NS) {
> period = MAX_PERIOD_NS;
> else
> period = state->period;
>
> if (state->duty_cycle > period)
> duty_cycle = period;
> else
> duty_cycle = state->duty_cycle;
>
> should work with even keeping the local variables as unsigned int.
With the min_t() macro, this becomes nice and short:
period = min_t(u64, state->period, MAX_PERIOD_NS);
duty = min_t(u64, state->duty_cycle, period);
period /= TIME_BASE_NS;
duty /= TIME_BASE_NS;
> > > I think I already asked, but I don't remember the reply: What happens to
> > > the output between these writes? A comment here about this would be
> > > suitable.
> >
> > I will add something like the following:
> >
> > /*
> > * Changes to the period and duty cycle take effect as soon as the
> > * corresponding low byte is written, so the hardware may be configured
> > * to an inconsistent state after the period is written and before the
> > * duty cycle is fully written. If, in such a case, the old duty cycle
> > * is longer than the new period, the EC will output 100% for a moment.
> > */
>
> Is the value pair taken over by hardware atomically? That is, is it
> really "will" in your last line, or only "might". (E.g. when changing
> from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> after reducing period, the new duty_cycle is probably written before the
> counter reaches 500. Do we get a 100% cycle here?)
I am not sure when exactly a new period or duty cycle value is applied,
and I don't have the test equipment to measure it. I'll change the text
to "may output 100%".
> Other than that the info is fine. Make sure to point this out in the
> Limitations paragraph at the top of the driver please, too.
Okay.
> > > /*
> > > * The current state cannot be read out, so there is no .get_state
> > > * callback.
> > > */
> > >
> > > Hmm, at least you could provice a .get_state() callback that reports the
> > > setting that was actually implemented for in the last call to .apply()?
> >
> > Yes... I see two options:
> >
> > 1. Caching the state in the driver's private struct. I'm not completely
> > convinced of the value, given that the information is mostly
> > available in the PWM core already (except for the adjustments that
> > the driver makes).
> >
> > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> > This seems a bit dirty.
>
> 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> can be made doing that)?
With regmap caching, I'd be concerned that a read operation may slip
through and reach the device, producing a bogus result. Not sure if
write-only/write-through caching can be configured in regmap.
Thanks,
Jonathan
On Fri, Nov 27, 2020 at 03:23:25PM +0100, Uwe Kleine-König wrote:
[...]
> I suggest for now we go for "don't provide a .get_state() callback"
> because in the only case where it is currently called there is no cached
> data to rely on anyhow.
Alright!
Thanks,
Jonathan