Hi Niklas,
   ah, ups, this was maybe the patch the other one I just reviewed was
   based on... sorry, I missed this one :)

On Sat, Feb 16, 2019 at 11:56:38PM +0100, Niklas Söderlund wrote:
> Later versions of the datasheet updates the reset procedure to more
> closely resemble the standby mode. Update the driver to enter and exit
> the standby mode instead of resetting the hardware before and after
> streaming is started and stopped.
>
> While at it break out the full start and stop procedures from
> rcsi2_s_stream() into the existing helper functions.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 69 +++++++++++++--------
>  1 file changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f64528d2be3c95dd..f3099f3e536d808a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -14,6 +14,7 @@
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/sys_soc.h>
>
>  #include <media/v4l2-ctrls.h>
> @@ -350,6 +351,7 @@ struct rcar_csi2 {
>       struct device *dev;
>       void __iomem *base;
>       const struct rcar_csi2_info *info;
> +     struct reset_control *rstc;
>
>       struct v4l2_subdev subdev;
>       struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> @@ -387,11 +389,19 @@ static void rcsi2_write(struct rcar_csi2 *priv, 
> unsigned int reg, u32 data)
>       iowrite32(data, priv->base + reg);
>  }
>
> -static void rcsi2_reset(struct rcar_csi2 *priv)
> +static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
>  {
> -     rcsi2_write(priv, SRST_REG, SRST_SRST);
> +     if (!on) {

minor thing: if (!on) { "wakeup"; } is confusing. What if you call the
variable "standby" or just "off" ?

> +             pm_runtime_get_sync(priv->dev);
> +             reset_control_deassert(priv->rstc);
> +             return;
> +     }
> +
> +     rcsi2_write(priv, PHYCNT_REG, 0);
> +     rcsi2_write(priv, PHTC_REG, PHTC_TESTCLR);
> +     reset_control_assert(priv->rstc);
>       usleep_range(100, 150);
> -     rcsi2_write(priv, SRST_REG, 0);
> +     pm_runtime_put(priv->dev);
>  }
>
>  static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> @@ -462,7 +472,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, 
> unsigned int bpp)
>       return mbps;
>  }
>
> -static int rcsi2_start(struct rcar_csi2 *priv)
> +static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>       const struct rcar_csi2_format *format;
>       u32 phycnt, vcdt = 0, vcdt2 = 0;
> @@ -506,7 +516,6 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>
>       /* Init */
>       rcsi2_write(priv, TREF_REG, TREF_TREF);
> -     rcsi2_reset(priv);
>       rcsi2_write(priv, PHTC_REG, 0);
>
>       /* Configure */
> @@ -564,19 +573,36 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>       return 0;
>  }
>
> +static int rcsi2_start(struct rcar_csi2 *priv)
> +{
> +     int ret;
> +
> +     rcsi2_standby_mode(priv, 0);
> +
> +     ret = rcsi2_start_receiver(priv);
> +     if (ret) {
> +             rcsi2_standby_mode(priv, 1);
> +             return ret;
> +     }
> +
> +     ret = v4l2_subdev_call(priv->remote, video, s_stream, 1);

minor thing as well, but I feel this one was better where it was, so
that "rcsi2_start()" only handles the hardware, while s_stream handles
the pipeline. But then _start() and _stop() becomes very short... so
yeah, feel free to keep it the way it is.

> +     if (ret) {
> +             rcsi2_standby_mode(priv, 1);
> +             return ret;
> +     }
> +
> +     return 0;
> +}
> +
>  static void rcsi2_stop(struct rcar_csi2 *priv)
>  {
> -     rcsi2_write(priv, PHYCNT_REG, 0);
> -
> -     rcsi2_reset(priv);
> -
> -     rcsi2_write(priv, PHTC_REG, PHTC_TESTCLR);
> +     v4l2_subdev_call(priv->remote, video, s_stream, 0);
> +     rcsi2_standby_mode(priv, 1);
>  }
>
>  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>       struct rcar_csi2 *priv = sd_to_csi2(sd);
> -     struct v4l2_subdev *nextsd;
>       int ret = 0;
>
>       mutex_lock(&priv->lock);
> @@ -586,27 +612,12 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int 
> enable)
>               goto out;
>       }
>
> -     nextsd = priv->remote;
> -
>       if (enable && priv->stream_count == 0) {
> -             pm_runtime_get_sync(priv->dev);
> -
>               ret = rcsi2_start(priv);
> -             if (ret) {
> -                     pm_runtime_put(priv->dev);
> +             if (ret)
>                       goto out;
> -             }
> -
> -             ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> -             if (ret) {
> -                     rcsi2_stop(priv);
> -                     pm_runtime_put(priv->dev);
> -                     goto out;
> -             }
>       } else if (!enable && priv->stream_count == 1) {
>               rcsi2_stop(priv);
> -             v4l2_subdev_call(nextsd, video, s_stream, 0);
> -             pm_runtime_put(priv->dev);
>       }
>
>       priv->stream_count += enable ? 1 : -1;
> @@ -936,6 +947,10 @@ static int rcsi2_probe_resources(struct rcar_csi2 *priv,
>       if (irq < 0)
>               return irq;
>
> +     priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +     if (IS_ERR(priv->rstc))
> +             return PTR_ERR(priv->rstc);
> +

I don't see 'resets' listed as a mandatory property of the rcar-csi2
bindings, shouldn't you fallback to software reset if not 'reset'
is specified? True that all mainline users have a reset property specified,
so you could also add 'resets' among the mandatory properties, could
that break out of tree implementations in your opinion?

Thanks
   j

>       return 0;
>  }
>
> --
> 2.20.1
>

Attachment: signature.asc
Description: PGP signature

Reply via email to