Re: [PATCH] rcar-csi2: restart CSI-2 link if error is detected

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

Thanks for your feedback.

On 2019-03-11 11:53:01 +0100, Hans Verkuil wrote:
> On 2/18/19 11:15 AM, Niklas Söderlund wrote:
> > Restart the CSI-2 link if the CSI-2 receiver detects an error during
> > reception. The driver did nothing when a link error happened and the
> > data flow simply stopped without the user knowing why.
> > 
> > Change the driver to try and recover from errors by restarting the link
> > and informing the user that something is not right. For obvious reasons
> > it's not possible to recover from all errors (video source disconnected
> > for example) but in such cases the user is at least informed of the
> > error and the same behavior of the stopped data flow is retained.
> 
> What you really would like to have is that when a CSI error is detected,
> the CSI driver can ask upstream whether or not a disconnect has taken place.
> 
> If that was the case, then there is no point in restarting the CSI.
> 
> While a disconnect is very uncommon for a sensor, it is of course perfectly
> normal if an HDMI-to-CSI bridge was connected to the CSI port.
> 
> Unfortunately, we don't have such functionality, but perhaps this is something
> to think about?

I think your idea sounds good and that such a functionality could be 
useful. I have a  feeling such a functionality could be related to 
notifications?

> 
> This does mean, however, that I don't like the dev_err since it doesn't have
> to be an error. I would suggest replacing the first dev_err by dev_info and
> the second by dev_warn.

With the background you provides I agree that they should not be 
dev_err. I will update as you suggest for the next version, thanks.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 52 ++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index f90b380478775015..0506fe4106d5c012 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -84,6 +84,9 @@ struct rcar_csi2;
> >  
> >  /* Interrupt Enable */
> >  #define INTEN_REG			0x30
> > +#define INTEN_INT_AFIFO_OF		BIT(27)
> > +#define INTEN_INT_ERRSOTHS		BIT(4)
> > +#define INTEN_INT_ERRSOTSYNCHS		BIT(3)
> >  
> >  /* Interrupt Source Mask */
> >  #define INTCLOSE_REG			0x34
> > @@ -540,6 +543,10 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	if (mbps < 0)
> >  		return mbps;
> >  
> > +	/* Enable interrupts. */
> > +	rcsi2_write(priv, INTEN_REG, INTEN_INT_AFIFO_OF | INTEN_INT_ERRSOTHS
> > +		    | INTEN_INT_ERRSOTSYNCHS);
> > +
> >  	/* Init */
> >  	rcsi2_write(priv, TREF_REG, TREF_TREF);
> >  	rcsi2_write(priv, PHTC_REG, 0);
> > @@ -702,6 +709,43 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> >  	.pad	= &rcar_csi2_pad_ops,
> >  };
> >  
> > +static irqreturn_t rcsi2_irq(int irq, void *data)
> > +{
> > +	struct rcar_csi2 *priv = data;
> > +	u32 status, err_status;
> > +
> > +	status = rcsi2_read(priv, INTSTATE_REG);
> > +	err_status = rcsi2_read(priv, INTERRSTATE_REG);
> > +
> > +	if (!status)
> > +		return IRQ_HANDLED;
> > +
> > +	rcsi2_write(priv, INTSTATE_REG, status);
> > +
> > +	if (!err_status)
> > +		return IRQ_HANDLED;
> > +
> > +	rcsi2_write(priv, INTERRSTATE_REG, err_status);
> > +
> > +	dev_err(priv->dev, "Transfer error, restarting CSI-2 receiver\n");
> > +
> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t rcsi2_irq_thread(int irq, void *data)
> > +{
> > +	struct rcar_csi2 *priv = data;
> > +
> > +	mutex_lock(&priv->lock);
> > +	rcsi2_stop(priv);
> > +	usleep_range(1000, 2000);
> > +	if (rcsi2_start(priv))
> > +		dev_err(priv->dev, "Failed to restart CSI-2 receiver\n");
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Async handling and registration of subdevices and links.
> >   */
> > @@ -982,7 +1026,7 @@ static int rcsi2_probe_resources(struct rcar_csi2 *priv,
> >  				 struct platform_device *pdev)
> >  {
> >  	struct resource *res;
> > -	int irq;
> > +	int irq, ret;
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	priv->base = devm_ioremap_resource(&pdev->dev, res);
> > @@ -993,6 +1037,12 @@ static int rcsi2_probe_resources(struct rcar_csi2 *priv,
> >  	if (irq < 0)
> >  		return irq;
> >  
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, rcsi2_irq,
> > +					rcsi2_irq_thread, IRQF_SHARED,
> > +					KBUILD_MODNAME, priv);
> > +	if (ret)
> > +		return ret;
> > +
> >  	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> >  	if (IS_ERR(priv->rstc))
> >  		return PTR_ERR(priv->rstc);
> > 
> 

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux