RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> 
> On Thu, Oct 06, 2022 at 02:57:17PM +0100, Biju Das wrote:
> > Add RZ/G2L MTU3 counter driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Hi Biju,
> 
> This commit message is rather brief for an introduction of a new
> driver.
> Provide a description of the functionality provided (e.g. two 16-bit
> Counts or one 32-bit Count) as well as the hardware this driver
> supports for context (e.g. what does MTU3 mean; is this a SoC; etc.).

OK Will do. MTU3- Multi-Function Timer Pulse Unit 3 (MTU3a). It is
an on-chip module on RZ/G2L SoC

> 
> > ---
> > v1->v3:
> >  * Modelled as a counter device supporting 3 counters(2 16-bit and
> >    32-bit)
> >  * Add kernel-doc comments to document struct rz_mtu3_cnt
> >  * Removed mmio variable from struct rz_mtu3_cnt
> >  * Removed cnt local variable from rz_mtu3_count_read()
> >  * Replaced -EINVAL->-ERANGE for out of range error conditions.
> >  * Removed explicit cast from write functions.
> >  * Removed local variable val from rz_mtu3_count_ceiling_read()
> >  * Added lock for RMW for counter/ceiling updates.
> >  * Added different synapses for counter0 and counter{1,2}
> >  * Used ARRAY for assigning num_counts.
> >  * Added PM runtime for managing clocks.
> >  * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
> > ---
> >  drivers/counter/Kconfig       |   9 +
> >  drivers/counter/Makefile      |   1 +
> >  drivers/counter/rz-mtu3-cnt.c | 568
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 578 insertions(+)
> >  create mode 100644 drivers/counter/rz-mtu3-cnt.c
> >
> > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig index
> > d388bf26f4dc..531b187e4630 100644
> > --- a/drivers/counter/Kconfig
> > +++ b/drivers/counter/Kconfig
> > @@ -39,6 +39,15 @@ config INTERRUPT_CNT
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called interrupt-cnt.
> >
> > +config RZ_MTU3_CNT
> > +	tristate "RZ/G2L MTU3 counter driver"
> > +	depends on MFD_RZ_MTU3 || COMPILE_TEST
> > +	help
> > +	  Select this option to enable RZ/G2L MTU3 counter driver.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called rz-mtu3-cnt.
> > +
> 
> Provide a bit more description of the hardware here; you should at
> least mention this is a Renesas RZ/G2L as opposed to some other
> manufacturer.

OK.

> 
> >  config STM32_TIMER_CNT
> >  	tristate "STM32 Timer encoder counter driver"
> >  	depends on MFD_STM32_TIMERS || COMPILE_TEST diff --git
> > a/drivers/counter/Makefile b/drivers/counter/Makefile index
> > b9a369e0d4fc..933fdd50b3e4 100644
> > --- a/drivers/counter/Makefile
> > +++ b/drivers/counter/Makefile
> > @@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o
> > counter-chrdev.o
> >
> >  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
> >  obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
> > +obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
> >  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> >  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> >  obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
> > diff --git a/drivers/counter/rz-mtu3-cnt.c
> > b/drivers/counter/rz-mtu3-cnt.c new file mode 100644 index
> > 000000000000..26b5ea3852f8
> > --- /dev/null
> > +++ b/drivers/counter/rz-mtu3-cnt.c
> > @@ -0,0 +1,568 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L MTU3a Counter driver
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation  */ #include
> > +<linux/counter.h> #include <linux/mfd/rz-mtu3.h> #include
> > +<linux/module.h> #include <linux/of.h> #include
> > +<linux/platform_device.h> #include <linux/pm_runtime.h> #include
> > +<linux/types.h>
> > +
> > +#define RZ_MTU3_TSR_TCFD	BIT(7)
> > +#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
> > +
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
> > +
> > +#define RZ_MTU3_TCR_CCLR	GENMASK(7, 5)
> > +#define RZ_MTU3_TCR_CCLR_NONE	FIELD_PREP(RZ_MTU3_TCR_CCLR, 0)
> > +
> > +#define RZ_MTU3_TMDR3_LWA	BIT(0)
> > +#define RZ_MTU3_32_BIT_CH	(2)
> 
> Providing a define to identify the 32-bit channel is a good idea.
> Defines for the other two 16-bit channels would also be good.

OK, will do.

> 
> > +
> > +#define RZ_MTU3_TIOR_IC_BOTH	(10)
> > +
> > +/**
> > + * struct rz_mtu3_cnt - MTU3 counter private data
> > + *
> > + * @clk: MTU3 module clock
> > + * @lock: Lock to prevent concurrent access for ceiling and count
> > + * @rz_mtu3_channel: HW channels for the counters  */ struct
> > +rz_mtu3_cnt {
> > +	struct clk *clk;
> > +	struct mutex lock;
> > +	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> 
> Does this need to be a pointer to an array of struct rz_mtu3_channel?

Yes, HW has MTU{0..8} channels and MTU{1,2} supports counters
At probe time this array is filled with *ch[0]= MTU1 and *ch[1]= MTU2

> You can avoid the double dereferences in your code if you leave it as
> a simple pointer to struct rz_mtu3_channel and use subscripts directly
> as you would an array. Or is there something I'm missing?

Please see above.

> 
> > +};
> > +
> > +static const enum counter_function rz_mtu3_count_functions[] = {
> > +	COUNTER_FUNCTION_QUADRATURE_X4,
> > +	COUNTER_FUNCTION_PULSE_DIRECTION,
> > +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> > +};
> > +
> > +static bool rz_mtu3_is_16_bit_cnt_mode(struct rz_mtu3_cnt *const
> > +priv) {
> > +	return (priv->ch[0]->function == RZ_MTU3_16BIT_PHASE_COUNTING ||
> > +		priv->ch[1]->function == RZ_MTU3_16BIT_PHASE_COUNTING);
> 
> Is there ever a situation where one channel is equal to
> RZ_MTU3_16BIT_PHASE_COUNTING while the other channel is equal to
> RZ_MTU3_32BIT_PHASE_COUNTING?

No that will never happen

The check is to detect runtime conditions. For eg:- user enables ch0 and then tries
to enable Ch2 

or 

ch1 and then tries ch2.

> 
> > +}
> > +
> > +static bool rz_mtu3_is_32_bit_cnt_mode(struct rz_mtu3_cnt *const
> > +priv) {
> > +	return (priv->ch[0]->function == RZ_MTU3_32BIT_PHASE_COUNTING &&
> > +		priv->ch[1]->function == RZ_MTU3_32BIT_PHASE_COUNTING); }
> > +
> > +static int rz_mtu3_count_read(struct counter_device *counter,
> > +			      struct counter_count *count, u64 *val) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> It is not immediately clear why you are ANDing the Count id here.
> After looking at the rest of the code in this function I realized it's
> because you want to call rz_mtu3_32bit_ch_read() with id = 0 when you
> have
> count->id = RZ_MTU3_32_BIT_CH.
> 
> I wouldn't even bother with the local id variable in this function and
> instead just hardcode priv->ch[0] in the rz_mtu3_32bit_ch_read() call
> below directly.

OK. Will do.

> 
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*val = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TCNTLW);
> > +	else
> > +		*val = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TCNT);
> 
> Is there a risk of these read calls returning an error code?

There is no risk. as the calls with these macros{RZ_MTU3_TCNTLW,RZ_MTU3_TCNT}
It never return error.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_write(struct counter_device *counter,
> > +			       struct counter_count *count, const u64 val) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment about local id variable as in rz_mtu3_count_read().
OK. Agreed.

> 
> > +	u32 ceiling;
> > +
> > +	mutex_lock(&priv->lock);
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		ceiling = rz_mtu3_32bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRALW);
> > +	else
> > +		ceiling = rz_mtu3_16bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRA);
> 
> The ceiling value isn't expected to change unless the user executes
> your
> ceiling_write() function, right? It might make sense to cache the
> current ceiling value in your rz_mtu3_cnt structure so that you don't
> have to read it out from the device every time you check it.

