Re: [PATCH v11 2/4] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

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

 



Hi!

Thanks for your review,

On Thu, May 03, 2018 at 12:54:57PM +0200, Hans Verkuil wrote:
> > +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> > +{
> > +	unsigned int i;
> > +
> > +	clk_prepare_enable(csi2rx->p_clk);
> > +	clk_disable_unprepare(csi2rx->sys_clk);
> > +
> > +	for (i = 0; i < csi2rx->max_streams; i++) {
> > +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +
> > +		clk_disable_unprepare(csi2rx->pixel_clk[i]);
> > +	}
> > +
> > +	clk_disable_unprepare(csi2rx->p_clk);
> > +
> > +	return v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false);
> > +}
> > +
> > +static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > +{
> > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +	int ret = 0;
> > +
> > +	mutex_lock(&csi2rx->lock);
> > +
> > +	if (enable) {
> > +		/*
> > +		 * If we're not the first users, there's no need to
> > +		 * enable the whole controller.
> > +		 */
> > +		if (!csi2rx->count) {
> > +			ret = csi2rx_start(csi2rx);
> > +			if (ret)
> > +				goto out;
> > +		}
> > +
> > +		csi2rx->count++;
> > +	} else {
> > +		csi2rx->count--;
> > +
> > +		/*
> > +		 * Let the last user turn off the lights.
> > +		 */
> > +		if (!csi2rx->count) {
> > +			ret = csi2rx_stop(csi2rx);
> > +			if (ret)
> > +				goto out;
> 
> Here the error from csi2rx_stop is propagated to the caller, but in the TX
> driver it is ignored. Is there a reason for the difference?

Even though that wasn't really intentional, TX only does a writel in
its stop (which cannot fail), while RX will need to communicate with
its subdev, and that can fail.

> In general I see little value in propagating errors when releasing/stopping
> something, since there is usually very little you can do to handle the error.
> It really shouldn't fail.

So do you want me to ignore the values in the s_stream function and
log the error, or should I just make the start / stop function return
void?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux