| Message ID | 20230731-upstream_csi-v8-8-fb7d3661c2c9@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CSI2RX support on J721E and AM62 | expand |
On 31/07/2023 11:29, Jai Luthra wrote: > From: Pratyush Yadav <p.yadav@ti.com> > > Some platforms like TI's J721E can have the CSI2RX paired with an > external DPHY. Use the generic PHY framework to configure the DPHY with > the correct link frequency. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Co-authored-by: Jai Luthra <j-luthra@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > v7->v8 > - Drop original patch in-lieu of already merged > https://lore.kernel.org/linux-media/20230523085626.3295-5-jack.zhu@starfivetech.com/ > - Add a new patch to configure DPHY using link_freq control from the > source > > drivers/media/platform/cadence/cdns-csi2rx.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 4f9654366485..2a80c66fb547 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -149,8 +149,33 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx) > { > union phy_configure_opts opts = { }; > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > + struct v4l2_subdev_format sd_fmt = { > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > + .pad = CSI2RX_PAD_SINK, > + .stream = 0, I would drop the above line, as this doesn't support streams yet. > + }; > + const struct csi2rx_fmt *fmt; > + s64 link_freq; > int ret; > > + ret = v4l2_subdev_call_state_active(&csi2rx->subdev, pad, get_fmt, > + &sd_fmt); > + if (ret < 0) > + return ret; > + > + fmt = csi2rx_get_fmt_by_code(sd_fmt.format.code); > + > + link_freq = v4l2_get_link_freq(csi2rx->source_subdev->ctrl_handler, > + fmt->bpp, 2 * csi2rx->num_lanes); > + if (link_freq < 0) > + return link_freq; > + > + ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq, > + csi2rx->num_lanes, cfg); > + if (ret) > + return ret; > + > ret = phy_power_on(csi2rx->dphy); > if (ret) > return ret; > I think this is fine, but a few things to think about: Here you lock and unlock the active state for the duration of the get_fmt call (and you call it for this same subdev). If the driver supports active state, and you need to get the fmt of the driver's subdev, I think it usually makes more sense to just use the state and get the format from there. And you might want to keep the active state locked for the duration of the s_stream operation, in which case the state would already be locked when csi2rx_configure_ext_dphy() gets called. And if you implement enable_streams/disable_streams, then the state has already been locked by the framework. However, I think enable_streams/disable_streams only works if the driver actually supports streams. But maybe you have already done all these with the out-of-tree streams support? Anyway, I think this is fine for now: Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index 4f9654366485..2a80c66fb547 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -149,8 +149,33 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx) { union phy_configure_opts opts = { }; + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; + struct v4l2_subdev_format sd_fmt = { + .which = V4L2_SUBDEV_FORMAT_ACTIVE, + .pad = CSI2RX_PAD_SINK, + .stream = 0, + }; + const struct csi2rx_fmt *fmt; + s64 link_freq; int ret; + ret = v4l2_subdev_call_state_active(&csi2rx->subdev, pad, get_fmt, + &sd_fmt); + if (ret < 0) + return ret; + + fmt = csi2rx_get_fmt_by_code(sd_fmt.format.code); + + link_freq = v4l2_get_link_freq(csi2rx->source_subdev->ctrl_handler, + fmt->bpp, 2 * csi2rx->num_lanes); + if (link_freq < 0) + return link_freq; + + ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq, + csi2rx->num_lanes, cfg); + if (ret) + return ret; + ret = phy_power_on(csi2rx->dphy); if (ret) return ret;