| Message ID | 20240529-buzzer_support-v1-2-fd3eb0a24442@cherry.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Mule PWM-over-I2C support | expand |
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@cherry.de> > --- > 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
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@cherry.de> >> --- >> 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
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@baylibre.com/ > > > + 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
Hi Uwe, On 7/15/24 5:09 PM, Uwe Kleine-König wrote: > Hello Quentin, > > On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote: [...] >> 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? > Not sure to understand what you meant by base-2 register here? I am guessing you rather wanted to suggest a different unit/representation of the duty cycle in the register in the FW API? Cheers, Quentin
Hello Quentin, On Wed, Jul 17, 2024 at 10:48:52AM +0200, Quentin Schulz wrote: > On 7/15/24 5:09 PM, Uwe Kleine-König wrote: > > On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote: > > > 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? > > Not sure to understand what you meant by base-2 register here? I am guessing > you rather wanted to suggest a different unit/representation of the duty > cycle in the register in the FW API? For humans 100 as maximal value for a register is natural, but hardware usually uses binary representation ("base-2") for values and usually a register (or bit field) is used completely. That is, valid values range beween 0 and 2^n (or 2^n - 1) for some n. Note this discussion isn't really relevant to the driver. Just me wondering about the hardware design. So if you don't want to follow up, that's fine for me. Best regards Uwe
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 + help + PWM driver for Mule PWM-over-I2C controller. Mule is a device + that can output a PWM signal based on I2C commands. + + 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 + */ + +#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)) + +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; + } + + if (state->enabled) + duty_cycle = pwm_get_relative_duty_cycle(state, 100); + else + duty_cycle = 0; + + mutex_lock(&priv->lock); + + 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); + +out: + mutex_unlock(&priv->lock); + return ret; +} + +static const struct pwm_ops pwm_mule_ops = { + .apply = pwm_mule_apply, +}; + +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); +} + +static const struct of_device_id pwm_mule_of_match[] = { + { .compatible = "tsd,pwm-mule", }, + { }, +}; +MODULE_DEVICE_TABLE(of, pwm_mule_of_match); + +static struct i2c_driver pwm_mule_driver = { + .driver = { + .name = "pwm-mule", + .of_match_table = pwm_mule_of_match, + }, + .probe = pwm_mule_probe, +}; +module_i2c_driver(pwm_mule_driver); + +MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@cherry.de>"); +MODULE_DESCRIPTION("Mule PWM driver"); +MODULE_LICENSE("GPL");
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@cherry.de> --- drivers/pwm/Kconfig | 10 +++++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+)