Hi Boris.
Very usefull feedback!
I am working on v2, and have addressed your review items.
Just a few comments below to a few items.
The rest is processed/done.
I hope to post v2 in the week to come.
> > +static int lcdc_dc_load(struct drm_device *drm)
> > +{
> > + const struct of_device_id *match;
> > + struct platform_device *pdev;
> > + struct lcdc_dc *lcdc_dc;
> > + struct device *dev;
> > + int ret;
> > +
> > + dev = drm->dev;
> > + pdev = to_platform_device(dev);
> > +
> > + match = of_match_node(atmel_lcdc_of_match, dev->parent->of_node);
> > + if (!match) {
> > + DRM_DEV_ERROR(dev, "invalid compatible string (node=%s)",
> > + dev->parent->of_node->name);
> > + return -ENODEV;
> > + }
> > +
> > + if (!match->data) {
> > + DRM_DEV_ERROR(dev, "invalid lcdc_dc description\n");
> > + return -EINVAL;
> > + }
> > +
> > + lcdc_dc = devm_kzalloc(dev, sizeof(*lcdc_dc), GFP_KERNEL);
> > + if (!lcdc_dc) {
> > + DRM_DEV_ERROR(dev, "Failed to allocate lcdc_dc\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* reset of lcdc might sleep and require a preemptible task context */
> > + INIT_WORK(&lcdc_dc->reset_lcdc_work, reset_lcdc_work);
> > +
> > + platform_set_drvdata(pdev, drm);
> > + dev_set_drvdata(dev, lcdc_dc);
> > +
> > + lcdc_dc->mfd_lcdc = dev_get_drvdata(dev->parent);
> > + drm->dev_private = lcdc_dc;
> > +
> > + lcdc_dc->regmap = lcdc_dc->mfd_lcdc->regmap;
> > + lcdc_dc->desc = match->data;
> > + lcdc_dc->dev = dev;
> > +
> > + lcdc_dc->lcd_supply = devm_regulator_get(dev, "lcd");
> > + if (IS_ERR(lcdc_dc->lcd_supply)) {
> > + DRM_DEV_ERROR(dev, "Failed to get lcd-supply (%ld)\n",
> > + PTR_ERR(lcdc_dc->lcd_supply));
> > + lcdc_dc->lcd_supply = NULL;
> > + }
> > +
> > + lcdc_dc_start_clock(lcdc_dc);
>
> Hm, do you really need to call that here? I'd make it part of the
> runtime PM resume hook, and put a lcdc_dc_stop_clock() in the suspend
> hook.
As suggested I have introduced suspend/resume hooks and have included
start/stop clock calls.
But as the suspend/resume hooks depends on CONFIG_PM_SLEEP I
still need the explicit call in this function.
> > +
> > + pm_runtime_enable(dev);
> > +
> > + ret = drm_vblank_init(drm, 1);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "failed to initialize vblank (%d)\n",
> > + ret);
> > + goto err_pm_runtime_disable;
> > + }
> > +
> > + ret = lcdc_dc_modeset_init(lcdc_dc, drm);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "modeset_init failed (%d)", ret);
> > + goto err_pm_runtime_disable;
> > + }
> > +
> > + pm_runtime_get_sync(dev);
>
> This call will automatically call the runtime PM resume hook, so if you
> need the clk to be enabled before that point you should put it earlier.
>
> Also, you should call pm_runtime_get_sync() in the ->enable() path and
> pm_runtime_put() in the ->disable() path.
Good catch! Added.
Sam
