Hi Laurent,

I've removed the comments I'll take into account

On Mon, Aug 07, 2017 at 11:42:01PM +0300, Laurent Pinchart wrote:
> > +	csi2rx_reset(csi2rx);
> > +
> > +	// TODO: modify the mapping of the DPHY lanes?
> 
> The mapping should be read from DT and applied here.

As far as I understand it, this is a mapping between logical and
physical lanes before forwarding it to the D-PHY. Would data-lanes
apply for such a case?

> > +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> > +
> > +	v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> > +			 enable);
> 
> Shouldn't you handle errors here ?

Yes.

> I'm not familiar with this IP core, but most D-PHYs need to synchronize to the 
> input and must be started before the source.

We don't have any kind of D-PHY support at the moment, but I guess we
should put it there once we do.

> > +		return PTR_ERR(csi2rx->base);
> > +	}
> > +
> > +	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> 
> Shouldn't you enable the register access clock(s) before reading the
> register ?

Argh, you're right.

> 
> > +	csi2rx->max_lanes = (reg & 7) + 1;
> 
> Up to 8 lanes ? Shouldn't this be (reg & 3) + 1 ?

The bit field is indeed documented 3 bits wide, hence the 7.

> > +	csi2rx->max_streams = ((reg >> 4) & 7);
> 
> Should you validate the value as you use it as an array access index ?

I should, yes.

> > +	csi2rx->cdns_dphy = reg & BIT(3);
> > +
> > +	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> > +	if (IS_ERR(csi2rx->sys_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get sys clock\n");
> 
> If you wrote this as
> 
> 		dev_err(&pdev->dev, "Couldn't get %s clock\n", "sys");
> 
> and the next messages in a similar fashion, most of the message wouldn't be 
> duplicated, resulting in smaller code size.

I'm usually not a big fan of this, since one side effect is also that
you can't grep / search it anymore, and I'm not sure it's worth the
hassle for a couple dozen bytes.

> 
> > +		return PTR_ERR(csi2rx->sys_clk);
> > +	}
> > +
> > +	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> > +	if (IS_ERR(csi2rx->p_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get P clock\n");
> > +		return PTR_ERR(csi2rx->p_clk);
> > +	}
> > +
> > +	csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> > +	if (IS_ERR(csi2rx->p_free_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> > +		return PTR_ERR(csi2rx->p_free_clk);
> > +	}
> > +
> > +	for (i = 0; i < csi2rx->max_streams; i++) {
> > +		char clk_name[16];
> 
> Isn't 13 enough ?

It is, if you have an int short enough.

> > +	csi2rx->sensor_node = remote;
> > +	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> > +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +
> > +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> 
> Could you store this in the csi2rx_priv structure to avoid a dynamic 
> allocation ?

It's used only in this function, so it seems a bit overkill to do
that. But I guess I could just allocate it on the stack.

> > +
> > +	csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
> 
> Please don't use devm_kzalloc() to allocate structures that can be accessed 
> from userspace (in this case because it embeds the subdev structure, 
> accessible at least through the subdevs ioctls). This is incompatible with 
> proper unplug handling. There are many other issues that we will need to solve 
> in the V4L2 core to handling unplugging properly, but let's not add a new one.

What's wrong with kzalloc in such a case? As far as I know, the
userspace can access such a memory, as long as you use copy_to_user,
right? Or is it because of the life-cycle of the allocation that would
be gone while the userspace might not be yet? I'm not sure what would
be a proper fix for it though.

Thanks for your review!
Maxime

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