[PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Niklas Söderlund]

Add basic support for outputting the test patterns on a 3-lane CSI-2
C-PHY bus. As the driver only can output frames form its internal test
pattern generator, enabling C-PHY output is as simple as setting the
output mode to C-PHY instead of D-PHY.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/staging/media/max96712/max96712.c | 36 +++++++++++++++++++----
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index 99b333b68198..d93dd985fb27 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -30,6 +30,7 @@ struct max96712_priv {
 	struct regmap *regmap;
 	struct gpio_desc *gpiod_pwdn;
 
+	bool cphy;
 	struct v4l2_mbus_config_mipi_csi2 mipi;
 
 	struct v4l2_subdev sd;
@@ -127,10 +128,18 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
 	/* Select 2x4 mode. */
 	max96712_write(priv, 0x8a0, 0x04);
 
-	/* Configure a 4-lane DPHY using PHY0 and PHY1. */
 	/* TODO: Add support for 2-lane and 1-lane configurations. */
-	/* TODO: Add support CPHY mode. */
-	max96712_write(priv, 0x94a, 0xc0);
+	if (priv->cphy) {
+		/* Configure a 3-lane C-PHY using PHY0 and PHY1. */
+		max96712_write(priv, 0x94a, 0xa0);
+
+		/* Configure C-PHY timings. */
+		max96712_write(priv, 0x8ad, 0x3f);
+		max96712_write(priv, 0x8ae, 0x7d);
+	} else {
+		/* Configure a 4-lane D-PHY using PHY0 and PHY1. */
+		max96712_write(priv, 0x94a, 0xc0);
+	}
 
 	/* Configure lane mapping for PHY0 and PHY1. */
 	/* TODO: Add support for lane swapping. */
@@ -332,8 +341,9 @@ static int max96712_parse_dt(struct max96712_priv *priv)
 {
 	struct fwnode_handle *ep;
 	struct v4l2_fwnode_endpoint v4l2_ep = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY
+		.bus_type = V4L2_MBUS_UNKNOWN,
 	};
+	unsigned int supported_lanes;
 	int ret;
 
 	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(&priv->client->dev), 4,
@@ -350,8 +360,22 @@ static int max96712_parse_dt(struct max96712_priv *priv)
 		return -EINVAL;
 	}
 
-	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != 4) {
-		dev_err(&priv->client->dev, "Only 4 data lanes supported\n");
+	switch (v4l2_ep.bus_type) {
+	case V4L2_MBUS_CSI2_DPHY:
+		supported_lanes = 4;
+		priv->cphy = false;
+		break;
+	case V4L2_MBUS_CSI2_CPHY:
+		supported_lanes = 3;
+		priv->cphy = true;
+		break;
+	default:
+		dev_err(&priv->client->dev, "Unsupported bus-type %u\n", v4l2_ep.bus_type);
+		return -EINVAL;
+	}
+
+	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != supported_lanes) {
+		dev_err(&priv->client->dev, "Only %u data lanes supported\n", supported_lanes);
 		return -EINVAL;
 	}
 
-- 
2.39.1

Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Niklas Söderlund]

Hi Mauro,

Gentle ping on this patch.

