Re: [PATCH v1 09/11] media: platform: Add NXP Neoisp Image Signal Processor

[Geert Uytterhoeven]

Hi Antoine,

On Thu, 7 May 2026 at 15:48, Antoine Bouyer <antoine.bouyer@xxxxxxx> wrote:
> Le 06/05/2026 à 16:26, Geert Uytterhoeven a écrit :
> > On Mon, 13 Apr 2026 at 18:10, Antoine Bouyer <antoine.bouyer@xxxxxxx> wrote:
> >> First NXP neoisp driver version with the following contents:
> >>
> >> This driver was initially inspired from raspberrypi pisp_be driver. It
> >> reuses same approach for ISP job scheduling.
> >>
> >> The Neoisp driver supports:
> >> * 8, 10, 12, 14 and 16-bits RAW Bayer images input.
> >> * Monochrome sensors input.
> >> * RGB/YUV, IR and Greyscale output formats.
> >>
> >> The neoisp features are:
> >> * Provides single context to limit amount of v4l2 devices.
> >> * Supports M2M operations.
> >> * Support SDR and HDR modes.
> >> * Supports generic v4l2-isp framework for extensible Parameters and
> >> Statistics buffers.
> >> * Provides a `core_media_register` API to register neoisp's media entities
> >> into another media graph.
> >> * A module parameter to run in standalone mode with its own media device.
> >>
> >> Co-developed-by: Alexi Birlinger <alexi.birlinger@xxxxxxx>
> >> Signed-off-by: Alexi Birlinger <alexi.birlinger@xxxxxxx>
> >> Signed-off-by: Antoine Bouyer <antoine.bouyer@xxxxxxx>

> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_MASK GENMASK(23, 16)
> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_SET(x) (((x) & GENMASK(7, 0)) << 16)
> >> +#define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2_GET(x) (((x) >> 16) &  GENMASK(7, 0))
> >
> > What about just:
> >
> >      #define NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2 GENMASK(23, 16)
> >
> > and using the helpers from include/linux/bitfield.h in the driver code?
> >
> >      FIELD_PREP(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, val)
> >      FIELD_GET(NEO_AUTOFOCUS_FIL0_COEFFS0_CAM0_COEFF2, reg)
>
> That file was auto generated, so I did not want to change it.

Oh, the pleasure of auto-generated files ;-)
Perhaps you can fix the generator?

Here you have 3 definitions per field, which is worse than the
auto-generated AMD header files, which have only two (MASK + SHIFT), and
have been dominating the changed-lines-of-code statistics recently...

> But I agree, these macro would help to save a couple of lines, and
> probably ease readability too. I need to double check the impact, making
> sure there is no regression with such update.

You can compare the generated assembler code, it should be more or
less the same before/after.

> Could that wait for a v3 ? as I was about to send a v2 with other
> changes first.

Sure, thanks!

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