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:
signature.asc
Description: PGP signature
