Hi Jacopo,
On Mon, Jun 15, 2020 at 11:19:49AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Mon, Jun 15, 2020 at 11:53:35AM +0300, Sakari Ailus wrote:
> > Jacopo,
> >
> > On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> > > > > Use the newly introduced get_mbus_config() subdevice pad operation to
> > > > > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
> > > > > the number of active data lanes accordingly.
> > > > >
> > > > > In order to be able to call the remote subdevice operation cache the
> > > > > index of the remote pad connected to the single CSI-2 input port.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> > > > > 1 file changed, 58 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > index 151e6a90c5fb..11769f004fd8 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > @@ -363,6 +363,7 @@ struct rcar_csi2 {
> > > > > struct v4l2_async_notifier notifier;
> > > > > struct v4l2_async_subdev asd;
> > > > > struct v4l2_subdev *remote;
> > > > > + unsigned int remote_pad;
> > > > >
> > > > > struct v4l2_mbus_framefmt mf;
> > > > >
> > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> > > > >
> > > > > unsigned short lanes;
> > > > > unsigned char lane_swap[4];
> > > > > + unsigned short active_lanes;
> > > >
> > > > Do you need this? I.e. should you not always request this from the
> > > > transmitter device?
> > >
> > > It's actually the other way around. The receiver queries the
> > > transmitter to know how many data lanes it intends to use and adjusts
> > > its configuration to accommodate it.
> >
> > I think we didn't have a different view on the process. But you basically
> > need this when you're starting streaming, so why is the information stored
> > here?
> >
>
> Still not sure I got your question, so pardon me if the reply is
> something obvious to you already, and I'm missing the real point.
>
> But, basically what happens is there at probe time the number of
> 'available' data lanes is parsed from firmware and stored in
> priv->lanes. At stream start time, the remote end gets queried and the
> number of 'active' lanes adjusted according to what it's reported.
>
> Then, during the whole CSI-2 interface startup process, the number of
> 'active' lanes is used in a few different places (ie.
> rcsi2_wait_phy_start() and rcsi2_calc_mbps()) so I had to store it
> somewhere.
>
> Now that I wrote that, I realize you might be wondering why a
> parameter that is valid for a single streaming session is stored in
> the main driver structure. This might not be optimal, but the driver
> struct already contains, in example, a v4l2_mbus_framefmt entry which
> is a session specific paramter as well, so I thought it was no harm
> adding the number of active 'lanes' there. Is this what's bothering
> you ?
The frame format is needed there as that's set by the user outside the
s_stream call. The number of active lanes is not needed elsewhere, so
therefore I wouldn't store in the device context either. It can be a local
variable which you may pass to another function.
--
Sakari Ailus
