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?
>
Just follow the style. (Sorry for late replay, I was OOO).
> >
> > > + 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
