On Thu, Jan 05, 2017 at 10:25:38AM +0100, Benjamin Gaignard wrote:

This hardware block could at used at same time for PWM generation and IIO timers. PWM and IIO timer configuration are mixed in the same registers so we need a multi fonction driver to be able to share those registers.

version 7:

• rebase on v4.10-rc2

version 6:

• rename files to stm32-timers
• rename functions to stm32_timers_xxx

version 5:

• fix Lee comments about detect function
• add missing dependency on REGMAP_MMIO

version 4:

• add a function to detect Auto Reload Register (ARR) size
• rename the structure shared with other drivers

version 2:

• rename driver "stm32-gptimer" to be align with SoC documentation
• only keep one compatible
• use of_platform_populate() instead of devm_mfd_add_devices()

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

.../devicetree/bindings/mfd/stm32-timers.txt | 2 +- drivers/mfd/Kconfig | 11 +++ drivers/mfd/Makefile | 2 + drivers/mfd/stm32-timers.c | 80 ++++++++++++++++++++++ include/linux/mfd/stm32-timers.h | 71 +++++++++++++++++++ 5 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 drivers/mfd/stm32-timers.c create mode 100644 include/linux/mfd/stm32-timers.h

diff --git a/Documentation/devicetree/bindings/mfd/stm32-timers.txt b/Documentation/devicetree/bindings/mfd/stm32-timers.txt index a73301d..897e7c2 100644 --- a/Documentation/devicetree/bindings/mfd/stm32-timers.txt +++ b/Documentation/devicetree/bindings/mfd/stm32-timers.txt @@ -12,7 +12,7 @@ Required parameters:

• reg: Physical base address and length of the controller's registers.

-- clock-names: Set to "int". +- clock-names: Set to "int".

This hunk should be dropped or moved to previous patch.

• clocks: Phandle to the clock used by the timer module. For Clk properties, please refer to ../clock/clock-bindings.txt

On Thu, Jan 05, 2017 at 10:25:39AM +0100, Benjamin Gaignard wrote:

Define bindings for pwm-stm32

version 6:

• change st,breakinput parameter format to make it usuable on stm32f7 too.

version 2:

• use parameters instead of compatible of handle the hardware configuration

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

.../devicetree/bindings/pwm/pwm-stm32.txt | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt new file mode 100644 index 0000000..866f222 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt @@ -0,0 +1,33 @@ +STMicroelectronics STM32 Timers PWM bindings

+Must be a sub-node of an STM32 Timers device tree node. +See ../mfd/stm32-timers.txt for details about the parent node.

+Required parameters: +- compatible: Must be "st,stm32-pwm". +- pinctrl-names: Set to "default". +- pinctrl-0: List of phandles pointing to pin configuration nodes for PWM module.

• 	For Pinctrl properties see ../pinctrl/pinctrl-bindings.txt
  

+Optional parameters: +- st,breakinput: Arrays of three u32 <index level filter> to describe break input configurations.

• 	"index" indicates on which break input the configuration should be applied.
  

It might be useful to specify what the valid values are for the break input index.

Also, u32 is kind of a Linuxism, perhaps "Arrays of three cells"? Also, does this mean there can be multiple entries? Such as 6 cells for two configurations? What's the maximum number of such configurations?

If it's possible to specify multiple configurations, maybe a slightly clearer wording would be: "One or more <index level filter> triplets to describe..."

• 	"level" gives the active level (0=low or 1=high) for this configuration.
  

So how does this work exactly? "level" specifies the output level if the filter value is matched?

• 	"filter" gives the filtering value to be applied.
  

Is this a single value at which "level" will be applied? Or is it an upper/lower bound that can be used to restrict the output to "level" if the signal goes beyond/below a certain threshold?

Maybe an example would clarify this. Or perhaps a reference to a manual where a more in-depth description of this functionality can be found.

Thierry

2017-01-18 11:08 GMT+01:00 Thierry Reding thierry.reding@gmail.com:

On Thu, Jan 05, 2017 at 10:25:40AM +0100, Benjamin Gaignard wrote:

...

This driver adds support for PWM driver on STM32 platform. The SoC have multiple instances of the hardware IP and each of them could have small differences: number of channels, complementary output, auto reload register size...

version 6:

• change st,breakinput parameter to make it usuable for stm32f7 too.

version 4:

• detect at probe time hardware capabilities
• fix comments done on v2 and v3
• use PWM atomic ops

version 2:

• only keep one comptatible
• use DT parameters to discover hardware block configuration

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-stm32.c | 434 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 444 insertions(+) create mode 100644 drivers/pwm/pwm-stm32.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index f92dd41..88035c0 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -397,6 +397,15 @@ config PWM_STI To compile this driver as a module, choose M here: the module will be called pwm-sti.

+config PWM_STM32

• tristate "STMicroelectronics STM32 PWM"
  

• depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) || COMPILE_TEST
  

• help
  

•   Generic PWM framework driver for STM32 SoCs.
  

•   To compile this driver as a module, choose M here: the module
  

•   will be called pwm-stm32.
  

config PWM_STMPE bool "STMPE expander PWM export" depends on MFD_STMPE diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index a48bdb5..346a83b 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_STI) += pwm-sti.o +obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c new file mode 100644 index 0000000..fcf0a78 --- /dev/null +++ b/drivers/pwm/pwm-stm32.c @@ -0,0 +1,434 @@ +/*

• • Copyright (C) STMicroelectronics 2016
• • Author: Gerald Baeza gerald.baeza@st.com
• • License terms: GNU General Public License (GPL), version 2
• • Inspired by timer-stm32.c from Maxime Coquelin

• •         pwm-atmel.c from Bo Shen
    

• */

+#include <linux/mfd/stm32-timers.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/of.h>

Can you please sort these alphabetically?

sure

...

+#define CCMR_CHANNEL_SHIFT 8 +#define CCMR_CHANNEL_MASK 0xFF +#define MAX_BREAKINPUT 2

Okay, this answers my question regarding the st,breakinput property. I still think it'd be good to have this in the binding documentation just to avoid having to look at implementation to find out.

...

+struct stm32_pwm {

• struct pwm_chip chip;
  

• struct device *dev;
  

• struct clk *clk;
  

• struct regmap *regmap;
  

• unsigned int caps;
  

This seems completely unused?

Yes I will remove it

...

• unsigned int npwm;
  

It's somewhat redundant to have this here, since the same information is already contained in struct pwm_chip.npwm.

Since you use this primarily for detection, how about you make the stm32_pwm_detect_channels() function return the value and store it in a local variable in ->probe()? That might be useful also because you need to check the return value of regmap_update_bits() which technically could fail.

I will remove npwm field and put the result of stm32_pwm_detect_channels() directly on chip.npwm.

regmap functions could failed (even if I haven't experiment that case) but testing all return make the code unreadable so I have decide to not test it....

...

• u32 max_arr;
  

• bool have_complementary_output;
  

+};

+struct stm32_breakinput {

• u32 index;
  

• u32 level;
  

• u32 filter;
  

+};

+static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip) +{

• return container_of(chip, struct stm32_pwm, chip);
  

+}

+static u32 active_channels(struct stm32_pwm *dev) +{

• u32 ccer;
  

• regmap_read(dev->regmap, TIM_CCER, &ccer);
  

• return ccer & TIM_CCER_CCXE;
  

+}

This looks like something that you could track in software, but this is probably fine, too. Again, technically regmap_read() could fail, so you might want to consider adding some code to handle it. In practice it probably won't, so maybe you don't.

TIM_CCER_CCXE is a value that IIO timer can also read (not write) so I have keep the same logic for pwm driver.

...

+static int write_ccrx(struct stm32_pwm *dev, struct pwm_device *pwm,

•               u32 value)
  

+{

• switch (pwm->hwpwm) {
  

• case 0:
  

•         return regmap_write(dev->regmap, TIM_CCR1, value);
  

• case 1:
  

•         return regmap_write(dev->regmap, TIM_CCR2, value);
  

• case 2:
  

•         return regmap_write(dev->regmap, TIM_CCR3, value);
  

• case 3:
  

•         return regmap_write(dev->regmap, TIM_CCR4, value);
  

• }
  

• return -EINVAL;
  

+}

+static int stm32_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

•                     int duty_ns, int period_ns)
  

+{

• struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
  

• unsigned long long prd, div, dty;
  

• unsigned int prescaler = 0;
  

• u32 ccmr, mask, shift;
  

• /* Period and prescaler values depends on clock rate */
  

• div = (unsigned long long)clk_get_rate(priv->clk) * period_ns;
  

• do_div(div, NSEC_PER_SEC);
  

• prd = div;
  

• while (div > priv->max_arr) {
  

•         prescaler++;
  

•         div = prd;
  

•         do_div(div, (prescaler + 1));
  

Nit: there's no need for the parentheses here.

okay

...

• }
  

• prd = div;
  

• if (prescaler > MAX_TIM_PSC) {
  

•         dev_err(chip->dev, "prescaler exceeds the maximum value\n");
  

•         return -EINVAL;
  

• }
  

• /*
  

•  * All channels share the same prescaler and counter so when two
  

•  * channels are active at the same we can't change them
  

Nit: "at the same time"?

okay

...

•  */
  

• if (active_channels(priv) & ~(1 << pwm->hwpwm * 4)) {
  

•         u32 psc, arr;
  

•         regmap_read(priv->regmap, TIM_PSC, &psc);
  

•         regmap_read(priv->regmap, TIM_ARR, &arr);
  

•         if ((psc != prescaler) || (arr != prd - 1))
  

•                 return -EBUSY;
  

• }
  

• regmap_write(priv->regmap, TIM_PSC, prescaler);
  

• regmap_write(priv->regmap, TIM_ARR, prd - 1);
  

• regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
  

• /* Calculate the duty cycles */
  

• dty = prd * duty_ns;
  

• do_div(dty, period_ns);
  

• write_ccrx(priv, pwm, dty);
  

• /* Configure output mode */
  

• shift = (pwm->hwpwm & 0x1) * CCMR_CHANNEL_SHIFT;
  

• ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
  

• mask = CCMR_CHANNEL_MASK << shift;
  

• if (pwm->hwpwm < 2)
  

•         regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
  

• else
  

•         regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
  

• regmap_update_bits(priv->regmap, TIM_BDTR,
  

•                    TIM_BDTR_MOE | TIM_BDTR_AOE,
  

•                    TIM_BDTR_MOE | TIM_BDTR_AOE);
  

• return 0;
  

+}

+static int stm32_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,

•                           enum pwm_polarity polarity)
  

+{

• u32 mask;
  

• struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
  

• mask = TIM_CCER_CC1P << (pwm->hwpwm * 4);
  

• if (priv->have_complementary_output)
  

•         mask |= TIM_CCER_CC1NP << (pwm->hwpwm * 4);
  

• regmap_update_bits(priv->regmap, TIM_CCER, mask,
  

•                    polarity == PWM_POLARITY_NORMAL ? 0 : mask);
  

• return 0;
  

+}

+static int stm32_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{

• u32 mask;
  

• struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
  

• clk_enable(priv->clk);
  

This can fail, so its return value should be checked. Also, I don't see a clk_prepare() anywhere. Is that something that maybe the MFD driver should be doing? It currently isn't.

I will check the return value. You are right clk_prepare() is done in mfd driver when calling devm_regmap_init_mmio_clk()

...

• /* Enable channel */
  

• mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
  

• if (priv->have_complementary_output)
  

•         mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
  

• regmap_update_bits(priv->regmap, TIM_CCER, mask, mask);
  

• /* Make sure that registers are updated */
  

• regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
  

• /* Enable controller */
  

• regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
  

• return 0;
  

+}

+static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{

• u32 mask;
  

• struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
  

• /* Disable channel */
  

• mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
  

• if (priv->have_complementary_output)
  

•         mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
  

• regmap_update_bits(priv->regmap, TIM_CCER, mask, 0);
  

• /* When all channels are disabled, we can disable the controller */
  

• if (!active_channels(priv))
  

•         regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
  

• clk_disable(priv->clk);
  

+}

+static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

•                    struct pwm_state *state)
  

+{

• struct pwm_state curstate;
  

• bool enabled;
  

• int ret;
  

• pwm_get_state(pwm, &curstate);
  

• enabled = curstate.enabled;
  

There should be no need to do this in drivers. pwm_get_state() is for PWM API users. Drivers can directly dereference pwm->state.

ok

...

• if (enabled && !state->enabled) {
  

•         stm32_pwm_disable(chip, pwm);
  

•         return 0;
  

• }
  

• if (state->polarity != curstate.polarity && enabled)
  

•         stm32_pwm_set_polarity(chip, pwm, state->polarity);
  

So that's kind of a violation of atomic API semantics. The above means that if you have a PWM in the following state:

    enabled: no
    polarity: normal

and want to set this:

    enabled: yes
    polarity: inversed

then you will ignore the new polarity setting. What's the reason for "&& enabled) in the conditional above?

There is no reason, I will remove it.

...

• ret = stm32_pwm_config(chip, pwm, state->duty_cycle, state->period);
  

• if (ret)
  

•         return ret;
  

• if (!enabled && state->enabled)
  

•         ret = stm32_pwm_enable(chip, pwm);
  

• return ret;
  

+}

Would it be possible to merge stm32_pwm_disable(), stm32_pwm_enable(), stm32_pwm_set_polarity() and stm32_pwm_config() into stm32_pwm_apply()? Part of the reason for the atomic API was to make it easier to write these drivers, but your implementation effectively copies what the transitional helpers do.

It might not make a difference technically in your case, but I think it'd make the implementation more compact and set a better example for future reference.

hmm... it will create a fat function with lot of where enabling/disabling/configuration will be mixed I'm really not convince that will more compact and readable.

...

+static const struct pwm_ops stm32pwm_ops = {

• .owner = THIS_MODULE,
  

• .apply = stm32_pwm_apply,
  

+};

+static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,

•                             int level, int filter)
  

+{

• u32 bdtr = TIM_BDTR_BKE;
  

• if (level)
  

•         bdtr |= TIM_BDTR_BKP;
  

• bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BKF_SHIFT;
  

• regmap_update_bits(priv->regmap,
  

•                    TIM_BDTR, TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF,
  

•                    bdtr);
  

• regmap_read(priv->regmap, TIM_BDTR, &bdtr);
  

• return (bdtr & TIM_BDTR_BKE) ? 0 : -EINVAL;
  

+}

+static int stm32_pwm_set_breakinput2(struct stm32_pwm *priv,

•                              int level, int filter)
  

+{

• u32 bdtr = TIM_BDTR_BK2E;
  

• if (level)
  

•         bdtr |= TIM_BDTR_BK2P;
  

• bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BK2F_SHIFT;
  

• regmap_update_bits(priv->regmap,
  

•                    TIM_BDTR, TIM_BDTR_BK2E |
  

•                    TIM_BDTR_BK2P |
  

•                    TIM_BDTR_BK2F,
  

•                    bdtr);
  

• regmap_read(priv->regmap, TIM_BDTR, &bdtr);
  

• return (bdtr & TIM_BDTR_BK2E) ? 0 : -EINVAL;
  

+}

As far as I can tell the only difference here is the various bit positions. Can you collapse the above two functions and add a new parameter to unify some code?

Yes it is all about bit shifting, I had try unify those two functions with index has additional parameter but it just add if() before each lines so no real benefit for code size.

...

+static int stm32_pwm_apply_breakinputs(struct stm32_pwm *priv,

•                                struct device_node *np)
  

+{

• struct stm32_breakinput breakinput[MAX_BREAKINPUT];
  

• int nb, ret, i, array_size;
  

• nb = of_property_count_elems_of_size(np, "st,breakinput",
  

•                                      sizeof(struct stm32_breakinput));
  

• /*
  

•  * Because "st,breakinput" parameter is optional do not make probe
  

•  * failed if it doesn't exist.
  

•  */
  

• if (nb <= 0)
  

•         return 0;
  

• if (nb > MAX_BREAKINPUT)
  

•         return -EINVAL;
  

• array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
  

• ret = of_property_read_u32_array(np, "st,breakinput",
  

•                                  &breakinput[0].index, array_size);
  

Maybe (u32 *)breakinput? That would make it more resilient against changes in ordering of fields in the struct. Granted, that's not likely to change, but I think it's a good idea in general to write code in a way that's safe in a more general case. That way if somebody ever were to copy from your code and then decide to reorder fields in their code things wouldn't fall apart.

Yes it is not suppose to change but I will use (u32 *)breakinput.

...

• if (ret)
  

•         return ret;
  

• for (i = 0; i < nb && !ret; i++) {
  

•         switch (breakinput[i].index) {
  

•         case 0:
  

•         {
  

•                 ret = stm32_pwm_set_breakinput(priv,
  

•                                                breakinput[i].level,
  

•                                                breakinput[i].filter);
  

•                 break;
  

•         }
  

Curly braces are unnecessary here.

removed

...

•         case 1:
  

•         {
  

•                 ret = stm32_pwm_set_breakinput2(priv,
  

•                                                 breakinput[i].level,
  

•                                                 breakinput[i].filter);
  

•                 break;
  

•         }
  

•         default:
  

•         {
  

•                 ret = -EINVAL;
  

•                 break;
  

•         }
  

•         }
  

• }
  

• return ret;
  

+}

+static void stm32_pwm_detect_complementary(struct stm32_pwm *priv) +{

• u32 ccer;
  

• /*
  

•  * If complementary bit doesn't exist writing 1 will have no
  

•  * effect so we can detect it.
  

•  */
  

• regmap_update_bits(priv->regmap,
  

•                    TIM_CCER, TIM_CCER_CC1NE, TIM_CCER_CC1NE);
  

• regmap_read(priv->regmap, TIM_CCER, &ccer);
  

• regmap_update_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE, 0);
  

This is strange: why are we disabling outputs here? Shouldn't the last line here undo the first instead?

Yes it should TIM_CCER_CC1NE not TIM_CCER_CCXE, I will fix it, thanks

...

• priv->have_complementary_output = (ccer != 0);
  

+}

+static void stm32_pwm_detect_channels(struct stm32_pwm *priv) +{

• u32 ccer;
  

• /*
  

•  * If channels enable bits don't exist writing 1 will have no
  

•  * effect so we can detect and count them.
  

•  */
  

• regmap_update_bits(priv->regmap,
  

•                    TIM_CCER, TIM_CCER_CCXE, TIM_CCER_CCXE);
  

• regmap_read(priv->regmap, TIM_CCER, &ccer);
  

• regmap_update_bits(priv->regmap, TIM_CCER, TIM_CCER_CCXE, 0);
  

Does this have the potential to glitch? I suspect that the clock may not be on at this point and therefore no PWM outputs will be generated, but is that guaranteed to always be the case?

Set TIM_CCER_CCXE isn't enough to enable PWM generation, TIM_CR1_CEN in TIM_CR1 register must also to set so no risk of glitch here

Thierry

2017-01-18 11:16 GMT+01:00 Thierry Reding thierry.reding@gmail.com:

On Thu, Jan 05, 2017 at 10:25:40AM +0100, Benjamin Gaignard wrote: [...]

...

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index f92dd41..88035c0 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -397,6 +397,15 @@ config PWM_STI To compile this driver as a module, choose M here: the module will be called pwm-sti.

+config PWM_STM32

• tristate "STMicroelectronics STM32 PWM"
  

• depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) || COMPILE_TEST
  

One other thing: is the dependency on ARCH_STM32 and OF necessary here? ARCH_STM32 and OF are both pulled in by MFD_STM32_TIMERS. The dependency is probably fine for MFD_STM32_TIMERS, though even there && OF seems too much, since it's already pulled in via ARCH_STM32 -> ARM_SINGLE_ARMV7M -> USE_OF.

Said like that MFD_STM32_TIMERS is enough so I will keep this one

Thanks

Thierry

On 05/01/17 09:25, Benjamin Gaignard wrote:

Define bindings for STM32 timer trigger

version 4:

• remove triggers enumeration from DT
• add reg parameter

version 3:

• change file name
• add cross reference with mfd bindings

version 2:

• only keep one compatible
• add DT parameters to set lists of the triggers: one list describe the triggers created by the device another one give the triggers accepted by the device

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

Acked-by: Jonathan Cameron jic23@kernel.org

---

.../bindings/iio/timer/stm32-timer-trigger.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt

diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt b/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt new file mode 100644 index 0000000..36a6c4a --- /dev/null +++ b/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt @@ -0,0 +1,23 @@ +STMicroelectronics STM32 Timers IIO timer bindings

+Must be a sub-node of an STM32 Timers device tree node. +See ../mfd/stm32-timers.txt for details about the parent node.

+Required parameters: +- compatible: Must be "st,stm32-timer-trigger". +- reg: Define triggers configuration of the hardware IP.

+Example:

• timers@40010000 {

• #address-cells = <1>;
  

• #size-cells = <0>;
  

• compatible = "st,stm32-timers";
  

• reg = <0x40010000 0x400>;
  

• clocks = <&rcc 0 160>;
  

• clock-names = "clk_int";
  

• timer {
  

• 	compatible = "st,stm32-timer-trigger";
  

• 	reg = <0>;
  

• };
  

• };

2017-01-09 19:04 GMT+01:00 Rob Herring robh@kernel.org:

On Thu, Jan 05, 2017 at 10:25:41AM +0100, Benjamin Gaignard wrote:

...

Define bindings for STM32 timer trigger

version 4:

• remove triggers enumeration from DT
• add reg parameter

version 3:

• change file name
• add cross reference with mfd bindings

version 2:

• only keep one compatible
• add DT parameters to set lists of the triggers: one list describe the triggers created by the device another one give the triggers accepted by the device

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

.../bindings/iio/timer/stm32-timer-trigger.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt

diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt b/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt new file mode 100644 index 0000000..36a6c4a --- /dev/null +++ b/Documentation/devicetree/bindings/iio/timer/stm32-timer-trigger.txt @@ -0,0 +1,23 @@ +STMicroelectronics STM32 Timers IIO timer bindings

+Must be a sub-node of an STM32 Timers device tree node. +See ../mfd/stm32-timers.txt for details about the parent node.

+Required parameters: +- compatible: Must be "st,stm32-timer-trigger". +- reg: Define triggers configuration of the hardware IP.

Valid values? Not sure this makes sense for use of reg. reg should be how you address or id a device.

I use reg like an id to distinguish device and get the right configuration inside the driver.

...

+Example:

• timers@40010000 {
  

•         #address-cells = <1>;
  

•         #size-cells = <0>;
  

•         compatible = "st,stm32-timers";
  

•         reg = <0x40010000 0x400>;
  

•         clocks = <&rcc 0 160>;
  

•         clock-names = "clk_int";
  

•         timer {
  

This would also need an unit address.

Ok I will fix that

...

•                 compatible = "st,stm32-timer-trigger";
  

•                 reg = <0>;
  

•         };
  

• };
  

-- 1.9.1

On 05/01/17 09:25, Benjamin Gaignard wrote:

Timers IPs can be used to generate triggers for other IPs like DAC or ADC. Each trigger may result of timer internals signals like counter enable, reset or edge, this configuration could be done through "master_mode" device attribute.

Since triggers could be used by DAC or ADC their names are defined in include/ nux/iio/timer/stm32-timer-trigger.h and is_stm32_iio_timer_trigger function could be used to check if the trigger is valid or not.

"trgo" trigger have a "sampling_frequency" attribute which allow to configure timer sampling frequency.

version 7:

• remove all iio_device related code
• move driver into trigger directory

version 5:

• simplify tables of triggers
• only create an IIO device when needed

version 4:

• get triggers configuration from "reg" in DT
• add tables of triggers
• sampling frequency is enable/disable when writing in trigger sampling_frequency attribute
• no more use of interruptions

version 3:

• change compatible to "st,stm32-timer-trigger"
• fix attributes access right
• use string instead of int for master_mode and slave_mode
• document device attributes in sysfs-bus-iio-timer-stm32

version 2:

• keep only one compatible
• use st,input-triggers-names and st,output-triggers-names to know which triggers are accepted and/or create by the device

Signed-off-by: Benjamin Gaignard benjamin.gaignard@st.com

One line snuck in that shouldn't be there (the one in iio/Kconfig). Other than that

Acked-by: Jonathan Cameron jic23@kernel.org

I didn't get a chance to look at doing trigger trees unfortunately. Not sure when I'll next get a window to work on new stuff.

Thanks,

Jonathan

---

.../ABI/testing/sysfs-bus-iio-timer-stm32 | 29 ++ drivers/iio/Kconfig | 1 - drivers/iio/trigger/Kconfig | 10 + drivers/iio/trigger/Makefile | 1 + drivers/iio/trigger/stm32-timer-trigger.c | 340 +++++++++++++++++++++ include/linux/iio/timer/stm32-timer-trigger.h | 62 ++++ 6 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 create mode 100644 drivers/iio/trigger/stm32-timer-trigger.c create mode 100644 include/linux/iio/timer/stm32-timer-trigger.h

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 new file mode 100644 index 0000000..a7f5177 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-timer-stm32 @@ -0,0 +1,29 @@ +What: /sys/bus/iio/devices/triggerX/master_mode_available +KernelVersion: 4.10 +Contact: benjamin.gaignard@st.com +Description:

• Reading returns the list possible master modes which are:
  

• - "reset"     :	The UG bit from the TIMx_EGR register is used as trigger output (TRGO).
  

• - "enable"    : The Counter Enable signal CNT_EN is used as trigger output.
  

• - "update"    : The update event is selected as trigger output.
  

• 		For instance a master timer can then be used as a prescaler for a slave timer.
  

• - "compare_pulse" : The trigger output send a positive pulse when the CC1IF flag is to be set.
  

• - "OC1REF"    : OC1REF signal is used as trigger output.
  

• - "OC2REF"    : OC2REF signal is used as trigger output.
  

• - "OC3REF"    : OC3REF signal is used as trigger output.
  

