On 1/15/22 6:21 PM, Uwe Kleine-König wrote: [...] >>>>>> The subsystems regulator, clk and gpio have the concept of a dummy >>>>>> resource. For regulator, clk and gpio there is a semantic difference >>>>>> between the regular _get() function and the _get_optional() variant. >>>>>> (One might return the dummy resource, the other won't. Unfortunately >>>>>> which one implements which isn't the same for these three.) The >>>>>> difference between platform_get_irq() and platform_get_irq_optional() is >>>>>> only that the former might emit an error message and the later won't. >>>> >>>> This is only a current difference but I'm still going to return 0 ISO >>>> -ENXIO from latform_get_irq_optional(), no way I'd leave that -ENXIO there >>>> alone... :-) >>> >>> This would address a bit of the critic in my commit log. But as 0 isn't >>> a dummy value like the dummy values that exist for clk, gpiod and >>> regulator I still think that the naming is a bad idea because it's not >>> in the spirit of the other *_get_optional functions. >>> >>> Seeing you say that -ENXIO is a bad return value for >>> platform_get_irq_optional() and 0 should be used instead, I wonder why >>> not changing platform_get_irq() to return 0 instead of -ENXIO, too. >>> This question is for now only about a sensible semantic. That actually >>> changing platform_get_irq() is probably harder than changing >>> platform_get_irq_optional() is a different story. >>> >>> If only platform_get_irq_optional() is changed and given that the >>> callers have to do something like: >>> >>> if (this_irq_exists()): >>> ... (e.g. request_irq) >>> else: >>> ... (e.g. setup polling) >>> >>> I really think it's a bad idea that this_irq_exists() has to be >>> different for platform_get_irq() vs. platform_get_irq_optional(). >> >> For platform_get_irq(), the IRQ being absent is an error condition, >> hence it should return an error code. >> For platform_get_irq_optional(), the IRQ being absent is not an error >> condition, hence it should not return an error code, and 0 is OK. > > Please show a few examples how this simplifies the code. If it's only As for platform_get_irq(), returning -ENXIO simplifies things a lot: you don't have to check for 0 at every freaking call site and have s/th like (every time!): irq = platform_get_irq(); if (irq <= 0) return irq ?: -ENXIO; // any error code you choose instead of just: irq = platform_get_irq(); if (irq < 0) return irq; This scales better in my book. > that a driver has to check for == 0 instead of == -ENXIO, than that's > not a good enough motivation to make platform_get_irq_optional() > different to platform_get_irq(). Again, it scales better... good motivation in my eyes. > Best regards > Uwe MBR, Sergey
