Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Fri, Dec 20, 2024 at 02:11:26PM +0500, Nikita Yushchenko wrote:
> > > +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
> > It wasn't shared previously, maybe some notes in commit message about
> > that.
> 
> It can be shared between several ports.
> 
> I will try to rephrase the commit message to make this stated explicitly.
> 
> > > +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> > > +	if (err == 0) {
> > Usually if (!err) is used.
> 
> Ok, will fix it.
> 
> > 
> > > +		if (irq_index < GWCA_NUM_IRQS)
> > > +			rdev->irq_index = irq_index;
> > > +		else
> > > +			dev_warn(&rdev->priv->pdev->dev,
> > > +				 "%pOF: irq-index out of range\n",
> > > +				 rdev->np_port);
> > Why not return here? It is a little counter intuitive, maybe:
> > if (err) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> It is meant to be optional, not having it defined shall not be an error
> 
> > if (irq_index < NUM_IRQS) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> Ok - although if erroring out, I think it shall be dev_err.
> 
> > > +	}
> > > +
> > > +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> > 
> > In case with not returning you are using invalid rdev_irq_index here
> > (probably 0, so may it be fine, I am only wondering).
> 
> Yes, the field is zero-initialized and that zero is a sane default.
> 
> > 
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +	err = platform_get_irq_byname(rdev->priv->pdev, name);
> > > +	kfree(name);
> > > +	if (err < 0)
> > > +		return err;
> > > +	rdev->irq = err;
> > 
> > If you will be changing sth here consider:
> > rdev->irq = platform()
> > if (rdev->irq < 0)
> > 	return rdev->irq;
> 
> Ok
> 
> > > +	err = rswitch_port_get_irq(rdev);
> > > +	if (err < 0)
> > You are returning 0 in case of success, the netdev code style is to
> > check it like that: if (!err)
> 
> I tried to follow the style already existing in the driver.
> Several checks just above and below are written this way.
> Shall I add this one check written differently?
> 

I am fine with following exsisting style.

Thanks

> > 
> > > +		goto out_get_irq;
> > If you will use the label name according to what does happen under label
> > you will not have to add another one. Feel free to leave it as it is, as
> > you have the same scheme across driver with is completle fine. You can
> > check Przemek's answer according "came from" convention [1].
> 
> Again, following existing style here.
> 
> My personal opinion is that "came from" labels are more reliable against
> future changes than other label styles. But if there is maintainer
> requirement here then definitely I will follow.
> 
> Nikita