Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Uwe Kleine-König]

Hello Quentin,

On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote:
> > > --- 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?

Increasing easy compile coverage.

> [...]
> 
> > > 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?

Yes. And then to calculate the duty_cycle setting use real_period.

> > 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 :)

Interesting perspective. I'd still go for using the register completely.

> > > +	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!

Right. A conversion example is available at
https://lore.kernel.org/linux-pwm/2102fe8189bdf1f02ff3785b551a69be27a65af4.1719520143.git.u.kleine-koenig@xxxxxxxxxxxx/

> > > +	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.

I'd call that a glitch, yes.

> 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.

Wouldn't it be more natural with these to have duty in a base-2 register
for duty, in the assumption that your MCUs habe this, too?

> 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?

dev_dbg is silent by default, so that's fine for me.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature