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

[Uwe Kleine-König]

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.

Given that that part doesn't seem to be available for individual sale, I
suggest to add something like:

	depends on (ARM64 && ARCH_ROCKCHIP) || COMPILE_TEST

to not annoy people configuring a kernel for x86 or s390.

> +	help
> +	  PWM driver for Mule PWM-over-I2C controller. Mule is a device
> +	  that can output a PWM signal based on I2C commands.

How is that different from a PWM that is an ordinary I2C device? If
there is no difference, I'd just call this "an I2C device". Also "can
output a PWM signal" is a given for PWM drivers :-)

> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-mule.
> +
>  config PWM_MXS
>  	tristate "Freescale MXS PWM support"
>  	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..cdd736ea3244 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
>  obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
> +obj-$(CONFIG_PWM_MULE)		+= pwm-mule.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
> 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.)

> + */
> +
> +#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 wonder why the hardware doesn't use the whole 8 bits here.

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

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

Maybe write all three registers in a bulk write, then you might even be
able to drop the lock.

Also please fail silently.

> +out:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static const struct pwm_ops pwm_mule_ops = {
> +	.apply = pwm_mule_apply,

Is .get_state not possible to implement (then please document that with a reason)?

> +};
> +
> +static int pwm_mule_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct pwm_chip *chip;
> +	struct mule_pwm *priv;
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*priv));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	priv = pwmchip_get_drvdata(chip);
> +
> +	mutex_init(&priv->lock);
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &pwm_mule_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "Failed to allocate i2c register map\n");
> +
> +	chip->ops = &pwm_mule_ops;
> +
> +	return devm_pwmchip_add(dev, chip);

Error message if this fails, please.

> +}

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature