Re: [PATCH v3 4/4] media: tc358746: add Toshiba TC358746 Parallel to CSI-2 bridge driver

From: Marco Felsch <m.felsch@pengutronix.de>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: mchehab@kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, kishon@ti.com,
	vkoul@kernel.org, laurent.pinchart@ideasonboard.com,
	jacopo@jmondi.org, hverkuil@xs4all.nl,
	linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-media@vger.kernel.org,
	kernel@pengutronix.de
Subject: Re: [PATCH v3 4/4] media: tc358746: add Toshiba TC358746 Parallel to CSI-2 bridge driver
Date: Fri, 23 Sep 2022 16:38:11 +0200	[thread overview]
Message-ID: <20220923143811.d5gk52vh2koczyhk@pengutronix.de> (raw)
In-Reply-To: <Yy2rNit+rRGkcFdL@paasikivi.fi.intel.com>

Hi Sakari,

On 22-09-23, Sakari Ailus wrote:
> Hi Marco,
> 
> Thanks for the update. A few small things below.
> 
> On Thu, Sep 22, 2022 at 03:48:43PM +0200, Marco Felsch wrote:
> ...
> > +static int tc358746_apply_misc_config(struct tc358746 *tc358746)
> > +{
> > +	const struct v4l2_mbus_framefmt *mbusfmt;
> > +	struct v4l2_subdev *sd = &tc358746->sd;
> > +	struct v4l2_subdev_state *sink_state;
> > +	const struct tc358746_format *fmt;
> > +	struct device *dev = sd->dev;
> > +	u32 val;
> > +	int err;
> > +
> > +	sink_state = v4l2_subdev_lock_and_get_active_state(sd);
> > +	mbusfmt = v4l2_subdev_get_pad_format(sd, sink_state, TC358746_SINK);
> > +	v4l2_subdev_unlock_state(sink_state);
> > +
> > +	fmt = tc358746_get_format_by_code(TC358746_SINK, mbusfmt->code);
> 
> You can no longer access mbusfmt here as you've already unlocked subdev
> state.

You're right, Laurent also mentioned this but this function gets called
during .s_stream(on) so the format shouldn't be changed that late.
However since you're the 2nd with the thought, I will fix it in the v4.

> > +
> > +	/* Self defined CSI user data type id's are not supported yet */
> > +	val = PDFMT(fmt->pdformat);
> > +	dev_dbg(dev, "DATAFMT: 0x%x\n", val);
> > +	err = tc358746_write(tc358746, DATAFMT_REG, val);
> > +	if (err)
> > +		return err;
> > +
> > +	val = PDATAF(fmt->pdataf);
> > +	dev_dbg(dev, "CONFCTL[PDATAF]: 0x%x\n", fmt->pdataf);
> > +	err = tc358746_update_bits(tc358746, CONFCTL_REG, PDATAF_MASK, val);
> > +	if (err)
> > +		return err;
> > +
> > +	val = tc358746->vb_size / 32;
> > +	dev_dbg(dev, "FIFOCTL: %u (0x%x)\n", val, val);
> > +	err = tc358746_write(tc358746, FIFOCTL_REG, val);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Total number of bytes for each line/width */
> > +	val = mbusfmt->width * fmt->bpp / 8;
> > +	dev_dbg(dev, "WORDCNT: %u (0x%x)\n", val, val);
> > +	return tc358746_write(tc358746, WORDCNT_REG, val);
> > +}
> 
> ...
> 
> > +static int tc358746_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct tc358746 *tc358746 = to_tc358746(sd);
> > +	struct v4l2_subdev *src;
> > +	int err;
> > +
> > +	dev_dbg(sd->dev, "%sable\n", enable ? "en" : "dis");
> > +
> > +	src = tc358746_get_remote_sd(&tc358746->pads[TC358746_SINK]);
> > +	if (!src)
> > +		return -EPIPE;
> > +
> > +	if (enable) {
> > +		err = pm_runtime_resume_and_get(sd->dev);
> > +		if (err)
> > +			return err;
> > +
> > +		err = tc358746_apply_dphy_config(tc358746);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		err = tc358746_apply_misc_config(tc358746);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		err = tc358746_enable_csi_lanes(tc358746, 1);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		err = tc358746_enable_csi_module(tc358746, 1);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		err = tc358746_enable_parallel_port(tc358746, 1);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		err = v4l2_subdev_call(src, video, s_stream, 1);
> > +		if (err)
> > +			goto err_out;
> > +
> > +		return err;
> 
> This can be return 0.

Sure.

Regards,
  Marco

> 
> > +
> > +err_out:
> > +		pm_runtime_mark_last_busy(sd->dev);
> > +		pm_runtime_put_sync_autosuspend(sd->dev);
> > +
> > +		return err;
> > +	}
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> 