Hi Laurent, On Mon, Dec 16, 2024 at 11:07:36AM +0200, Laurent Pinchart wrote: > On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote: > > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote: > > > Hi Sakari, > > > > > > Thank you for the patch. > > > > Thank you for the review. I asked you to review a set but it wasn't this > > one: > > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@xxxxxxxxxxxxxxx/T/#t>. > > :-) > > Are you complaining that I review too many patches ? :-) No, I'm complaining your selection of patches to review. ;-) > > > > I think this should come before 4/5. > > > > > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote: > > > > Obtain the link frequency from the sub-device instead of a control > > > > handler. This allows obtaining it using the get_mbus_config() sub-device > > > > pad op that which is only supported by the IVSC driver. > > > > > > "which is the only method supported by the IVSC driver" > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > > > --- > > > > drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++--------- > > > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > index 051898ce53f4..da8581a37e22 100644 > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c > > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = { > > > > s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2) > > > > { > > > > struct media_pad *src_pad; > > > > - struct v4l2_subdev *ext_sd; > > > > - struct device *dev; > > > > > > > > if (!csi2) > > > > return -EINVAL; > > > > > > > > - dev = &csi2->isys->adev->auxdev.dev; > > > > src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity); > > > > > > Not a candidate for this patch, but can the source change, or can it be > > > cached at probe time (or notifier bound time) ? > > > > It could be, but why would you do that? > > > > This would also prevent connecting multiple sensors to a single CSI-2 > > receiver. > > Precisely because people shouldn't do this :-) > > It would be more efficient to get the pad at probe time and cache it, > and would remove an error path at runtime. Until we have a use case I presume it'd take a bug somewhere for that to fail. I don't think a relatively small number of instructions would make measurable a difference in performance. If we thought this was a problem, there would be a lot to work on elsewhere in the framework, starting with the control framework and IOCTL handling. The problem is just that nearly all that code is there for sound and important reasons. > where we need to support more than one sensor on the same CSI-2 receiver > for this driver, I think that would be best. I don't disagree as such but we can't affect hardware design here. Nothing currently prevents that and adding a driver bug that will cause whatever ills when hit is not a solution either, even if the former were true. Well, if this were Windows, the situation could be different. -- Regards, Sakari Ailus
