RE: [PATCH v14 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v14 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> 
> Hello,
> 
> I have the feeling this driver is more complicated than necessary.

I agree, your suggestions made it less complex.

> 
> 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.
> >
> > Add basic support for RZ/G2L MTU3a PWM driver by creating separate PWM
> > channels for each IOs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > 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
> before
> >    devm_add_action_or_reset()
> >  * Added pm_runtime_idle() and simplified error paths for
> devm_add_action_or_reset()
> >    and devm_pwmchip_add().
> >
> > 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_sync()
> >    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_exclusive_put()
> > v11->v12:
> >  * Updated header file to <linux/mfd/rz-mtu3.h> 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
> >
> > 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.
> >
> > +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)	+= pwm-raspberrypi-
> poe.o
> >  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> > +obj-$(CONFIG_PWM_RZ_MTU3)	+= pwm-rz-mtu3.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> >  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> >  obj-$(CONFIG_PWM_SL28CPLD)	+= 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?
OK.

> 
> > + *
> > + * Hardware manual for this IP can be found here
> > + *
> > + https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-
> > + users-manual-hardware-0?language=en
> > + *
> > + * 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 (/
Done.
> 
> > + *   operations(The channels are MTU{0..4, 6, 7}).
> 
> s/s(/s. (/; s/)./.)/
Done.

> 
> > + * - 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 <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/limits.h>
> > +#include <linux/mfd/rz-mtu3.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> > +#include <linux/time.h>
> > +
> > +#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 <linux/mfd/rz-mtu3.h>, I
> suggest to move these there, too.

Yep moved the definitions.

/* Macros for setting registers */
+#define RZ_MTU3_TCR_CCLR       GENMASK(7, 5)
+#define RZ_MTU3_TCR_CKEG       GENMASK(4, 3)
+#define RZ_MTU3_TCR_TPCS       GENMASK(2, 0)
 #define RZ_MTU3_TCR_CCLR_TGRA  BIT(5)
+#define RZ_MTU3_TCR_CCLR_TGRC  FIELD_PREP(RZ_MTU3_TCR_CCLR, 5)
+#define RZ_MTU3_TCR_CKEG_RISING        FIELD_PREP(RZ_MTU3_TCR_CKEG, 0)
+
+#define RZ_MTU3_TIOR_IOB                       GENMASK(7, 4)
+#define RZ_MTU3_TIOR_IOA                       GENMASK(3, 0)
+#define RZ_MTU3_TIOR_OC_RETAIN                 (0)
+#define RZ_MTU3_TIOR_OC_INIT_OUT_LO_HI_OUT             (2)
+#define RZ_MTU3_TIOR_OC_INIT_OUT_HI_TOGGLE_OUT (7)
+
+#define RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH \
+       FIELD_PREP(RZ_MTU3_TIOR_IOA, RZ_MTU3_TIOR_OC_INIT_OUT_LO_HI_OUT)
+#define RZ_MTU3_TIOR_OC_IOB_TOGGLE \
+       FIELD_PREP(RZ_MTU3_TIOR_IOB, RZ_MTU3_TIOR_OC_INIT_OUT_HI_TOGGLE_OUT)

> 
> > +
> > +#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[] = { 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?

Yes that is correct.

Now as per your suggestion, it is mapped differently.

+/**
+ * struct rz_mtu3_channel_io_map - MTU3 channel io map
+ *
+ * @pwm_index: Index of the lowest pwm channel
+ * @num: number of IOs on the channel.
+ */
+
+struct rz_mtu3_channel_io_map {
+       u8 pwm_index;
+       u8 num;
+};
+
+static const struct rz_mtu3_channel_io_map channel_map[] =
+       { { 0, 2 }, { 2, 1 }, { 3, 1 }, { 4 , 2 }, { 6 ,2 } , { 8, 2 }, { 10, 2 } };


> 
> > +
> > +/**
> > + * 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_chip *chip) {
> > +	return container_of(chip, struct rz_mtu3_pwm_chip, chip); }
> > +
> > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip
> *rz_mtu3,
> > +					 u64 period_cycles)
> > +{
> > +	u32 prescaled_period_cycles;
> > +	u8 prescale;
> > +
> > +	prescaled_period_cycles = period_cycles >> 16;
> > +	if (prescaled_period_cycles >= 16)
> > +		prescale = 3;
> > +	else
> > +		prescale = (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 = 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(rz_mtu3_hw_channel_ios); i++) {
> > +		ch_index += 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.

OK.

> 
> > +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 = 0; i < ARRAY_SIZE(rz_mtu3_hw_channel_ios); i++) {
> > +		if (ch == rz_mtu3_pwm->ch[i])
> > +			break;
> > +	}
> > +
> > +	return i;
> > +}
> 
> This is a complicated way to express
> 
> 	ch - rz_mtu3_pwm->ch
> 
> isn't it?

Agreed.

> 
> > +
> > +static bool rz_mtu3_pwm_is_second_channel(u32 ch_index, u32 hwpwm) {
> > +	u32 i, pwm_ch_index = 0;
> > +
> > +	for (i = 0; i < ch_index; i++)
> > +		pwm_ch_index += rz_mtu3_hw_channel_ios[i];
> > +
> > +	return pwm_ch_index != 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 = 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.

Agreed defined below structure to take care this.

+/**
+ * struct rz_mtu3_pwm_channel - MTU3 pwm channel
+ *
+ * @rz_mtu3_channel: HW channel for the PWM
+ * @pwm_index: Index of the pwm channel
+ * @num: number of IOs on the channel.
+ */
+struct rz_mtu3_pwm_channel {
+       struct rz_mtu3_channel *mtu;
+       u8 pwm_index;
+       u8 num;
+};

@@ -67,9 +67,26 @@ struct rz_mtu3_pwm_chip {
        struct mutex lock;
        unsigned long rate;
        u32 user_count[RZ_MTU3_MAX_HW_CHANNELS];
-       struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CHANNELS];
+       struct rz_mtu3_pwm_channel channel[RZ_MTU3_MAX_HW_CHANNELS];
 };

