Re: [PATCH v1 7/7] drm: add Atmel LCDC display controller support

[Sam Ravnborg <sam@ravnborg.org>]

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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel