[PATCH v4 3/5] media: mali-c55: Add Mali-C55 ISP driver

[Sakari Ailus]

Hi Dan,

On Fri, May 24, 2024 at 11:31:28AM +0100, Dan Scally wrote:
> Hi Sakari
> 
> On 23/05/2024 21:57, Sakari Ailus wrote:
> > Hi Dan,
> > 
> > On Thu, May 23, 2024 at 03:27:36PM +0100, Dan Scally wrote:
> > > Hi Sakari - sorry, one part I missed...
> > > 
> > > On 23/05/2024 09:03, Sakari Ailus wrote:
> > > > > +
> > > > > +int mali_c55_isp_s_stream(struct mali_c55_isp *isp, int enable)
> > > > Have you considered {enable,disable}_streams? All new drivers should use
> > > > these instead of s_stream() now.
> > > 
> > > Although named s_stream this is actually a purely internal function - it's
> > > not exposed as part of the subdev video ops. The resizer subdevices
> > > similarly don't expose an .s_stream() operation, they're simply started in
> > > the callpath of mali_c55_vb2_start_streaming(). I'll split the stop
> > > functionality into mali_c55_isp_stop_stream() and rename this
> > > mali_c55_isp_start_stream() to make that less confusing.
> > Ack. But this might require some rework, depending on based on what
> > streaming is actually started. I'm referring to the discussion elsewhere
> > in the same thread.
> > 
> > > 
> > > The TPG subdevice on the other hand does expose an .s_stream() operation,
> > > since the intention was to model it exactly like a connected external
> > > subdevice. I can switch to the .enable_streams() operation there.
> > Sounds good.
> 
> 
> Actually, not sure about this...the TPG just has a single source pad, so
> there's no _routing_ to set and as far as I can tell that means there's also
> no _streams_ to set since they depend on the routing -
> v4l2_subdev_enable_streams() checks the state's stream_configs to make sure
> the stream you're asking to enable exists, and those stream_configs get
> created when routing is created - so I  think for now that the TPG has to
> stay as .s_stream().

With Tomi's patches
<URL:https://lore.kernel.org/linux-media/20240424-enable-streams-impro-v6-0-5fb14c20147d@ideasonboard.com/>
you no longer need routes to use {enable,disable}_streams.

> 
> 
> In the isp subdevice I can switch to v4l2_subdev_[enable|disable]_streams()
> and let it fallback to .s_stream() for the tpg - that part's fine.

-- 
Regards,

Sakari Ailus