On 2023-02-11 15:46:14 +0100, Niklas Söderlund wrote:
> Add basic support for outputting the test patterns on a 3-lane CSI-2
> C-PHY bus. As the driver only can output frames form its internal test
> pattern generator, enabling C-PHY output is as simple as setting the
> output mode to C-PHY instead of D-PHY.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/staging/media/max96712/max96712.c | 36 +++++++++++++++++++----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 99b333b68198..d93dd985fb27 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -30,6 +30,7 @@ struct max96712_priv {
>  	struct regmap *regmap;
>  	struct gpio_desc *gpiod_pwdn;
>  
> +	bool cphy;
>  	struct v4l2_mbus_config_mipi_csi2 mipi;
>  
>  	struct v4l2_subdev sd;
> @@ -127,10 +128,18 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	/* Select 2x4 mode. */
>  	max96712_write(priv, 0x8a0, 0x04);
>  
> -	/* Configure a 4-lane DPHY using PHY0 and PHY1. */
>  	/* TODO: Add support for 2-lane and 1-lane configurations. */
> -	/* TODO: Add support CPHY mode. */
> -	max96712_write(priv, 0x94a, 0xc0);
> +	if (priv->cphy) {
> +		/* Configure a 3-lane C-PHY using PHY0 and PHY1. */
> +		max96712_write(priv, 0x94a, 0xa0);
> +
> +		/* Configure C-PHY timings. */
> +		max96712_write(priv, 0x8ad, 0x3f);
> +		max96712_write(priv, 0x8ae, 0x7d);
> +	} else {
> +		/* Configure a 4-lane D-PHY using PHY0 and PHY1. */
> +		max96712_write(priv, 0x94a, 0xc0);
> +	}
>  
>  	/* Configure lane mapping for PHY0 and PHY1. */
>  	/* TODO: Add support for lane swapping. */
> @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct max96712_priv *priv)
>  {
>  	struct fwnode_handle *ep;
>  	struct v4l2_fwnode_endpoint v4l2_ep = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY
> +		.bus_type = V4L2_MBUS_UNKNOWN,
>  	};
> +	unsigned int supported_lanes;
>  	int ret;
>  
>  	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(&priv->client->dev), 4,
> @@ -350,8 +360,22 @@ static int max96712_parse_dt(struct max96712_priv *priv)
>  		return -EINVAL;
>  	}
>  
> -	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != 4) {
> -		dev_err(&priv->client->dev, "Only 4 data lanes supported\n");
> +	switch (v4l2_ep.bus_type) {
> +	case V4L2_MBUS_CSI2_DPHY:
> +		supported_lanes = 4;
> +		priv->cphy = false;
> +		break;
> +	case V4L2_MBUS_CSI2_CPHY:
> +		supported_lanes = 3;
> +		priv->cphy = true;
> +		break;
> +	default:
> +		dev_err(&priv->client->dev, "Unsupported bus-type %u\n", v4l2_ep.bus_type);
> +		return -EINVAL;
> +	}
> +
> +	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != supported_lanes) {
> +		dev_err(&priv->client->dev, "Only %u data lanes supported\n", supported_lanes);
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.39.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Sakari Ailus]

Hejssan,

On Sat, Feb 11, 2023 at 03:46:14PM +0100, Niklas Söderlund wrote:
> Add basic support for outputting the test patterns on a 3-lane CSI-2
> C-PHY bus. As the driver only can output frames form its internal test
> pattern generator, enabling C-PHY output is as simple as setting the
> output mode to C-PHY instead of D-PHY.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/staging/media/max96712/max96712.c | 36 +++++++++++++++++++----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 99b333b68198..d93dd985fb27 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -30,6 +30,7 @@ struct max96712_priv {
>  	struct regmap *regmap;
>  	struct gpio_desc *gpiod_pwdn;
>  
> +	bool cphy;
>  	struct v4l2_mbus_config_mipi_csi2 mipi;
>  
>  	struct v4l2_subdev sd;
> @@ -127,10 +128,18 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	/* Select 2x4 mode. */
>  	max96712_write(priv, 0x8a0, 0x04);
>  
> -	/* Configure a 4-lane DPHY using PHY0 and PHY1. */
>  	/* TODO: Add support for 2-lane and 1-lane configurations. */
> -	/* TODO: Add support CPHY mode. */
> -	max96712_write(priv, 0x94a, 0xc0);
> +	if (priv->cphy) {
> +		/* Configure a 3-lane C-PHY using PHY0 and PHY1. */
> +		max96712_write(priv, 0x94a, 0xa0);
> +
> +		/* Configure C-PHY timings. */
> +		max96712_write(priv, 0x8ad, 0x3f);
> +		max96712_write(priv, 0x8ae, 0x7d);
> +	} else {
> +		/* Configure a 4-lane D-PHY using PHY0 and PHY1. */
> +		max96712_write(priv, 0x94a, 0xc0);
> +	}
>  
>  	/* Configure lane mapping for PHY0 and PHY1. */
>  	/* TODO: Add support for lane swapping. */
> @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct max96712_priv *priv)
>  {
>  	struct fwnode_handle *ep;
>  	struct v4l2_fwnode_endpoint v4l2_ep = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY
> +		.bus_type = V4L2_MBUS_UNKNOWN,

The bindings don't require setting bus-type. Please change the bindings as
well. And assume D-PHY in the driver if bus-type isn't set.

>  	};
> +	unsigned int supported_lanes;
>  	int ret;
>  
>  	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(&priv->client->dev), 4,
> @@ -350,8 +360,22 @@ static int max96712_parse_dt(struct max96712_priv *priv)
>  		return -EINVAL;
>  	}
>  
> -	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != 4) {
> -		dev_err(&priv->client->dev, "Only 4 data lanes supported\n");
> +	switch (v4l2_ep.bus_type) {
> +	case V4L2_MBUS_CSI2_DPHY:
> +		supported_lanes = 4;
> +		priv->cphy = false;
> +		break;
> +	case V4L2_MBUS_CSI2_CPHY:
> +		supported_lanes = 3;
> +		priv->cphy = true;
> +		break;
> +	default:
> +		dev_err(&priv->client->dev, "Unsupported bus-type %u\n", v4l2_ep.bus_type);
> +		return -EINVAL;
> +	}
> +
> +	if (v4l2_ep.bus.mipi_csi2.num_data_lanes != supported_lanes) {
> +		dev_err(&priv->client->dev, "Only %u data lanes supported\n", supported_lanes);
>  		return -EINVAL;
>  	}
>  

-- 
Hälsningar,

Sakari Ailus

Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Niklas Söderlund]

Hej Sakari,

Tack för din feedback.

On 2023-03-31 15:21:34 +0300, Sakari Ailus wrote:

...