OK. will add channel specific array to cache these values.

> 
> > +
> > +	if (val > ceiling) {
> > +		mutex_unlock(&priv->lock);
> > +		return -ERANGE;
> > +	}
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TCNTLW, val);
> > +	else
> > +		rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TCNT, val);
> > +
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_function_read(struct counter_device
> *counter,
> > +				       struct counter_count *count,
> > +				       enum counter_function *function) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> As mentioned before, this AND operation obscures the intention of your
> code. Instead, rename this variable and account for RZ_MTU3_32_BIT_CH
> explicitly with something like this:
> 
>     const size_t ch_id = (count->id == RZ_MTU3_32_BIT_CH) ? 0 : count-
> >id;

OK.

> 
> You could wrap this into a preprocessor macro to reuse again in your
> code, but I'll leave it up to you if you want.

OK.

> 
> > +	u8 val;
> > +
> > +	val = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TMDR1);
> > +
> > +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_function_write(struct counter_device
> *counter,
> > +					struct counter_count *count,
> > +					enum counter_function function)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().

OK.

> 
> > +	u8 mode;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
> > +		break;
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1, mode);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_direction_read(struct counter_device
> *counter,
> > +					struct counter_count *count,
> > +					enum counter_count_direction *direction)
> {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().
OK.
> 
> > +	u8 cnt;
> > +
> > +	cnt = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TSR);
> 
> This is the timer status register, right? A variable name of 'cnt'
> seems a bit strange to me; would 'tsr' be a better name here?

Yes, it is timer status register. Will change it to tsr.

> 
> > +
> > +	if (cnt & RZ_MTU3_TSR_TCFD)
> > +		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
> > +	else
> > +		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_ceiling_read(struct counter_device
> *counter,
> > +				      struct counter_count *count,
> > +				      u64 *ceiling)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment about local id variable as in rz_mtu3_count_read().

OK.
> 
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*ceiling = rz_mtu3_32bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRALW);
> > +	else
> > +		*ceiling = rz_mtu3_16bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRA);
> 
> Assuming you're able to cache the ceiling value, you can set it here
> directly and avoid the reads out to the device.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_ceiling_write(struct counter_device
> *counter,
> > +				       struct counter_count *count,
> > +				       u64 ceiling)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().
OK.
> 
> > +	if (ceiling > U16_MAX && rz_mtu3_is_16_bit_cnt_mode(priv))
> > +		return -ERANGE;
> > +
> > +	if (ceiling > U32_MAX && rz_mtu3_is_32_bit_cnt_mode(priv))
> > +		return -ERANGE;
> 
> You can determine which maximum to consider by checking the count->id.
> Instead of those two conditional statments, try this instead:
> 
>     switch (count->id) {
>     case 0:
>     case 1:
>             if (ceiling > U16_MAX)
>                     return -ERANGE;
>             break;
>     case RZ_MTU3_32_BIT_CH:
>             if (ceiling > U32_MAX)
>                     return -ERANGE;
>             break;
>     }
> 
OK.

> If you need to check whether you're in 32-bit phase mode before
> setting the ceiling for the RZ_MTU3_32_BIT_CH Count (and similarly for
> the 16-bit Counts), check that separately and return -EBUSY as
> appropriate.

OK.
> 
> > +	mutex_lock(&priv->lock);
> > +	if (ceiling == 0) {
> > +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_NONE);
> 
> Looks like something different is done when ceiling is set to 0. Would
> you explain what's happening in this case and why it's different that
> then else case below; in other words, what's the difference between
> RZ_MTU3_TCR_CCLR_NONE and RZ_MTU3_TCR_CCLR_TGRA?

RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase signal.
RZ_MTU3_TCR_CCLR_NONE --> No clearing.

> 
> > +
> 
> You can remove this empty line.
OK.
> 
> > +	} else {
> > +		if (count->id == RZ_MTU3_32_BIT_CH)
> > +			rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TGRALW,
> ceiling);
> > +		else
> > +			rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA,
> ceiling);
> > +
> > +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_TGRA);
> > +	}
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rz_mtu3_32bit_cnt_setting(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	/*
> > +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit
> cascade
> > +	 * counter.
> > +	 */
> > +	priv->ch[0]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +	priv->ch[1]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +
> > +	rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3,
> > +RZ_MTU3_TMDR3_LWA);
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TMDR1,
> > +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TCR,
> RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TIOR,
> > +RZ_MTU3_TIOR_IC_BOTH);
> > +
> > +	rz_mtu3_enable(priv->ch[0]);
> > +	rz_mtu3_enable(priv->ch[1]);
> > +}
> > +
> > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> 
> If 16-bit phase counting is selected for one 16-bit counter, does the
> other 16-bit counter need to be configured as well?

Not required I guess, as it is run time decision.

After this, if user tries to enable 16-bit on other channel,
we will configure that channel. otherwise, we will return error,
if user tries to enable 32-bit channel.

Are you ok with this? 


> 
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1,
> > +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, U16_MAX);
> > +
> > +	rz_mtu3_enable(priv->ch[id]);
> > +}
> > +
> > +static int rz_mtu3_initialize_counter(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH && rz_mtu3_is_16_bit_cnt_mode(priv))
> > +		return -EBUSY;
> > +
> > +	if (id != RZ_MTU3_32_BIT_CH && rz_mtu3_is_32_bit_cnt_mode(priv))
> > +		return -EBUSY;
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH)
> > +		rz_mtu3_32bit_cnt_setting(counter, id);
> > +	else
> > +		rz_mtu3_16bit_cnt_setting(counter, id);
> 
> I think this code would flow better using a switch statement like
> this:
> 
>     switch (id) {
>     case 0:
>     case 1:
>             if (rz_mtu3_is_32_bit_cnt_mode(priv))
>                     return -EBUSY;
>             rz_mtu3_16bit_cnt_setting(counter, id);
>             break;
>     case RZ_MTU3_32_BIT_CH:
>             if (rz_mtu3_is_16_bit_cnt_mode(priv))
>                     return -EBUSY;
>             rz_mtu3_32bit_cnt_setting(counter, id);
>             break;
>     }
> 
> You should also protect this with a lock to prevent any races while
> you're accessing and modifying the settings.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void rz_mtu3_terminate_counter(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH) {
> > +		priv->ch[0]->function = RZ_MTU3_NORMAL;
> > +		priv->ch[1]->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, 0);
> > +		rz_mtu3_disable(priv->ch[1]);
> > +		rz_mtu3_disable(priv->ch[0]);
> > +	} else {
> > +		priv->ch[id]->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_disable(priv->ch[id]);
> > +	}
> 
> You probably need a lock in this function to prevent races.

OK.

> 
> > +}
> > +
> > +static int rz_mtu3_count_enable_read(struct counter_device
> *counter,
> > +				     struct counter_count *count, u8 *enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*enable = rz_mtu3_is_enabled(priv->ch[0]) &&
> > +			rz_mtu3_is_enabled(priv->ch[1]);
> 
> There's a race between checking for channel 0 and channel 1, so use a
> lock to prevent that.

OK. Agreed.
> 
> > +	else
> > +		*enable = rz_mtu3_is_enabled(priv->ch[count->id]);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_enable_write(struct counter_device
> *counter,
> > +				      struct counter_count *count, u8 enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch = priv->ch[count->id & 0x1];
> 
> Same comment about the AND operation as mentioned before.

OK.
> 
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		pm_runtime_get_sync(ch->dev);
> > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> 
> Are you using the Count's "enable" extension to switch between 16-bit
> and 32-bit phase modes?

No. But will use that for switching on the next version.


> 
> > +	} else {
> > +		rz_mtu3_terminate_counter(counter, count->id);
> > +		pm_runtime_put(ch->dev);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static struct counter_comp rz_mtu3_count_ext[] = {
> > +	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
> > +	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
> > +			    rz_mtu3_count_enable_write),
> > +	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
> > +			     rz_mtu3_count_ceiling_write), };
> > +
> > +static const enum counter_synapse_action rz_mtu3_synapse_actions[]
> = {
> > +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> > +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> > +	COUNTER_SYNAPSE_ACTION_NONE,
> > +};
> > +
> > +static int rz_mtu3_action_read(struct counter_device *counter,
> > +			       struct counter_count *count,
> > +			       struct counter_synapse *synapse,
> > +			       enum counter_synapse_action *action) {
> > +	const size_t signal_a_id = count->synapses[0].signal->id;
> > +	const size_t signal_b_id = count->synapses[1].signal->id;
> 
> If this is "Channel 2" Count then you will have four Synapses for four
> possible Signals (MTCLKA, MTCLKB, MTCLKC, MTCLKD), so you'll need to
> account for those two other Signals as well.

OK.

> 
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rz_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Default action mode */
> > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		/*
> > +		 * Rising edges on signal A updates the respective count.
> > +		 * The input level of signal B determines direction.
> > +		 */
> > +		if (synapse->signal->id == signal_a_id)
> > +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		/*
> > +		 * Any state transition on quadrature pair signal B updates
> > +		 * the respective count.
> > +		 */
> > +		if (synapse->signal->id == signal_b_id)
> > +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		/* counts up/down on both edges of A and B signal*/
> > +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct counter_ops rz_mtu3_cnt_ops = {
> > +	.count_read = rz_mtu3_count_read,
> > +	.count_write = rz_mtu3_count_write,
> > +	.function_read = rz_mtu3_count_function_read,
> > +	.function_write = rz_mtu3_count_function_write,
> > +	.action_read = rz_mtu3_action_read,
> > +};
> > +
> > +#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
> > +	.id = (_id),				\
> > +	.name = (_name),			\
> > +}
> > +
> > +static struct counter_signal rz_mtu3_signals[] = {
> > +	RZ_MTU3_PHASE_SIGNAL(0, "MTU1 MTCLKA"),
> > +	RZ_MTU3_PHASE_SIGNAL(1, "MTU1 MTCLKB"),
> > +	RZ_MTU3_PHASE_SIGNAL(2, "MTU2 MTCLKC"),
> > +	RZ_MTU3_PHASE_SIGNAL(3, "MTU2 MTCLKD"), };
> 
> These names can be in a human-readable form (e.g. "Multi-function
> Timer Clock A") if you think it's easier to understand, but I'll leave
> it up to you if you want to change it.

HW manual says MTCLK{A,B,C,D}. that is the reason.

> 
> > +
> > +#define RZ_MTU3_COUNT_SYNAPSES(_id) {					\
> > +	{								\
> > +		.actions_list = rz_mtu3_synapse_actions,		\
> > +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> > +		.signal = rz_mtu3_signals + 2 * (_id)			\
> > +	},								\
> > +	{								\
> > +		.actions_list = rz_mtu3_synapse_actions,		\
> > +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> > +		.signal = rz_mtu3_signals + 2 * (_id) + 1		\
> > +	}								\
> > +}
> > +
> > +static struct counter_synapse rz_mtu3_count_synapses[][2] = {
> > +	RZ_MTU3_COUNT_SYNAPSES(0), RZ_MTU3_COUNT_SYNAPSES(1) };
> 
> I know the 104-quad-8 module also creates a multidimensional array to
> represent the synapses, but I would advise against this pattern
> because it obscures the intention of the code.
> 
> Instead, create a separate distinct array for each group of Synapses;
> I suppose there will be two arrays in this case judging from your
> existing code.

OK. Will do.

> 
> > +
> > +static struct counter_count rz_mtu3_counts[] = {
> > +	{
> > +		.id = 0,
> > +		.name = "Channel 1 Count(16-bit)",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 2,
> 
> As mentioned in the comment above, refer to the distinct Synapse array
> for the particular Count and then use ARRAY_SIZE for that array to set
> num_synapses.

OK will do.
> 
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = 1,
> > +		.name = "Channel 2 Count(16-bit)",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 4,
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = 2,
> 
> Set id = RZ_MTU3_32_BIT_CH to make it more obvious here.
> 
> > +		.name = "Channel3 Count(32-bit)",
> 
> We probably don't need the "(32-bit)" in the name when it's obvious
> already from the channel id and ceiling value.

OK will remove it.
> 
> I wonder how this counter is described in the RZ/G2L user
> documentation; is it named "Channel 3" or "Channel 1 and 2"?

It is mentioned as MTU1 and MTU2 channels.

These channels can be used for phase counting and PWM operations.

> 
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 4,
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	}
> > +};
> > +
> > +static int __maybe_unused rz_mtu3_cnt_pm_runtime_suspend(struct
> > +device *dev) {
> > +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused rz_mtu3_cnt_pm_runtime_resume(struct
> device
> > +*dev) {
> > +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> > +
> > +	clk_prepare_enable(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rz_mtu3_cnt_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(rz_mtu3_cnt_pm_runtime_suspend,
> > +rz_mtu3_cnt_pm_runtime_resume, NULL) };
> > +
> > +static void rz_mtu3_cnt_pm_disable(void *data) {
> > +	struct device *dev = data;
> > +
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_set_suspended(dev);
> > +}
> > +
> > +static int rz_mtu3_cnt_probe(struct platform_device *pdev) {
> > +	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct counter_device *counter;
> > +	struct rz_mtu3_cnt *priv;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	counter = devm_counter_alloc(dev, sizeof(*priv));
> > +	if (!counter)
> > +		return -ENOMEM;
> > +
> > +	priv = counter_priv(counter);
> > +	priv->clk = ddata->clk;
> > +
> > +	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
> > +		priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
> > +		priv->ch[i]->dev = dev;
> > +		if (priv->ch[i]->function != RZ_MTU3_NORMAL)
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "channel '%u' is already claimed\n",
> i);
> > +	}
> > +
> > +	mutex_init(&priv->lock);
> > +	clk_prepare_enable(priv->clk);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rz_mtu3_cnt_pm_disable,
> > +				       dev);
> > +	if (ret < 0)
> > +		goto disable_clock;
> > +
> > +	counter->name = dev_name(dev);
> > +	counter->parent = dev;
> > +	counter->ops = &rz_mtu3_cnt_ops;
> > +	counter->counts = rz_mtu3_counts;
> > +	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
> > +	counter->signals = rz_mtu3_signals;
> > +	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
> > +	platform_set_drvdata(pdev, priv);
> 
> It looks like you only ever use clk in your callbacks; how about
> setting your drvdata to clk instead and removing it from your
> rz_mtu3_cnt structure?

OK. Will do. 

Note:
This change is based on feedback[1] on bindings.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20221006135717.1748560-2-biju.das.jz@xxxxxxxxxxxxxx/

Cheers,
Biju

> 
> > +
> > +	/* Register Counter device */
> > +	ret = devm_counter_add(dev, counter);
> > +	if (ret < 0) {
> > +		dev_err_probe(dev, ret, "Failed to add counter\n");
> > +		goto disable_clock;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_clock:
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id rz_mtu3_cnt_of_match[] = {
> > +	{ .compatible = "renesas,rz-mtu3-counter", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rz_mtu3_cnt_of_match);
> > +
> > +static struct platform_driver rz_mtu3_cnt_driver = {
> > +	.probe = rz_mtu3_cnt_probe,
> > +	.driver = {
> > +		.name = "rz-mtu3-counter",
> > +		.pm = &rz_mtu3_cnt_pm_ops,
> > +		.of_match_table = rz_mtu3_cnt_of_match,
> > +	},
> > +};
> > +module_platform_driver(rz_mtu3_cnt_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_ALIAS("platform:rz-mtu3-counter");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> > +MODULE_LICENSE("GPL"); MODULE_IMPORT_NS(COUNTER);
> > --
> > 2.25.1
> >




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux