[v2,2/3] media: Add lane checks for Cadence CSI2RX
diff mbox series

Message ID 20190905105601.27034-3-jank@cadence.com
State New
Headers show
Series
  • media: Add support for Cadence CSI2RX version 2.1
Related show

Commit Message

Jan Kotas Sept. 5, 2019, 10:56 a.m. UTC
This patch adds lane checks for CSI2RX, to prevent clock lane
being used as a data lane.

Signed-off-by: Jan Kotas <jank@cadence.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Sakari Ailus Sept. 6, 2019, 7:54 a.m. UTC | #1
Hi Jan,

Thanks for the patchset.

On Thu, Sep 05, 2019 at 11:56:00AM +0100, Jan Kotas wrote:
> This patch adds lane checks for CSI2RX, to prevent clock lane
> being used as a data lane.
> 
> Signed-off-by: Jan Kotas <jank@cadence.com>
> ---
>  drivers/media/platform/cadence/cdns-csi2rx.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 31ace114e..97ec09e72 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -2,7 +2,7 @@
>  /*
>   * Driver for Cadence MIPI-CSI2 RX Controller v1.3
>   *
> - * Copyright (C) 2017 Cadence Design Systems Inc.
> + * Copyright (C) 2017-2019 Cadence Design Systems Inc.
>   */
>  
>  #include <linux/clk.h>
> @@ -364,7 +364,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
>  	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
>  	struct fwnode_handle *fwh;
>  	struct device_node *ep;
> -	int ret;
> +	int ret, i;
>  
>  	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
>  	if (!ep)
> @@ -395,6 +395,15 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
>  		return -EINVAL;
>  	}
>  
> +	for (i = 0; i < csi2rx->num_lanes; i++) {
> +		if (csi2rx->lanes[i] < 1) {

Do you need this? v4l2_fwnode_parse_endpoint() already has a more thorough
check for the lane numbers.

> +			dev_err(csi2rx->dev, "Invalid lane[%d] number: %u\n",
> +				i, csi2rx->lanes[i]);
> +			of_node_put(ep);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
>  	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>  	of_node_put(ep);
Jan Kotas Sept. 9, 2019, 7:41 a.m. UTC | #2
Hello Sakari,

Thanks for your reply.
> On 6 Sep 2019, at 09:54, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> 
> Hi Jan,
> 
> Thanks for the patchset.
> 
> On Thu, Sep 05, 2019 at 11:56:00AM +0100, Jan Kotas wrote:
>> /*
>>  * Driver for Cadence MIPI-CSI2 RX Controller v1.3
>>  *
>> - * Copyright (C) 2017 Cadence Design Systems Inc.
>> + * Copyright (C) 2017-2019 Cadence Design Systems Inc.
>>  */
>> 
>> +	for (i = 0; i < csi2rx->num_lanes; i++) {
>> +		if (csi2rx->lanes[i] < 1) {
> 
> Do you need this? v4l2_fwnode_parse_endpoint() already has a more thorough
> check for the lane numbers.

I looked at the source code of v4l2_fwnode_endpoint_parse_csi2_bus
and this particular case doesn’t seem to be checked.

> 
> -- 
> Regards,
> 
> Sakari Ailus

Regards,
Jan
Sakari Ailus Sept. 9, 2019, 7:51 a.m. UTC | #3
On Mon, Sep 09, 2019 at 07:41:21AM +0000, Jan Kotas wrote:
> 
> 
> Hello Sakari,
> 
> Thanks for your reply.
> > On 6 Sep 2019, at 09:54, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > 
> > Hi Jan,
> > 
> > Thanks for the patchset.
> > 
> > On Thu, Sep 05, 2019 at 11:56:00AM +0100, Jan Kotas wrote:
> >> /*
> >>  * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> >>  *
> >> - * Copyright (C) 2017 Cadence Design Systems Inc.
> >> + * Copyright (C) 2017-2019 Cadence Design Systems Inc.
> >>  */
> >> 
> >> +	for (i = 0; i < csi2rx->num_lanes; i++) {
> >> +		if (csi2rx->lanes[i] < 1) {
> > 
> > Do you need this? v4l2_fwnode_parse_endpoint() already has a more thorough
> > check for the lane numbers.
> 
> I looked at the source code of v4l2_fwnode_endpoint_parse_csi2_bus
> and this particular case doesn’t seem to be checked.

Not specifically, since 0 is a valid lane number.

However, the driver only appears to be using the information on how many
lanes there are. If the hardware doesn't support lane routing, then this is
all you need. Otherwise additional checks should be added in case there are
limitations how the lanes can be routed.
Jan Kotas Sept. 9, 2019, 8:12 a.m. UTC | #4
> On 9 Sep 2019, at 09:51, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> 
> 
> On Mon, Sep 09, 2019 at 07:41:21AM +0000, Jan Kotas wrote:
>> 
>> 
>> Hello Sakari,
>> 
>> Thanks for your reply.
>>> On 6 Sep 2019, at 09:54, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>> 
>>> Hi Jan,
>>> 
>>> Thanks for the patchset.
>>> 
>>> On Thu, Sep 05, 2019 at 11:56:00AM +0100, Jan Kotas wrote:
>>>> /*
>>>> * Driver for Cadence MIPI-CSI2 RX Controller v1.3
>>>> *
>>>> - * Copyright (C) 2017 Cadence Design Systems Inc.
>>>> + * Copyright (C) 2017-2019 Cadence Design Systems Inc.
>>>> */
>>>> 
>>>> +	for (i = 0; i < csi2rx->num_lanes; i++) {
>>>> +		if (csi2rx->lanes[i] < 1) {
>>> 
>>> Do you need this? v4l2_fwnode_parse_endpoint() already has a more thorough
>>> check for the lane numbers.
>> 
>> I looked at the source code of v4l2_fwnode_endpoint_parse_csi2_bus
>> and this particular case doesn’t seem to be checked.
> 
> Not specifically, since 0 is a valid lane number.
> 
> However, the driver only appears to be using the information on how many
> lanes there are. If the hardware doesn't support lane routing, then this is
> all you need. Otherwise additional checks should be added in case there are
> limitations how the lanes can be routed.

The CSI2RX v1.3 does support that (CSI2RX_STATIC_CFG_REG)
and assumes the first data lane has number 1.

Regrads,
Jan

> -- 
> Sakari Ailus
Sakari Ailus Sept. 9, 2019, 9:40 a.m. UTC | #5
On Mon, Sep 09, 2019 at 08:12:54AM +0000, Jan Kotas wrote:
> 
> 
> > On 9 Sep 2019, at 09:51, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > 
> > 
> > On Mon, Sep 09, 2019 at 07:41:21AM +0000, Jan Kotas wrote:
> >> 
> >> 
> >> Hello Sakari,
> >> 
> >> Thanks for your reply.
> >>> On 6 Sep 2019, at 09:54, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >>> 
> >>> Hi Jan,
> >>> 
> >>> Thanks for the patchset.
> >>> 
> >>> On Thu, Sep 05, 2019 at 11:56:00AM +0100, Jan Kotas wrote:
> >>>> /*
> >>>> * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> >>>> *
> >>>> - * Copyright (C) 2017 Cadence Design Systems Inc.
> >>>> + * Copyright (C) 2017-2019 Cadence Design Systems Inc.
> >>>> */
> >>>> 
> >>>> +	for (i = 0; i < csi2rx->num_lanes; i++) {
> >>>> +		if (csi2rx->lanes[i] < 1) {
> >>> 
> >>> Do you need this? v4l2_fwnode_parse_endpoint() already has a more thorough
> >>> check for the lane numbers.
> >> 
> >> I looked at the source code of v4l2_fwnode_endpoint_parse_csi2_bus
> >> and this particular case doesn’t seem to be checked.
> > 
> > Not specifically, since 0 is a valid lane number.
> > 
> > However, the driver only appears to be using the information on how many
> > lanes there are. If the hardware doesn't support lane routing, then this is
> > all you need. Otherwise additional checks should be added in case there are
> > limitations how the lanes can be routed.
> 
> The CSI2RX v1.3 does support that (CSI2RX_STATIC_CFG_REG)
> and assumes the first data lane has number 1.

Oh. I missed this. Thanks for pointing it out.

If the first data lane must be 1, then your check is still different: it
requires all the data lanes are greater than 1. Are there requireents for
the clock lane position? Looking at the driver, it seems it does not care
about the clock lane, and an educated guess gives 0 for the clock lane.

The DT binding documentation actually omits documenting the data-lanes
altogether. Please add it in a separate patch. I don't think "clock-lanes"
is needed _if_ zero is all you need, so please remove it also from the
examples.

It's actually nice to have more users for this feature. :-)

Patch
diff mbox series

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 31ace114e..97ec09e72 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -2,7 +2,7 @@ 
 /*
  * Driver for Cadence MIPI-CSI2 RX Controller v1.3
  *
- * Copyright (C) 2017 Cadence Design Systems Inc.
+ * Copyright (C) 2017-2019 Cadence Design Systems Inc.
  */
 
 #include <linux/clk.h>
@@ -364,7 +364,7 @@  static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
 	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
 	struct fwnode_handle *fwh;
 	struct device_node *ep;
-	int ret;
+	int ret, i;
 
 	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
 	if (!ep)
@@ -395,6 +395,15 @@  static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
 		return -EINVAL;
 	}
 
+	for (i = 0; i < csi2rx->num_lanes; i++) {
+		if (csi2rx->lanes[i] < 1) {
+			dev_err(csi2rx->dev, "Invalid lane[%d] number: %u\n",
+				i, csi2rx->lanes[i]);
+			of_node_put(ep);
+			return -EINVAL;
+		}
+	}
+
 	csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
 	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
 	of_node_put(ep);