On Fri, Dec 20, 2024 at 02:11:26PM +0500, Nikita Yushchenko wrote:

quoted

quoted

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

quoted

quoted

+	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
+	if (err == 0) {

Usually if (!err) is used.

Ok, will fix it.

quoted

quoted

+		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

quoted

if (irq_index < NUM_IRQS) {
	dev_warn();
	return -ERR;
}

Ok - although if erroring out, I think it shall be dev_err.

quoted

quoted

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

quoted

quoted

+	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

quoted

quoted

+	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

quoted

quoted

+		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