Hi Prabhakar, On Mon, 28 Apr 2025 at 15:25, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > On Mon, Apr 28, 2025 at 12:36 PM Geert Uytterhoeven > <geert@xxxxxxxxxxxxxx> wrote: > > On Mon, 28 Apr 2025 at 13:33, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote: > > > On Mon, Apr 28, 2025 at 12:25 PM Laurent Pinchart > > > <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > > > On Mon, Apr 28, 2025 at 12:17:54PM +0100, Lad, Prabhakar wrote: > > > > > On Mon, Apr 28, 2025 at 10:59 AM Laurent Pinchart wrote: > > > > > > On Mon, Apr 28, 2025 at 10:52:08AM +0100, Prabhakar wrote: > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > > > > > > > > Simplify the `rzg2l_fifo_empty()` helper by removing the redundant > > > > > > > comparison in the return path. Now the function explicitly returns `true` > > > > > > > if the FIFO write and read pointers match, and `false` otherwise, improving > > > > > > > readability without changing behavior. > > > > > > > > > > > > > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > > > > > > Closes: https://lore.kernel.org/all/aAtQThCibZCROETx@stanley.mountain/ > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > > index 067c6af14e95..97faefcd6019 100644 > > > > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > > @@ -348,7 +348,7 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru) > > > > > > > if (amnfifopntr_w == amnfifopntr_r_y) > > > > > > > return true; > > > > > > > > > > > > > > - return amnfifopntr_w == amnfifopntr_r_y; > > > > > > > + return false; > > > > > > > > > > > > So the function always returned true. This seems to be a bug fix, please > > > > > > add a Fixes: tag. The commit message should also make it clear that > > > > > > you're fixing an issue, not just simplifying the code. > > > > > > > > > > No, the function returned true only if the pointers matched; > > > > > otherwise, amnfifopntr_w == amnfifopntr_r_y would return false. I was > > > > > simply removing the repetitive pointer check and directly returning > > > > > false at the end of the function, as we can be certain at that point. > > > > > Hence, I did not add a Fixes tag. Am I missing something? > > > > > > > > Oops, you're right, my bad. > > > > > > > > > > Personally I'd have written > > > > > > > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > index 067c6af14e95..3d0810b3c35e 100644 > > > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > > > > @@ -345,8 +345,6 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru) > > > > > > amnfifopntr_w = amnfifopntr & AMnFIFOPNTR_FIFOWPNTR; > > > > > > amnfifopntr_r_y = > > > > > > (amnfifopntr & AMnFIFOPNTR_FIFORPNTR_Y) >> 16; > > > > > > - if (amnfifopntr_w == amnfifopntr_r_y) > > > > > > - return true; > > > > > > > > > > > > return amnfifopntr_w == amnfifopntr_r_y; > > > > > > } > > > > > > > > > > > > but that's also a bit of a style preference. > > > > > > > > > > I wanted to keep this consistent with the rz3e_fifo_empty(). If you > > > > > prefer the above I'll do that in v2. > > > > > > > > Up to you. > > > > > > > Thanks. OK, let's keep this patch as is to stay consistent with > > > rz3e_fifo_empty(). > > > > rz3e_fifo_empty() has a rather complex conditional expression. > > > Hmm yes. > > > This one will probably be converted to a simple return statement by > > a random janitor, soon after its introduction ;-) > > > Agreed, are you already working on it? Nice try ;-) -EBUSY. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
