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