Hi Jose,
Thank you for addressing my review comments. Couple more suggestions below.
On 06/20/2017 07:26 PM, Jose Abreu wrote:
> This adds support for the Synopsys Designware HDMI RX PHY e405. This
> phy receives and decodes HDMI video that is delivered to a controller.
> +static int dw_phy_config(struct dw_phy_dev *dw_dev, unsigned char color_depth,
> + bool hdmi2, bool scrambling)
> +{
> + phy_reset(dw_dev, 1);
> + phy_pddq(dw_dev, 1);
> + phy_svsmode(dw_dev, 1);
> +
> + phy_zcal_reset(dw_dev);
> + do {
> + udelay(1000);
Could be mdelay(1) or better e.g. usleep_range(1000, 1100);
> + zcal_done = phy_zcal_done(dw_dev);
> + } while (!zcal_done && timeout--);
> +
> + if (!zcal_done) {
> + dev_err(dw_dev->dev, "Zcal calibration failed\n");
> + return -ETIMEDOUT;
> + }
> + return 0;
> +}
> +static int dw_phy_probe(struct platform_device *pdev)
> +{
> + struct dw_phy_pdata *pdata = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct dw_phy_dev *dw_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + dev_dbg(dev, "probe start\n");
> +
> + /* Resource allocation */
This comment is not needed.
> + dw_dev = devm_kzalloc(dev, sizeof(*dw_dev), GFP_KERNEL);
> + if (!dw_dev)
> + return -ENOMEM;
> +
> + /* Resource initialization */
Ditto.
> + if (!pdata) {
> + dev_err(dev, "no platform data suplied\n");
> + return -EINVAL;
> + }
> +
> + dw_dev->dev = dev;
> + dw_dev->config = pdata;
> + dw_dev->version = 405;
How about storing the version number in the dw_hdmi_phy_e405_id[] table
and retrieving it here with of_device_get_match_data() ?
> + mutex_init(&dw_dev->lock);
> +
> + /* Get config clock value */
The comment is not needed, it's clear from the code we are getting the clock
and its rate.
> + dw_dev->clk = devm_clk_get(dev, "cfg-clk");
As Rob suggested, it would be good to change name of the clock to just "cfg".
> + if (IS_ERR(dw_dev->clk)) {
> + dev_err(dev, "failed to get cfg-clk\n");
> + return PTR_ERR(dw_dev->clk);
> + }
> +
> + ret = clk_prepare_enable(dw_dev->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable cfg-clk\n");
> + return ret;
> + }
> +
> + dw_dev->cfg_clk = clk_get_rate(dw_dev->clk) / 1000000;
1000000U ?
> + if (!dw_dev->cfg_clk) {
> + dev_err(dev, "invalid cfg-clk frequency\n");> + ret = -EINVAL;
> + goto err_clk;
> + }
> +
> + /* V4L2 initialization */
> + sd = &dw_dev->sd;
> + v4l2_subdev_init(sd, &dw_phy_sd_ops);
> + strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
> + sd->dev = dev;
> +
> + /* Force phy disabling */
> + dw_dev->phy_enabled = true;
> + dw_phy_disable(dw_dev);
> +
> + /* Register subdev */
> + ret = v4l2_async_register_subdev(sd);
> + if (ret) {
> + dev_err(dev, "failed to register subdev\n");
> + goto err_clk;
> + }
> +
> + /* All done */
Superfluous comment.
> + dev_set_drvdata(dev, sd);
> + dev_info(dev, "driver probed (cfg-clk=%d)\n", dw_dev->cfg_clk);
This should be at dev_dbg() level.
> + return 0;
> +
> +err_clk:
> + clk_disable_unprepare(dw_dev->clk);
> + return ret;
> +}
> +
> +static int dw_phy_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
The assignment here could be dropped and &pdev->dev used directly below.
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct dw_phy_dev *dw_dev = to_dw_dev(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + clk_disable_unprepare(dw_dev->clk);
> + dev_info(dev, "driver removed\n");
This should be at dev_dbg() level or dropped entirely.
> + return 0;
> +}
> +
> +static const struct of_device_id dw_hdmi_phy_e405_id[] = {
> + { .compatible = "snps,dw-hdmi-phy-e405" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, dw_hdmi_phy_e405_id);
--
Regards,
Sylwester
