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
