Re: [PATCH net-next v2 10/10] net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

[Nuno Sá]

On Fri, May 29, 2026 at 03:03:58PM +0200, Andrew Lunn wrote:
> Hi Nuno
> 
> > > +static void adin1140_stats_work(struct work_struct *work)
> > > +{
> > > +	struct delayed_work *dwork = to_delayed_work(work);
> > > +	u64 stat_buff[ADIN1140_STATS_CNT] = {};
> > > +	struct adin1140_priv *priv;
> > > +	u32 reg_val;
> > > +	int ret;
> > > +	u32 i;
> > > +
> > > +	priv = container_of(dwork, struct adin1140_priv, stats_work);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(adin1140_stat_regs); i++) {
> > > +		ret = oa_tc6_read_register(priv->tc6, adin1140_stat_regs[i],
> > > +					   &reg_val);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		stat_buff[i] = reg_val;
> > > +	}
> > > +
> > > +	spin_lock(&priv->stat_lock);
> > 
> > Maybe consider using scoped_guard() and similar for other places?
> > Marginal win though so up to you.
> 
> Please trim the text when replying so just the needed context is
> provided. It is easy to miss comments when you need to repeatedly page
> down, page down, page down to find something.
> 

Sorry, I trimmed a bit but I guess not enough.

> > > +	ret = register_netdev(netdev);
> > > +	if (ret) {
> > > +		dev_err(&spi->dev, "Failed to register netdev (%d)", ret);
> > 
> > If we go to devm, this could be return dev_err_probe().
> 
> dev_err_probe() is not really about devm, but handling EPROBE_DEFFER,
> and not issues an error message when it is not wanted. I don't think

I know. I mixed a bit. I related it to devm because then we don't need
the error handling and then we can just do `return dev_err_probe()`.

> register_netdev() can return EPROBE_DEFFER, so it probably does not
> apply here.
>

And the above is a bit why I still like dev_err_probe() even if
EPROBE_DEFFER is not to be handled. I like that we can just return
rather than:

dev_err()
return ret;

Also it unifies error logs with the same style (for printing error
codes).

- Nuno Sá

>     Andrew