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]

 



On 03/05/18 17:13, Maxime Ripard wrote:
> 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?

You can't ignore errors from start(), those should always be returned to the
caller. But for stop() I'd just log the error and make csi2rx/tx_stop void functions.

Regards,

	Hans

> 
> Maxime
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[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