[PATCH v2 0/8] media: i2c: ds90ub9xx: Misc improvements

[PATCH v2 0/8] media: i2c: ds90ub9xx: Misc improvements

[Tomi Valkeinen]

This series contains small miscellaneous improvements to the FPD-Link
drivers.

These were sent originally in v14 of the "i2c-atr and FPDLink" series
(link below), but were then left out for v15. So I have assigned v2 to
this series.

I have trimmed the to/cc list a bit, as these don't really deal with i2c
and dt anymore.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v2:
- New patch which renames ASYNC to NONSYNC
- Minor cosmetic change
- I didn't take u32_fract into use (as suggested by Andy), as I think it
  makes the driver a bit more confusing.
- Link to v1: https://lore.kernel.org/r/20230616135922.442979-1-tomi.valkeinen@ideasonboard.com

---
Tomi Valkeinen (8):
      media: i2c: ds90ub960: Configure CSI-2 continuous clock
      media: i2c: ds90ub953: Use v4l2_fwnode_endpoint_parse()
      media: i2c: ds90ub913: Use v4l2_fwnode_endpoint_parse()
      media: i2c: ds90ub953: Handle V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK
      media: i2c: ds90ub960: Allow FPD-Link async mode
      media: i2c: ds90ub953: Restructure clkout management
      media: i2c: ds90ub953: Support non-sync mode
      media: i2c: ds90ub960: Rename RXPORT_MODE_CSI2_ASYNC to RXPORT_MODE_CSI2_NONSYNC

 drivers/media/i2c/ds90ub913.c |  32 ++++---
 drivers/media/i2c/ds90ub953.c | 193 +++++++++++++++++++++++++-----------------
 drivers/media/i2c/ds90ub960.c |  31 ++++---
 3 files changed, 152 insertions(+), 104 deletions(-)
---
base-commit: 28999781d15f94046e6c23a9a7d92ad28a436abf
change-id: 20230720-fpdlink-additions-fb5397336725

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v2 1/8] media: i2c: ds90ub960: Configure CSI-2 continuous clock

[Tomi Valkeinen]

Use 'clock-noncontinuous' from DT to configure the
continuous/non-continuous clock setting for the TX ports.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index b9a1ef63629b..a83091f47140 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -149,6 +149,7 @@
 
 #define UB960_TR_CSI_CTL			0x33
 #define UB960_TR_CSI_CTL_CSI_CAL_EN		BIT(6)
+#define UB960_TR_CSI_CTL_CSI_CONTS_CLOCK	BIT(1)
 #define UB960_TR_CSI_CTL_CSI_ENABLE		BIT(0)
 
 #define UB960_TR_CSI_CTL2			0x34
@@ -485,6 +486,7 @@ struct ub960_txport {
 	u8                      nport;	/* TX port number, and index in priv->txport[] */
 
 	u32 num_data_lanes;
+	bool non_continous_clk;
 };
 
 struct ub960_data {
@@ -1133,6 +1135,9 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
 		goto err_free_txport;
 	}
 
