Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver
From: Quentin Schulz
Date: Mon Jul 15 2024 - 08:17:32 EST
Hi Uwe,
Answering since Farouk's on PTO.
Just to clarify, below when I say HW, I mean the actual HW, the MCU
basically. There are two flavours of those, ATtiny816 and STM32F072CB.
They need to be swappable, so same API. When I saw FW, I mean the
firmware running on both MCUs (though we do have two different FW, one
for each MCU, but they expose the same API and we expect the same
behavior, which can be challenging).
The FW is only supposed to run on Cherry products fitted with one of
those MCUs, we do not plan on selling them separately or releasing the
FW for consumption in other devices. As such, there's no need on our
side for public documentation. I'll try to answer the FW questions to
the best of my ability though.
On 7/12/24 10:54 AM, Uwe Kleine-König wrote:
Hello,
On Wed, May 29, 2024 at 12:10:31PM +0200, Farouk Bouabid wrote:
Mule is a device that can output a PWM signal based on I2C commands.
Add pwm driver for Mule PWM-over-I2C controller.
Signed-off-by: Farouk Bouabid <farouk.bouabid@xxxxxxxxx>
---
drivers/pwm/Kconfig | 10 +++++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..eb8cfa113ec7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
To compile this driver as a module, choose M here: the module
will be called pwm-microchip-core.
+config PWM_MULE
+ tristate "Mule PWM-over-I2C support"
+ depends on I2C && OF
It would be easy to drop the hard dependency on OF. Please do that.
Just being curious here, what would be the benefit?
[...]
diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
new file mode 100644
index 000000000000..e8593a48b16e
--- /dev/null
+++ b/drivers/pwm/pwm-mule.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mule PWM-over-I2C controller driver
+ *
+ * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
Is there a publicly available datasheet? I guess not. (I ask because
adding a link there to such a document would be nice.)
Unfortunately no. It's also only part of our product line and there's no
plan to start selling it standalone or selling the IP.
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+struct mule_pwm {
+ struct mutex lock;
+ struct regmap *regmap;
+};
+
+static const struct regmap_config pwm_mule_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+#define MULE_PWM_DCY_REG 0x0
+#define MULE_PWM_FREQ_L_REG 0x1 /* LSB register */
+#define MULE_PWM_FREQ_H_REG 0x2 /* MSB register */
+
+#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))
Don't introduce such a macro if you only use it once. Having the
division in the function results in code that is easier to read (IMHO).
+static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct mule_pwm *priv = pwmchip_get_drvdata(chip);
+ u8 duty_cycle;
+ u64 freq;
+ int ret;
+
+ freq = NANOSECONDS_TO_HZ(state->period);
+
+ if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
+ dev_err(chip->dev,
+ "Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
+ return -EINVAL;
+ }
You're supposed to configure the biggest possible period not bigger than
the requested period. So this should be:
/*
* The period is configured using a 16 bit wide register holding
* the frequency in Hz.
*/
unsigned int period = min_t(u64, state->period, NSEC_PER_SEC);
unsigned int freq = DIV_ROUND_UP(NSEC_PER_SEC, period);
if (freq > U16_MAX)
return -EINVAL;
+ if (state->enabled)
+ duty_cycle = pwm_get_relative_duty_cycle(state, 100);
This is wrong for two reasons:
- It uses rounding to the nearest duty_cycle, however you're supposed
to round down.
- It uses the requested period instead of the real one.
I assume you want:
unsigned int real_period = ((u64) NSEC_PER_SEC * 100) / freq;
which rounds down?
I wonder why the hardware doesn't use the whole 8 bits here.
It's even a 16b register that the HW uses. I guess we just went with the
most human-friendly API :) I believe it's something we should be able to
change in the FW before releasing if this is something that really makes
sense. FYI, the register stores the number of clock ticks for the signal
to be high, once reached, put it low (or the opposite). So it's
necessarily a fraction of the clock frequency. 100% was easy because we
know that every clock frequency we support is a multiple of 100 so
there's no issue around rounding for example since we definitely do not
want to do float maths in MCUs :)
+ else
+ duty_cycle = 0;
+
+ mutex_lock(&priv->lock);
If you use the guard helper, you don't need to resort to goto for error
handling.
I would have liked a link or more precise hint at what this "guard
helper" was, but found something myself so here it is for anyone wondering:
https://lwn.net/Articles/934679/
Had never heard of that one before, neat!
The suggested implementation would then be:
guard(mutex)(&priv->lock);
I believe.
+ ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
+ if (ret) {
+ dev_err(chip->dev,
+ "Failed to set frequency: %llu Hz: %d\n", freq, ret);
+ goto out;
+ }
+
+ ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
+ if (ret)
+ dev_err(chip->dev,
+ "Failed to set duty cycle: %u: %d\n", duty_cycle, ret);
Please document how the hardware behaves here in a "Limitations" section
as several other drivers do. Questions to answer include: Does it
complete a period when the parameters are updated? Can it happen that a
glitch is emitted while MULE_PWM_FREQ_[LH]_REG is updated but
MULE_PWM_DCY_REG isn't yet? Maybe updating MULE_PWM_FREQ_[LH]_REG isn't
even atomic? "Doesn't support disabling, configures duty_cycle=0 when
disabled is requested."
Updating MULE_PWM_FREQ_[LH]_REG is atomic (L is stored in SRAM until H
reg is written, when LH are then written to the hardware IP).
We use double-buffering (supported by the HW directly) for the period
and comparator registers. I believe we still have a possible glitch
during a small time-frame in the current version of the FW. Basically,
trying to change the period AND duty cycle at the same time, the
following could happen:
- period A + duty-cycle AA
- period B + duty-cycle AA
- period B + duty-cycle BB
Depending on what we consider a glitch, the second element in the list
could be one. Even if we do a multibyte write to the actual HW, I'm not
sure if this window can be eliminated.
To give a bit more info on this, there are two possible flavors of the
MCU, ATtiny 816 (datasheet:
https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf)
and STM32F072CB (datasheet:
https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).
FYI, on ATtiny, we use TCA in single-slope PWM generation mode and
PERBUF and CMP2BUF as period and duty-cycle registers. On STM32, we use
TIM15 in PWM mode and ARR and CCR1 as period and duty-cycle registers.
Re-reading both datasheets, and if I understand correctly, we could have
glitch-free transitions by controlling the ARPE bit on STM32 and LUPD
bit on ATtiny816.
@Farouk, please confirm but the above makes sense to me and I guess we
could implement something in the FW. The question is how to detect if we
want to change both the duty-cycle and period at the same time (we could
decide to document this as a requirement for the API user though:
"changes to MULE_PWM_FREQ_[LH]_REG are only applied when
MULE_PWM_DCY_REG is written to").
Maybe write all three registers in a bulk write, then you might even be
able to drop the lock.
The bulk write wouldn't help with the glitch, but it's a good idea for
getting rid of the lock.
Also please fail silently.
Would dev_dbg() be fine here or would you rather see them gone entirely?
Cheers,
Quentin