Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller

From: Michael Walle
Date: Tue Jul 28 2020 - 04:21:27 EST

Next message: Thomas Gleixner: "Re: Kernel panic - not syncing: IO-APIC + timer doesn't work!"
Previous message: Linus Walleij: "Re: [PATCH -mmotm] pinctrl: mediatek: fix build for tristate changes"
In reply to: Uwe Kleine-KÃnig: "Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller"
Next in thread: Uwe Kleine-KÃnig: "Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Am 2020-07-28 09:43, schrieb Uwe Kleine-KÃnig:

Hello,

just a few minor issues left:

thanks for the review.

On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote:

diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
new file mode 100644
index 000000000000..956fa09f3aba
--- /dev/null
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld PWM driver
+ *
+ * Copyright (c) 2020 Michael Walle <michael@xxxxxxxx>
+ *
+ * There is no public datasheet available for this PWM core. But it is easy
+ * enough to be briefly explained. It consists of one 8-bit counter. The PWM
+ * supports four distinct frequencies by selecting when to reset the counter.
+ * With the prescaler setting you can select which bit of the counter is used
+ * to reset it. This implies that the higher the frequency the less remaining
+ * bits are available for the actual counter.
+ *
+ * Let cnt[7:0] be the counter, clocked at 32kHz:
+ * +-----------+--------+--------------+-----------+
+ * | prescaler | reset | counter bits | frequency |
+ * +-----------+--------+--------------+-----------+
+ * | 0 | cnt[7] | cnt[6:0] | 250Hz |
+ * | 1 | cnt[6] | cnt[5:0] | 500Hz |
+ * | 2 | cnt[5] | cnt[4:0] | 1kHz |
+ * | 3 | cnt[4] | cnt[3:0] | 2kHz |
+ * +-----------+--------+--------------+-----------+

Very nice. I'd add a "period length" column, as this is what the PWM
core uses.

For your convenience (and as I created that table anyhow for further
checking of the formulas below):

* +-----------+--------+--------------+-----------+--------+
* | prescaler | reset | counter bits | frequency | period |
* | | | | | length |
* +-----------+--------+--------------+-----------+--------+
* | 0 | cnt[7] | cnt[6:0] | 250Hz | 4000ns |
* | 1 | cnt[6] | cnt[5:0] | 500Hz | 2000ns |
* | 2 | cnt[5] | cnt[4:0] | 1kHz | 1000ns |
* | 3 | cnt[4] | cnt[3:0] | 2kHz | 500ns |
* +-----------+--------+--------------+-----------+--------+

sure :)

+ *
+ * Limitations:
+ * - The hardware cannot generate a 100% duty cycle if the prescaler is 0.
+ * - The hardware cannot atomically set the prescaler and the counter value,
+ * which might lead to glitches and inconsistent states if a write fails.
+ * - The counter is not reset if you switch the prescaler which leads
+ * to glitches, too.
+ * - The duty cycle will switch immediately and not after a complete cycle.
+ * - Depending on the actual implementation, disabling the PWM might have
+ * side effects. For example, if the output pin is shared with a GPIO pin
+ * it will automatically switch back to GPIO mode.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/*
+ * PWM timer block registers.
+ */
+#define SL28CPLD_PWM_CTRL 0x00
+#define SL28CPLD_PWM_CTRL_ENABLE BIT(7)
+#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0)
+#define SL28CPLD_PWM_CYCLE 0x01
+#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0)
+
+#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */
+#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler)))
+#define SL28CPLD_PWM_PERIOD(prescaler) \
+ (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler))
+
+/*
+ * We calculate the duty cycle like this:
+ * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle
+ *
+ * With
+ * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC
+ * max_duty_cycle = 1 << (7 - prescaler)

If you don't need parenthesis in the max_period_ns around 7 - prescaler,
you don't need them either in the max_duty_cycle line.

mhh this should be "1 << (7 - prescaler)" in both cases. So
max_period_ns is wrong:
max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC

+ * this then simplifies to:
+ * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC
+ */
+#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \
+ (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg))

For those who copy from your driver maybe add a comment like:

* NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer here, so we're not loosing
* precision by doing the division first.

ok.

+#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \
+ (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK))
+
+struct sl28cpld_pwm {
+ struct pwm_chip pwm_chip;
+ struct regmap *regmap;
+ u32 offset;
+};
+
+static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
+ unsigned int reg;
+ int prescaler;
+
+ regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, &reg);

Would it make sense to hide this using e.g.:

#define sl28cpkd_pwm_read(priv, reg, val) regmap_read((priv)->regmap,
(priv)->offset + (reg), val)

The line would then become:

sl28cpkd_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);

which is a bit prettier. Up to you to decide. If you do it, please do
the same for write

I don't have a strong opinion on that. I can change it. Although there
will be checkpatch warning about multiple uses of the macro argument,
I'd presume.

+ state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
+
+ prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
+ state->period = SL28CPLD_PWM_PERIOD(prescaler);
+
+ regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, &reg);
+ state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
+ state->polarity = PWM_POLARITY_NORMAL;
+}
+
+static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
+ unsigned int cycle, prescaler;
+ int ret;
+ u8 ctrl;
+
+ /* Polarity inversion is not supported */
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EINVAL;
+
+ /*
+ * Calculate the prescaler. Pick the the biggest period that isn't
+ * bigger than the requested period.
+ */
+ prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period);
+ prescaler = order_base_2(prescaler);
+
+ if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK))
+ return -ERANGE;

The calculation looks right.
Did you check the generated code? Maybe using an if or switch here is
more effective? (optional task for bonus points :-)

I varied between this and some if/switch. This hard to read IMHO (as
was your your ilog(n+1)+1), but you could easily change the range
of the prescaler without having to change this. Also if/switch
looked ugly too *g*. I'll check again.

+ ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler);
+ if (state->enabled)
+ ctrl |= SL28CPLD_PWM_CTRL_ENABLE;
+
+ cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle);
+ cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler));
+
+ /*
+ * Work around the hardware limitation. See also above. Trap 100% duty
+ * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't
+ * care about the frequency because its "all-one" in either case.
+ *
+ * We don't need to check the actual prescaler setting, because only
+ * if the prescaler is 0 we can have this particular value.
+ */
+ if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) {
+ ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK;
+ ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1);
+ cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1);
+ }
+
+ ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl);
+ if (ret)
+ return ret;
+
+ return regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, (u8)cycle);

This cast isn't needed, is it?

Due to the clamping, it is not, correct. I'll remove it.

+}
+
+static const struct pwm_ops sl28cpld_pwm_ops = {
+ .apply = sl28cpld_pwm_apply,
+ .get_state = sl28cpld_pwm_get_state,
+ .owner = THIS_MODULE,
+};
+
+static int sl28cpld_pwm_probe(struct platform_device *pdev)
+{
+ struct sl28cpld_pwm *priv;
+ struct pwm_chip *chip;
+ int ret;
+
+ if (!pdev->dev.parent)
+ return -ENODEV;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!priv->regmap)

Error message here?

This shouldn't really happen and I put it into the same category
as the two above and report no error. But I can add it.

Generally, it looked to me that more and more drivers don't
really report errors anymore, but just return with an -EWHATEVER.
So if someone can shed some light here, I'm all ears.

-michael

Next message: Thomas Gleixner: "Re: Kernel panic - not syncing: IO-APIC + timer doesn't work!"
Previous message: Linus Walleij: "Re: [PATCH -mmotm] pinctrl: mediatek: fix build for tristate changes"
In reply to: Uwe Kleine-KÃnig: "Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller"
Next in thread: Uwe Kleine-KÃnig: "Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]