Re: [PATCH 01/13] media: ipu6: Use correct pads for xlate_streams()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Jun 19, 2025 at 02:28:48PM +0000, Sakari Ailus wrote:
> On Thu, Jun 19, 2025 at 05:15:35PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 19, 2025 at 01:55:08PM +0000, Sakari Ailus wrote:
> > > On Thu, Jun 19, 2025 at 04:27:04PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Jun 19, 2025 at 11:15:34AM +0300, Sakari Ailus wrote:
> > > > > The arguments to v4l2_subdev_state_xlate_streams() were incorrect, the
> > > > 
> > > > s/were/are/
> > > > 
> > > > > source pads was used as the sink pad and the source pad was a constant
> > > > 
> > > > s/pads was/pad is/
> > > > s/pad was/pad is/
> > > > 
> > > > Are you sure though ? Unless I misread the code, you replace
> > > > 
> > > > pad0 = CSI2_PAD_SRC
> > > > pad1 = CSI2_PAD_SINK
> > > > 
> > > > with
> > > > 
> > > > pad0 = pad
> > > > pad1 = CSI2_PAD_SINK
> > > > 
> > > > This seems to be a correct fix, but I don't see where "the source pad
> > > > was used as the sink pad".
> > > 
> > > Right, I'll reword it for v2.
> > > 
> > > > > (rather than the actual source pad). Fix these.
> > > > > 
> > > > > Fixes: 3a5c59ad926b ("media: ipu6: Rework CSI-2 sub-device streaming control")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > index da8581a37e22..6030bd23b4b9 100644
> > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > @@ -354,9 +354,9 @@ static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
> > > > >  	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
> > > > >  	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> > > > >  
> > > > > -	sink_streams = v4l2_subdev_state_xlate_streams(state, CSI2_PAD_SRC,
> > > > > -						       CSI2_PAD_SINK,
> > > > > -						       &streams_mask);
> > > > > +	sink_streams =
> > > > > +		v4l2_subdev_state_xlate_streams(state, pad, CSI2_PAD_SINK,
> > > > > +						&streams_mask);
> > > > 
> > > > This is one of the cases where I'd write
> > > > 
> > > > 	sink_streams = v4l2_subdev_state_xlate_streams(state, pad, CSI2_PAD_SINK,
> > > > 						       &streams_mask);
> > > > 
> > > > even if it goes to 81 columns.
> > > 
> > > The limit is still 80, not 81.
> > 
> > The global limit is 100 for the kernel. There's somehow of a consensus
> > in the media subsystem to keep it closer to 80, with different people
> > have different sensitivities. We occasionally go over 80, and that's
> > usually left as a driver maintainer decision.
> 
> It's 80, not 100. The checkpatch.pl limit is higher than 80 though, as
> there are still commonly cases where the code is more readable with longer
> lines. See e.g. Documentation/process/coding-style.rst .

It states

----
The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information.
----

In this specific case, going over 80 columns improves readability in my
opinion, and doesn't hide information. As I wrote, we treat this as a
driver maintainer preference at the moment, so I won't make a call for
the ipu6 driver.

-- 
Regards,

Laurent Pinchart