Re: [PATCH] media: imx-jpeg: Add encoder V1 support for i.MX952

[Frank Li <Frank.li@nxp.com>]

On Thu, Jan 29, 2026 at 09:42:39AM +0800, Ming Qian(OSS) wrote:
>
> Hi Frank,
>
> On 1/29/2026 4:48 AM, Frank Li wrote:
> > On Tue, Jan 27, 2026 at 03:37:00PM +0800, ming.qian@oss.nxp.com wrote:
> > > From: Ming Qian <ming.qian@oss.nxp.com>
> > >
> > > The i.MX952 SoC features an upgraded JPEG encoder (version 1) with
> > > enhanced descriptor-based configuration capabilities.
> > >
> > > The hardware version can be determined by reading the
> > > version register.
> > >
> > > The v1 encoder uses an expanded descriptor format that allows
> > > configuring all encoding parameters, including JPEG quality,
> > > directly in the descriptor. This eliminates the manual
> > > configuration phase required by v0 and reduces the interrupt
> > > count from two to one per frame.
> > >
> > > V0 encoding flow:
> > >    1. Write quality to registers -> trigger config interrupt
> > >    2. Start encoding -> trigger completion interrupt
> > >
> > > V1 encoding flow:
> > >    1. Configure descriptor with all parameters including quality
> > >    2. Start encoding -> trigger completion interrupt
> >
...
> > > +	/* Manual configuration (v0 hardware) - two-phase process */
> > > +	void (*enter_config_mode)(struct mxc_jpeg_ctx *ctx);
> > > +	void (*exit_config_mode)(struct mxc_jpeg_ctx *ctx);
> > > +
> > > +	/* Descriptor-based configuration (v1 hardware) - single-phase */
> > > +	void (*setup_desc)(struct mxc_jpeg_ctx *ctx);
> >
> > You use callback, suppose callback name should be the same for v0 and v1.
> >
> > for example:
> >
> > 	void (*setup_config)()
> > 	void (*clean_config)()
> >
> > v0	enter_config_mode() -> .setup_config
> > 	exit_config_mode() -> .clean_config
> >
> > v1
> > 	setup_desc -> .setup_config
> > 	NULL -> .clean_config.
> >
> > Frank
> >
>
> However, the flow for calling the callback in v0 and v1 are different.
> If use the same callback name, then additional checks will be required
> during the call.
> Therefore, I prefer to use different names to handle the different flow.

Okay, you use hidden null pointer check.  Generanlly, if no reuse for
callback, simple direct call to function.

But it is not big deal.  This ways is also okay.

Frank

>
> Regards,
> Ming
>
> > > +};
> > > +
> > >   struct mxc_jpeg_dev {
> > >   	spinlock_t			hw_lock; /* hardware access lock */
> > >   	unsigned int			mode;
> > > @@ -142,6 +163,7 @@ struct mxc_jpeg_dev {
> > >   	struct device			**pd_dev;
> > >   	struct device_link		**pd_link;
> > >   	struct gen_pool			*sram_pool;
> > > +	const struct mxc_jpeg_enc_ops	*enc_cfg_ops;
> > >   };
> > >
> > >   /**
> > >
> > > base-commit: c824345288d11e269ce41b36c105715bc2286050
> > > prerequisite-patch-id: 0000000000000000000000000000000000000000
> > > --
> > > 2.52.0
> > >