| Message ID | 20200803093559.12289-1-michael@walle.cc |
|---|---|
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
Hello Michael, I'm nearly happy now; see below. On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 7dbcf6973d33..a0d50d70c3b9 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -428,6 +428,16 @@ config PWM_SIFIVE > To compile this driver as a module, choose M here: the module > will be called pwm-sifive. > > +config PWM_SL28CPLD > + tristate "Kontron sl28cpld PWM support" > + select MFD_SIMPLE_MFD_I2C Is it sensible to present this option to everyone? Maybe depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST . > + help > + Generic PWM framework driver for board management controller > + found on the Kontron sl28 CPLD. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-sl28cpld. > + > config PWM_SPEAR > tristate "STMicroelectronics SPEAr PWM support" > depends on PLAT_SPEAR || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 2c2ba0a03557..cbdcd55d69ee 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > +obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o > obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o > obj-$(CONFIG_PWM_STI) += pwm-sti.o > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c > new file mode 100644 > index 000000000000..bb298af36f0b > --- /dev/null > +++ b/drivers/pwm/pwm-sl28cpld.c > @@ -0,0 +1,235 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * sl28cpld PWM driver > + * > + * Copyright (c) 2020 Michael Walle <michael@walle.cc> > + * > + * There is no public datasheet available for this PWM core. But it is easy > + * enough to be briefly explained. It consists of one 8-bit counter. The PWM > + * supports four distinct frequencies by selecting when to reset the counter. > + * With the prescaler setting you can select which bit of the counter is used > + * to reset it. This implies that the higher the frequency the less remaining > + * bits are available for the actual counter. > + * > + * Let cnt[7:0] be the counter, clocked at 32kHz: > + * +-----------+--------+--------------+-----------+---------------+ > + * | prescaler | reset | counter bits | frequency | period length | > + * +-----------+--------+--------------+-----------+---------------+ > + * | 0 | cnt[7] | cnt[6:0] | 250 Hz | 4000000 ns | > + * | 1 | cnt[6] | cnt[5:0] | 500 Hz | 2000000 ns | > + * | 2 | cnt[5] | cnt[4:0] | 1 kHz | 1000000 ns | > + * | 3 | cnt[4] | cnt[3:0] | 2 kHz | 500000 ns | > + * +-----------+--------+--------------+-----------+---------------+ > + * > + * Limitations: > + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0. > + * - The hardware cannot atomically set the prescaler and the counter value, > + * which might lead to glitches and inconsistent states if a write fails. > + * - The counter is not reset if you switch the prescaler which leads > + * to glitches, too. > + * - The duty cycle will switch immediately and not after a complete cycle. > + * - Depending on the actual implementation, disabling the PWM might have > + * side effects. For example, if the output pin is shared with a GPIO pin > + * it will automatically switch back to GPIO mode. Very nice. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/kernel.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +/* > + * PWM timer block registers. > + */ > +#define SL28CPLD_PWM_CTRL 0x00 > +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7) > +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0) > +#define SL28CPLD_PWM_CYCLE 0x01 > +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0) > + > +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ > +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler))) > +#define SL28CPLD_PWM_PERIOD(prescaler) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) > + > +/* > + * We calculate the duty cycle like this: > + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle > + * > + * With > + * max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC > + * max_duty_cycle = 1 << (7 - prescaler) > + * this then simplifies to: > + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC > + * > + * NSEC_PER_SEC and SL28CPLD_PWM_CLK is integer here, so we're not losing > + * precision by doing the divison first. Apart from the grammatical issue (s/is/are/) this is not the relevant fact. The relevant thing is that NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer. (In case this is not clear, assume SL28CPLD_PWM_CLK to be 30000 and reg 0x12345. Then we have: NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg) -> 0x94255749 NSEC_PER_SEC * (reg) / SL28CPLD_PWM_CLK -> 0x9425b860 .) > + */ > +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) > +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ > + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) > + > +#define sl28cpld_pwm_read(priv, reg, val) \ > + regmap_read((priv)->regmap, (priv)->offset + (reg), (val)) > +#define sl28cpld_pwm_write(priv, reg, val) \ > + regmap_write((priv)->regmap, (priv)->offset + (reg), (val)) > + > +struct sl28cpld_pwm { > + struct pwm_chip pwm_chip; > + struct regmap *regmap; > + u32 offset; > +}; > + > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > + unsigned int reg; > + int prescaler; > + > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); > + > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; > + > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); > + state->period = SL28CPLD_PWM_PERIOD(prescaler); > + > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, ®); > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that the upper bits are zero? > + state->polarity = PWM_POLARITY_NORMAL; > +} Best regards Uwe
Hi Uwe, Hi Lee, Am 2020-08-06 10:40, schrieb Uwe Kleine-König: > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index 7dbcf6973d33..a0d50d70c3b9 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -428,6 +428,16 @@ config PWM_SIFIVE >> To compile this driver as a module, choose M here: the module >> will be called pwm-sifive. >> >> +config PWM_SL28CPLD >> + tristate "Kontron sl28cpld PWM support" >> + select MFD_SIMPLE_MFD_I2C > > Is it sensible to present this option to everyone? Maybe > > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST Because there is now no real MFD driver anymore, there is also no symbol for that. The closest would be ARCH_ARM64 but I don't think that is a good idea. Lee, what do you think about adding a symbol to the MFD, which selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? I.e. config MFD_SL28CPLD tristate "Kontron sl28cpld" select MFD_SIMPLE_MFD_I2C help Say yes here to add support for the Kontron sl28cpld board management controller. Then all the other device driver could depend on the MFD_SL28CPLD symbol. [..] >> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, >> + struct pwm_device *pwm, >> + struct pwm_state *state) >> +{ >> + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> + unsigned int reg; >> + int prescaler; >> + >> + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); >> + >> + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; >> + >> + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); >> + state->period = SL28CPLD_PWM_PERIOD(prescaler); >> + >> + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, ®); >> + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); > > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed > that > the upper bits are zero? Mh, the hardware guarantees that bit7 is zero. So masking with SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think could go wrong is this: someone set the prescaler to != 0 and the duty cycle to a value greater than the max value for this particular prescaler mode. For the above calculations this would result in a duty_cycle greater than the period, if I'm not mistaken. The behavior of the hardware is undefined in that case (at the moment it will be always on, I guess). So this isn't a valid setting. Nevertheless it might happen. So what about the following: state->duty_cycle = min(state->duty_cycle, state->period); -michael
On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote: > Hi Uwe, Hi Lee, > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König: > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index 7dbcf6973d33..a0d50d70c3b9 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -428,6 +428,16 @@ config PWM_SIFIVE > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-sifive. > > > > > > +config PWM_SL28CPLD > > > + tristate "Kontron sl28cpld PWM support" > > > + select MFD_SIMPLE_MFD_I2C > > > > Is it sensible to present this option to everyone? Maybe > > > > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST > > Because there is now no real MFD driver anymore, there is also > no symbol for that. The closest would be ARCH_ARM64 but I don't > think that is a good idea. > > Lee, what do you think about adding a symbol to the MFD, which > selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? > > I.e. > config MFD_SL28CPLD > tristate "Kontron sl28cpld" > select MFD_SIMPLE_MFD_I2C > help > Say yes here to add support for the Kontron sl28cpld board > management controller. > > Then all the other device driver could depend on the MFD_SL28CPLD > symbol. > > [..] > > > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > > > + unsigned int reg; > > > + int prescaler; > > > + > > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); > > > + > > > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; > > > + > > > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); > > > + state->period = SL28CPLD_PWM_PERIOD(prescaler); > > > + > > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, ®); > > > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); > > > > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that > > the upper bits are zero? > > Mh, the hardware guarantees that bit7 is zero. So masking with > SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think > could go wrong is this: someone set the prescaler to != 0 and the > duty cycle to a value greater than the max value for this particular > prescaler mode. For the above calculations this would result in a > duty_cycle greater than the period, if I'm not mistaken. > > The behavior of the hardware is undefined in that case (at the moment > it will be always on, I guess). So this isn't a valid setting. > Nevertheless it might happen. So what about the following: > > state->duty_cycle = min(state->duty_cycle, state->period); If you care about this: This can also happen (at least shortly) in sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before SL28CPLD_PWM_CYCLE there. I wonder if we want to sanitize the values returned from driver's .get_state in the core; or scream loud (maybe only if PWM_DEBUG is on). Something like: if (state->enabled && state->duty_cycle > state->period) { if (IS_ENABLED(CONFIG_PWM_DEBUG)) dev_warn(chip->dev, ".get_state() returned invalid setting.\n"); state->duty_cycle = state->period; } Do we want to catch state->period = 0, too? Do we interpret this as disabled? Best regards Uwe
Am 2020-08-07 09:45, schrieb Uwe Kleine-König: > On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote: >> Hi Uwe, Hi Lee, >> >> Am 2020-08-06 10:40, schrieb Uwe Kleine-König: >> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: >> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> > > index 7dbcf6973d33..a0d50d70c3b9 100644 >> > > --- a/drivers/pwm/Kconfig >> > > +++ b/drivers/pwm/Kconfig >> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE >> > > To compile this driver as a module, choose M here: the module >> > > will be called pwm-sifive. >> > > >> > > +config PWM_SL28CPLD >> > > + tristate "Kontron sl28cpld PWM support" >> > > + select MFD_SIMPLE_MFD_I2C >> > >> > Is it sensible to present this option to everyone? Maybe >> > >> > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST >> >> Because there is now no real MFD driver anymore, there is also >> no symbol for that. The closest would be ARCH_ARM64 but I don't >> think that is a good idea. >> >> Lee, what do you think about adding a symbol to the MFD, which >> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? >> >> I.e. >> config MFD_SL28CPLD >> tristate "Kontron sl28cpld" >> select MFD_SIMPLE_MFD_I2C >> help >> Say yes here to add support for the Kontron sl28cpld board >> management controller. >> >> Then all the other device driver could depend on the MFD_SL28CPLD >> symbol. >> >> [..] >> >> > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, >> > > + struct pwm_device *pwm, >> > > + struct pwm_state *state) >> > > +{ >> > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> > > + unsigned int reg; >> > > + int prescaler; >> > > + >> > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); >> > > + >> > > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; >> > > + >> > > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); >> > > + state->period = SL28CPLD_PWM_PERIOD(prescaler); >> > > + >> > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, ®); >> > > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); >> > >> > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that >> > the upper bits are zero? >> >> Mh, the hardware guarantees that bit7 is zero. So masking with >> SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think >> could go wrong is this: someone set the prescaler to != 0 and the >> duty cycle to a value greater than the max value for this particular >> prescaler mode. For the above calculations this would result in a >> duty_cycle greater than the period, if I'm not mistaken. >> >> The behavior of the hardware is undefined in that case (at the moment >> it will be always on, I guess). So this isn't a valid setting. >> Nevertheless it might happen. So what about the following: >> >> state->duty_cycle = min(state->duty_cycle, state->period); > > If you care about this: This can also happen (at least shortly) in > sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before > SL28CPLD_PWM_CYCLE there. It could also happen if it was the other way around, couldn't it? Changing modes might glitch. I care more about returning valid values to the PWM core ;) -michael
Hi Michael, On Fri, Aug 07, 2020 at 09:55:19AM +0200, Michael Walle wrote: > Am 2020-08-07 09:45, schrieb Uwe Kleine-König: > > On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote: > > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König: > > > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: > > > > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, > > > > > + struct pwm_device *pwm, > > > > > + struct pwm_state *state) > > > > > +{ > > > > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > > > > > + unsigned int reg; > > > > > + int prescaler; > > > > > + > > > > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); > > > > > + > > > > > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; > > > > > + > > > > > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); > > > > > + state->period = SL28CPLD_PWM_PERIOD(prescaler); > > > > > + > > > > > + sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, ®); > > > > > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); > > > > > > > > Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that > > > > the upper bits are zero? > > > > > > Mh, the hardware guarantees that bit7 is zero. So masking with > > > SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think > > > could go wrong is this: someone set the prescaler to != 0 and the > > > duty cycle to a value greater than the max value for this particular > > > prescaler mode. For the above calculations this would result in a > > > duty_cycle greater than the period, if I'm not mistaken. > > > > > > The behavior of the hardware is undefined in that case (at the moment > > > it will be always on, I guess). So this isn't a valid setting. > > > Nevertheless it might happen. So what about the following: > > > > > > state->duty_cycle = min(state->duty_cycle, state->period); > > > > If you care about this: This can also happen (at least shortly) in > > sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before > > SL28CPLD_PWM_CYCLE there. > > It could also happen if it was the other way around, couldn't it? > Changing modes might glitch. If you want to prevent this, you have to order the writes depending on prescaler increasing or decreasing. Best regards Uwe
On Fri, 07 Aug 2020, Michael Walle wrote: > Hi Uwe, Hi Lee, > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König: > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index 7dbcf6973d33..a0d50d70c3b9 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -428,6 +428,16 @@ config PWM_SIFIVE > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-sifive. > > > > > > +config PWM_SL28CPLD > > > + tristate "Kontron sl28cpld PWM support" > > > + select MFD_SIMPLE_MFD_I2C > > > > Is it sensible to present this option to everyone? Maybe > > > > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST > > Because there is now no real MFD driver anymore, there is also > no symbol for that. The closest would be ARCH_ARM64 but I don't > think that is a good idea. > > Lee, what do you think about adding a symbol to the MFD, which > selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? > > I.e. > config MFD_SL28CPLD > tristate "Kontron sl28cpld" > select MFD_SIMPLE_MFD_I2C > help > Say yes here to add support for the Kontron sl28cpld board > management controller. > > Then all the other device driver could depend on the MFD_SL28CPLD > symbol. You want to add a virtual symbol to prevent having to present a real one? How is that a reasonable solution?
Am 2020-08-10 09:13, schrieb Lee Jones: > On Fri, 07 Aug 2020, Michael Walle wrote: > >> Hi Uwe, Hi Lee, >> >> Am 2020-08-06 10:40, schrieb Uwe Kleine-König: >> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: >> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> > > index 7dbcf6973d33..a0d50d70c3b9 100644 >> > > --- a/drivers/pwm/Kconfig >> > > +++ b/drivers/pwm/Kconfig >> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE >> > > To compile this driver as a module, choose M here: the module >> > > will be called pwm-sifive. >> > > >> > > +config PWM_SL28CPLD >> > > + tristate "Kontron sl28cpld PWM support" >> > > + select MFD_SIMPLE_MFD_I2C >> > >> > Is it sensible to present this option to everyone? Maybe >> > >> > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST >> >> Because there is now no real MFD driver anymore, there is also >> no symbol for that. The closest would be ARCH_ARM64 but I don't >> think that is a good idea. >> >> Lee, what do you think about adding a symbol to the MFD, which >> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? >> >> I.e. >> config MFD_SL28CPLD >> tristate "Kontron sl28cpld" >> select MFD_SIMPLE_MFD_I2C >> help >> Say yes here to add support for the Kontron sl28cpld board >> management controller. >> >> Then all the other device driver could depend on the MFD_SL28CPLD >> symbol. > > You want to add a virtual symbol to prevent having to present a real > one? How is that a reasonable solution? (1) Its a symbol on which all sl28cpld will depend on. Thus they will all be hidden if that is not set. (2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C, which is more correct, because they don't have anything to do with i2c. -michael
Am 2020-08-10 09:31, schrieb Michael Walle: > Am 2020-08-10 09:13, schrieb Lee Jones: >> On Fri, 07 Aug 2020, Michael Walle wrote: >> >>> Hi Uwe, Hi Lee, >>> >>> Am 2020-08-06 10:40, schrieb Uwe Kleine-König: >>> > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: >>> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>> > > index 7dbcf6973d33..a0d50d70c3b9 100644 >>> > > --- a/drivers/pwm/Kconfig >>> > > +++ b/drivers/pwm/Kconfig >>> > > @@ -428,6 +428,16 @@ config PWM_SIFIVE >>> > > To compile this driver as a module, choose M here: the module >>> > > will be called pwm-sifive. >>> > > >>> > > +config PWM_SL28CPLD >>> > > + tristate "Kontron sl28cpld PWM support" >>> > > + select MFD_SIMPLE_MFD_I2C >>> > >>> > Is it sensible to present this option to everyone? Maybe >>> > >>> > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST >>> >>> Because there is now no real MFD driver anymore, there is also >>> no symbol for that. The closest would be ARCH_ARM64 but I don't >>> think that is a good idea. >>> >>> Lee, what do you think about adding a symbol to the MFD, which >>> selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? >>> >>> I.e. >>> config MFD_SL28CPLD >>> tristate "Kontron sl28cpld" >>> select MFD_SIMPLE_MFD_I2C >>> help >>> Say yes here to add support for the Kontron sl28cpld board >>> management controller. >>> >>> Then all the other device driver could depend on the MFD_SL28CPLD >>> symbol. >> >> You want to add a virtual symbol to prevent having to present a real >> one? How is that a reasonable solution? > > (1) Its a symbol on which all sl28cpld will depend on. Thus they will > all be hidden if that is not set. > (2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C, > which is more correct, because they don't have anything to do with > i2c. Lee, would you accept such a symbol? Otherwise, I'd leave it as is. -michael
On Mon, 10 Aug 2020, Michael Walle wrote: > Am 2020-08-10 09:13, schrieb Lee Jones: > > On Fri, 07 Aug 2020, Michael Walle wrote: > > > > > Hi Uwe, Hi Lee, > > > > > > Am 2020-08-06 10:40, schrieb Uwe Kleine-König: > > > > On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote: > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > index 7dbcf6973d33..a0d50d70c3b9 100644 > > > > > --- a/drivers/pwm/Kconfig > > > > > +++ b/drivers/pwm/Kconfig > > > > > @@ -428,6 +428,16 @@ config PWM_SIFIVE > > > > > To compile this driver as a module, choose M here: the module > > > > > will be called pwm-sifive. > > > > > > > > > > +config PWM_SL28CPLD > > > > > + tristate "Kontron sl28cpld PWM support" > > > > > + select MFD_SIMPLE_MFD_I2C > > > > > > > > Is it sensible to present this option to everyone? Maybe > > > > > > > > depends on SOME_SYMBOL_ONLY_TRUE_ON_SL28CPLD || COMPILE_TEST > > > > > > Because there is now no real MFD driver anymore, there is also > > > no symbol for that. The closest would be ARCH_ARM64 but I don't > > > think that is a good idea. > > > > > > Lee, what do you think about adding a symbol to the MFD, which > > > selects MFD_SIMPLE_MFD_I2C but doesn't enable any C modules? > > > > > > I.e. > > > config MFD_SL28CPLD > > > tristate "Kontron sl28cpld" > > > select MFD_SIMPLE_MFD_I2C > > > help > > > Say yes here to add support for the Kontron sl28cpld board > > > management controller. > > > > > > Then all the other device driver could depend on the MFD_SL28CPLD > > > symbol. > > > > You want to add a virtual symbol to prevent having to present a real > > one? How is that a reasonable solution? > > (1) Its a symbol on which all sl28cpld will depend on. Thus they will > all be hidden if that is not set. > (2) the drivers itself wouldn't need to depend on MFD_SIMPLE_MFD_I2C, > which is more correct, because they don't have anything to do with > i2c. Yes, okay.
The Kontron sl28cpld is a board management chip providing gpio, pwm, fan monitoring and an interrupt controller. For now this controller is used on the Kontron SMARC-sAL28 board. But because of its flexible nature, it might also be used on other boards in the future. The individual blocks (like gpio, pwm, etc) are kept intentionally small. The MFD core driver then instantiates different (or multiple of the same) blocks. It also provides the register layout so it might be updated in the future without a device tree change; and support other boards with a different layout or functionalities. See also [1] for more information. This is my first take of a MFD driver. I don't know whether the subsystem maintainers should only be CCed on the patches which affect the subsystem or on all patches for this series. I've chosen the latter so you can get a more complete picture. [1] https://lore.kernel.org/linux-devicetree/0e3e8204ab992d75aa07fc36af7e4ab2@walle.cc/ Changes for v4 and above are tracked in the patches, suggested by Lee. Changes since v3: - use of_platform_populate() to populate internal devices using the internal register offsets as unit-addresses - because we don't use mfd_cells anymore, we cannot use IORESOURCE_REG, but instead parse the reg property in each individual driver - dropped the following patches because they were already merged: gpiolib: Introduce gpiochip_irqchip_add_domain() gpio: add a reusable generic gpio_chip using regmap - dropped the following patches because they are no longer needed: include/linux/ioport.h: add helper to define REG resource constructs mfd: mfd-core: Don't overwrite the dma_mask of the child device mfd: mfd-core: match device tree node against reg property - rephrase commit messages, as suggested by Thomas Gleixner Changes since v2: As suggested by Guenter Roeck: - added sl28cpld.rst to index.rst - removed sl28cpld_wdt_status() - reverse christmas tree local variable ordering - assign device_property_read_bool() retval directly - introduce WDT_DEFAULT_TIMEOUT and use it if the hardware reports 0 as timeout. - set WDOG_HW_RUNNING if applicable - remove platform_set_drvdata() leftover As suggested by Bartosz Golaszewski: - don't export gpio_regmap_simple_xlate() - combine local variable declaration of the same type - drop the "struct gpio_regmap_addr", instead use -1 to force an address offset of zero - fix typo - use "struct gpio_regmap_config" pattern, keep "struct gpio_regmap" private. this also means we need a getter/setter for the driver_data element. As suggested by Linus Walleij: - don't store irq_domain in gpio-regmap. drop to_irq() altogether for now. Instead there is now a new patch which lets us set the irqdomain of the gpiochip_irqchip and use its .to_irq() function. This way we don't have to expose the gpio_chip inside the gpio-regmap to the user. Changes since v1: - use of_match_table in all drivers, needed for automatic module loading, when using OF_MFD_CELL() - add new gpio-regmap.c which adds a generic regmap gpio_chip implementation - new patch for reqmap_irq, so we can reuse its implementation - remove almost any code from gpio-sl28cpld.c, instead use gpio-regmap and regmap-irq - change the handling of the mfd core vs device tree nodes; add a new property "of_reg" to the mfd_cell struct which, when set, is matched to the unit-address of the device tree nodes. - fix sl28cpld watchdog when it is not initialized by the bootloader. Explicitly set the operation mode. - also add support for kontron,assert-wdt-timeout-pin in sl28cpld-wdt. As suggested by Bartosz Golaszewski: - define registers as hex - make gpio enum uppercase - move parent regmap check before memory allocation - use device_property_read_bool() instead of the of_ version - mention the gpio flavors in the bindings documentation As suggested by Guenter Roeck: - cleanup #includes and sort them - use devm_watchdog_register_device() - use watchdog_stop_on_reboot() - provide a Documentation/hwmon/sl28cpld.rst - cleaned up the weird tristate->bool and I2C=y issue. Instead mention that the MFD driver is bool because of the following intc patch - removed the SL28CPLD_IRQ typo As suggested by Rob Herring: - combine all dt bindings docs into one patch - change the node name for all gpio flavors to "gpio" - removed the interrupts-extended rule - cleaned up the unit-address space, see above Michael Walle (13): mfd: add simple regmap based I2C driver dt-bindings: mfd: Add bindings for sl28cpld mfd: simple-mfd-i2c: add sl28cpld support irqchip: add sl28cpld interrupt controller support watchdog: add support for sl28cpld watchdog pwm: add support for sl28cpld PWM controller gpio: add support for the sl28cpld GPIO controller hwmon: add support for the sl28cpld hardware monitoring controller arm64: dts: freescale: sl28: enable sl28cpld arm64: dts: freescale: sl28: map GPIOs to input events arm64: dts: freescale: sl28: enable LED support arm64: dts: freescale: sl28: enable fan support arm64: defconfig: enable the sl28cpld board management controller .../bindings/gpio/kontron,sl28cpld-gpio.yaml | 54 ++++ .../hwmon/kontron,sl28cpld-hwmon.yaml | 27 ++ .../kontron,sl28cpld-intc.yaml | 54 ++++ .../bindings/mfd/kontron,sl28cpld.yaml | 153 ++++++++++++ .../bindings/pwm/kontron,sl28cpld-pwm.yaml | 35 +++ .../watchdog/kontron,sl28cpld-wdt.yaml | 35 +++ Documentation/hwmon/index.rst | 1 + Documentation/hwmon/sl28cpld.rst | 36 +++ .../fsl-ls1028a-kontron-kbox-a-230-ls.dts | 14 ++ .../fsl-ls1028a-kontron-sl28-var3-ads2.dts | 9 + .../freescale/fsl-ls1028a-kontron-sl28.dts | 134 ++++++++++ arch/arm64/configs/defconfig | 5 + drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-sl28cpld.c | 161 ++++++++++++ drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/sl28cpld-hwmon.c | 142 +++++++++++ drivers/irqchip/Kconfig | 8 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-sl28cpld.c | 96 +++++++ drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 1 + drivers/mfd/simple-mfd-i2c.c | 57 +++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sl28cpld.c | 235 ++++++++++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sl28cpld_wdt.c | 229 +++++++++++++++++ 30 files changed, 1546 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml create mode 100644 Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml create mode 100644 Documentation/devicetree/bindings/interrupt-controller/kontron,sl28cpld-intc.yaml create mode 100644 Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml create mode 100644 Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml create mode 100644 Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml create mode 100644 Documentation/hwmon/sl28cpld.rst create mode 100644 drivers/gpio/gpio-sl28cpld.c create mode 100644 drivers/hwmon/sl28cpld-hwmon.c create mode 100644 drivers/irqchip/irq-sl28cpld.c create mode 100644 drivers/mfd/simple-mfd-i2c.c create mode 100644 drivers/pwm/pwm-sl28cpld.c create mode 100644 drivers/watchdog/sl28cpld_wdt.c