Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Noralf,
On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
A solution where we have the command sequences as part of the DT-overlay
is IMO a much better way to solve this:
- The users needs to deal only with a single file, so there is less that
  goes wrong
- The user need not reading special instructions how to handle a .bin
  file, if the overlay is present everything is fine
- The file that contains the command sequences can be properly annotated
  with comments
- The people that creates the command sequences has no need for a special
  script to create the file for a display - this is all readable ascii.
- Using a DT-overlay the parsing of the DT-overlay can be done by
  well-tested functions, no need to invent something new to parse the
  file


The idea with a common mipi DBI SPI driver is super, it is the detail
with the .bin file that I am against.

With the above said, a few comments to the current implementation below.
As we know it from you - a very well-written driver.

	Sam

> Acked-by: Maxime Ripard <maxime@xxxxxxxxxx>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
>  MAINTAINERS                           |   8 +
>  drivers/gpu/drm/tiny/Kconfig          |  13 +
>  drivers/gpu/drm/tiny/Makefile         |   1 +
>  drivers/gpu/drm/tiny/panel-mipi-dbi.c | 413 ++++++++++++++++++++++++++
>  4 files changed, 435 insertions(+)
>  create mode 100644 drivers/gpu/drm/tiny/panel-mipi-dbi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e6e892f99f0..3a0e57f23ad0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6107,6 +6107,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>  
> +DRM DRIVER FOR MIPI DBI compatible panels
> +M:	Noralf Trønnes <noralf@xxxxxxxxxxx>
> +S:	Maintained
> +W:	https://github.com/notro/panel-mipi-dbi/wiki
Nice with a wiki for this, I can see this will grow over time and be a
place to find how to support more panels.

> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> +F:	drivers/gpu/drm/tiny/panel-mipi-dbi.c
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:	Rob Clark <robdclark@xxxxxxxxx>
>  M:	Sean Paul <sean@xxxxxxxxxx>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 712e0004e96e..d552e1618da7 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -51,6 +51,19 @@ config DRM_GM12U320
>  	 This is a KMS driver for projectors which use the GM12U320 chipset
>  	 for video transfer over USB2/3, such as the Acer C120 mini projector.
>  
> +config DRM_PANEL_MIPI_DBI
> +	tristate "DRM support for MIPI DBI compatible panels"
> +	depends on DRM && SPI
> +	select DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
This symbol is not present in my drm-misc-next tree (which is a few
weeks old, so it may be newer).

> +	select DRM_MIPI_DBI
> +	select BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for MIPI DBI compatible
> +	  panels. The controller command setup can be provided using a
> +	  firmware file.
Consider adding a link to the wiki here - this may make it easier for
the user to find it.

> +	  To compile this driver as a module, choose M here.
> +
>  config DRM_SIMPLEDRM
>  	tristate "Simple framebuffer driver"
>  	depends on DRM && MMU
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 5d5505d40e7b..1d9d6227e7ab 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
>  obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
>  obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
>  obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI)	+= panel-mipi-dbi.o
>  obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
>  obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
>  obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
> diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> new file mode 100644
> index 000000000000..9240fdec38d6
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for MIPI DBI compatible display panels
> + *
> + * Copyright 2022 Noralf Trønnes
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +
> +#include <video/display_timing.h>
> +#include <video/mipi_display.h>
> +#include <video/of_display_timing.h>
> +#include <video/videomode.h>
videomode should not be used in new drivers, it is an fbdev artifact.
But that said - we are still missing a direct display_timing =>
display_mode - so maybe we need it here.

If it is needed Kconfig needs to be extended with:
select VIDEOMODE_HELPERS

