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
