Re: [PATCH 2/2] v4l: cadence: Add Cadence MIPI-CSI2 TX driver

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

Hi Benoit,

On Fri, Sep 29, 2017 at 06:21:25PM +0000, Benoit Parrot wrote:
> > +struct csi2tx_priv {
> > +	struct device			*dev;
> > +	atomic_t			count;
> > +
> > +	void __iomem			*base;
> > +
> > +	struct clk			*esc_clk;
> > +	struct clk			*p_clk;
> > +	struct clk			*pixel_clk[CSI2TX_STREAMS_MAX];
> > +
> > +	struct v4l2_subdev		subdev;
> > +	struct media_pad		pads[CSI2TX_PAD_MAX];
> > +	struct v4l2_mbus_framefmt	pad_fmts[CSI2TX_PAD_MAX];
> > +
> > +	bool				has_internal_dphy;
> 
> I assume dphy support is for a subsequent revision?

Yes, the situation is similar to the CSI2-RX driver.

> > +		/*
> > +		 * We use the stream ID there, but it's wrong.
> > +		 *
> > +		 * A stream could very well send a data type that is
> > +		 * not equal to its stream ID. We need to find a
> > +		 * proper way to address it.
> > +		 */
> 
> I don't quite get the above comment, from the code below it looks like
> you are using the current fmt as a source to provide the correct DT.
> Am I missing soemthing?

Yes, so far the datatype is inferred from the format set. Is there
anything wrong with that?

> 
> > +		writel(CSI2TX_DT_CFG_DT(fmt->dt),
> > +		       csi2tx->base + CSI2TX_DT_CFG_REG(stream));
> > +
> > +		writel(CSI2TX_DT_FORMAT_BYTES_PER_LINE(mfmt->width * fmt->bpp) |
> > +		       CSI2TX_DT_FORMAT_MAX_LINE_NUM(mfmt->height + 1),
> > +		       csi2tx->base + CSI2TX_DT_FORMAT_REG(stream));
> > +
> > +		/*
> > +		 * TODO: This needs to be calculated based on the
> > +		 * clock rate.
> > +		 */
> > +		writel(CSI2TX_STREAM_IF_CFG_FILL_LEVEL(4),
> > +		       csi2tx->base + CSI2TX_STREAM_IF_CFG_REG(stream));
> > +	}
> > +
> > +	/* Disable the configuration mode */
> > +	writel(0, csi2tx->base + CSI2TX_CONFIG_REG);
> > +
> > +	return 0;
> > +}
> > +
> > +static int csi2tx_stop(struct csi2tx_priv *csi2tx)
> > +{
> > +	/*
> > +	 * Let the last user turn off the lights
> > +	 */
> > +	if (!atomic_dec_and_test(&csi2tx->count))
> > +		return 0;
> > +
> > +	/* FIXME: Disable the IP here */
> > +
> > +	return 0;
> > +}
> 
> At this point both _start() and _stop() only return 0.
> Are there any cases where any of these might fail to "start" or "stop"?

None that I know of.

There might be some errors with the video stream itself, but that
can be detected after the block has been enabled.

> > +	csi2tx->lanes = csi2tx_get_num_lanes(&pdev->dev);
> > +	if (csi2tx->lanes < 0) {
> > +		dev_err(&pdev->dev, "Invalid number of lanes, bailing out.\n");
> > +		ret = csi2tx->lanes;
> > +		goto err_free_priv;
> > +	}
> 
> csi2tx->lanes is unsigned so it will never be negative.

Ah, right, I'll change that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]