> +
> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
> +					     0, 0, 0, 0, 0, 0, 0 };
> +
> +/*
> + * The optional display controller configuration is stored in a firmware file.
> + * The Device Tree 'compatible' property value with a '.bin' suffix is passed
> + * to request_firmware() to fetch this file.
> + */
> +struct panel_mipi_dbi_config {
> +	/* Magic string: panel_mipi_dbi_magic */
> +	u8 magic[15];
> +
> +	/* Config file format version */
> +	u8 file_format_version;
> +
> +	/*
> +	 * MIPI commands to execute when the display pipeline is enabled.
> +	 * This is used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
> +	u8 commands[];
> +};
> +
> +struct panel_mipi_dbi_commands {
> +	const u8 *buf;
> +	size_t len;
> +};
> +
> +static struct panel_mipi_dbi_commands *
> +panel_mipi_dbi_check_commands(struct device *dev, const struct firmware *fw)
> +{
> +	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
> +	struct panel_mipi_dbi_commands *commands;
> +	size_t size = fw->size, commands_len;
> +	unsigned int i = 0;
> +
> +	if (size < sizeof(*config) + 2) { /* At least 1 command */
> +		dev_err(dev, "config: file size=%zu is too small\n", size);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
> +		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (config->file_format_version != 1) {
> +		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	drm_dev_dbg(dev, DRM_UT_DRIVER, "size=%zu version=%u\n", size, config->file_format_version);
> +
> +	commands_len = size - sizeof(*config);
> +
> +	while ((i + 1) < commands_len) {
> +		u8 command = config->commands[i++];
> +		u8 num_parameters = config->commands[i++];
> +		const u8 *parameters = &config->commands[i];
> +
> +		i += num_parameters;
> +		if (i > commands_len) {
> +			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
> +				command, num_parameters);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			drm_dev_dbg(dev, DRM_UT_DRIVER, "sleep %ums\n", parameters[0]);
> +		else
> +			drm_dev_dbg(dev, DRM_UT_DRIVER, "command %02x %*ph\n",
> +				    command, num_parameters, parameters);
> +	}
> +
> +	if (i != commands_len) {
> +		dev_err(dev, "config: malformed command array\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	commands = devm_kzalloc(dev, sizeof(*commands), GFP_KERNEL);
> +	if (!commands)
> +		return ERR_PTR(-ENOMEM);
> +
> +	commands->len = commands_len;
> +	commands->buf = devm_kmemdup(dev, config->commands, commands->len, GFP_KERNEL);
> +	if (!commands->buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return commands;
> +}
> +
> +static struct panel_mipi_dbi_commands *panel_mipi_dbi_commands_from_fw(struct device *dev)
> +{
> +	struct panel_mipi_dbi_commands *commands;
> +	const struct firmware *fw;
> +	const char *compatible;
> +	struct property *prop;
> +	char fw_name[40];
> +	int ret;
> +
> +	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
> +		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
> +		ret = firmware_request_nowarn(&fw, fw_name, dev);
> +		if (ret) {
> +			drm_dev_dbg(dev, DRM_UT_DRIVER,
> +				    "No config file found for compatible: '%s' (error=%d)\n",
> +				    compatible, ret);
It would be more helpful to spell out that we failed to find a file
named compatible.bin here as the user may not be aware that the .bin
file is needed.

> +			continue;
> +		}
> +
> +		commands = panel_mipi_dbi_check_commands(dev, fw);
> +		release_firmware(fw);
> +		return commands;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void panel_mipi_dbi_commands_execute(struct mipi_dbi *dbi,
> +					    struct panel_mipi_dbi_commands *commands)
> +{
> +	unsigned int i = 0;
> +
> +	if (!commands)
> +		return;
> +
> +	while (i < commands->len) {
> +		u8 command = commands->buf[i++];
> +		u8 num_parameters = commands->buf[i++];
> +		const u8 *parameters = &commands->buf[i];
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			msleep(parameters[0]);
> +		else if (num_parameters)
> +			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
> +		else
> +			mipi_dbi_command(dbi, command);
> +
> +		i += num_parameters;
> +	}
> +}
> +
> +static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
> +				  struct drm_crtc_state *crtc_state,
> +				  struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +		return;
> +
> +	drm_dbg(pipe->crtc.dev, "\n");
> +
> +	ret = mipi_dbi_poweron_conditional_reset(dbidev);
> +	if (ret < 0)
> +		goto out_exit;
> +	if (!ret)
> +		panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
> +
> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> +out_exit:
> +	drm_dev_exit(idx);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
> +	.enable = panel_mipi_dbi_enable,
> +	.disable = mipi_dbi_pipe_disable,
> +	.update = mipi_dbi_pipe_update,
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(panel_mipi_dbi_fops);
> +
> +static const struct drm_driver panel_mipi_dbi_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &panel_mipi_dbi_fops,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "panel-mipi-dbi",
> +	.desc			= "MIPI DBI compatible display panel",
> +	.date			= "20220103",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
> +{
> +	struct device *dev = dbidev->drm.dev;
> +	u32 width_mm = 0, height_mm = 0;
> +	struct display_timing timing;
> +	struct videomode vm;
> +	int ret;
> +
> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> +	if (ret) {
> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
> +		return ret;
> +	}
> +
> +	videomode_from_timing(&timing, &vm);
> +
> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> +	    vm.flags) {
> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> +		return -EINVAL;
> +	}
We should have a helper that implements this. Maybe the display_timing
=> display_mode helper could do it.

> +
> +	/* The driver doesn't use the pixel clock but it is mandatory so fake one if not set */
> +	if (!vm.pixelclock)
> +		vm.pixelclock = (vm.hback_porch + vm.hactive) * (vm.vback_porch + vm.vactive) * 60;
> +
> +	dbidev->top_offset = vm.vback_porch;
> +	dbidev->left_offset = vm.hback_porch;
> +
> +	memset(mode, 0, sizeof(*mode));
> +	drm_display_mode_from_videomode(&vm, mode);
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +	ret = device_property_read_u32(dev, "width-mm", &width_mm);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "height-mm", &height_mm);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +
> +	mode->width_mm = width_mm;
> +	mode->height_mm = height_mm;
> +
> +	drm_mode_debug_printmodeline(mode);
> +
> +	return 0;
> +}
> +
> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct drm_display_mode mode;
> +	struct mipi_dbi_dev *dbidev;
> +	struct drm_device *drm;
> +	struct mipi_dbi *dbi;
> +	struct gpio_desc *dc;
> +	int ret;
> +
> +	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
> +	if (IS_ERR(dbidev))
> +		return PTR_ERR(dbidev);
> +
> +	dbi = &dbidev->dbi;
> +	drm = &dbidev->drm;
> +
> +	ret = panel_mipi_dbi_get_mode(dbidev, &mode);
> +	if (ret)
> +		return ret;
> +
> +	dbidev->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(dbidev->regulator))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
> +				     "Failed to get regulator 'power'\n");
> +
> +	dbidev->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(dbidev->backlight))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
> +
> +	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(dbi->reset))
> +		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc))
> +		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
> +
> +	ret = mipi_dbi_spi_init(spi, dbi, dc);
> +	if (ret)
> +		return ret;
> +
> +	if (device_property_present(dev, "write-only"))
> +		dbi->read_commands = NULL;
read_commands are unused - so the write-only property is in practice
ignored.