• - "OC4REF"    : OC4REF signal is used as trigger output.
  

+What: /sys/bus/iio/devices/triggerX/master_mode +KernelVersion: 4.10 +Contact: benjamin.gaignard@st.com +Description:

• Reading returns the current master modes.
  

• Writing set the master mode
  

+What: /sys/bus/iio/devices/triggerX/sampling_frequency +KernelVersion: 4.10 +Contact: benjamin.gaignard@st.com +Description:

• Reading returns the current sampling frequency.
  

• Writing an value different of 0 set and start sampling.
  

• Writing 0 stop sampling.
  

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index a918270..6b0c427 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig @@ -92,5 +92,4 @@ source "drivers/iio/potentiostat/Kconfig" source "drivers/iio/pressure/Kconfig" source "drivers/iio/proximity/Kconfig" source "drivers/iio/temperature/Kconfig"

Please fix this!!

endif # IIO diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig index 809b2e7..72d3a32 100644 --- a/drivers/iio/trigger/Kconfig +++ b/drivers/iio/trigger/Kconfig @@ -24,6 +24,16 @@ config IIO_INTERRUPT_TRIGGER To compile this driver as a module, choose M here: the module will be called iio-trig-interrupt. +config IIO_STM32_TIMER_TRIGGER

• tristate "STM32 Timer Trigger"
• depends on (ARCH_STM32 && OF && MFD_STM32_TIMERS) || COMPILE_TEST
• help

•  Select this option to enable STM32 Timer Trigger
  

•  To compile this driver as a module, choose M here: the
  

•  module will be called stm32-timer-trigger.
  

config IIO_TIGHTLOOP_TRIGGER tristate "A kthread based hammering loop trigger" depends on IIO_SW_TRIGGER diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile index aab4dc2..5c4ecd3 100644 --- a/drivers/iio/trigger/Makefile +++ b/drivers/iio/trigger/Makefile @@ -6,5 +6,6 @@ obj-$(CONFIG_IIO_HRTIMER_TRIGGER) += iio-trig-hrtimer.o obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o +obj-$(CONFIG_IIO_STM32_TIMER_TRIGGER) += stm32-timer-trigger.o obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o obj-$(CONFIG_IIO_TIGHTLOOP_TRIGGER) += iio-trig-loop.o diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c new file mode 100644 index 0000000..409fb0c --- /dev/null +++ b/drivers/iio/trigger/stm32-timer-trigger.c @@ -0,0 +1,340 @@ +/*

• • Copyright (C) STMicroelectronics 2016
• • Author: Benjamin Gaignard benjamin.gaignard@st.com
• • License terms: GNU General Public License (GPL), version 2
• */

+#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/timer/stm32-timer-trigger.h> +#include <linux/iio/trigger.h> +#include <linux/mfd/stm32-timers.h> +#include <linux/module.h> +#include <linux/platform_device.h>

+#define MAX_TRIGGERS 6

+/* List the triggers created by each timer */ +static const void *triggers_table[][MAX_TRIGGERS] = {

• { TIM1_TRGO, TIM1_CH1, TIM1_CH2, TIM1_CH3, TIM1_CH4,},
• { TIM2_TRGO, TIM2_CH1, TIM2_CH2, TIM2_CH3, TIM2_CH4,},
• { TIM3_TRGO, TIM3_CH1, TIM3_CH2, TIM3_CH3, TIM3_CH4,},
• { TIM4_TRGO, TIM4_CH1, TIM4_CH2, TIM4_CH3, TIM4_CH4,},
• { TIM5_TRGO, TIM5_CH1, TIM5_CH2, TIM5_CH3, TIM5_CH4,},
• { TIM6_TRGO,},
• { TIM7_TRGO,},
• { TIM8_TRGO, TIM8_CH1, TIM8_CH2, TIM8_CH3, TIM8_CH4,},
• { TIM9_TRGO, TIM9_CH1, TIM9_CH2,},
• { TIM12_TRGO, TIM12_CH1, TIM12_CH2,},

+};

+struct stm32_timer_trigger {

• struct device *dev;
• struct regmap *regmap;
• struct clk *clk;
• u32 max_arr;
• const void *triggers;

+};

+static int stm32_timer_start(struct stm32_timer_trigger *priv,

• 	     unsigned int frequency)
  

+{

• unsigned long long prd, div;
• int prescaler = 0;
• u32 ccer, cr1;
• /* Period and prescaler values depends of clock rate */
• div = (unsigned long long)clk_get_rate(priv->clk);
• do_div(div, frequency);
• prd = div;
• /*

• * Increase prescaler value until we get a result that fit
  

• * with auto reload register maximum value.
  

• */
  

• while (div > priv->max_arr) {

• prescaler++;
  

• div = prd;
  

• do_div(div, (prescaler + 1));
  

• }
• prd = div;
• if (prescaler > MAX_TIM_PSC) {

• dev_err(priv->dev, "prescaler exceeds the maximum value\n");
  

• return -EINVAL;
  

• }
• /* Check if nobody else use the timer */
• regmap_read(priv->regmap, TIM_CCER, &ccer);
• if (ccer & TIM_CCER_CCXE)

• return -EBUSY;
  

• regmap_read(priv->regmap, TIM_CR1, &cr1);
• if (!(cr1 & TIM_CR1_CEN))

• clk_enable(priv->clk);
  

• regmap_write(priv->regmap, TIM_PSC, prescaler);
• regmap_write(priv->regmap, TIM_ARR, prd - 1);
• regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
• /* Force master mode to update mode */
• regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
• /* Make sure that registers are updated */
• regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
• /* Enable controller */
• regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
• return 0;

+}

+static void stm32_timer_stop(struct stm32_timer_trigger *priv) +{

• u32 ccer, cr1;
• regmap_read(priv->regmap, TIM_CCER, &ccer);
• if (ccer & TIM_CCER_CCXE)

• return;
  

• regmap_read(priv->regmap, TIM_CR1, &cr1);
• if (cr1 & TIM_CR1_CEN)

• clk_disable(priv->clk);
  

• /* Stop timer */
• regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
• regmap_write(priv->regmap, TIM_PSC, 0);
• regmap_write(priv->regmap, TIM_ARR, 0);
• /* Make sure that registers are updated */
• regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);

+}

+static ssize_t stm32_tt_store_frequency(struct device *dev,

• 			struct device_attribute *attr,
  

• 			const char *buf, size_t len)
  

+{

• struct iio_trigger *trig = to_iio_trigger(dev);
• struct stm32_timer_trigger *priv = iio_trigger_get_drvdata(trig);
• unsigned int freq;
• int ret;
• ret = kstrtouint(buf, 10, &freq);
• if (ret)

• return ret;
  

• if (freq == 0) {

• stm32_timer_stop(priv);
  

• } else {

• ret = stm32_timer_start(priv, freq);
  

• if (ret)
  

• 	return ret;
  

• }
• return len;

+}

+static ssize_t stm32_tt_read_frequency(struct device *dev,

• 		       struct device_attribute *attr, char *buf)
  

+{

• struct iio_trigger *trig = to_iio_trigger(dev);
• struct stm32_timer_trigger *priv = iio_trigger_get_drvdata(trig);
• u32 psc, arr, cr1;
• unsigned long long freq = 0;
• regmap_read(priv->regmap, TIM_CR1, &cr1);
• regmap_read(priv->regmap, TIM_PSC, &psc);
• regmap_read(priv->regmap, TIM_ARR, &arr);
• if (psc && arr && (cr1 & TIM_CR1_CEN)) {

• freq = (unsigned long long)clk_get_rate(priv->clk);
  

• do_div(freq, psc);
  

• do_div(freq, arr);
  

• }
• return sprintf(buf, "%d\n", (unsigned int)freq);

+}

+static IIO_DEV_ATTR_SAMP_FREQ(0660,

• 	      stm32_tt_read_frequency,
  

• 	      stm32_tt_store_frequency);
  

+static char *master_mode_table[] = {

• "reset",
• "enable",
• "update",
• "compare_pulse",
• "OC1REF",
• "OC2REF",
• "OC3REF",
• "OC4REF"

+};

+static ssize_t stm32_tt_show_master_mode(struct device *dev,

• 			 struct device_attribute *attr,
  

• 			 char *buf)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
• u32 cr2;
• regmap_read(priv->regmap, TIM_CR2, &cr2);
• cr2 = (cr2 & TIM_CR2_MMS) >> TIM_CR2_MMS_SHIFT;
• return snprintf(buf, PAGE_SIZE, "%s\n", master_mode_table[cr2]);

+}

+static ssize_t stm32_tt_store_master_mode(struct device *dev,

• 			  struct device_attribute *attr,
  

• 			  const char *buf, size_t len)
  

+{

• struct iio_dev *indio_dev = dev_to_iio_dev(dev);
• struct stm32_timer_trigger *priv = iio_priv(indio_dev);
• int i;
• for (i = 0; i < ARRAY_SIZE(master_mode_table); i++) {

• if (!strncmp(master_mode_table[i], buf,
  

• 	     strlen(master_mode_table[i]))) {
  

• 	regmap_update_bits(priv->regmap, TIM_CR2,
  

• 			   TIM_CR2_MMS, i << TIM_CR2_MMS_SHIFT);
  

• 	/* Make sure that registers are updated */
  

• 	regmap_update_bits(priv->regmap, TIM_EGR,
  

• 			   TIM_EGR_UG, TIM_EGR_UG);
  

• 	return len;
  

• }
  

• }
• return -EINVAL;

+}

+static IIO_CONST_ATTR(master_mode_available,

• "reset enable update compare_pulse OC1REF OC2REF OC3REF OC4REF");

+static IIO_DEVICE_ATTR(master_mode, 0660,

•        stm32_tt_show_master_mode,
  

•        stm32_tt_store_master_mode,
  

•        0);
  

+static struct attribute *stm32_trigger_attrs[] = {

• &iio_dev_attr_sampling_frequency.dev_attr.attr,
• &iio_dev_attr_master_mode.dev_attr.attr,
• &iio_const_attr_master_mode_available.dev_attr.attr,
• NULL,

+};

+static const struct attribute_group stm32_trigger_attr_group = {

• .attrs = stm32_trigger_attrs,

+};

+static const struct attribute_group *stm32_trigger_attr_groups[] = {

• &stm32_trigger_attr_group,
• NULL,

+};

+static const struct iio_trigger_ops timer_trigger_ops = {

• .owner = THIS_MODULE,

+};

+static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv) +{

• int ret;
• const char * const *cur = priv->triggers;
• while (cur && *cur) {

• struct iio_trigger *trig;
  

• trig = devm_iio_trigger_alloc(priv->dev, "%s", *cur);
  

• if  (!trig)
  

• 	return -ENOMEM;
  

• trig->dev.parent = priv->dev->parent;
  

• trig->ops = &timer_trigger_ops;
  

• /*
  

•  * sampling frequency and master mode attributes
  

•  * should only be available on trgo trigger which
  

•  * is always the first in the list.
  

•  */
  

• if (cur == priv->triggers)
  

• 	trig->dev.groups = stm32_trigger_attr_groups;
  

• iio_trigger_set_drvdata(trig, priv);
  

• ret = devm_iio_trigger_register(priv->dev, trig);
  

• if (ret)
  

• 	return ret;
  

• cur++;
  

• }
• return 0;

+}

+/**

• • is_stm32_timer_trigger
• • @trig: trigger to be checked
• • return true if the trigger is a valid stm32 iio timer trigger
• • either return false
• */

+bool is_stm32_timer_trigger(struct iio_trigger *trig) +{

• return (trig->ops == &timer_trigger_ops);

+} +EXPORT_SYMBOL(is_stm32_timer_trigger);

+static int stm32_timer_trigger_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct stm32_timer_trigger *priv;
• struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
• unsigned int index;
• int ret;
• if (of_property_read_u32(dev->of_node, "reg", &index))

• return -EINVAL;
  

• if (index >= ARRAY_SIZE(triggers_table))

• return -EINVAL;
  

• priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
• if (!priv)

• return -ENOMEM;
  

• priv->dev = dev;
• priv->regmap = ddata->regmap;
• priv->clk = ddata->clk;
• priv->max_arr = ddata->max_arr;
• priv->triggers = triggers_table[index];
• ret = stm32_setup_iio_triggers(priv);
• if (ret)

• return ret;
  

• platform_set_drvdata(pdev, priv);
• return 0;

+}

+static const struct of_device_id stm32_trig_of_match[] = {

• { .compatible = "st,stm32-timer-trigger", },
• { /* end node */ },

+}; +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);

+static struct platform_driver stm32_timer_trigger_driver = {

• .probe = stm32_timer_trigger_probe,
• .driver = {

• .name = "stm32-timer-trigger",
  

• .of_match_table = stm32_trig_of_match,
  

• },

+}; +module_platform_driver(stm32_timer_trigger_driver);

+MODULE_ALIAS("platform: stm32-timer-trigger"); +MODULE_DESCRIPTION("STMicroelectronics STM32 Timer Trigger driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/iio/timer/stm32-timer-trigger.h b/include/linux/iio/timer/stm32-timer-trigger.h new file mode 100644 index 0000000..55535ae --- /dev/null +++ b/include/linux/iio/timer/stm32-timer-trigger.h @@ -0,0 +1,62 @@ +/*

• • Copyright (C) STMicroelectronics 2016
• • Author: Benjamin Gaignard benjamin.gaignard@st.com
• • License terms: GNU General Public License (GPL), version 2
• */

+#ifndef _STM32_TIMER_TRIGGER_H_ +#define _STM32_TIMER_TRIGGER_H_

+#define TIM1_TRGO "tim1_trgo" +#define TIM1_CH1 "tim1_ch1" +#define TIM1_CH2 "tim1_ch2" +#define TIM1_CH3 "tim1_ch3" +#define TIM1_CH4 "tim1_ch4"

+#define TIM2_TRGO "tim2_trgo" +#define TIM2_CH1 "tim2_ch1" +#define TIM2_CH2 "tim2_ch2" +#define TIM2_CH3 "tim2_ch3" +#define TIM2_CH4 "tim2_ch4"

+#define TIM3_TRGO "tim3_trgo" +#define TIM3_CH1 "tim3_ch1" +#define TIM3_CH2 "tim3_ch2" +#define TIM3_CH3 "tim3_ch3" +#define TIM3_CH4 "tim3_ch4"

+#define TIM4_TRGO "tim4_trgo" +#define TIM4_CH1 "tim4_ch1" +#define TIM4_CH2 "tim4_ch2" +#define TIM4_CH3 "tim4_ch3" +#define TIM4_CH4 "tim4_ch4"

+#define TIM5_TRGO "tim5_trgo" +#define TIM5_CH1 "tim5_ch1" +#define TIM5_CH2 "tim5_ch2" +#define TIM5_CH3 "tim5_ch3" +#define TIM5_CH4 "tim5_ch4"

+#define TIM6_TRGO "tim6_trgo"

+#define TIM7_TRGO "tim7_trgo"

+#define TIM8_TRGO "tim8_trgo" +#define TIM8_CH1 "tim8_ch1" +#define TIM8_CH2 "tim8_ch2" +#define TIM8_CH3 "tim8_ch3" +#define TIM8_CH4 "tim8_ch4"

+#define TIM9_TRGO "tim9_trgo" +#define TIM9_CH1 "tim9_ch1" +#define TIM9_CH2 "tim9_ch2"

+#define TIM12_TRGO "tim12_trgo" +#define TIM12_CH1 "tim12_ch1" +#define TIM12_CH2 "tim12_ch2"

+bool is_stm32_timer_trigger(struct iio_trigger *trig);

+#endif