Re: [PATCH 00/49] DRM driver for Hikey 970

[Sam Ravnborg]

Hi Mauro.

On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 19 Aug 2020 19:35:58 +0200
> Sam Ravnborg <sam@xxxxxxxxxxxx> escreveu:
> 
> I'm already handling the other comments from your review (I'll send a
> more complete comment about them after finishing),
If you get back only on things you do not understand or do not agree on
that would be fine. The rest should be visible in the changelog on the
updated patch - no need to do extra work here.

> but I have a doubt what you meant about this:
> 
> > +static int kirin_drm_bind(struct device *dev)
> > > +{
> > > +	struct drm_driver *driver = &kirin_drm_driver;
> > > +	struct drm_device *drm_dev;
> > > +	struct kirin_drm_private *priv;
> > > +	int ret;
> > > +
> > > +	drm_dev = drm_dev_alloc(driver, dev);
> > > +	if (!drm_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = kirin_drm_kms_init(drm_dev);
> > > +	if (ret)
> > > +		goto err_drm_dev_unref;
> > > +
> > > +	ret = drm_dev_register(drm_dev, 0);  
> > There is better ways to do this. See drm_drv.c for the code example.
> 
> Not sure if I understood your comment here. The drm_drv.c example also calls 
> drm_dev_register().

This is indeed not obvious from my comments but what I wnated to say is
that the driver should embed drm_device in some struct,
maybe in "struct kirin_drm_private".

This should also be part of the referenced example.

I hope this clarifies it.

	Sam

> 
> The only difference is that it calls it inside driver_probe(), while
> on this driver, it uses:
> 
> 	static const struct component_master_ops kirin_drm_ops = {
> 		.bind = kirin_drm_bind,
> 		.unbind = kirin_drm_unbind,
> 	};
> 
> 	static int kirin_drm_platform_probe(struct platform_device *pdev)
> 	{
> ...
> 		drm_of_component_match_add(dev, &match, compare_of, remote);
> ...
> 	return component_master_add_with_match(dev, &kirin_drm_ops, match);
> }
> 
> Are you meaning that I should get rid of this component API and call
> kirin_drm_bind() from kirin_drm_platform_probe()?
> 
> Thanks,
> Mauro