linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rcar-csi2: Use standby mode instead of resetting
@ 2019-02-16 22:56 Niklas Söderlund
  2019-02-17 19:41 ` Jacopo Mondi
  2019-02-18  9:07 ` Geert Uytterhoeven
  0 siblings, 2 replies; 5+ messages in thread
From: Niklas Söderlund @ 2019-02-16 22:56 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

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+renesas@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) {
+		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);
+	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);
+
 	return 0;
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcar-csi2: Use standby mode instead of resetting
  2019-02-16 22:56 [PATCH] rcar-csi2: Use standby mode instead of resetting Niklas Söderlund
@ 2019-02-17 19:41 ` Jacopo Mondi
  2019-03-07  0:03   ` Niklas Söderlund
  2019-02-18  9:07 ` Geert Uytterhoeven
  1 sibling, 1 reply; 5+ messages in thread
From: Jacopo Mondi @ 2019-02-17 19:41 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc

[-- Attachment #1: Type: text/plain, Size: 5542 bytes --]

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+renesas@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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcar-csi2: Use standby mode instead of resetting
  2019-02-16 22:56 [PATCH] rcar-csi2: Use standby mode instead of resetting Niklas Söderlund
  2019-02-17 19:41 ` Jacopo Mondi
@ 2019-02-18  9:07 ` Geert Uytterhoeven
  2019-03-07  0:12   ` Niklas Söderlund
  1 sibling, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2019-02-18  9:07 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Laurent Pinchart, Linux Media Mailing List, Linux-Renesas

Hi Niklas,

On Sun, Feb 17, 2019 at 8:54 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> 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+renesas@ragnatech.se>

Thanks for your patch!

> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c

> @@ -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);
> +
>         return 0;

Does the driver Kconfig option need "select RESET_CONTROLLER"?
If the option is not enabled, devm_reset_control_get() will return -ENOTSUPP.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcar-csi2: Use standby mode instead of resetting
  2019-02-17 19:41 ` Jacopo Mondi
@ 2019-03-07  0:03   ` Niklas Söderlund
  0 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2019-03-07  0:03 UTC (permalink / raw)
  To: Jacopo Mondi; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc

Hi Jacopo,

Thanks for your feedback.

On 2019-02-17 20:41:24 +0100, Jacopo Mondi wrote:
> 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+renesas@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" ?

I agree this was a bad design, I will split this function in two.

    rcsi2_enter_standby()
    rcsi2_exit_standby()

> 
> > +		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.

Do not agree, I like this :-)

> 
> > +	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?

Nice catch! I will add a patch to this series listing it as mandatory.  
It's a good thing the resets property always have been part of the dts 
sources so it will not create any regressions.

> 
> Thanks
>    j
> 
> >  	return 0;
> >  }
> >
> > --
> > 2.20.1
> >



-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rcar-csi2: Use standby mode instead of resetting
  2019-02-18  9:07 ` Geert Uytterhoeven
@ 2019-03-07  0:12   ` Niklas Söderlund
  0 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2019-03-07  0:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Laurent Pinchart, Linux Media Mailing List, Linux-Renesas

Hi Geert,

Thanks for your feedback.

On 2019-02-18 10:07:04 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Sun, Feb 17, 2019 at 8:54 AM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> 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+renesas@ragnatech.se>
> 
> Thanks for your patch!
> 
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> 
> > @@ -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);
> > +
> >         return 0;
> 
> Does the driver Kconfig option need "select RESET_CONTROLLER"?
> If the option is not enabled, devm_reset_control_get() will return -ENOTSUPP.

Yes it does, thanks for pointing this out, will add this in v2.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-07  0:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-16 22:56 [PATCH] rcar-csi2: Use standby mode instead of resetting Niklas Söderlund
2019-02-17 19:41 ` Jacopo Mondi
2019-03-07  0:03   ` Niklas Söderlund
2019-02-18  9:07 ` Geert Uytterhoeven
2019-03-07  0:12   ` Niklas Söderlund

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).