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
>
