From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8DC6C77B60 for ; Mon, 27 Mar 2023 20:40:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229452AbjC0UkV (ORCPT ); Mon, 27 Mar 2023 16:40:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37452 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229456AbjC0UkU (ORCPT ); Mon, 27 Mar 2023 16:40:20 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 08C3610D1 for ; Mon, 27 Mar 2023 13:40:18 -0700 (PDT) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pgtdM-0008NT-5F; Mon, 27 Mar 2023 22:40:04 +0200 Received: from [2a0a:edc0:0:900:1d::77] (helo=ptz.office.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.94.2) (envelope-from ) id 1pgtdJ-0079gp-In; Mon, 27 Mar 2023 22:40:01 +0200 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.94.2) (envelope-from ) id 1pgtdI-008VtN-Qn; Mon, 27 Mar 2023 22:40:00 +0200 Date: Mon, 27 Mar 2023 22:40:00 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Biju Das Cc: Thierry Reding , linux-pwm@vger.kernel.org, Lee Jones , Daniel Lezcano , William Breathitt Gray , Geert Uytterhoeven , Chris Paterson , Prabhakar Mahadev Lad , linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v14 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver Message-ID: <20230327204000.x67sybfbp34udwfg@pengutronix.de> References: <20230310170654.268047-1-biju.das.jz@bp.renesas.com> <20230310170654.268047-7-biju.das.jz@bp.renesas.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="wtyunm6bb7l6o573" Content-Disposition: inline In-Reply-To: <20230310170654.268047-7-biju.das.jz@bp.renesas.com> X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-renesas-soc@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-renesas-soc@vger.kernel.org --wtyunm6bb7l6o573 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello, I have the feeling this driver is more complicated than necessary. On Fri, Mar 10, 2023 at 05:06:54PM +0000, Biju Das wrote: > The RZ/G2L Multi-Function Timer Pulse Unit 3 (a.k.a MTU3a) uses > one counter and two match components to configure duty_cycle > and period to generate PWM output waveform. >=20 > Add basic support for RZ/G2L MTU3a PWM driver by creating separate > PWM channels for each IOs. >=20 > Signed-off-by: Biju Das > --- > v13->v14: > * Updated commit description > * Updated Limitations section. > * Replaced the macros RZ_MTU*->RZ_MTU3_CHAN_* in probe() > * Fixed a kernel crash in error path by moving rz_mtu3_pwm->chip.dev bef= ore > devm_add_action_or_reset() > * Added pm_runtime_idle() and simplified error paths for devm_add_action= _or_reset() > and devm_pwmchip_add(). >=20 > v12->v13: > * Updated commit description > * Moved RZ_MTU3_TMDR1_MD_* macros to rz_mtu3.h > * Updated Limitations section. > * Removed PWM mode1 references from the driver. > * Dropped prescale and duty_cycle from struct rz_mtu3_pwm_chip. > * Replaced rz_mtu3_pwm_mode1_num_ios->rz_mtu3_hw_channel_ios. > * Avoided race condition in rz_mtu3_pwm_request()/rz_mtu3_pwm_free(). > * Updated get_state() by adding dc > pv check and added a comment about > overflow condition. > * Moved overflow condition check from config->probe() > * Replaced pm_runtime_resume_and_get with unconditional pm_runtime_get_s= ync() > in config() > * Added error check for clk_prepare_enable() in probe() and propagating = error > to the caller for pm_runtime_resume() > * clk_get_rate() is called after enabling the clock and clk_rate_exclusi= ve_put() > v11->v12: > * Updated header file to as core driver is in MFD. > * Reordered get_state() > v10->v11: > * No change. > v9->v10: > * No change. > v8->v9: > * Added prescale/duty_cycle variables to struct rz_mtu3_pwm_chip and > cached this values in rz_mtu3_pwm_config and used this cached values > in get_state(), if PWM is disabled. > * Added return code for get_state() > v7->v8: > * Simplified rz_mtu3_pwm_request by calling rz_mtu3_request_channel() > * Simplified rz_mtu3_pwm_free by calling rz_mtu3_release_channel() > v6->v7: > * Added channel specific mutex lock to avoid race between counter > device and rz_mtu3_pwm_{request,free} > * Added pm_runtime_resume_and_get in rz_mtu3_pwm_enable() > * Added pm_runtime_put_sync in rz_mtu3_pwm_disable() > * Updated rz_mtu3_pwm_config() > * Updated rz_mtu3_pwm_apply() > v5->v6: > * Updated commit and Kconfig description > * Sorted the header > * Replaced dev_get_drvdata from rz_mtu3_pwm_pm_disable() > * Replaced SET_RUNTIME_PM_OPS->DEFINE_RUNTIME_DEV_PM_OPS and removed > __maybe_unused from suspend/resume() > v4->v5: > * pwm device is instantiated by mtu3a core driver. > v3->v4: > * There is no resource associated with "rz-mtu3-pwm" compatible > and moved the code to mfd subsystem as it binds against "rz-mtu". > * Removed struct platform_driver rz_mtu3_pwm_driver. > v2->v3: > * No change. > v1->v2: > * Modelled as a single PWM device handling multiple channles. > * Used PM framework to manage the clocks. > --- > drivers/pwm/Kconfig | 11 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rz-mtu3.c | 482 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 494 insertions(+) > create mode 100644 drivers/pwm/pwm-rz-mtu3.c >=20 > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index dae023d783a2..ccc0299fd0dd 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -481,6 +481,17 @@ config PWM_ROCKCHIP > Generic PWM framework driver for the PWM controller found on > Rockchip SoCs. > =20 > +config PWM_RZ_MTU3 > + tristate "Renesas RZ/G2L MTU3a PWM Timer support" > + depends on RZ_MTU3 || COMPILE_TEST > + depends on HAS_IOMEM > + help > + This driver exposes the MTU3a PWM Timer controller found in Renesas > + RZ/G2L like chips through the PWM API. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-rz-mtu3. > + > config PWM_SAMSUNG > tristate "Samsung PWM support" > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 7bf1a29f02b8..b85fc9fba326 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_PWM_RASPBERRYPI_POE) +=3D pwm-raspberrypi-= poe.o > obj-$(CONFIG_PWM_RCAR) +=3D pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU) +=3D pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) +=3D pwm-rockchip.o > +obj-$(CONFIG_PWM_RZ_MTU3) +=3D pwm-rz-mtu3.o > obj-$(CONFIG_PWM_SAMSUNG) +=3D pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) +=3D pwm-sifive.o > obj-$(CONFIG_PWM_SL28CPLD) +=3D pwm-sl28cpld.o > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c > new file mode 100644 > index 000000000000..146948656755 > --- /dev/null > +++ b/drivers/pwm/pwm-rz-mtu3.c > @@ -0,0 +1,482 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/G2L MTU3a PWM Timer driver > + * > + * Copyright (C) 2022 Renesas Electronics Corporation 2023? > + * > + * Hardware manual for this IP can be found here > + * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-u= sers-manual-hardware-0?language=3Den > + * > + * Limitations: > + * - When PWM is disabled, the output is driven to Hi-Z. > + * - While the hardware supports both polarities, the driver (for now) > + * only handles normal polarity. > + * - HW uses one counter and two match components to configure duty_cycle > + * and period. > + * - Multi-Function Timer Pulse Unit(a.k.a MTU) has 7 HW channels for PWM s/t(/t (/ > + * operations(The channels are MTU{0..4, 6, 7}). s/s(/s. (/; s/)./.)/ > + * - MTU{1, 2} channels have a single IO, whereas all other HW channels = have > + * 2 IOs. > + * - Each IO is modelled as an independent PWM channel. > + * - rz_mtu3_hw_channel_ios table is used to map the PWM channel to the > + * corresponding HW channel as there are difference in number of IOs > + * between HW channels. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RZ_MTU3_TIOR_OC_RETAIN (0) > +#define RZ_MTU3_TIOR_OC_0_H_COMP_MATCH (2) > +#define RZ_MTU3_TIOR_OC_1_TOGGLE (7) > +#define RZ_MTU3_TIOR_OC_IOA GENMASK(3, 0) > + > +#define RZ_MTU3_TCR_CCLR_TGRC (5 << 5) > +#define RZ_MTU3_TCR_CKEG_RISING (0 << 3) > + > +#define RZ_MTU3_TCR_TPCS GENMASK(2, 0) I assume the other register definitions are in , I suggest to move these there, too. > + > +#define RZ_MTU3_MAX_PWM_CHANNELS (12) > + > +#define RZ_MTU3_MAX_HW_CHANNELS (7) > + > +/* The table represents the number of IOs on each MTU HW channel. */ > +static const u8 rz_mtu3_hw_channel_ios[] =3D { 2, 1, 1, 2, 2, 2, 2 }; OK, so you have twelve PWM outputs, each one is driven by a "channel". Each channel drives one or two PWMs according to the above array, right? > + > +/** > + * struct rz_mtu3_pwm_chip - MTU3 pwm private data > + * > + * @chip: MTU3 pwm chip data > + * @clk: MTU3 module clock > + * @lock: Lock to prevent concurrent access for usage count > + * @rate: MTU3 clock rate > + * @user_count: MTU3 usage count > + * @rz_mtu3_channel: HW channels for the PWM > + */ > + > +struct rz_mtu3_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + struct mutex lock; > + unsigned long rate; > + u32 user_count[RZ_MTU3_MAX_HW_CHANNELS]; > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CHANNELS]; > +}; > + > +static inline struct rz_mtu3_pwm_chip *to_rz_mtu3_pwm_chip(struct pwm_ch= ip *chip) > +{ > + return container_of(chip, struct rz_mtu3_pwm_chip, chip); > +} > + > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip *rz_mtu= 3, > + u64 period_cycles) > +{ > + u32 prescaled_period_cycles; > + u8 prescale; > + > + prescaled_period_cycles =3D period_cycles >> 16; > + if (prescaled_period_cycles >=3D 16) > + prescale =3D 3; > + else > + prescale =3D (fls(prescaled_period_cycles) + 1) / 2; > + > + return prescale; > +} > + > +static struct rz_mtu3_channel * > +rz_mtu3_get_hw_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 channel) > +{ > + unsigned int i, ch_index =3D 0; > + > + for (i =3D 0; i < ARRAY_SIZE(rz_mtu3_hw_channel_ios); i++) { > + ch_index +=3D rz_mtu3_hw_channel_ios[i]; > + > + if (ch_index > channel) > + break; > + } > + > + return rz_mtu3_pwm->ch[i]; > +} This function determines the channel that drives pwm $channel. Maybe pick more sensible names? Something like "hwpwm" instead of "channel" and "channel" instead of "hw_channel" (and "ch"). I think the driver would be easier to understand if the naming was used in a consistent way. > +static u32 rz_mtu3_get_hw_channel_index(struct rz_mtu3_pwm_chip *rz_mtu3= _pwm, > + struct rz_mtu3_channel *ch) > +{ > + u32 i; > + > + for (i =3D 0; i < ARRAY_SIZE(rz_mtu3_hw_channel_ios); i++) { > + if (ch =3D=3D rz_mtu3_pwm->ch[i]) > + break; > + } > + > + return i; > +} This is a complicated way to express ch - rz_mtu3_pwm->ch isn't it? > + > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) > +{ > + u32 i, pwm_ch_index =3D 0; > + > + for (i =3D 0; i < ch_index; i++) > + pwm_ch_index +=3D rz_mtu3_hw_channel_ios[i]; > + > + return pwm_ch_index !=3D hwpwm; > +} I think the three functions above could be simplified (or not needed) if you represent the channel -> pwm mapping in a different way. I'd do something like: ch =3D rz_mtu3_pwm_get_channel(..., hwpwm); and in *ch store the index of the lowest PWM and the number of PWMs handled by this channel. > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_p= wm, > + u32 hwpwm) > +{ > + struct rz_mtu3_channel *ch; > + bool is_channel_en; > + u32 ch_index; > + u8 val; > + > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + is_channel_en =3D rz_mtu3_is_enabled(ch); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm)) > + val =3D rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL); > + else > + val =3D rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORH); > + > + return (is_channel_en && (val & RZ_MTU3_TIOR_OC_IOA)); You could exit early (and so skip reading from the hardware) if is_channel_en is false. > +} > + > +static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct pwm_device = *pwm) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + mutex_lock(&rz_mtu3_pwm->lock); > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); ch and ch_index can be determined before taking the lock, can it not? I'd only work with a single variable that holds the value of your ch_index, call it "ch" and use rz_mtu3_pwm->channel[ch]. This is simpler and so easier to understand for a code reader. > + if (!rz_mtu3_pwm->user_count[ch_index] && !rz_mtu3_request_channel(ch))= { > + mutex_unlock(&rz_mtu3_pwm->lock); > + return -EBUSY; > + } This would be easier to read if written as follows: /* * Each channel must be requested only once, so if the channel * serves two PWMs and the other is already requested, skip over * rz_mtu3_request_channel() */ if (!rz_mtu3_pwm->user_count[ch_index]) { ret =3D rz_mtu3_request_channel(ch); if (ret) return ret; /* or -EBUSY? */ } > + > + rz_mtu3_pwm->user_count[ch_index]++; > + mutex_unlock(&rz_mtu3_pwm->lock); > + > + return 0; > +} > + > +static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device *p= wm) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + mutex_lock(&rz_mtu3_pwm->lock); > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + > + rz_mtu3_pwm->user_count[ch_index]--; > + if (!rz_mtu3_pwm->user_count[ch_index]) > + rz_mtu3_release_channel(ch); > + > + mutex_unlock(&rz_mtu3_pwm->lock); > +} > + > +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + struct pwm_device *pwm) > +{ > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + u8 val; > + int rc; > + > + rc =3D pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev); > + if (rc) > + return rc; > + > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + val =3D (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) | RZ_MTU3_TIOR_OC_0_H_COMP_MATC= H; Introduce a symbol for the 4 above? This is the only use of RZ_MTU3_TIOR_OC_1_TOGGLE, maybe move the shift to the definition of that symbol? > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWMMODE1); > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, val); > + else > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, val); > + > + if (rz_mtu3_pwm->user_count[ch_index] <=3D 1) > + rz_mtu3_enable(ch); That looks wrong. user_count signals if the channel is used for one or two PWMs, right? If both PWMs are requested before any of them is enabled, rz_mtu3_enable is never called, isn't it? > + return 0; > +} > + > +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, > + struct pwm_device *pwm) > +{ > + struct rz_mtu3_channel *ch; > + u32 ch_index; > + > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + > + /* Return to normal mode and disable output pins of MTU3 channel */ > + if (rz_mtu3_pwm->user_count[ch_index] <=3D 1) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_NORMAL); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN); > + else > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN); > + > + if (rz_mtu3_pwm->user_count[ch_index] <=3D 1) > + rz_mtu3_disable(ch); > + > + pm_runtime_put_sync(rz_mtu3_pwm->chip.dev); > +} > + > +static int rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct pwm_devic= e *pwm, > + struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + u8 prescale, val; > + u32 ch_index; > + u16 dc, pv; > + u64 tmp; > + > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + pm_runtime_get_sync(chip->dev); > + state->enabled =3D rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm->hwpwm); > + if (state->enabled) { Most variables can have a narrower scope and be declared here. > + val =3D rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR); > + prescale =3D FIELD_GET(RZ_MTU3_TCR_TPCS, val); > + > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > + dc =3D rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD); > + pv =3D rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC); > + } else { > + dc =3D rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB); > + pv =3D rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA); > + } > + > + /* With prescale <=3D 7 and pv <=3D 0xffff this doesn't overflow. */ > + tmp =3D NSEC_PER_SEC * (u64)pv << (2 * prescale); > + state->period =3D DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > + tmp =3D NSEC_PER_SEC * (u64)dc << (2 * prescale); > + state->duty_cycle =3D DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate); > + } > + > + if (state->duty_cycle > state->period) > + state->duty_cycle =3D state->period; > + > + state->polarity =3D PWM_POLARITY_NORMAL; > + pm_runtime_put(chip->dev); > + > + return 0; > +} > + > +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *= pwm, > + const struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D to_rz_mtu3_pwm_chip(chip); > + struct rz_mtu3_channel *ch; > + unsigned long pv, dc; > + u64 period_cycles; > + u64 duty_cycles; > + u32 ch_index; > + u8 prescale; > + u8 val; > + > + ch =3D rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm); > + ch_index =3D rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch); > + period_cycles =3D mul_u64_u32_div(state->period, rz_mtu3_pwm->rate, > + NSEC_PER_SEC); > + prescale =3D rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles); > + > + if (period_cycles >> (2 * prescale) <=3D U16_MAX) > + pv =3D period_cycles >> (2 * prescale); > + else > + pv =3D U16_MAX; > + > + duty_cycles =3D mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate, > + NSEC_PER_SEC); > + if (duty_cycles >> (2 * prescale) <=3D U16_MAX) > + dc =3D duty_cycles >> (2 * prescale); > + else > + dc =3D U16_MAX; > + > + /* > + * If the PWM channel is disabled, make sure to turn on the clock > + * before writing the register. > + */ > + if (!pwm->state.enabled) > + pm_runtime_get_sync(chip->dev); > + > + val =3D RZ_MTU3_TCR_CKEG_RISING | prescale; > + if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) { > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > + RZ_MTU3_TCR_CCLR_TGRC | val); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRD, dc); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRC, pv); > + } else { > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, > + RZ_MTU3_TCR_CCLR_TGRA | val); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRB, dc); > + rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, pv); > + } > + > + /* If the PWM is not enabled, turn the clock off again to save power. */ > + if (!pwm->state.enabled) > + pm_runtime_put(chip->dev); > + > + return 0; > +} > + > +static int rz_mtu3_pwm_apply(struct pwm_chip *chip, struct pwm_device *p= wm, > + const struct pwm_state *state) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D to_rz_mtu3_pwm_chip(chip); > + bool enabled =3D pwm->state.enabled; > + int ret; > + > + if (state->polarity !=3D PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (!state->enabled) { > + if (enabled) > + rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm); > + > + return 0; > + } > + > + ret =3D rz_mtu3_pwm_config(chip, pwm, state); > + if (ret) > + return ret; > + > + if (!enabled) > + ret =3D rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm); > + > + return ret; > +} > + > +static const struct pwm_ops rz_mtu3_pwm_ops =3D { > + .request =3D rz_mtu3_pwm_request, > + .free =3D rz_mtu3_pwm_free, > + .get_state =3D rz_mtu3_pwm_get_state, > + .apply =3D rz_mtu3_pwm_apply, > + .owner =3D THIS_MODULE, > +}; > + > +static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D dev_get_drvdata(dev); > + > + clk_disable_unprepare(rz_mtu3_pwm->clk); > + > + return 0; > +} > + > +static int rz_mtu3_pwm_pm_runtime_resume(struct device *dev) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D dev_get_drvdata(dev); > + > + return clk_prepare_enable(rz_mtu3_pwm->clk); > +} > + > +static DEFINE_RUNTIME_DEV_PM_OPS(rz_mtu3_pwm_pm_ops, > + rz_mtu3_pwm_pm_runtime_suspend, > + rz_mtu3_pwm_pm_runtime_resume, NULL); > + > +static void rz_mtu3_pwm_pm_disable(void *data) > +{ > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm =3D data; > + > + clk_rate_exclusive_put(rz_mtu3_pwm->clk); > + pm_runtime_disable(rz_mtu3_pwm->chip.dev); > + pm_runtime_set_suspended(rz_mtu3_pwm->chip.dev); > +} > + > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) > +{ > + struct rz_mtu3 *ddata =3D dev_get_drvdata(pdev->dev.parent); > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm; > + struct device *dev =3D &pdev->dev; > + int num_pwm_hw_ch =3D 0; > + unsigned int i; > + int ret; > + > + rz_mtu3_pwm =3D devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm), GFP_KERN= EL); > + if (!rz_mtu3_pwm) > + return -ENOMEM; > + > + rz_mtu3_pwm->clk =3D ddata->clk; > + > + for (i =3D 0; i < RZ_MTU_NUM_CHANNELS; i++) { > + if (i =3D=3D RZ_MTU3_CHAN_5 || i =3D=3D RZ_MTU3_CHAN_8) > + continue; > + > + rz_mtu3_pwm->ch[num_pwm_hw_ch] =3D &ddata->channels[i]; > + rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev =3D dev; > + num_pwm_hw_ch++; > + } > + > + mutex_init(&rz_mtu3_pwm->lock); > + platform_set_drvdata(pdev, rz_mtu3_pwm); > + ret =3D clk_prepare_enable(rz_mtu3_pwm->clk); > + if (ret) > + return dev_err_probe(dev, ret, "Clock enable failed\n"); > + > + clk_rate_exclusive_get(rz_mtu3_pwm->clk); > + > + rz_mtu3_pwm->rate =3D clk_get_rate(rz_mtu3_pwm->clk); > + /* > + * Refuse clk rates > 1 GHz to prevent overflow later for computing > + * period and duty cycle. > + */ > + if (rz_mtu3_pwm->rate > NSEC_PER_SEC) { > + ret =3D -EINVAL; > + clk_rate_exclusive_put(rz_mtu3_pwm->clk); > + goto disable_clock; > + } > + > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + rz_mtu3_pwm->chip.dev =3D &pdev->dev; > + ret =3D devm_add_action_or_reset(&pdev->dev, > + rz_mtu3_pwm_pm_disable, > + rz_mtu3_pwm); > + if (ret < 0) > + return ret; > + > + rz_mtu3_pwm->chip.ops =3D &rz_mtu3_pwm_ops; > + rz_mtu3_pwm->chip.npwm =3D RZ_MTU3_MAX_PWM_CHANNELS; > + ret =3D devm_pwmchip_add(&pdev->dev, &rz_mtu3_pwm->chip); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > + > + pm_runtime_idle(&pdev->dev); > + > + return 0; > + > +disable_clock: > + clk_disable_unprepare(rz_mtu3_pwm->clk); > + return ret; > +} =2Eprobe() enables rz_mtu3_pwm->clk, but there is no .remove() that cares about disabling it again. > + > +static struct platform_driver rz_mtu3_pwm_driver =3D { > + .driver =3D { > + .name =3D "pwm-rz-mtu3", > + .pm =3D pm_ptr(&rz_mtu3_pwm_pm_ops), > + }, > + .probe =3D rz_mtu3_pwm_probe, > +}; > +module_platform_driver(rz_mtu3_pwm_driver); > + > +MODULE_AUTHOR("Biju Das "); > +MODULE_ALIAS("platform:pwm-rz-mtu3"); > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver"); > +MODULE_LICENSE("GPL"); Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --wtyunm6bb7l6o573 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmQh/x8ACgkQj4D7WH0S /k5HQgf+K4ZE+69Wj595XJUbvAFKSZt2vx+xbN23laaBSgce+5KfGbiutlKopP5O EnDz/pfzoZ/1MNGOlKT7btfkW0gwVrGRDujM9+hErKTo5NlPTGrE2rtZqndnn3JZ KlQSoW3mEV9gTfgmuixjsyWsQrehAy6po4BpLIBYh05aUWT4ii0auU593RDpJSnA 8POwh36U0AX+uZJn8VnaRnLLssFfg9wrBTgGdCZVtPQL6+qIQ1OKRvfqZzcnWZgS y7FVsYikNQhZUETTJ1MMmGzgQ2h/XXfzF8taw5Smajg0KwdM5W8bHpMKXSAxKCQA XrwedVkz1Xr8q+qG49s24rQgoGAuig== =pU3p -----END PGP SIGNATURE----- --wtyunm6bb7l6o573--