> > @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct 
> > max96712_priv *priv)
> >  {
> >  	struct fwnode_handle *ep;
> >  	struct v4l2_fwnode_endpoint v4l2_ep = {
> > -		.bus_type = V4L2_MBUS_CSI2_DPHY
> > +		.bus_type = V4L2_MBUS_UNKNOWN,
> 
> The bindings don't require setting bus-type. Please change the bindings as
> well. And assume D-PHY in the driver if bus-type isn't set.

Thanks for spotting this, I will send out an update to update the 
binding to require setting bus-type.

About updating the driver to assume D-PHY if bus-type is not set. I 
wonder if we can avoid that and keep the driver change as is? The only 
in-tree user of this binding is in r8a779a0-falcon-csi-dsi.dtsi, and I 
intend to send out a patch to set the bus-type for that together with 
the updated bindings.

I have tested this driver change on the Falcon board and if bus-type is 
not explicitly set in the DTS it is reported as D-PHY and everything 
works as expected. So if I

- Send out a patch to update the bindings to require bus-type being set.
- Send out patch to update the only in-tree use of the driver to set 
  bus-type.

Can't we avoid issues in the future by not assuming no bus-type is D-PHY 
in the driver while still being backward compatible with the old DTS?

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Sakari Ailus]

Hejssan, Niklas!

On Fri, Mar 31, 2023 at 03:24:04PM +0200, Niklas Söderlund wrote:
> Hej Sakari,
> 
> Tack för din feedback.
> 
> On 2023-03-31 15:21:34 +0300, Sakari Ailus wrote:
> 
> ...
> 
> > > @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct 
> > > max96712_priv *priv)
> > >  {
> > >  	struct fwnode_handle *ep;
> > >  	struct v4l2_fwnode_endpoint v4l2_ep = {
> > > -		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > +		.bus_type = V4L2_MBUS_UNKNOWN,
> > 
> > The bindings don't require setting bus-type. Please change the bindings as
> > well. And assume D-PHY in the driver if bus-type isn't set.
> 
> Thanks for spotting this, I will send out an update to update the 
> binding to require setting bus-type.
> 
> About updating the driver to assume D-PHY if bus-type is not set. I 
> wonder if we can avoid that and keep the driver change as is? The only 
> in-tree user of this binding is in r8a779a0-falcon-csi-dsi.dtsi, and I 
> intend to send out a patch to set the bus-type for that together with 
> the updated bindings.
> 
> I have tested this driver change on the Falcon board and if bus-type is 
> not explicitly set in the DTS it is reported as D-PHY and everything 
> works as expected. So if I
> 
> - Send out a patch to update the bindings to require bus-type being set.
> - Send out patch to update the only in-tree use of the driver to set 
>   bus-type.
> 
> Can't we avoid issues in the future by not assuming no bus-type is D-PHY 
> in the driver while still being backward compatible with the old DTS?

If you want to keep supporting D-PHY as a default, you should instead try
parsing with V4L2_MBUS_CSI2_DPHY as bus_type. Then CPHY if that fails.

Although if bus_type is mandatory, then this patch is fine.

-- 
Trevliga hälsningar,

Sakari Ailus

Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

[Niklas Söderlund]

Hej Sakari,

On 2023-03-31 17:08:53 +0300, Sakari Ailus wrote:
> Hejssan, Niklas!
> 
> On Fri, Mar 31, 2023 at 03:24:04PM +0200, Niklas Söderlund wrote:
> > Hej Sakari,
> > 
> > Tack för din feedback.
> > 
> > On 2023-03-31 15:21:34 +0300, Sakari Ailus wrote:
> > 
> > ...
> > 
> > > > @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct 
> > > > max96712_priv *priv)
> > > >  {
> > > >  	struct fwnode_handle *ep;
> > > >  	struct v4l2_fwnode_endpoint v4l2_ep = {
> > > > -		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > > +		.bus_type = V4L2_MBUS_UNKNOWN,
> > > 
> > > The bindings don't require setting bus-type. Please change the bindings as
> > > well. And assume D-PHY in the driver if bus-type isn't set.
> > 
> > Thanks for spotting this, I will send out an update to update the 
> > binding to require setting bus-type.
> > 
> > About updating the driver to assume D-PHY if bus-type is not set. I 
> > wonder if we can avoid that and keep the driver change as is? The only 
> > in-tree user of this binding is in r8a779a0-falcon-csi-dsi.dtsi, and I 
> > intend to send out a patch to set the bus-type for that together with 
> > the updated bindings.
> > 
> > I have tested this driver change on the Falcon board and if bus-type is 
> > not explicitly set in the DTS it is reported as D-PHY and everything 
> > works as expected. So if I
> > 
> > - Send out a patch to update the bindings to require bus-type being set.
> > - Send out patch to update the only in-tree use of the driver to set 
> >   bus-type.
> > 
> > Can't we avoid issues in the future by not assuming no bus-type is D-PHY 
> > in the driver while still being backward compatible with the old DTS?
> 
> If you want to keep supporting D-PHY as a default, you should instead try
> parsing with V4L2_MBUS_CSI2_DPHY as bus_type. Then CPHY if that fails.
> 
> Although if bus_type is mandatory, then this patch is fine.

Thanks. I prefers to make bus-type a mandatory property, it feels like 
being explicit in the bindings is the best way going forward. I have 
posted the patches to make this change and to update the only user.

There is only one user in-tree, and making the change now would still be 
backward compatible with it. So better bite the bullet now before the 
binding spreads.

Trevlig helg!

> 
> -- 
> Trevliga hälsningar,
> 
> Sakari Ailus

-- 
Kind Regards,
Niklas Söderlund