On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote: > On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote: > > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote: ... > > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev, > > > + const struct iio_trigger_ops *trigger_ops, > > > + int (*int_config)(struct bmp280_data *data), > > > > > + irqreturn_t (*irq_thread_handler)(int irq, void *p)) > > > > irq_handler_t > > But the function returns an irqreturn_t type, no? The type of the last parameter is irq_handler_t, no need to open code it. ... > > > + fwnode = dev_fwnode(data->dev); > > > + if (!fwnode) > > > + return -ENODEV; > > > > Why do you need this? The below will fail anyway. > > Because If I don't make this check then fwnode might be garbage and I will > pass garbage to the fwnode_irq_get() function. Or do I miss something? Yes, the function validates fwnode before use. So, please drop unneeded (or even duplicate) check. ... > > > + irq = fwnode_irq_get(fwnode, 0); > > > + if (!irq) > > > > Are you sure this is correct check? > > > Well, I think yes, because the function return either the Linux IRQ number > on success or a negative errno on failure. Where is 0 mentioned in this? > https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987 > > > > + return dev_err_probe(data->dev, -ENODEV, > > > > Shadowed error code. > > I am not sure I understand what you mean here. You mean that there is no > chance that the first one will pass and this one will fail? -ENODEV is not what fwnode_irq_get() returns on error. > > > + "No interrupt found.\n"); ... > > > + desc = irq_get_irq_data(irq); > > > + if (!desc) > > > + return -EINVAL; > > > > When may this fail? > > I think that this will fail when Linux were not able to actually > register that interrupt. Wouldn't fwnode_irq_get() fail already? ... > > if (ret) > > dev_err(data->dev, "Could not enable/disable interrupt\n"); Btw you may use str_enable_disable() here. > > return ret; > > > > ? > > All the other if statements follow the style that I typed. If I > follow yours, will make it different just for this one, does it > make sense? When a comment is given, it's assumed that the _full_ patch (or patch series) should be revisited for it. Or should I add to every comment something like this: "Please, check the entire code for the same or similar case and amend accordingly." ? -- With Best Regards, Andy Shevchenko