> +
> +	dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dev);
> +	if (IS_ERR(dbidev->driver_private))
> +		return PTR_ERR(dbidev->driver_private);
> +
> +	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_pipe_funcs, &mode, 0);
> +	if (ret)
> +		return ret;
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, drm);
> +
> +	drm_fbdev_generic_setup(drm, 0);
> +
> +	return 0;
> +}
> +
> +static int panel_mipi_dbi_spi_remove(struct spi_device *spi)
> +{
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);
> +
> +	return 0;
> +}
> +
> +static void panel_mipi_dbi_spi_shutdown(struct spi_device *spi)
> +{
> +	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static int __maybe_unused panel_mipi_dbi_pm_suspend(struct device *dev)
> +{
> +	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int __maybe_unused panel_mipi_dbi_pm_resume(struct device *dev)
> +{
> +	drm_mode_config_helper_resume(dev_get_drvdata(dev));
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops panel_mipi_dbi_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(panel_mipi_dbi_pm_suspend, panel_mipi_dbi_pm_resume)
> +};
> +
> +static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
> +	{ .compatible = "panel-mipi-dbi-spi" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
> +
> +static const struct spi_device_id panel_mipi_dbi_spi_id[] = {
> +	{ "panel-mipi-dbi-spi", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, panel_mipi_dbi_spi_id);
> +
> +static struct spi_driver panel_mipi_dbi_spi_driver = {
> +	.driver = {
> +		.name = "panel-mipi-dbi-spi",
> +		.owner = THIS_MODULE,
> +		.of_match_table = panel_mipi_dbi_spi_of_match,
> +		.pm = &panel_mipi_dbi_pm_ops,
> +	},
> +	.id_table = panel_mipi_dbi_spi_id,
> +	.probe = panel_mipi_dbi_spi_probe,
> +	.remove = panel_mipi_dbi_spi_remove,
> +	.shutdown = panel_mipi_dbi_spi_shutdown,
> +};
> +module_spi_driver(panel_mipi_dbi_spi_driver);
> +
> +MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
> +MODULE_AUTHOR("Noralf Trønnes");
> +MODULE_LICENSE("GPL");
> -- 
> 2.33.0



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux