From: thierry.reding@gmail•com (Thierry Reding)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm
Date: Mon, 5 Dec 2016 12:30:58 +0100 [thread overview]
Message-ID: <20161205113058.GG19891@ulmo.ba.sec> (raw)
In-Reply-To: <CA+M3ks7_utdw1c0A0m+RzuPTfvFAv5d8HoQvQDzXwHR8M6RKJA@mail.gmail.com>
On Mon, Dec 05, 2016 at 12:02:45PM +0100, Benjamin Gaignard wrote:
> 2016-12-05 8:23 GMT+01:00 Thierry Reding <thierry.reding@gmail•com>:
> > On Fri, Dec 02, 2016 at 11:17:19AM +0100, Benjamin Gaignard wrote:
> >> This driver add support for pwm driver on stm32 platform.
> >
> > "adds". Also please use PWM in prose because it's an abbreviation.
> >
> >> The SoC have multiple instances of the hardware IP and each
> >> of them could have small differences: number of channels,
> >> complementary output, counter register size...
> >> Use DT parameters to handle those differentes configuration
> >
> > "different configurations"
>
> ok
>
> >
> >>
> >> version 2:
> >> - only keep one comptatible
> >> - use DT paramaters to discover hardware block configuration
> >
> > "parameters"
>
> ok
>
> >
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st•com>
> >> ---
> >> drivers/pwm/Kconfig | 8 ++
> >> drivers/pwm/Makefile | 1 +
> >> drivers/pwm/pwm-stm32.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 294 insertions(+)
> >> create mode 100644 drivers/pwm/pwm-stm32.c
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index bf01288..a89fdba 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -388,6 +388,14 @@ config PWM_STI
> >> To compile this driver as a module, choose M here: the module
> >> will be called pwm-sti.
> >>
> >> +config PWM_STM32
> >> + bool "STMicroelectronics STM32 PWM"
> >> + depends on ARCH_STM32
> >> + depends on OF
> >> + select MFD_STM32_GP_TIMER
> >
> > Should that be a "depends on"?
>
> I think select is fine because the wanted feature is PWM not the mfd part
I think in that case you may want to hide the MFD Kconfig option. See
Documentation/kbuild/kconfig-language.txt (notes about select) for the
details.
[...]
> >> +};
> >> +
> >> +#define to_stm32_pwm_dev(x) container_of(chip, struct stm32_pwm_dev, chip)
> >
> > Please turn this into a static inline.
>
> with putting struct pwm_chip as first filed I will just cast the structure
Don't do that, please. container_of() is still preferred because it is
correct and won't break even if you (or somebody else) changes the order
in the future. The fact that it might be optimized away is a detail, or
a micro-optimization. Force-casting is a bad idea because it won't catch
errors if for some reason the field doesn't remain in the first position
forever.
> >> +static void stm32_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> + u32 mask;
> >> + struct stm32_pwm_dev *dev = to_stm32_pwm_dev(chip);
> >> +
> >> + /* Disable channel */
> >> + mask = TIM_CCER_CC1E << (pwm->hwpwm * 4);
> >> + if (dev->caps & CAP_COMPLEMENTARY)
> >> + mask |= TIM_CCER_CC1NE << (pwm->hwpwm * 4);
> >> +
> >> + regmap_update_bits(dev->regmap, TIM_CCER, mask, 0);
> >> +
> >> + /* When all channels are disabled, we can disable the controller */
> >> + if (!__active_channels(dev))
> >> + regmap_update_bits(dev->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> >> +
> >> + clk_disable(dev->clk);
> >> +}
> >
> > All of the above can be folded into the ->apply() hook for atomic PWM
> > support.
> >
> > Also, in the above you use clk_enable() in the ->enable() callback and
> > clk_disable() in ->disable(). If you need the clock to access registers
> > you'll have to enabled it in the others as well because there are no
> > guarantees that configuration will only happen between ->enable() and
> > ->disable(). Atomic PWM simplifies this a bit, but you still need to be
> > careful about when to enable the clock in your ->apply() hook.
>
> I have used regmap functions enable/disable clk for me when accessing to
> the registers.
> I only have to take care of clk enable/disable when PWM state change.
Okay, that's fine then.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161205/79897d78/attachment.sig>
next prev parent reply other threads:[~2016-12-05 11:30 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-02 10:17 [PATCH v3 0/7] Add pwm and IIO timer drivers for stm32 Benjamin Gaignard
2016-12-02 10:17 ` [PATCH v3 1/7] MFD: add bindings for stm32 general purpose timer driver Benjamin Gaignard
2016-12-02 10:17 ` [PATCH v3 2/7] MFD: add " Benjamin Gaignard
2016-12-02 14:29 ` Lee Jones
2016-12-02 10:17 ` [PATCH v3 3/7] PWM: add pwm-stm32 DT bindings Benjamin Gaignard
2016-12-02 14:23 ` Lee Jones
2016-12-05 6:53 ` Thierry Reding
2016-12-05 8:35 ` Lee Jones
2016-12-05 10:50 ` Thierry Reding
2016-12-05 11:08 ` Benjamin Gaignard
2016-12-05 11:23 ` Thierry Reding
2016-12-05 12:12 ` Benjamin Gaignard
2016-12-05 12:21 ` Thierry Reding
2016-12-02 10:17 ` [PATCH v3 4/7] PWM: add pwm driver for stm32 plaftorm Benjamin Gaignard
2016-12-05 7:23 ` Thierry Reding
2016-12-05 8:31 ` Lee Jones
2016-12-05 11:02 ` Benjamin Gaignard
2016-12-05 11:30 ` Thierry Reding [this message]
2016-12-02 10:17 ` [PATCH v3 5/7] IIO: add bindings for stm32 timer trigger driver Benjamin Gaignard
2016-12-02 13:59 ` Lee Jones
2016-12-02 14:23 ` Benjamin Gaignard
2016-12-03 9:35 ` Jonathan Cameron
2016-12-02 10:17 ` [PATCH v3 6/7] IIO: add STM32 " Benjamin Gaignard
2016-12-02 13:57 ` Lee Jones
2016-12-02 10:17 ` [PATCH v3 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT Benjamin Gaignard
2016-12-02 13:22 ` Lee Jones
2016-12-03 9:42 ` Jonathan Cameron
2016-12-05 8:54 ` Lee Jones
2016-12-05 16:09 ` Alexandre Torgue
2016-12-06 9:48 ` Lee Jones
2016-12-06 9:56 ` Alexandre Torgue
2016-12-06 12:54 ` Lee Jones
2016-12-02 13:41 ` Alexandre Torgue
2016-12-03 9:45 ` Jonathan Cameron
2016-12-03 9:32 ` [PATCH v3 0/7] Add pwm and IIO timer drivers for stm32 Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161205113058.GG19891@ulmo.ba.sec \
--to=thierry.reding@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox