| Message ID | 20230211144614.3816247-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: max96712: Add support for 3-lane C-PHY | expand |
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 >
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; > } >
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?
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.
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
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; }
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(-)