| Message ID | 20230720-fpdlink-additions-v2-0-b91b1eca2ad3@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: i2c: ds90ub9xx: Misc improvements | expand |
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.
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.
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); > } >
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; >
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; >
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,