Re: [PATCH v3 2/2] media: i2c: Add Sony IMX908 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Thu, Oct 01, 2026 at 04:31:38PM +0300, Laurent Pinchart wrote:
> On Thu, Oct 01, 2026 at 03:24:54PM +0300, Sakari Ailus wrote:
> > > +static int imx908_set_selection(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_state *sd_state,
> > > +				struct v4l2_subdev_selection *sel)
> > 
> > We don't really have cropping behaviour documented before the common raw
> > sensor model. I'd just postpone this until we have the common raw sensor
> > model support
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=metadata> merged.
> 
> There's real progress on merging the raw camera sensor model, with Jai
> finalizing the implementation in libcamera. We could drop crop support
> for the initial version of the IMX908 driver or wait for the raw camera
> sensor model to be merged. The former is probably better to avoid
> delays.
> 
> Lachlan, sorry about this last-minute request. Hopefully it will be
> quite simple to drop the .set_selection() handler and hardcode full
> resolution. imx908_update_framing_limits() will then have no user and
> could be dropped too. We'll add it back one kernel version later by
> adding crop support based on the raw camera sensor model.

As an added bonus, it'll be more simple to support that in the driver, too.

> > > +static int imx908_disable_streams(struct v4l2_subdev *sd,
> > > +				  struct v4l2_subdev_state *sd_state,
> > > +				  u32 pad,
> > > +				  u64 streams_mask)
> > > +{
> > > +	struct imx908 *imx = to_imx908(sd);
> > > +	int ret;
> > > +
> > > +	ret = imx908_stop_streaming(imx);
> > 
> > imx908_stop_streaming() is used in a single location only. I think you
> > should move the code here. Same for imx908_enable_streams() and
> > imx906_start_streaming(), too.
> 
> For imx908_start_streaming() it could make error handling a tiny bit
> more complex. This is the kind of detail I'd leave to the appreciation
> of the driver author.

Works for me. In that case I'd move the other function just above the
caller.

...

> > > +	/* Device is powered; keep it resumed across registration, release at end */
> > > +	pm_runtime_set_active(imx->dev);
> > > +	pm_runtime_get_noresume(imx->dev);
> > > +	ret = devm_pm_runtime_enable(imx->dev);
> > 
> > What will happen on error here? PM runtime will be disabled after probe()
> > exits on error, but you'd need to set the state to suspended after that.
> > I'm not sure you can correctly use this as things stand currently. This
> > also conflicts currently with remove() below disabling Runtime PM
> > explicitly.
> 
> I'm not sure to understand what's requested here. Are you saying there's
> a potential issue that can't be fixed currently ? Or should this code be
> modified ? In the latter case, could you please tell what should be done
> ?

I'd just avoid using devm_pm_runtime_enable() here. The problem really is
that you'll need to set the device to suspended state (as well as power it
off) *after* devm does its teardown work. You can't do that, at least not
right now, without introducing devm variant of setting the state active.
There may be dependencies to some assumptions on how devices are unbound.

-- 
Regards,

Sakari Ailus