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

[Nikita Yushchenko]

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

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