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. > > > > > > +{ > > > + struct mali_c55 *mali_c55 = isp->mali_c55; > > > + struct media_pad *source_pad; > > > + struct media_pad *sink_pad; > > > + int ret; > > > + > > > + if (!enable) { > > > + if (isp->source) > > > + v4l2_subdev_call(isp->source, video, s_stream, false); This call could be v4l2_subdev_disable_streams(). > > > + isp->source = NULL; > > > + > > > + mali_c55_isp_stop(mali_c55); > > > + > > > + return 0; > > > + } > > > + > > > + sink_pad = &isp->pads[MALI_C55_ISP_PAD_SINK_VIDEO]; > > > + source_pad = media_pad_remote_pad_unique(sink_pad); > > > + if (IS_ERR(source_pad)) { > > > + dev_err(mali_c55->dev, "Failed to get source for ISP\n"); > > > + return PTR_ERR(source_pad); > > > + } > > > + > > > + isp->source = media_entity_to_v4l2_subdev(source_pad->entity); > > > + > > > + isp->frame_sequence = 0; > > > + ret = mali_c55_isp_start(mali_c55); > > > + if (ret) { > > > + dev_err(mali_c55->dev, "Failed to start ISP\n"); > > > + isp->source = NULL; > > > + return ret; > > > + } > > > + > > > + ret = v4l2_subdev_call(isp->source, video, s_stream, true); And this could be v4l2_subdev_enable_streams() as well. > > > + if (ret) { > > > + dev_err(mali_c55->dev, "Failed to start ISP source\n"); > > > + mali_c55_isp_stop(mali_c55); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + -- Regards, Sakari Ailus