Re: [PATCH RFC 5/8] counter: Add RZ/G2L MTU3 counter driver

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 7587 bytes --]

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@bp.renesas.com>

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?

> +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.

> +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.

> +
> +	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.

> +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().

> +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().

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

> +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.

> +		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.

> +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.

> +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()?

> +
> +	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.

> +	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.

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

> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +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.

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

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]