Re: [PATCH v2 1/4] media: rcar-vin: Parse digital input in mc-path

[Niklas Söderlund]

Hi Jacopo,

On 2018-05-17 12:13:06 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>thanks for review.
> 
> On Wed, May 16, 2018 at 10:32:49PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work!
> >
> > First let me apologies for the use of the keyword 'digital' in the
> > driver it should have been parallel... Someday we should remedy this.
> >
> > If you touch any parts of the code where such a transition make sens I
> > would not complain about the intermixed use of digital/parallel. Once
> > your work is done we could follow up with a cleanup patch to complete
> > the transition. Or if you rather stick with digital here I'm fine with
> > that too.
> 
> I would go with a major s/digital/parallel/ after this has been
> merged, if that' fine with you.

I'm totally fine whit that.

> >
> > On 2018-05-16 14:16:53 +0200, Jacopo Mondi wrote:
> > > Add support for digital input subdevices to Gen-3 rcar-vin.
> > > The Gen-3, media-controller compliant, version has so far only accepted
> > > CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> > > accepted number of subdevices, and allow digital input connections on 
> > > port@0.
> > >
> > > Signed-off-by: Jacopo Mondi 
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 99 
> > > +++--
> > >  drivers/media/platform/rcar-vin/rcar-vin.h  | 15 +
> > >  2 files changed, 93 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index d3072e1..0ea21ab 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -562,7 +562,7 @@ static int rvin_digital_graph_init(struct rvin_dev 
> > > *vin)
> > >   return ret;
> > >
> > >   if (!vin->digital)
> > > - return -ENODEV;
> > > + return -ENOTCONN;
> > >
> > >   vin_dbg(vin, "Found digital subdevice %pOF\n",
> > >   to_of_node(vin->digital->asd.match.fwnode));
> > > @@ -703,15 +703,13 @@ static int rvin_mc_parse_of_endpoint(struct device 
> > > *dev,
> > >  {
> > >   struct rvin_dev *vin = dev_get_drvdata(dev);
> > >
> > > - if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> > > + if (vep->base.port != RVIN_PORT_CSI2 || vep->base.id >= RVIN_CSI_MAX)
> >
> > I don't like this RVIN_PORT_CSI2. It makes the code harder to read as I
> > have have to go and lookup which port RVIN_PORT_CSI2 represents. I would
> 
> Why do you have to go and look? It's an enum, it abstracts away the numerical
> value it represents with a human readable string. If you want to check
> which number it represent you can got and look at the enum definition,
> once. While reading the code, the most important part is "this is the CSI-2
> port" or "this is port 1"? You wrote the driver and for you there is
> no ambiguity there, I understand.
> 
> > rater just keep vep->base.port != 1 as I think it's much clearer whats
> > going on. And it's not as we will move the CSI-2 input to a different
> > port as it's described in the bindings.
> 
> That's one more reason to have an enum for that.
> 
> Anyway, that's pure bikeshedding, I like discussing these things
> too but I'm surely not making an argument for this. If you don't like
> the enum I'll remove that.

I'm sorry, I don't like the enum :-(

> 
> >
> > >   return -EINVAL;
> > >
> > >   if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> > > -
> > >   vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> > >   to_of_node(asd->match.fwnode));
> > >   return -ENOTCONN;
> > > -
> > >   }
> > >
> > >   if (vin->group->csi[vep->base.id].fwnode) {
> > > @@ -720,6 +718,8 @@ static int rvin_mc_parse_of_endpoint(struct device 
> > > *dev,
> > >   return -ENOTCONN;
> > >   }
> > >
> > > + vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> > > + vin->mbus_cfg.flags = 0;
> >
> > I like this move of mbus_cfg handling! Makes the two cases more aligned
> > which are good. Unfortunately I fear it needs more work :-(
> >
> > With this series addition of parallel input support. There are now two
> > input types CSI-2 and parallel which can be changed at runtime for the
> > same VIN. The mbus connected to the VIN will therefor be different
> 
> Wait, are you suggesting the parallel input connection can be switched
> with CSI-2 ones at runtime? I undestand the CSI-2 - VIN routing as the
> physical connections are already in place in the SoC, but if we're
> connecting a parallel input to a VIN instance that accepts an input
> connection then that hardly can be switched to another port with a
> software configuration.

Sure it can. Check 'Figure 26.2.1 Functional Block Diagram of Video 
Input Modules 4 to 7 (VIN4 to VIN7) (R-Car H3, M3-W)'. Here VIN4 and 
VIN5 can have both a parallel input and a CSI-2 input.

In the block diagram the "CSI or Digial Pin" is the 

Re: [PATCH v2 1/4] media: rcar-vin: Parse digital input in mc-path

[jacopo mondi]

Hi Niklas,
   thanks for review.

On Wed, May 16, 2018 at 10:32:49PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work!
>
> First let me apologies for the use of the keyword 'digital' in the
> driver it should have been parallel... Someday we should remedy this.
>
> If you touch any parts of the code where such a transition make sens I
> would not complain about the intermixed use of digital/parallel. Once
> your work is done we could follow up with a cleanup patch to complete
> the transition. Or if you rather stick with digital here I'm fine with
> that too.

I would go with a major s/digital/parallel/ after this has been
merged, if that' fine with you.
>
> On 2018-05-16 14:16:53 +0200, Jacopo Mondi wrote:
> > Add support for digital input subdevices to Gen-3 rcar-vin.
> > The Gen-3, media-controller compliant, version has so far only accepted
> > CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> > accepted number of subdevices, and allow digital input connections on 
> > port@0.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 99 
> > +++--
> >  drivers/media/platform/rcar-vin/rcar-vin.h  | 15 +
> >  2 files changed, 93 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index d3072e1..0ea21ab 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -562,7 +562,7 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> > return ret;
> >
> > if (!vin->digital)
> > -   return -ENODEV;
> > +   return -ENOTCONN;
> >
> > vin_dbg(vin, "Found digital subdevice %pOF\n",
> > to_of_node(vin->digital->asd.match.fwnode));
> > @@ -703,15 +703,13 @@ static int rvin_mc_parse_of_endpoint(struct device 
> > *dev,
> >  {
> > struct rvin_dev *vin = dev_get_drvdata(dev);
> >
> > -   if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> > +   if (vep->base.port != RVIN_PORT_CSI2 || vep->base.id >= RVIN_CSI_MAX)
>
> I don't like this RVIN_PORT_CSI2. It makes the code harder to read as I
> have have to go and lookup which port RVIN_PORT_CSI2 represents. I would

Why do you have to go and look? It's an enum, it abstracts away the numerical
value it represents with a human readable string. If you want to check
which number it represent you can got and look at the enum definition,
once. While reading the code, the most important part is "this is the CSI-2
port" or "this is port 1"? You wrote the driver and for you there is
no ambiguity there, I understand.

> rater just keep vep->base.port != 1 as I think it's much clearer whats
> going on. And it's not as we will move the CSI-2 input to a different
> port as it's described in the bindings.

That's one more reason to have an enum for that.

Anyway, that's pure bikeshedding, I like discussing these things
too but I'm surely not making an argument for this. If you don't like
the enum I'll remove that.

>
> > return -EINVAL;
> >
> > if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> > -
> > vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> > to_of_node(asd->match.fwnode));
> > return -ENOTCONN;
> > -
> > }
> >
> > if (vin->group->csi[vep->base.id].fwnode) {
> > @@ -720,6 +718,8 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
> > return -ENOTCONN;
> > }
> >
> > +   vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> > +   vin->mbus_cfg.flags = 0;
>
> I like this move of mbus_cfg handling! Makes the two cases more aligned
> which are good. Unfortunately I fear it needs more work :-(
>
> With this series addition of parallel input support. There are now two
> input types CSI-2 and parallel which can be changed at runtime for the
> same VIN. The mbus connected to the VIN will therefor be different

Wait, are you suggesting the parallel input connection can be switched
with CSI-2 ones at runtime? I undestand the CSI-2 - VIN routing as the
physical connections are already in place in the SoC, but if we're
connecting a parallel input to a VIN instance that accepts an input
connection then that hardly can be switched to another port with a
software configuration.

My understanding was even different: when a port has a digital video
input connected, a CSI-2 input cannot be routed there, because, well,
there is already a non modifiable connection, possibly part of the PCB
design.

> depending on which media graph link is connected to a particular VIN and
> this effects hardware configuration, see 'vin->mbus_cfg.type ==
> V4L2_MBUS_CSI2' in rvin_setup().
>
> Maybe the best solution is to move mbus_cfg into struct
> rvin_graph_entity, rename that struct rvin_parallel_input and cache the
> parallel input settings in there, much 

Re: [PATCH v2 1/4] media: rcar-vin: Parse digital input in mc-path

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work!

First let me apologies for the use of the keyword 'digital' in the 
driver it should have been parallel... Someday we should remedy this.

If you touch any parts of the code where such a transition make sens I 
would not complain about the intermixed use of digital/parallel. Once 
your work is done we could follow up with a cleanup patch to complete 
the transition. Or if you rather stick with digital here I'm fine with 
that too.

On 2018-05-16 14:16:53 +0200, Jacopo Mondi wrote:
> Add support for digital input subdevices to Gen-3 rcar-vin.
> The Gen-3, media-controller compliant, version has so far only accepted
> CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> accepted number of subdevices, and allow digital input connections on port@0.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 99 
> +++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 15 +
>  2 files changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..0ea21ab 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -562,7 +562,7 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
>   return ret;
>  
>   if (!vin->digital)
> - return -ENODEV;
> + return -ENOTCONN;
>  
>   vin_dbg(vin, "Found digital subdevice %pOF\n",
>   to_of_node(vin->digital->asd.match.fwnode));
> @@ -703,15 +703,13 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  {
>   struct rvin_dev *vin = dev_get_drvdata(dev);
>  
> - if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> + if (vep->base.port != RVIN_PORT_CSI2 || vep->base.id >= RVIN_CSI_MAX)

I don't like this RVIN_PORT_CSI2. It makes the code harder to read as I 
have have to go and lookup which port RVIN_PORT_CSI2 represents. I would 
rater just keep vep->base.port != 1 as I think it's much clearer whats 
going on. And it's not as we will move the CSI-2 input to a different 
port as it's described in the bindings.

>   return -EINVAL;
>  
>   if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> -
>   vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
>   to_of_node(asd->match.fwnode));
>   return -ENOTCONN;
> -
>   }
>  
>   if (vin->group->csi[vep->base.id].fwnode) {
> @@ -720,6 +718,8 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>   return -ENOTCONN;
>   }
>  
> + vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> + vin->mbus_cfg.flags = 0;

I like this move of mbus_cfg handling! Makes the two cases more aligned 
which are good. Unfortunately I fear it needs more work :-(

With this series addition of parallel input support. There are now two 
input types CSI-2 and parallel which can be changed at runtime for the 
same VIN. The mbus connected to the VIN will therefor be different 
depending on which media graph link is connected to a particular VIN and 
this effects hardware configuration, see 'vin->mbus_cfg.type == 
V4L2_MBUS_CSI2' in rvin_setup().

Maybe the best solution is to move mbus_cfg into struct 
rvin_graph_entity, rename that struct rvin_parallel_input and cache the 
parallel input settings in there, much like we do today for the pads.
Remove mbus_cfg from struct rvin_dev and replace it with a bool flag 
(input_csi or similar)?

In rvin_setup() use this flag to check which input type is in use and if 
it's needed fetch mbus_cfg from this cache. Then in 
rvin_group_link_notify() you could handle this input_csi flag depending 
on which link is activated. But I'm open to other solutions.

>   vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>   vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -742,7 +742,14 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>   return -EINVAL;
>   }
>  
> - /* If not all VIN's are registered don't register the notifier. */
> + /* Collect digital subdevices in this VIN device node. */
> + ret = rvin_digital_graph_init(vin);
> + if (ret < 0 && ret != -ENOTCONN) {
> + mutex_unlock(>group->lock);
> + return ret;
> + }

Why do you need to add this here? Is it not better to in 
rcar_vin_probe() do something like:

ret = rvin_digital_graph_init(vin);
if (ret < 0)
goto error;

if (vin->info->use_mc) {
ret = rvin_mc_init(vin);
if (ret < 0)
goto error;
}

That way we can try and keep to two code paths separated and at lest for 
me that increases readability a lot.

> +
> + /* Only the last registered VIN instance collects CSI-2 subdevices. */
>   for (i = 0; i < RCAR_VIN_NUM; i++)
>   if