Hi Laurent, On Tue, Jan 02, 2018 at 05:50:38PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thursday, 28 December 2017 16:01:21 EET Jacopo Mondi wrote: > > - priv->clk = v4l2_clk_get(&client->dev, "mclk"); > > - if (IS_ERR(priv->clk)) > > + priv->clk = clk_get(&client->dev, "mclk"); > > The clock signal is called XTI (see page 60 of http://www.tecworth.com/ > administrator/upload/200956419240864.pdf). You should add a clock alias as for > the ov7725. I don't think Migo-R board code should provide any alias for XTI clock, as the clock is generated by an on-board oscillator and not from the SoC. Also, changing the name in the driver seems safe, as `git grep tw9910 arch/` returns that only mach-ecovec uses TW9910 (Migo-R apart) and it seems like also in that case the clock comes from an oscillator and it is not provided by the SoC. > > > + if (PTR_ERR(priv->clk) == -ENOENT) { > > + priv->clk = NULL; > > + } else if (IS_ERR(priv->clk)) { > > + dev_err(&client->dev, "Unable to get mclk clock\n"); > > return PTR_ERR(priv->clk); > > + } > > + > > + priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(priv->pdn_gpio)) { > > + dev_info(&client->dev, "Unable to get GPIO \"pdn\""); > > + ret = PTR_ERR(priv->pdn_gpio); > > + goto error_clk_put; > > + } > > > > ret = tw9910_video_probe(client); > > if (ret < 0) > > - v4l2_clk_put(priv->clk); > > + goto error_gpio_put; > > + > > + ret = v4l2_async_register_subdev(&priv->subdev); > > + if (ret) > > + goto error_gpio_put; > > + > > + return ret; > > + > > +error_gpio_put: > > + if (priv->pdn_gpio) > > + gpiod_put(priv->pdn_gpio); > > +error_clk_put: > > + clk_put(priv->clk); > > > > return ret; > > } > > [snip] > > With these small issues fixed, and the comments to ov7725 that apply to tw9910 > addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> Thanks j > > -- > Regards, > > Laurent Pinchart >
