Re: [PATCH v6 01/10] drm: Add MIPI DBI bus support for MIPI DBI Type A and B

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4710 bytes --]

On Fri, Nov 06, 2026 at 03:59:05PM +0200, Svyatoslav Ryhel wrote:
> пт, 6 лист. 2026 р. о 15:14 Thierry Reding <thierry.reding@kernel.org> пише:
> >
> > On Thu, Nov 05, 2026 at 04:05:01PM +0200, Svyatoslav Ryhel wrote:
> > [...]
> > > diff --git a/drivers/gpu/drm/drm_mipi_dbi_bus.c b/drivers/gpu/drm/drm_mipi_dbi_bus.c
> > [...]
> > > +/**
> > > + * devm_mipi_dbi_device_register() - create a managed MIPI DBI device
> > > + * @dev: device to tie the MIPI-DBI device lifetime to
> > > + * @host: DBI host to which this device is connected
> > > + * @np: device tree node associated with the panel MIPI DBI device
> > > + *
> > > + * This function is called by DBI host driver to create panel device.
> > > + * It automatically calls mipi_dbi_device_unregister() when @dev is
> > > + * unbound. It does not automatically free passed device node pointer,
> > > + * it must be put manually.
> > > + *
> > > + * Returns:
> > > + * A pointer to the newly created MIPI DBI device, or, a pointer encoded
> > > + * with an error.
> > > + */
> > > +struct mipi_dbi_device *
> > > +devm_mipi_dbi_device_register(struct device *dev, struct mipi_dbi_host *host,
> > > +                           struct device_node *np)
> > > +{
> > > +     struct mipi_dbi_device *dbidev;
> > > +     int ret;
> > > +
> > > +     if (!host || !host->dev || !host->dbi || !host->dbi->command ||
> > > +         !host->ops || !host->ops->attach || !host->ops->detach || !np)
> > > +             return ERR_PTR(-EINVAL);
> >
> > Do we want dev->of_node to serve as a fallback for !np?
> >
> 
> No we don't dev->of_node is hosts node, and np is panel node.

Fair enough. The parameter name is a bit generic, but I guess if you
read the documentation it becomes clearer what it means.

> > > +
> > > +     if (!of_device_is_available(np))
> > > +             return ERR_PTR(-ENODEV);
> > > +
> > > +     dbidev = kzalloc_obj(*dbidev);
> > > +     if (!dbidev)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     if (of_node_test_and_set_flag(np, OF_POPULATED)) {
> > > +             kfree(dbidev);
> > > +             return ERR_PTR(-EBUSY);
> > > +     }
> > > +
> > > +     dbidev->host = host;
> > > +     dbidev->dbi = host->dbi;
> > > +     device_initialize(&dbidev->dev);
> > > +     dbidev->dev.parent = host->dev;
> > > +     dbidev->dev.bus = &mipi_dbi_bus_type;
> > > +     dbidev->dev.release = mipi_dbi_device_release;
> > > +     device_set_node(&dbidev->dev, of_fwnode_handle(of_node_get(np)));
> > > +
> > > +     ret = dev_set_name(&dbidev->dev, "%s:%s", dev_name(host->dev), np->full_name);
> > > +     if (ret)
> > > +             goto put_device;
> > > +
> > > +     ret = device_add(&dbidev->dev);
> > > +     if (ret)
> > > +             goto put_device;
> > > +
> > > +     ret = devm_add_action_or_reset(dev, mipi_dbi_device_unregister,
> > > +                                    &dbidev->dev);
> > > +     if (ret)
> > > +             return ERR_PTR(ret);
> > > +
> > > +     return dbidev;
> > > +
> > > +put_device:
> > > +     of_node_clear_flag(np, OF_POPULATED);
> > > +     put_device(&dbidev->dev);
> > > +     return ERR_PTR(ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(devm_mipi_dbi_device_register);
> >
> > Don't we need to of_node_put() here on failure? Otherwise we're leaking
> > the reference that we obtained inside the device_set_node() argument.
> >
> 
> This is fully repeats device_register logic of drm_mipi_dsi.c so
> unless DSI framework has leak, this should be correct

Nobody said that drm_mipi_dsi.c is correct. I'm reviewing this patch and
it seems like it's wrong. If drm_mipi_dsi.c does the same thing it
probably is wrong, too.

> > > +static int mipi_dbi_drv_remove(struct device *dev)
> > > +{
> > > +     struct mipi_dbi_driver *drv = to_mipi_dbi_driver(dev->driver);
> > > +     struct mipi_dbi_device *dbidev = to_mipi_dbi_device(dev);
> > > +
> > > +     drv->remove(dbidev);
> > > +
> > > +     return 0;
> > > +}
> >
> > Might as well make drv->remove() return an error and propagate that
> > here.
> >
> 
> Have you even checked the code? remove() callback is and should be void.

I don't know what you're trying to say here. Yes I can see that the
remove callback returns void. My point is that device_driver.remove does
*not* return void and since this is merely a wrapper around it for DBI
it could also just return int for symmetry.

But there's also continuous work trying to convert all driver remove
callbacks to return void, so that's eventually going to get "fixed" here
anyway, so might as well leave it like this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]