RE: [PATCH RFC 5/8] 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 RFC 5/8] counter: Add RZ/G2L MTU3 counter driver
> 
> On Mon, Sep 26, 2022 at 02:21:11PM +0100, Biju Das wrote:
> > Add RZ/G2L MTU3 counter driver. Currently it supports 16-bit phase
> > counting mode on MTU{1,2} channels.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Hi Biju,
> 
> This driver will likely change in your next revision, but I want to
> give some feedback anyway on a few things I noticed. See the comments
> below.
> 
> > +struct rzg2l_mtu3_cnt {
> > +	struct clk *clk;
> > +	void __iomem *mmio;
> > +	struct rzg2l_mtu3_channel *ch;
> > +};
> 
> Add kernel-doc comments to document this structure. It seems that
> neither clk nor mmio is access in the code from this structure; what's
> the purpose of having them here?

OK, will do. mmio is not required. But clk is needed.

> 
> > +static int rzg2l_mtu3_count_read(struct counter_device *counter,
> > +				 struct counter_count *count, u64 *val) {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 cnt;
> > +
> > +	cnt = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TCNT);
> > +	*val = cnt;
> 
> The rzg2l_mtu3_16bit_ch_read() function returns a u16, so there's no
> need for the cnt variable; just return the count via val directly.

OK. Agreed.

> 
> > +static int rzg2l_mtu3_count_write(struct counter_device *counter,
> > +				  struct counter_count *count, const u64 val) {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 ceiling;
> > +
> > +	ceiling = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> > +
> > +	if (val > ceiling)
> > +		return -EINVAL;
> 
> Return -ERANGE instead to indicate the request is outside the
> boundary.

Ok. Agreed.

> 
> > +
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TCNT, (u16)val);
> 
> Remove the explicit cast to u16, it's already implicit in the call.
> You probably also need some sort of lock in this function to ensure
> that your ceiling value does not change before you write to the
> register.

OK agreed.

> 
> > +static int rzg2l_mtu3_count_ceiling_read(struct counter_device
> *counter,
> > +					 struct counter_count *count,
> > +					 u64 *ceiling)
> > +{
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 val;
> > +
> > +	val = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> > +	*ceiling = val;
> 
> Same comment as in rzg2l_mtu3_count_read().

OK agreed.
> 
> > +static int rzg2l_mtu3_count_ceiling_write(struct counter_device
> *counter,
> > +					  struct counter_count *count,
> > +					  u64 ceiling)
> > +{
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (ceiling > U16_MAX)
> > +		return -ERANGE;
> > +
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA,
> (u16)ceiling);
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> > +				 RZG2L_MTU3_TCR_CCLR_TGRA);
> 
> Same comments about cast and lock as in rzg2l_mtu3_count_write().

OK agreed.

> 
> > +static int rzg2l_mtu3_count_enable_read(struct counter_device
> *counter,
> > +					struct counter_count *count, u8 *enable)
> {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	int ch = priv->ch->index;
> > +
> > +	*enable = (rzg2l_mtu3_shared_reg_read(priv->ch, RZG2L_MTU3_TSTRA)
> &
> > +		(0x1 << ch)) >> ch;
> 
> A lot of operations happening in a single line; can this be broken
> down to clearer distinct steps?

OK agreed.

> 
> > +static int rzg2l_mtu3_action_read(struct counter_device *counter,
> > +				  struct counter_count *count,
> > +				  struct counter_synapse *synapse,
> > +				  enum counter_synapse_action *action) {
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rzg2l_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		/*
> > +		 * Rising edges on signal A updates the respective count.
> > +		 * The input level of signal B determines direction.
> > +		 */
> > +		*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> 
> You need to differentiate between signal A and B here: the Synapse for
> signal A will have an action mode of COUNTER_SYNAPSE_ACTION_RING_EDGE,
> but the Synapse for Signal B will have an action mode of
> COUNTER_SYNAPSE_ACTION_NONE because its not the trigger point for the
> respective Count value update.

OK, Will do.

> 
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		/*
> > +		 * Any state transition on quadrature pair signal B updates
> > +		 * the respective count.
> > +		 */
> > +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> 
> Similar to above, you need to differentiate between signal A and B
> here as well.

OK, will do.

> 
> > +static struct counter_count rzg2l_mtu3_counts = {
> > +	.id = 0,
> 
> The id member is an optional way for driver authors to identify their
> own Counts; it can be set to anything your like, and if you don't use
> it in your code then you don't need to set it at all.

It is being used in the next version.

> 
> > +static int rzg2l_mtu3_cnt_probe(struct platform_device *pdev) {
> > +	struct rzg2l_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct counter_device *counter;
> > +	struct rzg2l_mtu3_cnt *priv;
> > +	int ret;
> > +	u32 ch;
> > +
> > +	if (IS_ERR_OR_NULL(ddata))
> > +		return -EINVAL;
> 
> Is this actually possible? What situation would cause this, and why is
> it not handled before we reach probe()?

Theoretically not required as parent device populates child devices.
I will remove it from here.

> 
> > +
> > +	counter = devm_counter_alloc(dev, sizeof(*priv));
> > +	if (!counter)
> > +		return -ENOMEM;
> > +
> > +	priv = counter_priv(counter);
> > +
> > +	ret = of_property_read_u32(dev->of_node, "reg", &ch);
> > +	if (ret) {
> > +		dev_err(dev, "%pOF: No reg property found\n", dev-
> >of_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ch != RZG2L_MTU1 && ch != RZG2L_MTU2) {
> > +		dev_err(dev, "%pOF: Invalid channel '%u'\n", dev->of_node,
> ch);
> > +		return -EINVAL;
> > +	}
> > +
> > +	priv->clk = ddata->clk;
> > +	priv->ch = &ddata->channels[ch];
> > +	priv->ch->dev = dev;
> > +
> > +	counter->name = dev_name(dev);
> > +	counter->parent = dev;
> > +	counter->ops = &rzg2l_mtu3_cnt_ops;
> > +	counter->counts = &rzg2l_mtu3_counts;
> > +	counter->num_counts = 1;
> 
> Even though you only have one Count defined, use ARRAY_SIZE here for
> consistency with the other Counter drivers as well as making the
> intention of the code clear.

OK will use array.

> 
> > +	counter->signals = rzg2l_mtu3_signals;
> > +	counter->num_signals = ARRAY_SIZE(rzg2l_mtu3_signals);
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	/* Register Counter device */
> > +	ret = devm_counter_add(dev, counter);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to add counter\n");
> 
> The Counter driver goes live with the call to devm_counter_add() so
> move it to the end after your device initialization code below.

OK. Agreed.

> 
> > +
> > +	priv->ch->function = RZG2L_MTU3_16BIT_PHASE_COUNTING;
> > +	ret = clk_prepare_enable(ddata->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Phase counting mode 1 will be used as default
> > +	 * when initializing counters.
> > +	 */
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TMDR1,
> > +				 RZG2L_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	/* Initialize 16-bit counter max value */
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> > +				 RZG2L_MTU3_TCR_CCLR_TGRA);
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, U16_MAX);
> > +
> > +	clk_disable(ddata->clk);
> 
> Should this be moved up near the clk_prepare_enable() call above?

OK.

> 
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_ALIAS("platform:rzg2l-mtu3-counter");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> > +MODULE_LICENSE("GPL");
> 
> Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.

OK.

> 
> Make sure you're based on top of the counter-next branch. You can find
> the Counter tree here:
> https://git.kernel.org/pub/scm/linux/kernel/git/wbg/counter.git

Agreed.


Cheers,
Biju




[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