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: 2806 bytes --]

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?

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

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

Thierry

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