+	txport->non_continous_clk = vep.bus.mipi_csi2.flags &
+				    V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	txport->num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;
 
 	if (vep.nr_of_link_frequencies != 1) {
@@ -1744,6 +1749,9 @@ static void ub960_init_tx_port(struct ub960_data *priv,
 
 	csi_ctl |= (4 - txport->num_data_lanes) << 4;
 
+	if (!txport->non_continous_clk)
+		csi_ctl |= UB960_TR_CSI_CTL_CSI_CONTS_CLOCK;
+
 	ub960_txport_write(priv, nport, UB960_TR_CSI_CTL, csi_ctl);
 }
 

-- 
2.34.1

Re: [PATCH v2 1/8] media: i2c: ds90ub960: Configure CSI-2 continuous clock

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:32PM +0300, Tomi Valkeinen wrote:
> Use 'clock-noncontinuous' from DT to configure the
> continuous/non-continuous clock setting for the TX ports.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/ds90ub960.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index b9a1ef63629b..a83091f47140 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -149,6 +149,7 @@
>  
>  #define UB960_TR_CSI_CTL			0x33
>  #define UB960_TR_CSI_CTL_CSI_CAL_EN		BIT(6)
> +#define UB960_TR_CSI_CTL_CSI_CONTS_CLOCK	BIT(1)
>  #define UB960_TR_CSI_CTL_CSI_ENABLE		BIT(0)
>  
>  #define UB960_TR_CSI_CTL2			0x34
> @@ -485,6 +486,7 @@ struct ub960_txport {
>  	u8                      nport;	/* TX port number, and index in priv->txport[] */
>  
>  	u32 num_data_lanes;
> +	bool non_continous_clk;
>  };
>  
>  struct ub960_data {
> @@ -1133,6 +1135,9 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
>  		goto err_free_txport;
>  	}
>  
> +	txport->non_continous_clk = vep.bus.mipi_csi2.flags &
> +				    V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>  	txport->num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;
>  
>  	if (vep.nr_of_link_frequencies != 1) {
> @@ -1744,6 +1749,9 @@ static void ub960_init_tx_port(struct ub960_data *priv,
>  
>  	csi_ctl |= (4 - txport->num_data_lanes) << 4;
>  
> +	if (!txport->non_continous_clk)
> +		csi_ctl |= UB960_TR_CSI_CTL_CSI_CONTS_CLOCK;
> +
>  	ub960_txport_write(priv, nport, UB960_TR_CSI_CTL, csi_ctl);
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/8] media: i2c: ds90ub953: Use v4l2_fwnode_endpoint_parse()

[Tomi Valkeinen]

Use v4l2_fwnode_endpoint_parse() to parse the sink endpoint parameters.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 591b52bf71c2..ad964bd6c7eb 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -25,6 +25,8 @@
 #include <media/i2c/ds90ub9xx.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
 #define UB953_PAD_SINK			0
@@ -1111,7 +1113,9 @@ static const struct regmap_config ub953_regmap_config = {
 static int ub953_parse_dt(struct ub953_data *priv)
 {
 	struct device *dev = &priv->client->dev;
+	struct v4l2_fwnode_endpoint vep = {};
 	struct fwnode_handle *ep_fwnode;
+	unsigned char nlanes;
 	int ret;
 
 	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
@@ -1119,19 +1123,21 @@ static int ub953_parse_dt(struct ub953_data *priv)
 	if (!ep_fwnode)
 		return dev_err_probe(dev, -ENOENT, "no endpoint found\n");
 
-	ret = fwnode_property_count_u32(ep_fwnode, "data-lanes");
+	vep.bus_type = V4L2_MBUS_CSI2_DPHY;
+	ret = v4l2_fwnode_endpoint_parse(ep_fwnode, &vep);
 
 	fwnode_handle_put(ep_fwnode);
 
-	if (ret < 0)
+	if (ret)
 		return dev_err_probe(dev, ret,
-				     "failed to parse property 'data-lanes'\n");
+				     "failed to parse sink endpoint data\n");
 
-	if (ret != 1 && ret != 2 && ret != 4)
+	nlanes = vep.bus.mipi_csi2.num_data_lanes;
+	if (nlanes != 1 && nlanes != 2 && nlanes != 4)
 		return dev_err_probe(dev, -EINVAL,
-				     "bad number of data-lanes: %d\n", ret);
+				     "bad number of data-lanes: %d\n", nlanes);
 
-	priv->num_data_lanes = ret;
+	priv->num_data_lanes = nlanes;
 
 	return 0;
 }

-- 
2.34.1

Re: [PATCH v2 2/8] media: i2c: ds90ub953: Use v4l2_fwnode_endpoint_parse()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:33PM +0300, Tomi Valkeinen wrote:
> Use v4l2_fwnode_endpoint_parse() to parse the sink endpoint parameters.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub953.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index 591b52bf71c2..ad964bd6c7eb 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -25,6 +25,8 @@
>  #include <media/i2c/ds90ub9xx.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>  
>  #define UB953_PAD_SINK			0
> @@ -1111,7 +1113,9 @@ static const struct regmap_config ub953_regmap_config = {
>  static int ub953_parse_dt(struct ub953_data *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> +	struct v4l2_fwnode_endpoint vep = {};
>  	struct fwnode_handle *ep_fwnode;
> +	unsigned char nlanes;
>  	int ret;
>  
>  	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> @@ -1119,19 +1123,21 @@ static int ub953_parse_dt(struct ub953_data *priv)
>  	if (!ep_fwnode)
>  		return dev_err_probe(dev, -ENOENT, "no endpoint found\n");
>  
> -	ret = fwnode_property_count_u32(ep_fwnode, "data-lanes");
> +	vep.bus_type = V4L2_MBUS_CSI2_DPHY;

I would initialize .bus_type when declaring the variable.

> +	ret = v4l2_fwnode_endpoint_parse(ep_fwnode, &vep);
>  
>  	fwnode_handle_put(ep_fwnode);
>  
> -	if (ret < 0)
> +	if (ret)
>  		return dev_err_probe(dev, ret,
> -				     "failed to parse property 'data-lanes'\n");
> +				     "failed to parse sink endpoint data\n");
>  
> -	if (ret != 1 && ret != 2 && ret != 4)
> +	nlanes = vep.bus.mipi_csi2.num_data_lanes;
> +	if (nlanes != 1 && nlanes != 2 && nlanes != 4)
>  		return dev_err_probe(dev, -EINVAL,
> -				     "bad number of data-lanes: %d\n", ret);
> +				     "bad number of data-lanes: %d\n", nlanes);

%u as nlanes is now unsigned.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> -	priv->num_data_lanes = ret;
> +	priv->num_data_lanes = nlanes;
>  
>  	return 0;
>  }
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/8] media: i2c: ds90ub913: Use v4l2_fwnode_endpoint_parse()

[Tomi Valkeinen]

Use v4l2_fwnode_endpoint_parse() to parse the sink endpoint parameters.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 4dae5afa9fa9..cb53b0654a43 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -21,6 +21,8 @@
 #include <linux/regmap.h>
 
 #include <media/i2c/ds90ub9xx.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
 #define UB913_PAD_SINK			0
@@ -83,7 +85,7 @@ struct ub913_data {
 
 	struct ds90ub9xx_platform_data *plat_data;
 
-	u32			pclk_polarity;
+	u32			pclk_polarity_rising;
 };
 
 static inline struct ub913_data *sd_to_ub913(struct v4l2_subdev *sd)
@@ -675,25 +677,31 @@ static int ub913_add_i2c_adapter(struct ub913_data *priv)
 static int ub913_parse_dt(struct ub913_data *priv)
 {
 	struct device *dev = &priv->client->dev;
+	struct v4l2_fwnode_endpoint vep = {};
 	struct fwnode_handle *ep_fwnode;
 	int ret;
 
 	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
 						    UB913_PAD_SINK, 0, 0);
-	if (!ep_fwnode) {
-		dev_err_probe(dev, -ENOENT, "No sink endpoint\n");
-		return -ENOENT;
-	}
+	if (!ep_fwnode)
+		return dev_err_probe(dev, -ENOENT, "No sink endpoint\n");
 
-	ret = fwnode_property_read_u32(ep_fwnode, "pclk-sample",
-				       &priv->pclk_polarity);
+	vep.bus_type = V4L2_MBUS_PARALLEL;
+	ret = v4l2_fwnode_endpoint_parse(ep_fwnode, &vep);
 
 	fwnode_handle_put(ep_fwnode);
 
-	if (ret) {
-		dev_err_probe(dev, ret, "failed to parse 'pclk-sample'\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to parse sink endpoint data\n");
+
+	if (vep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		priv->pclk_polarity_rising = true;
+	else if (vep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+		priv->pclk_polarity_rising = false;
+	else
+		return dev_err_probe(dev, -EINVAL,
+				     "bad value for 'pclk-sample'\n");
 
 	return 0;
 }
@@ -726,7 +734,7 @@ static int ub913_hw_init(struct ub913_data *priv)
 
 	ub913_read(priv, UB913_REG_GENERAL_CFG, &v);
 	v &= ~UB913_REG_GENERAL_CFG_PCLK_RISING;
-	v |= priv->pclk_polarity ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
+	v |= priv->pclk_polarity_rising ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
 	ub913_write(priv, UB913_REG_GENERAL_CFG, v);
 
 	return 0;

-- 
2.34.1

Re: [PATCH v2 3/8] media: i2c: ds90ub913: Use v4l2_fwnode_endpoint_parse()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:34PM +0300, Tomi Valkeinen wrote:
> Use v4l2_fwnode_endpoint_parse() to parse the sink endpoint parameters.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub913.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 4dae5afa9fa9..cb53b0654a43 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -21,6 +21,8 @@
>  #include <linux/regmap.h>
>  
>  #include <media/i2c/ds90ub9xx.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>  
>  #define UB913_PAD_SINK			0
> @@ -83,7 +85,7 @@ struct ub913_data {
>  
>  	struct ds90ub9xx_platform_data *plat_data;
>  
> -	u32			pclk_polarity;
> +	u32			pclk_polarity_rising;

bool ?

>  };
>  
>  static inline struct ub913_data *sd_to_ub913(struct v4l2_subdev *sd)
> @@ -675,25 +677,31 @@ static int ub913_add_i2c_adapter(struct ub913_data *priv)
>  static int ub913_parse_dt(struct ub913_data *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> +	struct v4l2_fwnode_endpoint vep = {};
>  	struct fwnode_handle *ep_fwnode;
>  	int ret;
>  
>  	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
>  						    UB913_PAD_SINK, 0, 0);
> -	if (!ep_fwnode) {
> -		dev_err_probe(dev, -ENOENT, "No sink endpoint\n");
> -		return -ENOENT;
> -	}
> +	if (!ep_fwnode)
> +		return dev_err_probe(dev, -ENOENT, "No sink endpoint\n");
>  
> -	ret = fwnode_property_read_u32(ep_fwnode, "pclk-sample",
> -				       &priv->pclk_polarity);
> +	vep.bus_type = V4L2_MBUS_PARALLEL;

I'd initialize this when declaring the variable.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	ret = v4l2_fwnode_endpoint_parse(ep_fwnode, &vep);
>  
>  	fwnode_handle_put(ep_fwnode);
>  
> -	if (ret) {
> -		dev_err_probe(dev, ret, "failed to parse 'pclk-sample'\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to parse sink endpoint data\n");
> +
> +	if (vep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +		priv->pclk_polarity_rising = true;
> +	else if (vep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> +		priv->pclk_polarity_rising = false;
> +	else
> +		return dev_err_probe(dev, -EINVAL,
> +				     "bad value for 'pclk-sample'\n");
>  
>  	return 0;
>  }
> @@ -726,7 +734,7 @@ static int ub913_hw_init(struct ub913_data *priv)
>  
>  	ub913_read(priv, UB913_REG_GENERAL_CFG, &v);
>  	v &= ~UB913_REG_GENERAL_CFG_PCLK_RISING;
> -	v |= priv->pclk_polarity ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
> +	v |= priv->pclk_polarity_rising ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
>  	ub913_write(priv, UB913_REG_GENERAL_CFG, v);
>  
>  	return 0;
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/8] media: i2c: ds90ub953: Handle V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK

[Tomi Valkeinen]

Handle V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag to configure the CSI-2 RX
continuous/non-continuous clock register.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index ad964bd6c7eb..ad479923d2b4 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -138,6 +138,7 @@ struct ub953_data {
 	struct regmap		*regmap;
 
 	u32			num_data_lanes;
+	bool			non_cont_clk;
 
 	struct gpio_chip	gpio_chip;
 
@@ -1139,6 +1140,9 @@ static int ub953_parse_dt(struct ub953_data *priv)
 
 	priv->num_data_lanes = nlanes;
 
+	priv->non_cont_clk = vep.bus.mipi_csi2.flags &
+			     V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	return 0;
 }
 
@@ -1201,7 +1205,7 @@ static int ub953_hw_init(struct ub953_data *priv)
 		return dev_err_probe(dev, ret, "i2c init failed\n");
 
 	ub953_write(priv, UB953_REG_GENERAL_CFG,
-		    UB953_REG_GENERAL_CFG_CONT_CLK |
+		    (priv->non_cont_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK) |
 		    ((priv->num_data_lanes - 1) << UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT) |
 		    UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE);
 

-- 
2.34.1

Re: [PATCH v2 4/8] media: i2c: ds90ub953: Handle V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:35PM +0300, Tomi Valkeinen wrote:
> Handle V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag to configure the CSI-2 RX
> continuous/non-continuous clock register.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub953.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index ad964bd6c7eb..ad479923d2b4 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -138,6 +138,7 @@ struct ub953_data {
>  	struct regmap		*regmap;
>  
>  	u32			num_data_lanes;
> +	bool			non_cont_clk;

Maybe non_continous_clk for consistency with 1/8 ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	struct gpio_chip	gpio_chip;
>  
> @@ -1139,6 +1140,9 @@ static int ub953_parse_dt(struct ub953_data *priv)
>  
>  	priv->num_data_lanes = nlanes;
>  
> +	priv->non_cont_clk = vep.bus.mipi_csi2.flags &
> +			     V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>  	return 0;
>  }
>  
> @@ -1201,7 +1205,7 @@ static int ub953_hw_init(struct ub953_data *priv)
>  		return dev_err_probe(dev, ret, "i2c init failed\n");
>  
>  	ub953_write(priv, UB953_REG_GENERAL_CFG,
> -		    UB953_REG_GENERAL_CFG_CONT_CLK |
> +		    (priv->non_cont_clk ? 0 : UB953_REG_GENERAL_CFG_CONT_CLK) |
>  		    ((priv->num_data_lanes - 1) << UB953_REG_GENERAL_CFG_CSI_LANE_SEL_SHIFT) |
>  		    UB953_REG_GENERAL_CFG_CRC_TX_GEN_ENABLE);
>  
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/8] media: i2c: ds90ub960: Allow FPD-Link async mode

[Tomi Valkeinen]

Allow using FPD-Link in async mode. The driver handles it correctly, but
the mode was blocked at probe time as there wasn't HW to test this with.
Now the mode has been tested, and it works.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ds90ub960.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index a83091f47140..ea819fb6e99b 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -3240,7 +3240,6 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
 	switch (rx_mode) {
 	case RXPORT_MODE_RAW12_HF:
 	case RXPORT_MODE_RAW12_LF:
-	case RXPORT_MODE_CSI2_ASYNC:
 		dev_err(dev, "rx%u: unsupported 'ti,rx-mode' %u\n", nport,
 			rx_mode);
 		return -EINVAL;

-- 
2.34.1

Re: [PATCH v2 5/8] media: i2c: ds90ub960: Allow FPD-Link async mode

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:36PM +0300, Tomi Valkeinen wrote:
> Allow using FPD-Link in async mode. The driver handles it correctly, but
> the mode was blocked at probe time as there wasn't HW to test this with.
> Now the mode has been tested, and it works.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/ds90ub960.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index a83091f47140..ea819fb6e99b 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -3240,7 +3240,6 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
>  	switch (rx_mode) {
>  	case RXPORT_MODE_RAW12_HF:
>  	case RXPORT_MODE_RAW12_LF:
> -	case RXPORT_MODE_CSI2_ASYNC:
>  		dev_err(dev, "rx%u: unsupported 'ti,rx-mode' %u\n", nport,
>  			rx_mode);
>  		return -EINVAL;
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 6/8] media: i2c: ds90ub953: Restructure clkout management

[Tomi Valkeinen]

Separate clkout calculations and register writes into two functions:
ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
ub953_clkout_data that is used to store the clkout parameters.

This simplifies the clkout management.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 135 ++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 65 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index ad479923d2b4..3a19c6dedd2f 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -131,6 +131,13 @@ struct ub953_hw_data {
 	bool is_ub971;
 };
 
+struct ub953_clkout_data {
+	u32 hs_div;
+	u32 m;
+	u32 n;
+	unsigned long rate;
+};
+
 struct ub953_data {
 	const struct ub953_hw_data	*hw_data;
 
@@ -906,6 +913,58 @@ static unsigned long ub953_calc_clkout_ub971(struct ub953_data *priv,
 	return res;
 }
 
+static void ub953_calc_clkout_params(struct ub953_data *priv,
+				     unsigned long target_rate,
+				     struct ub953_clkout_data *clkout_data)
+{
+	struct device *dev = &priv->client->dev;
+	unsigned long clkout_rate;
+	u64 fc_rate;
+
+	fc_rate = ub953_get_fc_rate(priv);
+
+	if (priv->hw_data->is_ub971) {
+		u8 m, n;
+
+		clkout_rate = ub953_calc_clkout_ub971(priv, target_rate,
+						      fc_rate, &m, &n);
+
+		clkout_data->m = m;
+		clkout_data->n = n;
+
+		dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
+			__func__, fc_rate, m, n, clkout_rate, target_rate);
+	} else {
+		u8 hs_div, m, n;
+
+		clkout_rate = ub953_calc_clkout_ub953(priv, target_rate,
+						      fc_rate, &hs_div, &m, &n);
+
+		clkout_data->hs_div = hs_div;
+		clkout_data->m = m;
+		clkout_data->n = n;
+
+		dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
+			__func__, fc_rate, hs_div, m, n, clkout_rate,
+			target_rate);
+	}
+
+	clkout_data->rate = clkout_rate;
+}
+
+static void ub953_write_clkout_regs(struct ub953_data *priv,
+				    struct ub953_clkout_data *clkout_data)
+{
+	if (priv->hw_data->is_ub971) {
+		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_data->m);
+		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);
+	} else {
+		ub953_write(priv, UB953_REG_CLKOUT_CTRL0,
+			    (__ffs(clkout_data->hs_div) << 5) | clkout_data->m);
+		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);
+	}
+}
+
 static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
 					      unsigned long parent_rate)
 {
@@ -965,52 +1024,25 @@ static long ub953_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
 				    unsigned long *parent_rate)
 {
 	struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
-	struct device *dev = &priv->client->dev;
-	unsigned long res;
-	u64 fc_rate;
-	u8 hs_div, m, n;
-
-	fc_rate = ub953_get_fc_rate(priv);
+	struct ub953_clkout_data clkout_data;
 
-	if (priv->hw_data->is_ub971) {
-		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
+	ub953_calc_clkout_params(priv, rate, &clkout_data);
 
-		dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
-			__func__, fc_rate, m, n, res, rate);
-	} else {
-		res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
-
-		dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
-			__func__, fc_rate, hs_div, m, n, res, rate);
-	}
-
-	return res;
+	return clkout_data.rate;
 }
 
 static int ub953_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 				 unsigned long parent_rate)
 {
 	struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
-	u64 fc_rate;
-	u8 hs_div, m, n;
-	unsigned long res;
+	struct ub953_clkout_data clkout_data;
 
-	fc_rate = ub953_get_fc_rate(priv);
-
-	if (priv->hw_data->is_ub971) {
-		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
-
-		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
-		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-	} else {
-		res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
+	ub953_calc_clkout_params(priv, rate, &clkout_data);
 
-		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
-		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-	}
+	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
+		clkout_data.rate, rate);
 
-	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__, res,
-		rate);
+	ub953_write_clkout_regs(priv, &clkout_data);
 
 	return 0;
 }
@@ -1021,32 +1053,6 @@ static const struct clk_ops ub953_clkout_ops = {
 	.set_rate	= ub953_clkout_set_rate,
 };
 
-static void ub953_init_clkout_ub953(struct ub953_data *priv)
-{
-	u64 fc_rate;
-	u8 hs_div, m, n;
-
-	fc_rate = ub953_get_fc_rate(priv);
-
-	ub953_calc_clkout_ub953(priv, 25000000, fc_rate, &hs_div, &m, &n);
-
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-}
-
-static void ub953_init_clkout_ub971(struct ub953_data *priv)
-{
-	u64 fc_rate;
-	u8 m, n;
-
-	fc_rate = ub953_get_fc_rate(priv);
-
-	ub953_calc_clkout_ub971(priv, 25000000, fc_rate, &m, &n);
-
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
-	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
-}
-
 static int ub953_register_clkout(struct ub953_data *priv)
 {
 	struct device *dev = &priv->client->dev;
@@ -1055,16 +1061,15 @@ static int ub953_register_clkout(struct ub953_data *priv)
 				  priv->hw_data->model, dev_name(dev)),
 		.ops = &ub953_clkout_ops,
 	};
+	struct ub953_clkout_data clkout_data;
 	int ret;
 
 	if (!init.name)
 		return -ENOMEM;
 
 	/* Initialize clkout to 25MHz by default */
-	if (priv->hw_data->is_ub971)
-		ub953_init_clkout_ub971(priv);
-	else
-		ub953_init_clkout_ub953(priv);
+	ub953_calc_clkout_params(priv, 25000000, &clkout_data);
+	ub953_write_clkout_regs(priv, &clkout_data);
 
 	priv->clkout_clk_hw.init = &init;
 

-- 
2.34.1

Re: [PATCH v2 6/8] media: i2c: ds90ub953: Restructure clkout management

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 01:30:37PM +0300, Tomi Valkeinen wrote:
> Separate clkout calculations and register writes into two functions:
> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
> ub953_clkout_data that is used to store the clkout parameters.
> 
> This simplifies the clkout management.

...

> +struct ub953_clkout_data {
> +	u32 hs_div;
> +	u32 m;
> +	u32 n;

I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
used widely, hence I see no issues in using u32_fract here.

OTOH I'm not a maintainer, so...

> +	unsigned long rate;
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/8] media: i2c: ds90ub953: Restructure clkout management

[Tomi Valkeinen]

On 21/07/2023 13:29, Andy Shevchenko wrote:
> On Thu, Jul 20, 2023 at 01:30:37PM +0300, Tomi Valkeinen wrote:
>> Separate clkout calculations and register writes into two functions:
>> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
>> ub953_clkout_data that is used to store the clkout parameters.
>>
>> This simplifies the clkout management.
> 
> ...
> 
>> +struct ub953_clkout_data {
>> +	u32 hs_div;
>> +	u32 m;
>> +	u32 n;
> 
> I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
> used widely, hence I see no issues in using u32_fract here.

I think it makes sense to use u32_fract in common code. My argument for 
not using it here is:

- There is no actual functionality that u32_fract brings, so it's really 
only about field naming
- m and n matches the terms in the HW documentation, making it easier to 
compare the code and the docs
- This is private to the driver
- I'm (currently) the most likely person to edit the driver, and I would 
have to check which one that numerator/denominator was again when 
looking at this part of the code (but maybe I would learn eventually)

So, in my view, the change doesn't really have any pros but does have cons.

That said, it's not a biggie. If others chime in and say it's a good 
idea to use u32_fract, I'm fine doing that change.

  Tomi

Re: [PATCH v2 6/8] media: i2c: ds90ub953: Restructure clkout management

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 04:23:54PM +0300, Tomi Valkeinen wrote:
> On 21/07/2023 13:29, Andy Shevchenko wrote:
> > On Thu, Jul 20, 2023 at 01:30:37PM +0300, Tomi Valkeinen wrote:

...

> > > +struct ub953_clkout_data {
> > > +	u32 hs_div;
> > > +	u32 m;
> > > +	u32 n;
> > 
> > I don't think it makes driver worse. The V4L2 UAPI has similar struct which is
> > used widely, hence I see no issues in using u32_fract here.
> 
> I think it makes sense to use u32_fract in common code. My argument for not
> using it here is:
> 
> - There is no actual functionality that u32_fract brings, so it's really
> only about field naming
> - m and n matches the terms in the HW documentation, making it easier to
> compare the code and the docs
> - This is private to the driver
> - I'm (currently) the most likely person to edit the driver, and I would
> have to check which one that numerator/denominator was again when looking at
> this part of the code (but maybe I would learn eventually)
> 
> So, in my view, the change doesn't really have any pros but does have cons.
> 
> That said, it's not a biggie. If others chime in and say it's a good idea to
> use u32_fract, I'm fine doing that change.

Thank you for a good summary of your point of view.
I agree that others, esp. maintainers, can decide
on how to proceed with this suggestion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/8] media: i2c: ds90ub953: Restructure clkout management

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:37PM +0300, Tomi Valkeinen wrote:
> Separate clkout calculations and register writes into two functions:
> ub953_calc_clkout_params and ub953_write_clkout_regs, and add a struct
> ub953_clkout_data that is used to store the clkout parameters.
> 
> This simplifies the clkout management.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub953.c | 135 ++++++++++++++++++++++--------------------
>  1 file changed, 70 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index ad479923d2b4..3a19c6dedd2f 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -131,6 +131,13 @@ struct ub953_hw_data {
>  	bool is_ub971;
>  };
>  
> +struct ub953_clkout_data {
> +	u32 hs_div;
> +	u32 m;
> +	u32 n;
> +	unsigned long rate;
> +};
> +
>  struct ub953_data {
>  	const struct ub953_hw_data	*hw_data;
>  
> @@ -906,6 +913,58 @@ static unsigned long ub953_calc_clkout_ub971(struct ub953_data *priv,
>  	return res;
>  }
>  
> +static void ub953_calc_clkout_params(struct ub953_data *priv,
> +				     unsigned long target_rate,
> +				     struct ub953_clkout_data *clkout_data)
> +{
> +	struct device *dev = &priv->client->dev;
> +	unsigned long clkout_rate;
> +	u64 fc_rate;
> +
> +	fc_rate = ub953_get_fc_rate(priv);
> +
> +	if (priv->hw_data->is_ub971) {
> +		u8 m, n;
> +
> +		clkout_rate = ub953_calc_clkout_ub971(priv, target_rate,
> +						      fc_rate, &m, &n);
> +
> +		clkout_data->m = m;
> +		clkout_data->n = n;
> +
> +		dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
> +			__func__, fc_rate, m, n, clkout_rate, target_rate);
> +	} else {
> +		u8 hs_div, m, n;
> +
> +		clkout_rate = ub953_calc_clkout_ub953(priv, target_rate,
> +						      fc_rate, &hs_div, &m, &n);
> +
> +		clkout_data->hs_div = hs_div;
> +		clkout_data->m = m;
> +		clkout_data->n = n;
> +
> +		dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
> +			__func__, fc_rate, hs_div, m, n, clkout_rate,
> +			target_rate);
> +	}
> +
> +	clkout_data->rate = clkout_rate;
> +}
> +
> +static void ub953_write_clkout_regs(struct ub953_data *priv,
> +				    struct ub953_clkout_data *clkout_data)

const

> +{
> +	if (priv->hw_data->is_ub971) {
> +		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_data->m);
> +		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);

This line is common to both branches, you could move it after the
conditional part.

> +	} else {
> +		ub953_write(priv, UB953_REG_CLKOUT_CTRL0,
> +			    (__ffs(clkout_data->hs_div) << 5) | clkout_data->m);
> +		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_data->n);
> +	}
> +}
> +
>  static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
>  					      unsigned long parent_rate)
>  {
> @@ -965,52 +1024,25 @@ static long ub953_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
>  				    unsigned long *parent_rate)
>  {
>  	struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
> -	struct device *dev = &priv->client->dev;
> -	unsigned long res;
> -	u64 fc_rate;
> -	u8 hs_div, m, n;
> -
> -	fc_rate = ub953_get_fc_rate(priv);
> +	struct ub953_clkout_data clkout_data;
>  
> -	if (priv->hw_data->is_ub971) {
> -		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
> +	ub953_calc_clkout_params(priv, rate, &clkout_data);
>  
> -		dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
> -			__func__, fc_rate, m, n, res, rate);
> -	} else {
> -		res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
> -
> -		dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
> -			__func__, fc_rate, hs_div, m, n, res, rate);
> -	}
> -
> -	return res;
> +	return clkout_data.rate;
>  }
>  
>  static int ub953_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
>  				 unsigned long parent_rate)
>  {
>  	struct ub953_data *priv = container_of(hw, struct ub953_data, clkout_clk_hw);
> -	u64 fc_rate;
> -	u8 hs_div, m, n;
> -	unsigned long res;
> +	struct ub953_clkout_data clkout_data;
>  
> -	fc_rate = ub953_get_fc_rate(priv);
> -
> -	if (priv->hw_data->is_ub971) {
> -		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
> -
> -		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
> -		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
> -	} else {
> -		res = ub953_calc_clkout_ub953(priv, rate, fc_rate, &hs_div, &m, &n);
> +	ub953_calc_clkout_params(priv, rate, &clkout_data);
>  
> -		ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
> -		ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
> -	}
> +	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
> +		clkout_data.rate, rate);
>  
> -	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__, res,
> -		rate);
> +	ub953_write_clkout_regs(priv, &clkout_data);
>  
>  	return 0;
>  }
> @@ -1021,32 +1053,6 @@ static const struct clk_ops ub953_clkout_ops = {
>  	.set_rate	= ub953_clkout_set_rate,
>  };
>  
> -static void ub953_init_clkout_ub953(struct ub953_data *priv)
> -{
> -	u64 fc_rate;
> -	u8 hs_div, m, n;
> -
> -	fc_rate = ub953_get_fc_rate(priv);
> -
> -	ub953_calc_clkout_ub953(priv, 25000000, fc_rate, &hs_div, &m, &n);
> -
> -	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, (__ffs(hs_div) << 5) | m);
> -	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
> -}
> -
> -static void ub953_init_clkout_ub971(struct ub953_data *priv)
> -{
> -	u64 fc_rate;
> -	u8 m, n;
> -
> -	fc_rate = ub953_get_fc_rate(priv);
> -
> -	ub953_calc_clkout_ub971(priv, 25000000, fc_rate, &m, &n);
> -
> -	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, m);
> -	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, n);
> -}
> -
>  static int ub953_register_clkout(struct ub953_data *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> @@ -1055,16 +1061,15 @@ static int ub953_register_clkout(struct ub953_data *priv)
>  				  priv->hw_data->model, dev_name(dev)),
>  		.ops = &ub953_clkout_ops,
>  	};
> +	struct ub953_clkout_data clkout_data;
>  	int ret;
>  
>  	if (!init.name)
>  		return -ENOMEM;
>  
>  	/* Initialize clkout to 25MHz by default */
> -	if (priv->hw_data->is_ub971)
> -		ub953_init_clkout_ub971(priv);
> -	else
> -		ub953_init_clkout_ub953(priv);
> +	ub953_calc_clkout_params(priv, 25000000, &clkout_data);

While at it, a macro to replace the numerical constant could be nice.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	ub953_write_clkout_regs(priv, &clkout_data);
>  
>  	priv->clkout_clk_hw.init = &init;
>  
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 7/8] media: i2c: ds90ub953: Support non-sync mode

[Tomi Valkeinen]

Add support for FPD-Link non-sync mode with external clock. The only
thing that needs to be added is the calculation for the clkout.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 3a19c6dedd2f..846542e6e20d 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -143,6 +143,7 @@ struct ub953_data {
 
 	struct i2c_client	*client;
 	struct regmap		*regmap;
+	struct clk		*clkin;
 
 	u32			num_data_lanes;
 	bool			non_cont_clk;
@@ -842,15 +843,21 @@ static int ub953_i2c_master_init(struct ub953_data *priv)
 
 static u64 ub953_get_fc_rate(struct ub953_data *priv)
 {
-	if (priv->mode != UB953_MODE_SYNC) {
+	switch (priv->mode) {
+	case UB953_MODE_SYNC:
+		if (priv->hw_data->is_ub971)
+			return priv->plat_data->bc_rate * 160ull;
+		else
+			return priv->plat_data->bc_rate / 2 * 160ull;
+
+	case UB953_MODE_NONSYNC_EXT:
+		/* CLKIN_DIV = 1 always */
+		return clk_get_rate(priv->clkin) * 80ull;
+
+	default:
 		/* Not supported */
 		return 0;
 	}
-
-	if (priv->hw_data->is_ub971)
-		return priv->plat_data->bc_rate * 160ull;
-	else
-		return priv->plat_data->bc_rate / 2 * 160ull;
 }
 
 static unsigned long ub953_calc_clkout_ub953(struct ub953_data *priv,
@@ -1188,9 +1195,15 @@ static int ub953_hw_init(struct ub953_data *priv)
 	dev_dbg(dev, "mode from %s: %#x\n", mode_override ? "reg" : "strap",
 		priv->mode);
 
-	if (priv->mode != UB953_MODE_SYNC)
+	if (priv->mode != UB953_MODE_SYNC &&
+	    priv->mode != UB953_MODE_NONSYNC_EXT)
 		return dev_err_probe(dev, -ENODEV,
-				     "Only synchronous mode supported\n");
+				     "Unsupported mode selected: %d\n",
+				     priv->mode);
+
+	if (priv->mode == UB953_MODE_NONSYNC_EXT && !priv->clkin)
+		return dev_err_probe(dev, -EINVAL,
+				     "clkin required for non-sync ext mode\n");
 
 	ret = ub953_read(priv, UB953_REG_REV_MASK_ID, &v);
 	if (ret)
@@ -1318,6 +1331,11 @@ static int ub953_probe(struct i2c_client *client)
 		goto err_mutex_destroy;
 	}
 
+	priv->clkin = devm_clk_get_optional(dev, "clkin");
+	if (IS_ERR(priv->clkin))
+		return dev_err_probe(dev, PTR_ERR(priv->clkin),
+				     "failed to parse 'clkin'\n");
+
 	ret = ub953_parse_dt(priv);
 	if (ret)
 		goto err_mutex_destroy;

-- 
2.34.1

Re: [PATCH v2 7/8] media: i2c: ds90ub953: Support non-sync mode

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:38PM +0300, Tomi Valkeinen wrote:
> Add support for FPD-Link non-sync mode with external clock. The only
> thing that needs to be added is the calculation for the clkout.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/ds90ub953.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index 3a19c6dedd2f..846542e6e20d 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -143,6 +143,7 @@ struct ub953_data {
>  
>  	struct i2c_client	*client;
>  	struct regmap		*regmap;
> +	struct clk		*clkin;
>  
>  	u32			num_data_lanes;
>  	bool			non_cont_clk;
> @@ -842,15 +843,21 @@ static int ub953_i2c_master_init(struct ub953_data *priv)
>  
>  static u64 ub953_get_fc_rate(struct ub953_data *priv)
>  {
> -	if (priv->mode != UB953_MODE_SYNC) {
> +	switch (priv->mode) {
> +	case UB953_MODE_SYNC:
> +		if (priv->hw_data->is_ub971)
> +			return priv->plat_data->bc_rate * 160ull;
> +		else
> +			return priv->plat_data->bc_rate / 2 * 160ull;
> +
> +	case UB953_MODE_NONSYNC_EXT:
> +		/* CLKIN_DIV = 1 always */
> +		return clk_get_rate(priv->clkin) * 80ull;
> +
> +	default:
>  		/* Not supported */
>  		return 0;
>  	}
> -
> -	if (priv->hw_data->is_ub971)
> -		return priv->plat_data->bc_rate * 160ull;
> -	else
> -		return priv->plat_data->bc_rate / 2 * 160ull;
>  }
>  
>  static unsigned long ub953_calc_clkout_ub953(struct ub953_data *priv,
> @@ -1188,9 +1195,15 @@ static int ub953_hw_init(struct ub953_data *priv)
>  	dev_dbg(dev, "mode from %s: %#x\n", mode_override ? "reg" : "strap",
>  		priv->mode);
>  
> -	if (priv->mode != UB953_MODE_SYNC)
> +	if (priv->mode != UB953_MODE_SYNC &&
> +	    priv->mode != UB953_MODE_NONSYNC_EXT)
>  		return dev_err_probe(dev, -ENODEV,
> -				     "Only synchronous mode supported\n");
> +				     "Unsupported mode selected: %d\n",

%u

> +				     priv->mode);
> +
> +	if (priv->mode == UB953_MODE_NONSYNC_EXT && !priv->clkin)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "clkin required for non-sync ext mode\n");
>  
>  	ret = ub953_read(priv, UB953_REG_REV_MASK_ID, &v);
>  	if (ret)
> @@ -1318,6 +1331,11 @@ static int ub953_probe(struct i2c_client *client)
>  		goto err_mutex_destroy;
>  	}
>  
> +	priv->clkin = devm_clk_get_optional(dev, "clkin");
> +	if (IS_ERR(priv->clkin))
> +		return dev_err_probe(dev, PTR_ERR(priv->clkin),
> +				     "failed to parse 'clkin'\n");

You need to goto err_mutex_destroy.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	ret = ub953_parse_dt(priv);
>  	if (ret)
>  		goto err_mutex_destroy;
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 8/8] media: i2c: ds90ub960: Rename RXPORT_MODE_CSI2_ASYNC to RXPORT_MODE_CSI2_NONSYNC

[Tomi Valkeinen]

FPD-Link has an operating mode that used to be called "asynchronous" in
the hardware documentation, but that has been changed to non-synchronous
already quite a while back. The ub960 driver still had one instance of
the old naming, so let's rename it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub960.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index ea819fb6e99b..2ed4d4763a02 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -415,8 +415,8 @@ enum ub960_rxport_mode {
 	RXPORT_MODE_RAW12_HF = 1,
 	RXPORT_MODE_RAW12_LF = 2,
 	RXPORT_MODE_CSI2_SYNC = 3,