static struct rz_mtu3_pwm_channel *
+rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
 {
+       unsigned int i;
+       for (i = 0; i < RZ_MTU3_MAX_HW_CHANNELS; i++) {
+               if (rz_mtu3_pwm->channel[i].pwm_index + rz_mtu3_pwm->channel[i].num >  hwpwm)
                        break;
        }

+       return &rz_mtu3_pwm->channel[i];
 }


> 
> > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip
> *rz_mtu3_pwm,
> > +				      u32 hwpwm)
> > +{
> > +	struct rz_mtu3_channel *ch;
> > +	bool is_channel_en;
> > +	u32 ch_index;
> > +	u8 val;
> > +
> > +	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, hwpwm);
> > +	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> > +	is_channel_en = rz_mtu3_is_enabled(ch);
> > +
> > +	if (rz_mtu3_pwm_is_second_channel(ch_index, hwpwm))
> > +		val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TIORL);
> > +	else
> > +		val = 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.

Agreed.

> 
> > +}
> > +
> > +static int rz_mtu3_pwm_request(struct pwm_chip *chip, struct
> > +pwm_device *pwm) {
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +	struct rz_mtu3_channel *ch;
> > +	u32 ch_index;
> > +
> > +	mutex_lock(&rz_mtu3_pwm->lock);
> > +	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> 
> ch and ch_index can be determined before taking the lock, can it not?


Agreed.

> 
> 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 = rz_mtu3_request_channel(ch);
> 		if (ret)
> 			return ret; /* or -EBUSY? */
> 	}

Ok, it is rewritten as

+       if (!rz_mtu3_pwm->user_count[index]) {
+               ch_idle = rz_mtu3_request_channel(ch->mtu);
+               if (!ch_idle) {
+                       mutex_unlock(&rz_mtu3_pwm->lock);
+                       return -EBUSY;
+               }
        }

Where index = ch - rz_mtu3_pwm->channel;


> 
> > +
> > +	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
> > +*pwm) {
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +	struct rz_mtu3_channel *ch;
> > +	u32 ch_index;
> > +
> > +	mutex_lock(&rz_mtu3_pwm->lock);
> > +	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = 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 = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> > +	val = (RZ_MTU3_TIOR_OC_1_TOGGLE << 4) |
> > +RZ_MTU3_TIOR_OC_0_H_COMP_MATCH;
> 
> 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?

Done.
+       val = RZ_MTU3_TIOR_OC_IOB_TOGGLE | RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH;

> 
> > +	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] <= 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?

OK, Introduced enable_count to take care this in enable/disable function.


@@ -67,9 +67,26 @@ struct rz_mtu3_pwm_chip {
        struct mutex lock;
        unsigned long rate;
        u32 user_count[RZ_MTU3_MAX_HW_CHANNELS];
+       u32 enable_count[RZ_MTU3_MAX_HW_CHANNELS];
};

Enable:

+       mutex_lock(&rz_mtu3_pwm->lock);
+       if (!rz_mtu3_pwm->enable_count[index])
+               rz_mtu3_enable(ch->mtu);
+       
+       rz_mtu3_pwm->enable_count[index]++;
+       mutex_unlock(&rz_mtu3_pwm->lock);

Disable:
+       mutex_lock(&rz_mtu3_pwm->lock);
+       rz_mtu3_pwm->enable_count[index]--;
+       if (!rz_mtu3_pwm->enable_count[index])
+               rz_mtu3_disable(ch->mtu);
 
+       mutex_unlock(&rz_mtu3_pwm->lock);

> 
> > +	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 = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = 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] <= 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] <= 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_device
> *pwm,
> > +				 struct pwm_state *state)
> > +{
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +	struct rz_mtu3_channel *ch;
> > +	u8 prescale, val;
> > +	u32 ch_index;
> > +	u16 dc, pv;
> > +	u64 tmp;
> > +
> > +	ch = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> > +	pm_runtime_get_sync(chip->dev);
> > +	state->enabled = 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.

OK.

> 
> > +		val = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TCR);
> > +		prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val);
> > +
> > +		if (rz_mtu3_pwm_is_second_channel(ch_index, pwm->hwpwm)) {
> > +			dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRD);
> > +			pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRC);
> > +		} else {
> > +			dc = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRB);
> > +			pv = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TGRA);
> > +		}
> > +
> > +		/* With prescale <= 7 and pv <= 0xffff this doesn't overflow.
> */
> > +		tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale);
> > +		state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
> > +		tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale);
> > +		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
> > +	}
> > +
> > +	if (state->duty_cycle > state->period)
> > +		state->duty_cycle = state->period;
> > +
> > +	state->polarity = 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 = 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 = rz_mtu3_get_hw_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch_index = rz_mtu3_get_hw_channel_index(rz_mtu3_pwm, ch);
> > +	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> > +					NSEC_PER_SEC);
> > +	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm,
> > +period_cycles);
> > +
> > +	if (period_cycles >> (2 * prescale) <= U16_MAX)
> > +		pv = period_cycles >> (2 * prescale);
> > +	else
> > +		pv = U16_MAX;
> > +
> > +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate,
> > +				      NSEC_PER_SEC);
> > +	if (duty_cycles >> (2 * prescale) <= U16_MAX)
> > +		dc = duty_cycles >> (2 * prescale);
> > +	else
> > +		dc = 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 = 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
> *pwm,
> > +			     const struct pwm_state *state) {
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +	bool enabled = pwm->state.enabled;
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	if (!state->enabled) {
> > +		if (enabled)
> > +			rz_mtu3_pwm_disable(rz_mtu3_pwm, pwm);
> > +
> > +		return 0;
> > +	}
> > +
> > +	ret = rz_mtu3_pwm_config(chip, pwm, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!enabled)
> > +		ret = rz_mtu3_pwm_enable(rz_mtu3_pwm, pwm);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct pwm_ops rz_mtu3_pwm_ops = {
> > +	.request = rz_mtu3_pwm_request,
> > +	.free = rz_mtu3_pwm_free,
> > +	.get_state = rz_mtu3_pwm_get_state,
> > +	.apply = rz_mtu3_pwm_apply,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int rz_mtu3_pwm_pm_runtime_suspend(struct device *dev) {
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = 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 = 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 = 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 = dev_get_drvdata(pdev->dev.parent);
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm;
> > +	struct device *dev = &pdev->dev;
> > +	int num_pwm_hw_ch = 0;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm),
> GFP_KERNEL);
> > +	if (!rz_mtu3_pwm)
> > +		return -ENOMEM;
> > +
> > +	rz_mtu3_pwm->clk = ddata->clk;
> > +
> > +	for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
> > +		if (i == RZ_MTU3_CHAN_5 || i == RZ_MTU3_CHAN_8)
> > +			continue;
> > +
> > +		rz_mtu3_pwm->ch[num_pwm_hw_ch] = &ddata->channels[i];
> > +		rz_mtu3_pwm->ch[num_pwm_hw_ch]->dev = dev;
> > +		num_pwm_hw_ch++;
> > +	}
> > +
> > +	mutex_init(&rz_mtu3_pwm->lock);
> > +	platform_set_drvdata(pdev, rz_mtu3_pwm);
> > +	ret = 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 = 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 = -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 = &pdev->dev;
> > +	ret = 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 = &rz_mtu3_pwm_ops;
> > +	rz_mtu3_pwm->chip.npwm = RZ_MTU3_MAX_PWM_CHANNELS;
> > +	ret = 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;
> > +}
> 
> .probe() enables rz_mtu3_pwm->clk, but there is no .remove() that cares
> about disabling it again.

