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

[Antoine Bouyer]

Hi Geert

Le 06/05/2026 à 16:26, Geert Uytterhoeven a écrit :
> Hi Antoine,
>
> 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>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/media/platform/nxp/neoisp/Kconfig
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config VIDEO_NXP_NEOISP
> > +       tristate "NXP NEOISP v4l2 hardware driver"
> > +       depends on VIDEO_DEV
>
> depends on ARCH_MXC || COMPILE_TEST

ack

Will integrate in v2. Thanks for proposal.

> > +       select MEDIA_CONTROLLER
> > +       select VIDEOBUF2_DMA_CONTIG
> > +       select V4L2_ISP
> > +       help
> > +         Enable this to support the NXP NEO Image Signal Processing (ISP)
> > +         module present in various NXP SoCs. This module offers multiple
> > +         functions for processing RAW images and generating RGB or YUV images.
> > +
> > +         To compile this driver as a module, choose M here: the module
> > +         will be called neoisp.
>
>
> > +#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.

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.

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

Thanks
Antoine

> 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