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

[Svyatoslav Ryhel]

пт, 6 лист. 2026 р. о 17:19 Thierry Reding <thierry.reding@xxxxxxxxxx> пише:
>
> On Fri, Nov 06, 2026 at 03:59:05PM +0200, Svyatoslav Ryhel wrote:
> > пт, 6 лист. 2026 р. о 15:14 Thierry Reding <thierry.reding@xxxxxxxxxx> пише:
> > >
> > > 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.
>

May anyone from DRM maintainers clarify this please. I would like to
know if I need to change here anything. Thank you.

> > > > +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