I believe it is ok. I have added WARN_ON for double checking.
Please correct me, if it is wrong.

Probe:
Clock enable by "clk_prepare_enable"
[    6.923881]  rzg2l_mod_clock_endisable+0x130/0x180
[    6.929295]  rzg2l_mod_clock_enable+0x54/0x70
[    6.934229]  clk_core_enable+0xdc/0x278
[    6.938599]  clk_enable+0x28/0x44
[    6.942403]  rz_mtu3_pwm_probe+0xf0/0x230
[    6.946944]  platform_probe+0x64/0xcc


Clock disable by " pm_runtime_idle"
[    7.283961]  rzg2l_mod_clock_endisable+0x130/0x180
[    7.289383]  rzg2l_mod_clock_disable+0x50/0x5c
[    7.294415]  clk_core_disable+0x8c/0xd8
[    7.298787]  clk_disable+0x2c/0x44
[    7.302685]  rz_mtu3_pwm_pm_runtime_suspend+0x1c/0x34
[    7.308361]  pm_generic_runtime_suspend+0x28/0x3c
[    7.313667]  __rpm_callback+0x44/0x134
[    7.317936]  rpm_callback+0x64/0x70
[    7.321920]  rpm_suspend+0x130/0x628
[    7.325999]  rpm_idle+0x118/0x2c0
[    7.329796]  __pm_runtime_idle+0x4c/0x84
[    7.334250]  rz_mtu3_pwm_probe+0x1b8/0x230


Unbind:
cd /sys/bus/platform/drivers/pwm-rz-mtu3/
echo pwm-rz-mtu3.0 > unbind

Clock enable by "__pm_runtime_resume"
[  263.003787]  rzg2l_mod_clock_endisable+0x130/0x180
[  263.009196]  rzg2l_mod_clock_enable+0x54/0x70
[  263.014122]  clk_core_enable+0xdc/0x278
[  263.018484]  clk_enable+0x28/0x44
[  263.022278]  rz_mtu3_pwm_pm_runtime_resume+0x3c/0x5c
[  263.027850]  pm_generic_runtime_resume+0x28/0x3c
[  263.033054]  __rpm_callback+0x44/0x134
[  263.037314]  rpm_callback+0x64/0x70
[  263.041287]  rpm_resume+0x428/0x6a0
[  263.045260]  __pm_runtime_resume+0x50/0x74
[  263.049893]  device_release_driver_internal+0xcc/0x234
[  263.055670]  device_driver_detach+0x14/0x1c


Clock disable by "__pm_runtime_idle"
[  263.304970]  rzg2l_mod_clock_endisable+0x130/0x180
[  263.309812]  rzg2l_mod_clock_disable+0x50/0x5c
[  263.314301]  clk_core_disable+0x8c/0xd8
[  263.318180]  clk_disable+0x2c/0x44
[  263.321620]  rz_mtu3_pwm_pm_runtime_suspend+0x1c/0x34
[  263.326720]  pm_generic_runtime_suspend+0x28/0x3c
[  263.331472]  __rpm_callback+0x44/0x134
[  263.335261]  rpm_callback+0x64/0x70
[  263.338787]  rpm_suspend+0x130/0x628
[  263.342400]  rpm_idle+0x118/0x2c0
[  263.345751]  __pm_runtime_idle+0x4c/0x84
[  263.349713]  device_release_driver_internal+0x1d0/0x234


Please correct me if anything wrong or clarifications needed.

Cheers,
Biju

> 
> > +
> > +static struct platform_driver rz_mtu3_pwm_driver = {
> > +	.driver = {
> > +		.name = "pwm-rz-mtu3",
> > +		.pm = pm_ptr(&rz_mtu3_pwm_pm_ops),
> > +	},
> > +	.probe = rz_mtu3_pwm_probe,
> > +};
> > +module_platform_driver(rz_mtu3_pwm_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_ALIAS("platform:pwm-rz-mtu3");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a PWM Timer Driver");
> > +MODULE_LICENSE("GPL");
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |