[PATCH v3 0/9] media: i2c: ds90ub9xx: Misc improvements

[PATCH v3 0/9] 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
the "first" version of 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 v3:
- New patch which fixes the async sub-device matching
- Addressed all the comments from Laurent
- Added Laurent's & Andy's Rbs
- Link to v2: https://lore.kernel.org/r/20230720-fpdlink-additions-v2-0-b91b1eca2ad3@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 (9):
      media: i2c: ds90ub9x3: Fix sub-device matching
      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 |  47 +++++----
 drivers/media/i2c/ds90ub953.c | 215 ++++++++++++++++++++++++------------------
 drivers/media/i2c/ds90ub960.c |  31 +++---
 3 files changed, 164 insertions(+), 129 deletions(-)
---
base-commit: 38d6a03d2511bee97455b2ea0314c918537ef1ec
change-id: 20230720-fpdlink-additions-fb5397336725

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

[PATCH v3 1/9] media: i2c: ds90ub9x3: Fix sub-device matching

[Tomi Valkeinen]

1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode
matching") recently changed how async sub-device matching works. This
breaks the UB9x3 drivers, as they set the subdev.fwnode to an endpoint.
Afaiu, the fix is simply to not set subdev.fwnode at all.

Fixes: 1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode matching")
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ds90ub913.c | 14 +-------------
 drivers/media/i2c/ds90ub953.c | 13 +------------
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 80d9cf6dd945..5410ccb54057 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -749,19 +749,9 @@ static int ub913_subdev_init(struct ub913_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init pads\n");
 
-	priv->sd.fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
-							  UB913_PAD_SOURCE, 0,
-							  0);
-
-	if (!priv->sd.fwnode) {
-		ret = -ENODEV;
-		dev_err_probe(dev, ret, "Missing TX endpoint\n");
-		goto err_entity_cleanup;
-	}
-
 	ret = v4l2_subdev_init_finalize(&priv->sd);
 	if (ret)
-		goto err_fwnode_put;
+		goto err_entity_cleanup;
 
 	ret = ub913_v4l2_notifier_register(priv);
 	if (ret) {
@@ -782,8 +772,6 @@ static int ub913_subdev_init(struct ub913_data *priv)
 	ub913_v4l2_nf_unregister(priv);
 err_subdev_cleanup:
 	v4l2_subdev_cleanup(&priv->sd);
-err_fwnode_put:
-	fwnode_handle_put(priv->sd.fwnode);
 err_entity_cleanup:
 	media_entity_cleanup(&priv->sd.entity);
 
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index cadf75eb0773..20b9cf893f74 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -1221,18 +1221,9 @@ static int ub953_subdev_init(struct ub953_data *priv)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init pads\n");
 
-	priv->sd.fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
-							  UB953_PAD_SOURCE, 0,
-							  0);
-	if (!priv->sd.fwnode) {
-		ret = -ENODEV;
-		dev_err_probe(dev, ret, "Missing TX endpoint\n");
-		goto err_entity_cleanup;
-	}
-
 	ret = v4l2_subdev_init_finalize(&priv->sd);
 	if (ret)
