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
