On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote: > 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? > Hi Andy, By looking at it again, I didn't reply correct here. This function internally calls the irq_to_desc() which basically returns the irq desctiptor for this irq. This function can return NULL in case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ) or in case the interrupt number is bigger than the NR_IRQs which the irq controller can handle (!CONFIG_SPARSE_IRQ). So in my opinion, it makes sense to keep this check. Cheers, Vasilis https://elixir.bootlin.com/linux/v6.10.6/source/kernel/irq/chip.c#L155 > ... > > > > 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 > >