-		goto err_fwnode_put;
+		goto err_entity_cleanup;
 
 	ret = ub953_v4l2_notifier_register(priv);
 	if (ret) {
@@ -1253,8 +1244,6 @@ static int ub953_subdev_init(struct ub953_data *priv)
 	ub953_v4l2_notifier_unregister(priv);
 err_free_state:
 	v4l2_subdev_cleanup(&priv->sd);
-err_fwnode_put:
-	fwnode_handle_put(priv->sd.fwnode);
 err_entity_cleanup:
 	media_entity_cleanup(&priv->sd.entity);
 

-- 
2.34.1

Re: [PATCH v3 1/9] media: i2c: ds90ub9x3: Fix sub-device matching

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Jul 31, 2023 at 04:24:35PM +0300, Tomi Valkeinen wrote:
> 1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode

s/^/Commit /

> matching") recently changed how async sub-device matching works. This
> breaks the UB9x3 drivers, as they set the subdev.fwnode to an endpoint.
> Afaiu, the fix is simply to not set subdev.fwnode at all.
> 
> Fixes: 1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode matching")
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

Sakari, was the v4l2-async series meant to break these drivers ? I
understand the two series got merged for the same kernel version, is
this a merge conflict, or is there an issue in the v4l2-async rework ?

> ---
>  drivers/media/i2c/ds90ub913.c | 14 +-------------
>  drivers/media/i2c/ds90ub953.c | 13 +------------
>  2 files changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 80d9cf6dd945..5410ccb54057 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -749,19 +749,9 @@ static int ub913_subdev_init(struct ub913_data *priv)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to init pads\n");
>  
> -	priv->sd.fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> -							  UB913_PAD_SOURCE, 0,
> -							  0);
> -
> -	if (!priv->sd.fwnode) {
> -		ret = -ENODEV;
> -		dev_err_probe(dev, ret, "Missing TX endpoint\n");
> -		goto err_entity_cleanup;
> -	}
> -
>  	ret = v4l2_subdev_init_finalize(&priv->sd);
>  	if (ret)
> -		goto err_fwnode_put;
> +		goto err_entity_cleanup;
>  
>  	ret = ub913_v4l2_notifier_register(priv);
>  	if (ret) {
> @@ -782,8 +772,6 @@ static int ub913_subdev_init(struct ub913_data *priv)
>  	ub913_v4l2_nf_unregister(priv);
>  err_subdev_cleanup:
>  	v4l2_subdev_cleanup(&priv->sd);
> -err_fwnode_put:
> -	fwnode_handle_put(priv->sd.fwnode);
>  err_entity_cleanup:
>  	media_entity_cleanup(&priv->sd.entity);
>  
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index cadf75eb0773..20b9cf893f74 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -1221,18 +1221,9 @@ static int ub953_subdev_init(struct ub953_data *priv)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to init pads\n");
>  
> -	priv->sd.fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> -							  UB953_PAD_SOURCE, 0,
> -							  0);
> -	if (!priv->sd.fwnode) {
> -		ret = -ENODEV;
> -		dev_err_probe(dev, ret, "Missing TX endpoint\n");
> -		goto err_entity_cleanup;
> -	}
> -
>  	ret = v4l2_subdev_init_finalize(&priv->sd);
>  	if (ret)
> -		goto err_fwnode_put;
> +		goto err_entity_cleanup;
>  
>  	ret = ub953_v4l2_notifier_register(priv);
>  	if (ret) {
> @@ -1253,8 +1244,6 @@ static int ub953_subdev_init(struct ub953_data *priv)
>  	ub953_v4l2_notifier_unregister(priv);
>  err_free_state:
>  	v4l2_subdev_cleanup(&priv->sd);
> -err_fwnode_put:
> -	fwnode_handle_put(priv->sd.fwnode);
>  err_entity_cleanup:
>  	media_entity_cleanup(&priv->sd.entity);
>  
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/9] media: i2c: ds90ub9x3: Fix sub-device matching

[Sakari Ailus]

Hi Laurent,

On Mon, Jul 31, 2023 at 05:43:56PM +0300, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Jul 31, 2023 at 04:24:35PM +0300, Tomi Valkeinen wrote:
> > 1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode
> 
> s/^/Commit /
> 
> > matching") recently changed how async sub-device matching works. This
> > breaks the UB9x3 drivers, as they set the subdev.fwnode to an endpoint.
> > Afaiu, the fix is simply to not set subdev.fwnode at all.
> > 
> > Fixes: 1029939b3782 ("media: v4l: async: Simplify async sub-device fwnode matching")
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Sakari, was the v4l2-async series meant to break these drivers ? I
> understand the two series got merged for the same kernel version, is
> this a merge conflict, or is there an issue in the v4l2-async rework ?

The ds90ub9xx drivers were merged after I had written the patch that
converted all drivers and I didn't remember to revisit it.

If you look at the patch, it's doing very similar things than the patch in
the Fixes: tag.

There's also a workaround for sub-device drivers (that register async
sub-devices) but not for drivers that register a notifier. It probably
doesn't make sense to add a workaround for those how, rather remove the one
that exists (after some time).

-- 
Kind regards,

Sakari Ailus

[PATCH v3 2/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 4833b39b9178..b840c9160bf2 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

[PATCH v3 3/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ds90ub953.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 20b9cf893f74..1032c8633809 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,11 @@ 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 = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
 	struct fwnode_handle *ep_fwnode;
+	unsigned char nlanes;
 	int ret;
 
 	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
@@ -1119,19 +1125,20 @@ 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");
+	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: %u\n", nlanes);
 
-	priv->num_data_lanes = ret;
+	priv->num_data_lanes = nlanes;
 
 	return 0;
 }

-- 
2.34.1

[PATCH v3 4/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ds90ub913.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 5410ccb54057..85b664e33482 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;
+	bool			pclk_polarity_rising;
 };
 
 static inline struct ub913_data *sd_to_ub913(struct v4l2_subdev *sd)
@@ -675,25 +677,32 @@ 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 = {
+		.bus_type = V4L2_MBUS_PARALLEL,
+	};
 	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);
+	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 +735,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

[PATCH v3 5/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 1032c8633809..eedbca986928 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_continous_clk;
 
 	struct gpio_chip	gpio_chip;
 
@@ -1140,6 +1141,9 @@ static int ub953_parse_dt(struct ub953_data *priv)
 
 	priv->num_data_lanes = nlanes;
 
+	priv->non_continous_clk = vep.bus.mipi_csi2.flags &
+				  V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	return 0;
 }
 
@@ -1202,7 +1206,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_continous_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

[PATCH v3 6/9] 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>
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 b840c9160bf2..c29db59e340b 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

[PATCH v3 7/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ds90ub953.c | 141 +++++++++++++++++++++++-------------------
 1 file changed, 76 insertions(+), 65 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index eedbca986928..e1bd33e91eff 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -34,6 +34,8 @@
 
 #define UB953_NUM_GPIOS			4
 
+#define UB953_DEFAULT_CLKOUT_RATE	25000000UL
+
 #define UB953_REG_RESET_CTL			0x01
 #define UB953_REG_RESET_CTL_DIGITAL_RESET_1	BIT(1)
 #define UB953_REG_RESET_CTL_DIGITAL_RESET_0	BIT(0)
@@ -131,6 +133,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 +915,62 @@ 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,
+				    const struct ub953_clkout_data *clkout_data)
+{
+	u8 clkout_ctrl0, clkout_ctrl1;
+
+	if (priv->hw_data->is_ub971)
+		clkout_ctrl0 = clkout_data->m;
+	else
+		clkout_ctrl0 = (__ffs(clkout_data->hs_div) << 5) |
+			       clkout_data->m;
+
+	clkout_ctrl1 = clkout_data->n;
+
+	ub953_write(priv, UB953_REG_CLKOUT_CTRL0, clkout_ctrl0);
+	ub953_write(priv, UB953_REG_CLKOUT_CTRL1, clkout_ctrl1);
+}
+
 static unsigned long ub953_clkout_recalc_rate(struct clk_hw *hw,
 					      unsigned long parent_rate)
 {
@@ -965,52 +1030,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);
-
-	if (priv->hw_data->is_ub971) {
-		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
+	struct ub953_clkout_data 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);
+	ub953_calc_clkout_params(priv, rate, &clkout_data);
 
-		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);
+	ub953_calc_clkout_params(priv, rate, &clkout_data);
 
-	if (priv->hw_data->is_ub971) {
-		res = ub953_calc_clkout_ub971(priv, rate, fc_rate, &m, &n);
+	dev_dbg(&priv->client->dev, "%s %lu (requested %lu)\n", __func__,
+		clkout_data.rate, rate);
 
-		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_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__, res,
-		rate);
+	ub953_write_clkout_regs(priv, &clkout_data);
 
 	return 0;
 }
@@ -1021,32 +1059,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 +1067,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, UB953_DEFAULT_CLKOUT_RATE, &clkout_data);
+	ub953_write_clkout_regs(priv, &clkout_data);
 
 	priv->clkout_clk_hw.init = &init;
 

-- 
2.34.1

Re: [PATCH v3 7/9] media: i2c: ds90ub953: Restructure clkout management

[Andy Shevchenko]

On Mon, Jul 31, 2023 at 04:24:41PM +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.

...

> +#define UB953_DEFAULT_CLKOUT_RATE	25000000UL

HZ_PER_MHZ (from units.h)?

...

> +struct ub953_clkout_data {
> +	u32 hs_div;
> +	u32 m;
> +	u32 n;
> +	unsigned long rate;

You may save 4 bytes on some architectures (which do not allow 4-byte alignment
for 64-bit members) by reshuffling the members.

(besides using u32-fract :-)

> +};

...

> +		dev_dbg(dev, "%s %llu * %u / (8 * %u) = %lu (requested %lu)",
> +			__func__, fc_rate, m, n, clkout_rate, target_rate);

__func__ in dev_dbg() is not needed. It's very rare nowadays to debug a kernel
without Dynamic Debug to be on.

...

> +		dev_dbg(dev, "%s %llu / %u * %u / %u = %lu (requested %lu)",
> +			__func__, fc_rate, hs_div, m, n, clkout_rate,
> +			target_rate);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 8/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ds90ub953.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index e1bd33e91eff..d56c1dda89b3 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -145,6 +145,7 @@ struct ub953_data {
 
 	struct i2c_client	*client;
 	struct regmap		*regmap;
+	struct clk		*clkin;
 
 	u32			num_data_lanes;
 	bool			non_continous_clk;
@@ -844,15 +845,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,
@@ -1195,9 +1202,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: %u\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)
@@ -1314,6 +1327,13 @@ 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)) {
+		ret = PTR_ERR(priv->clkin);
+		dev_err_probe(dev, ret, "failed to parse 'clkin'\n");
+		goto err_mutex_destroy;
+	}
+
 	ret = ub953_parse_dt(priv);
 	if (ret)
 		goto err_mutex_destroy;

-- 
2.34.1

[PATCH v3 9/9] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 c29db59e340b..f619c23e084a 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