-	RXPORT_MODE_CSI2_ASYNC = 4,
-	RXPORT_MODE_LAST = RXPORT_MODE_CSI2_ASYNC,
+	RXPORT_MODE_CSI2_NONSYNC = 4,
+	RXPORT_MODE_LAST = RXPORT_MODE_CSI2_NONSYNC,
 };
 
 enum ub960_rxport_cdr {
@@ -1609,7 +1609,7 @@ static unsigned long ub960_calc_bc_clk_rate_ub960(struct ub960_data *priv,
 		div = 1;
 		break;
 
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		mult = 2;
 		div = 5;
 		break;
@@ -1633,7 +1633,7 @@ static unsigned long ub960_calc_bc_clk_rate_ub9702(struct ub960_data *priv,
 	case RXPORT_MODE_CSI2_SYNC:
 		return 47187500;
 
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		return 9437500;
 
 	default:
@@ -1840,7 +1840,7 @@ static void ub960_init_rx_port_ub960(struct ub960_data *priv,
 		bc_freq_val = 0;
 		break;
 
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		bc_freq_val = 2;
 		break;
 
@@ -1878,7 +1878,7 @@ static void ub960_init_rx_port_ub960(struct ub960_data *priv,
 		return;
 
 	case RXPORT_MODE_CSI2_SYNC:
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		/* CSI-2 Mode (DS90UB953-Q1 compatible) */
 		ub960_rxport_update_bits(priv, nport, UB960_RR_PORT_CONFIG, 0x3,
 					 0x0);
@@ -1938,7 +1938,7 @@ static void ub960_init_rx_port_ub9702_fpd3(struct ub960_data *priv,
 		fpd_func_mode = 2;
 		break;
 
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		bc_freq_val = 2;
 		fpd_func_mode = 2;
 		break;
@@ -2032,7 +2032,7 @@ static void ub960_init_rx_port_ub9702_fpd4(struct ub960_data *priv,
 		bc_freq_val = 6;
 		break;
 
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 		bc_freq_val = 2;
 		break;
 
@@ -2098,7 +2098,7 @@ static void ub960_init_rx_port_ub9702(struct ub960_data *priv,
 		return;
 
 	case RXPORT_MODE_CSI2_SYNC:
-	case RXPORT_MODE_CSI2_ASYNC:
+	case RXPORT_MODE_CSI2_NONSYNC:
 
 		break;
 	}
@@ -2444,7 +2444,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 
 		/* For the rest, we are only interested in parallel busses */
 		if (rxport->rx_mode == RXPORT_MODE_CSI2_SYNC ||
-		    rxport->rx_mode == RXPORT_MODE_CSI2_ASYNC)
+		    rxport->rx_mode == RXPORT_MODE_CSI2_NONSYNC)
 			continue;
 
 		if (rx_data[nport].num_streams > 2)
@@ -2508,7 +2508,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 			break;
 
 		case RXPORT_MODE_CSI2_SYNC:
-		case RXPORT_MODE_CSI2_ASYNC:
+		case RXPORT_MODE_CSI2_NONSYNC:
 			if (!priv->hw_data->is_ub9702) {
 				/* Map all VCs from this port to the same VC */
 				ub960_rxport_write(priv, nport, UB960_RR_CSI_VC_MAP,

-- 
2.34.1

Re: [PATCH v2 8/8] media: i2c: ds90ub960: Rename RXPORT_MODE_CSI2_ASYNC to RXPORT_MODE_CSI2_NONSYNC

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Thu, Jul 20, 2023 at 01:30:39PM +0300, Tomi Valkeinen wrote:
> FPD-Link has an operating mode that used to be called "asynchronous" in
> the hardware documentation, but that has been changed to non-synchronous
> already quite a while back. The ub960 driver still had one instance of
> the old naming, so let's rename it.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/ds90ub960.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index ea819fb6e99b..2ed4d4763a02 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -415,8 +415,8 @@ enum ub960_rxport_mode {
>  	RXPORT_MODE_RAW12_HF = 1,
>  	RXPORT_MODE_RAW12_LF = 2,
>  	RXPORT_MODE_CSI2_SYNC = 3,
> -	RXPORT_MODE_CSI2_ASYNC = 4,
> -	RXPORT_MODE_LAST = RXPORT_MODE_CSI2_ASYNC,
> +	RXPORT_MODE_CSI2_NONSYNC = 4,
> +	RXPORT_MODE_LAST = RXPORT_MODE_CSI2_NONSYNC,
>  };
>  
>  enum ub960_rxport_cdr {
> @@ -1609,7 +1609,7 @@ static unsigned long ub960_calc_bc_clk_rate_ub960(struct ub960_data *priv,
>  		div = 1;
>  		break;
>  
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		mult = 2;
>  		div = 5;
>  		break;
> @@ -1633,7 +1633,7 @@ static unsigned long ub960_calc_bc_clk_rate_ub9702(struct ub960_data *priv,
>  	case RXPORT_MODE_CSI2_SYNC:
>  		return 47187500;
>  
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		return 9437500;
>  
>  	default:
> @@ -1840,7 +1840,7 @@ static void ub960_init_rx_port_ub960(struct ub960_data *priv,
>  		bc_freq_val = 0;
>  		break;
>  
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		bc_freq_val = 2;
>  		break;
>  
> @@ -1878,7 +1878,7 @@ static void ub960_init_rx_port_ub960(struct ub960_data *priv,
>  		return;
>  
>  	case RXPORT_MODE_CSI2_SYNC:
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		/* CSI-2 Mode (DS90UB953-Q1 compatible) */
>  		ub960_rxport_update_bits(priv, nport, UB960_RR_PORT_CONFIG, 0x3,
>  					 0x0);
> @@ -1938,7 +1938,7 @@ static void ub960_init_rx_port_ub9702_fpd3(struct ub960_data *priv,
>  		fpd_func_mode = 2;
>  		break;
>  
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		bc_freq_val = 2;
>  		fpd_func_mode = 2;
>  		break;
> @@ -2032,7 +2032,7 @@ static void ub960_init_rx_port_ub9702_fpd4(struct ub960_data *priv,
>  		bc_freq_val = 6;
>  		break;
>  
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  		bc_freq_val = 2;
>  		break;
>  
> @@ -2098,7 +2098,7 @@ static void ub960_init_rx_port_ub9702(struct ub960_data *priv,
>  		return;
>  
>  	case RXPORT_MODE_CSI2_SYNC:
> -	case RXPORT_MODE_CSI2_ASYNC:
> +	case RXPORT_MODE_CSI2_NONSYNC:
>  
>  		break;
>  	}
> @@ -2444,7 +2444,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
>  
>  		/* For the rest, we are only interested in parallel busses */
>  		if (rxport->rx_mode == RXPORT_MODE_CSI2_SYNC ||
> -		    rxport->rx_mode == RXPORT_MODE_CSI2_ASYNC)
> +		    rxport->rx_mode == RXPORT_MODE_CSI2_NONSYNC)
>  			continue;
>  
>  		if (rx_data[nport].num_streams > 2)
> @@ -2508,7 +2508,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
>  			break;
>  
>  		case RXPORT_MODE_CSI2_SYNC:
> -		case RXPORT_MODE_CSI2_ASYNC:
> +		case RXPORT_MODE_CSI2_NONSYNC:
>  			if (!priv->hw_data->is_ub9702) {
>  				/* Map all VCs from this port to the same VC */
>  				ub960_rxport_write(priv, nport, UB960_RR_CSI_VC_MAP,
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/8] media: i2c: ds90ub9xx: Misc improvements

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 01:30:31PM +0300, Tomi Valkeinen wrote:
> This series contains small miscellaneous improvements to the FPD-Link
> drivers.
> 
> These were sent originally in v14 of the "i2c-atr and FPDLink" series
> (link below), but were then left out for v15. So I have assigned v2 to
> this series.
> 
> I have trimmed the to/cc list a bit, as these don't really deal with i2c
> and dt anymore.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
for all, but patch 6. I still think u32_fract makes sense.

-- 
With Best Regards,
Andy Shevchenko