Re: [PATCH v8 08/16] media: cadence: csi2rx: Configure DPHY using link freq

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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