mbox series

[0/3] drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel

Message ID cover.1597412076.git.agx@sigxcpu.org
Headers show
Series drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel | expand

Message

Guido Günther Aug. 14, 2020, 1:36 p.m. UTC 
The panel uses a Focaltech FT8006p, the touch part is handled by the already
existing edt-ft5x06. It can be found in e.g. the Librem 5.

This series is against next-20200814.

Guido Günther (3):
  dt-bindings: vendor-prefixes: Add mantix vendor prefix
  dt-bindings: Add Mantix MLAF057WE51-X panel bindings
  drm/panel: Add panel driver for the Mantix MLAF057WE51-X DSI panel

 .../display/panel/mantix,mlaf057we51-x.yaml   |  73 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 362 ++++++++++++++++++
 6 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c

Comments

Sam Ravnborg Aug. 15, 2020, 9:32 a.m. UTC | #1 
Hi Guido.

On Fri, Aug 14, 2020 at 03:36:23PM +0200, Guido Günther wrote:
> The panel uses a Focaltech FT8006p, the touch part is handled by the
> already existing edt-ft5x06.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>

Looks good.
A few notes in the following, nothing major.

	Sam

> ---
>  MAINTAINERS                                   |   7 +
>  drivers/gpu/drm/panel/Kconfig                 |  11 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 362 ++++++++++++++++++
>  4 files changed, 381 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83ba7b62651f7..7dfe4cc3d4ec8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5474,6 +5474,13 @@ S:	Maintained
>  F:	drivers/gpu/drm/panel/panel-lvds.c
>  F:	Documentation/devicetree/bindings/display/panel/lvds.yaml
>  
> +DRM DRIVER FOR MANTIX MLAF057WE51 PANELS
> +M:	Guido Günther <agx@sigxcpu.org>
> +R:	Purism Kernel Team <kernel@puri.sm>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> +F:	drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> +
>  DRM DRIVER FOR MATROX G200/G400 GRAPHICS CARDS
>  S:	Orphan / Obsolete
>  F:	drivers/gpu/drm/mga/
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index de2f2a452be55..8d97d07c58713 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -217,6 +217,17 @@ config DRM_PANEL_NOVATEK_NT39016
>  	  Say Y here if you want to enable support for the panels built
>  	  around the Novatek NT39016 display controller.
>  
> +config DRM_PANEL_MANTIX_MLAF057WE51
> +	tristate "Mantix MLAF057WE51-X MIPI-DSI LCD panel"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for the Mantix
> +	  MLAF057WE51-X MIPI DSI panel as e.g. used in the Librem 5. It
> +	  has a resolution of 720x1440 pixels, a built in backlight and touch
> +	  controller.
> +
>  config DRM_PANEL_OLIMEX_LCD_OLINUXINO
>  	tristate "Olimex LCD-OLinuXino panel"
>  	depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index e45ceac6286fd..15a4e77529514 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
> +obj-$(CONFIG_DRM_PANEL_MANTIX_MLAF057WE51) += panel-mantix-mlaf057we51.o
>  obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
>  obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
>  obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
> diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> new file mode 100644
> index 0000000000000..6c07bcdb75937
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mantix MLAF057WE51 5.7" MIPI-DSI panel driver
> + *
> + * Copyright (C) Purism SPC 2020
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/media-bus-format.h>
Include not needed.

> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <video/display_timing.h>
I do not think this include is needed

> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +#define DRV_NAME "panel-mantix-mlaf057we51"
> +
> +/* Manufacturer specific Commands send via DSI */
> +#define MANTIX_CMD_OTP_STOP_RELOAD_MIPI 0x41
> +#define MANTIX_CMD_INT_CANCEL           0x4C
> +
> +struct mantix {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +
> +	struct regulator *avdd;
> +	struct regulator *avee;
> +	struct regulator *vddi;
> +};
> +
> +static inline struct mantix *panel_to_mantix(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct mantix, panel);
> +}
> +
> +#define dsi_generic_write_seq(dsi, seq...) do {				\
> +		static const u8 d[] = { seq };				\
> +		int ret;						\
> +		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
> +		if (ret < 0)						\
> +			return ret;					\
> +	} while (0)
> +
> +static int mantix_init_sequence(struct mantix *ctx)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	struct device *dev = ctx->dev;
> +
> +	/*
> +	 * Init sequence was supplied by the panel vendor.
> +	 */
> +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A);
> +
> +	dsi_generic_write_seq(dsi, MANTIX_CMD_INT_CANCEL, 0x03);
> +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x03);
> +	dsi_generic_write_seq(dsi, 0x80, 0xA9, 0x00);
> +
> +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x09);
> +	dsi_generic_write_seq(dsi, 0x80, 0x64, 0x00, 0x64, 0x00, 0x00);
> +	msleep(20);
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
> +	return 0;
> +}
> +
> +static int mantix_enable(struct drm_panel *panel)
> +{
> +	struct mantix *ctx = panel_to_mantix(panel);
> +	struct device *dev = ctx->dev;
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
> +	int ret;
> +
> +	ret = mantix_init_sequence(ctx);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> +		return ret;
> +	}
> +	msleep(20);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret)
> +		return ret;
> +	usleep_range(10000, 12000);
> +
> +	ret = mipi_dsi_turn_on_peripheral(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to turn on peripheral\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mantix_disable(struct drm_panel *panel)
> +{
> +	struct mantix *ctx = panel_to_mantix(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to turn off the display: %d\n", ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enter sleep mode: %d\n", ret);
> +
> +	mipi_dsi_dcs_enter_sleep_mode(dsi);
Does the display really need to enter sleep mode twice?

> +	return 0;
> +}
> +
> +static int mantix_unprepare(struct drm_panel *panel)
> +{
> +	struct mantix *ctx = panel_to_mantix(panel);
> +
> +	regulator_disable(ctx->avdd);
> +	regulator_disable(ctx->avee);
> +	regulator_disable(ctx->vddi);

The order the regulators are disabled is not the reverse of the order
thay are enabled. Unless this is on purpose it would be more logical and
less confusing if unprepare is the reverse of prepare.


> +
> +	return 0;
> +}
> +
> +static int mantix_prepare(struct drm_panel *panel)
> +{
> +	struct mantix *ctx = panel_to_mantix(panel);
> +	int ret;
> +
> +	/* Focaltech FT8006P, section 7.3.1 and 7.3.4 */
> +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> +	ret = regulator_enable(ctx->vddi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable vddi supply: %d\n", ret);
> +		return ret;
> +	}
> +	/* T1 + T2 */
> +	usleep_range(8000, 10000);
> +
> +	ret = regulator_enable(ctx->avdd);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable avdd supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* T2d */
> +	usleep_range(3500, 4000);
> +	ret = regulator_enable(ctx->avee);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(ctx->dev,
> +			      "Failed to enable avee supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* T3+T5 */
> +	usleep_range(10000, 12000);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	usleep_range(5150, 7000);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +
> +	/* T6 */
> +	msleep(50);
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode default_mode = {
> +	.hdisplay    = 720,
> +	.hsync_start = 720 + 45,
> +	.hsync_end   = 720 + 45 + 14,
> +	.htotal	     = 720 + 45 + 14 + 25,
> +	.vdisplay    = 1440,
> +	.vsync_start = 1440 + 130,
> +	.vsync_end   = 1440 + 130 + 8,
> +	.vtotal	     = 1440 + 130 + 8 + 106,
> +	.clock	     = 85298,
> +	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	.width_mm    = 65,
> +	.height_mm   = 130,
> +};
> +
> +static int mantix_get_modes(struct drm_panel *panel,
> +			    struct drm_connector *connector)
> +{
> +	struct mantix *ctx = panel_to_mantix(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, &default_mode);
> +	if (!mode) {
> +		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u\n",
> +			      default_mode.hdisplay, default_mode.vdisplay,
> +			      drm_mode_vrefresh(mode));
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs mantix_drm_funcs = {
> +	.disable   = mantix_disable,
> +	.unprepare = mantix_unprepare,
> +	.prepare   = mantix_prepare,
> +	.enable	   = mantix_enable,
> +	.get_modes = mantix_get_modes,
> +};
> +
> +static int mantix_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct mantix *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> +		return PTR_ERR(ctx->reset_gpio);
> +	}
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dev = dev;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +
> +	ctx->avdd = devm_regulator_get(dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		ret = PTR_ERR(ctx->avdd);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request avdd regulator: %d\n",
> +				      ret);
In these modern times linelegth of 100 is acceptable, so avoid
linewrapping here and likewise below.

> +		return ret;
> +	}
> +	ctx->avee = devm_regulator_get(dev, "avee");
> +	if (IS_ERR(ctx->avee)) {
> +		ret = PTR_ERR(ctx->avee);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request avee regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +	ctx->vddi = devm_regulator_get(dev, "vddi");
> +	if (IS_ERR(ctx->vddi)) {
> +		ret = PTR_ERR(ctx->vddi);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request vddi regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +
> +	drm_panel_init(&ctx->panel, dev, &mantix_drm_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
empty line?
> +	drm_panel_add(&ctx->panel);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev,
> +			      "mipi_dsi_attach failed (%d). Is host ready?\n",
> +			      ret);
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> +		     default_mode.hdisplay, default_mode.vdisplay,
> +		     drm_mode_vrefresh(&default_mode),
> +		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> +
> +	return 0;
> +}
> +
> +static void mantix_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = drm_panel_unprepare(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> +			      ret);
> +
> +	ret = drm_panel_disable(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
> +			      ret);
> +}
In shutdown we usually ignore any errors.

> +
> +static int mantix_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	mantix_shutdown(dsi);
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
> +			      ret);
Likewise in remove we ignore any errors.

> +
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mantix_of_match[] = {
> +	{ .compatible = "mantix,mlaf057we51-x" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mantix_of_match);
> +
> +static struct mipi_dsi_driver mantix_driver = {
> +	.probe	= mantix_probe,
> +	.remove = mantix_remove,
> +	.shutdown = mantix_shutdown,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = mantix_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(mantix_driver);
> +
> +MODULE_AUTHOR("Guido Günther <agx@sigxcpu.org>");
> +MODULE_DESCRIPTION("DRM driver for Mantix MLAF057WE51-X MIPI DSI panel");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sam Ravnborg Aug. 15, 2020, 10:02 a.m. UTC | #2 
Hi Guido.

> +static int mantix_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct mantix *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> +		return PTR_ERR(ctx->reset_gpio);
> +	}
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +	ctx->dev = dev;
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +
> +	ctx->avdd = devm_regulator_get(dev, "avdd");
> +	if (IS_ERR(ctx->avdd)) {
> +		ret = PTR_ERR(ctx->avdd);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request avdd regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}

Consider to use the recently added dev_err_probe() here and below.
Note: Not part of drm-misc-next yet - but hopefully after -rc1
when a backmerge is done.

	Sam

> +	ctx->avee = devm_regulator_get(dev, "avee");
> +	if (IS_ERR(ctx->avee)) {
> +		ret = PTR_ERR(ctx->avee);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request avee regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +	ctx->vddi = devm_regulator_get(dev, "vddi");
> +	if (IS_ERR(ctx->vddi)) {
> +		ret = PTR_ERR(ctx->vddi);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev,
> +				      "Failed to request vddi regulator: %d\n",
> +				      ret);
> +		return ret;
> +	}
> +
> +	drm_panel_init(&ctx->panel, dev, &mantix_drm_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +	drm_panel_add(&ctx->panel);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev,
> +			      "mipi_dsi_attach failed (%d). Is host ready?\n",
> +			      ret);
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> +		     default_mode.hdisplay, default_mode.vdisplay,
> +		     drm_mode_vrefresh(&default_mode),
> +		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> +
> +	return 0;
> +}
> +
> +static void mantix_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = drm_panel_unprepare(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> +			      ret);
> +
> +	ret = drm_panel_disable(&ctx->panel);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
> +			      ret);
> +}
> +
> +static int mantix_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	mantix_shutdown(dsi);
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
> +			      ret);
> +
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mantix_of_match[] = {
> +	{ .compatible = "mantix,mlaf057we51-x" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mantix_of_match);
> +
> +static struct mipi_dsi_driver mantix_driver = {
> +	.probe	= mantix_probe,
> +	.remove = mantix_remove,
> +	.shutdown = mantix_shutdown,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = mantix_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(mantix_driver);
> +
> +MODULE_AUTHOR("Guido Günther <agx@sigxcpu.org>");
> +MODULE_DESCRIPTION("DRM driver for Mantix MLAF057WE51-X MIPI DSI panel");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Guido Günther Aug. 15, 2020, 10:40 a.m. UTC | #3 
Hi Sam,
On Sat, Aug 15, 2020 at 12:02:30PM +0200, Sam Ravnborg wrote:
> Hi Guido.
> 
> > +static int mantix_probe(struct mipi_dsi_device *dsi)
> > +{
> > +	struct device *dev = &dsi->dev;
> > +	struct mantix *ctx;
> > +	int ret;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(ctx->reset_gpio)) {
> > +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> > +		return PTR_ERR(ctx->reset_gpio);
> > +	}
> > +
> > +	mipi_dsi_set_drvdata(dsi, ctx);
> > +	ctx->dev = dev;
> > +
> > +	dsi->lanes = 4;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> > +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +
> > +	ctx->avdd = devm_regulator_get(dev, "avdd");
> > +	if (IS_ERR(ctx->avdd)) {
> > +		ret = PTR_ERR(ctx->avdd);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request avdd regulator: %d\n",
> > +				      ret);
> > +		return ret;
> > +	}
> 
> Consider to use the recently added dev_err_probe() here and below.
> Note: Not part of drm-misc-next yet - but hopefully after -rc1
> when a backmerge is done.

In fact I did decided against it since i was told that missing dev_* and
DRM_* logging shouldn't be done. So is that o.k. nowadays?
Cheers,
 -- Guido

> 
> 	Sam
> 
> > +	ctx->avee = devm_regulator_get(dev, "avee");
> > +	if (IS_ERR(ctx->avee)) {
> > +		ret = PTR_ERR(ctx->avee);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request avee regulator: %d\n",
> > +				      ret);
> > +		return ret;
> > +	}
> > +	ctx->vddi = devm_regulator_get(dev, "vddi");
> > +	if (IS_ERR(ctx->vddi)) {
> > +		ret = PTR_ERR(ctx->vddi);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request vddi regulator: %d\n",
> > +				      ret);
> > +		return ret;
> > +	}
> > +
> > +	drm_panel_init(&ctx->panel, dev, &mantix_drm_funcs,
> > +		       DRM_MODE_CONNECTOR_DSI);
> > +
> > +	ret = drm_panel_of_backlight(&ctx->panel);
> > +	if (ret)
> > +		return ret;
> > +	drm_panel_add(&ctx->panel);
> > +
> > +	ret = mipi_dsi_attach(dsi);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(dev,
> > +			      "mipi_dsi_attach failed (%d). Is host ready?\n",
> > +			      ret);
> > +		drm_panel_remove(&ctx->panel);
> > +		return ret;
> > +	}
> > +
> > +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> > +		     default_mode.hdisplay, default_mode.vdisplay,
> > +		     drm_mode_vrefresh(&default_mode),
> > +		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mantix_shutdown(struct mipi_dsi_device *dsi)
> > +{
> > +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> > +	int ret;
> > +
> > +	ret = drm_panel_unprepare(&ctx->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> > +			      ret);
> > +
> > +	ret = drm_panel_disable(&ctx->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
> > +			      ret);
> > +}
> > +
> > +static int mantix_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> > +	int ret;
> > +
> > +	mantix_shutdown(dsi);
> > +
> > +	ret = mipi_dsi_detach(dsi);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
> > +			      ret);
> > +
> > +	drm_panel_remove(&ctx->panel);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mantix_of_match[] = {
> > +	{ .compatible = "mantix,mlaf057we51-x" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mantix_of_match);
> > +
> > +static struct mipi_dsi_driver mantix_driver = {
> > +	.probe	= mantix_probe,
> > +	.remove = mantix_remove,
> > +	.shutdown = mantix_shutdown,
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.of_match_table = mantix_of_match,
> > +	},
> > +};
> > +module_mipi_dsi_driver(mantix_driver);
> > +
> > +MODULE_AUTHOR("Guido Günther <agx@sigxcpu.org>");
> > +MODULE_DESCRIPTION("DRM driver for Mantix MLAF057WE51-X MIPI DSI panel");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.26.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Sam Ravnborg Aug. 15, 2020, 10:46 a.m. UTC | #4 
On Sat, Aug 15, 2020 at 12:40:22PM +0200, Guido Günther wrote:
> Hi Sam,
> On Sat, Aug 15, 2020 at 12:02:30PM +0200, Sam Ravnborg wrote:
> > Hi Guido.
> > 
> > > +static int mantix_probe(struct mipi_dsi_device *dsi)
> > > +{
> > > +	struct device *dev = &dsi->dev;
> > > +	struct mantix *ctx;
> > > +	int ret;
> > > +
> > > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > > +	if (!ctx)
> > > +		return -ENOMEM;
> > > +
> > > +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > +	if (IS_ERR(ctx->reset_gpio)) {
> > > +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> > > +		return PTR_ERR(ctx->reset_gpio);
> > > +	}
> > > +
> > > +	mipi_dsi_set_drvdata(dsi, ctx);
> > > +	ctx->dev = dev;
> > > +
> > > +	dsi->lanes = 4;
> > > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> > > +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > > +
> > > +	ctx->avdd = devm_regulator_get(dev, "avdd");
> > > +	if (IS_ERR(ctx->avdd)) {
> > > +		ret = PTR_ERR(ctx->avdd);
> > > +		if (ret != -EPROBE_DEFER)
> > > +			DRM_DEV_ERROR(dev,
> > > +				      "Failed to request avdd regulator: %d\n",
> > > +				      ret);
> > > +		return ret;
> > > +	}
> > 
> > Consider to use the recently added dev_err_probe() here and below.
> > Note: Not part of drm-misc-next yet - but hopefully after -rc1
> > when a backmerge is done.
> 
> In fact I did decided against it since i was told that missing dev_* and
> DRM_* logging shouldn't be done. So is that o.k. nowadays?
s/missing/mixing/

I often request that logging is consistent - so I recognize the
argument.

For panel/* I have not made up my mind what I think is the best
approach. The DRM_DEV_* and DRM_* logging do not add much value.
So I have been tempted several times to convert all logging in
panel/ to dev_* and pr_* (when no struct device * is available).
That would also avoid that we mix up logging.

We have drm_* logging - but they require a valid drm_device * which we
do not have in the the panel drivers. So they are ruled out here.

Do you have any opinions/comments on this?

	Sam
Sam Ravnborg Aug. 15, 2020, 12:55 p.m. UTC | #5 
Hi Guido.

> > In fact I did decided against it since i was told that missing dev_* and
> > DRM_* logging shouldn't be done. So is that o.k. nowadays?
> s/missing/mixing/
> 
> I often request that logging is consistent - so I recognize the
> argument.
> 
> For panel/* I have not made up my mind what I think is the best
> approach. The DRM_DEV_* and DRM_* logging do not add much value.
> So I have been tempted several times to convert all logging in
> panel/ to dev_* and pr_* (when no struct device * is available).
> That would also avoid that we mix up logging.
> 
> We have drm_* logging - but they require a valid drm_device * which we
> do not have in the the panel drivers. So they are ruled out here.
> 
> Do you have any opinions/comments on this?
I went ahead and dropped the DRM_ logging in drm/panel, you are copied
on the patchset. Feedback on the patches would be nice.

	Sam
Guido Günther Aug. 15, 2020, 4:25 p.m. UTC | #6 
Hi,
On Sat, Aug 15, 2020 at 12:46:51PM +0200, Sam Ravnborg wrote:
> On Sat, Aug 15, 2020 at 12:40:22PM +0200, Guido Günther wrote:
> > Hi Sam,
> > On Sat, Aug 15, 2020 at 12:02:30PM +0200, Sam Ravnborg wrote:
> > > Hi Guido.
> > > 
> > > > +static int mantix_probe(struct mipi_dsi_device *dsi)
> > > > +{
> > > > +	struct device *dev = &dsi->dev;
> > > > +	struct mantix *ctx;
> > > > +	int ret;
> > > > +
> > > > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > > > +	if (!ctx)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > > +	if (IS_ERR(ctx->reset_gpio)) {
> > > > +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> > > > +		return PTR_ERR(ctx->reset_gpio);
> > > > +	}
> > > > +
> > > > +	mipi_dsi_set_drvdata(dsi, ctx);
> > > > +	ctx->dev = dev;
> > > > +
> > > > +	dsi->lanes = 4;
> > > > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > > > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> > > > +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > > > +
> > > > +	ctx->avdd = devm_regulator_get(dev, "avdd");
> > > > +	if (IS_ERR(ctx->avdd)) {
> > > > +		ret = PTR_ERR(ctx->avdd);
> > > > +		if (ret != -EPROBE_DEFER)
> > > > +			DRM_DEV_ERROR(dev,
> > > > +				      "Failed to request avdd regulator: %d\n",
> > > > +				      ret);
> > > > +		return ret;
> > > > +	}
> > > 
> > > Consider to use the recently added dev_err_probe() here and below.
> > > Note: Not part of drm-misc-next yet - but hopefully after -rc1
> > > when a backmerge is done.
> > 
> > In fact I did decided against it since i was told that missing dev_* and
> > DRM_* logging shouldn't be done. So is that o.k. nowadays?
> s/missing/mixing/
> 
> I often request that logging is consistent - so I recognize the
> argument.
> 
> For panel/* I have not made up my mind what I think is the best
> approach. The DRM_DEV_* and DRM_* logging do not add much value.
> So I have been tempted several times to convert all logging in
> panel/ to dev_* and pr_* (when no struct device * is available).
> That would also avoid that we mix up logging.
> 
> We have drm_* logging - but they require a valid drm_device * which we
> do not have in the the panel drivers. So they are ruled out here.
> 
> Do you have any opinions/comments on this?

I think for panel drivers DRM_* does not give any bonus so moving to
{dev,pr}_* sounds good. I just wonder if other drm parts don't need
`dev_drm_err_probe()` (or similar) anyway. But then maybe dyn_debug
is enough nowadays to not need DRM_DEV_DEBUG_* either?
Cheers,
 -- Guido

> 
> 	Sam
>
Guido Günther Aug. 15, 2020, 9:28 p.m. UTC | #7 
Hi Sam,
On Sat, Aug 15, 2020 at 11:32:26AM +0200, Sam Ravnborg wrote:
> Hi Guido.
> 
> On Fri, Aug 14, 2020 at 03:36:23PM +0200, Guido Günther wrote:
> > The panel uses a Focaltech FT8006p, the touch part is handled by the
> > already existing edt-ft5x06.
> > 
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> 
> Looks good.
> A few notes in the following, nothing major.

Thanks. I've added your suggestions to v2.
 -- Guido
> 
> 	Sam
> 
> > ---
> >  MAINTAINERS                                   |   7 +
> >  drivers/gpu/drm/panel/Kconfig                 |  11 +
> >  drivers/gpu/drm/panel/Makefile                |   1 +
> >  .../gpu/drm/panel/panel-mantix-mlaf057we51.c  | 362 ++++++++++++++++++
> >  4 files changed, 381 insertions(+)
> >  create mode 100644 drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 83ba7b62651f7..7dfe4cc3d4ec8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5474,6 +5474,13 @@ S:	Maintained
> >  F:	drivers/gpu/drm/panel/panel-lvds.c
> >  F:	Documentation/devicetree/bindings/display/panel/lvds.yaml
> >  
> > +DRM DRIVER FOR MANTIX MLAF057WE51 PANELS
> > +M:	Guido Günther <agx@sigxcpu.org>
> > +R:	Purism Kernel Team <kernel@puri.sm>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/display/panel/mantix,mlaf057we51-x.yaml
> > +F:	drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> > +
> >  DRM DRIVER FOR MATROX G200/G400 GRAPHICS CARDS
> >  S:	Orphan / Obsolete
> >  F:	drivers/gpu/drm/mga/
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index de2f2a452be55..8d97d07c58713 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -217,6 +217,17 @@ config DRM_PANEL_NOVATEK_NT39016
> >  	  Say Y here if you want to enable support for the panels built
> >  	  around the Novatek NT39016 display controller.
> >  
> > +config DRM_PANEL_MANTIX_MLAF057WE51
> > +	tristate "Mantix MLAF057WE51-X MIPI-DSI LCD panel"
> > +	depends on OF
> > +	depends on DRM_MIPI_DSI
> > +	depends on BACKLIGHT_CLASS_DEVICE
> > +	help
> > +	  Say Y here if you want to enable support for the Mantix
> > +	  MLAF057WE51-X MIPI DSI panel as e.g. used in the Librem 5. It
> > +	  has a resolution of 720x1440 pixels, a built in backlight and touch
> > +	  controller.
> > +
> >  config DRM_PANEL_OLIMEX_LCD_OLINUXINO
> >  	tristate "Olimex LCD-OLinuXino panel"
> >  	depends on OF
> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > index e45ceac6286fd..15a4e77529514 100644
> > --- a/drivers/gpu/drm/panel/Makefile
> > +++ b/drivers/gpu/drm/panel/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> >  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
> >  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
> >  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
> > +obj-$(CONFIG_DRM_PANEL_MANTIX_MLAF057WE51) += panel-mantix-mlaf057we51.o
> >  obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
> >  obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
> >  obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
> > diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> > new file mode 100644
> > index 0000000000000..6c07bcdb75937
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c
> > @@ -0,0 +1,362 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Mantix MLAF057WE51 5.7" MIPI-DSI panel driver
> > + *
> > + * Copyright (C) Purism SPC 2020
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/media-bus-format.h>
> Include not needed.
> 
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <video/display_timing.h>
> I do not think this include is needed
> 
> > +#include <video/mipi_display.h>
> > +
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +
> > +#define DRV_NAME "panel-mantix-mlaf057we51"
> > +
> > +/* Manufacturer specific Commands send via DSI */
> > +#define MANTIX_CMD_OTP_STOP_RELOAD_MIPI 0x41
> > +#define MANTIX_CMD_INT_CANCEL           0x4C
> > +
> > +struct mantix {
> > +	struct device *dev;
> > +	struct drm_panel panel;
> > +	struct gpio_desc *reset_gpio;
> > +
> > +	struct regulator *avdd;
> > +	struct regulator *avee;
> > +	struct regulator *vddi;
> > +};
> > +
> > +static inline struct mantix *panel_to_mantix(struct drm_panel *panel)
> > +{
> > +	return container_of(panel, struct mantix, panel);
> > +}
> > +
> > +#define dsi_generic_write_seq(dsi, seq...) do {				\
> > +		static const u8 d[] = { seq };				\
> > +		int ret;						\
> > +		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +	} while (0)
> > +
> > +static int mantix_init_sequence(struct mantix *ctx)
> > +{
> > +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > +	struct device *dev = ctx->dev;
> > +
> > +	/*
> > +	 * Init sequence was supplied by the panel vendor.
> > +	 */
> > +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A);
> > +
> > +	dsi_generic_write_seq(dsi, MANTIX_CMD_INT_CANCEL, 0x03);
> > +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x03);
> > +	dsi_generic_write_seq(dsi, 0x80, 0xA9, 0x00);
> > +
> > +	dsi_generic_write_seq(dsi, MANTIX_CMD_OTP_STOP_RELOAD_MIPI, 0x5A, 0x09);
> > +	dsi_generic_write_seq(dsi, 0x80, 0x64, 0x00, 0x64, 0x00, 0x00);
> > +	msleep(20);
> > +
> > +	DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
> > +	return 0;
> > +}
> > +
> > +static int mantix_enable(struct drm_panel *panel)
> > +{
> > +	struct mantix *ctx = panel_to_mantix(panel);
> > +	struct device *dev = ctx->dev;
> > +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
> > +	int ret;
> > +
> > +	ret = mantix_init_sequence(ctx);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> > +			      ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> > +		return ret;
> > +	}
> > +	msleep(20);
> > +
> > +	ret = mipi_dsi_dcs_set_display_on(dsi);
> > +	if (ret)
> > +		return ret;
> > +	usleep_range(10000, 12000);
> > +
> > +	ret = mipi_dsi_turn_on_peripheral(dsi);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(dev, "Failed to turn on peripheral\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mantix_disable(struct drm_panel *panel)
> > +{
> > +	struct mantix *ctx = panel_to_mantix(panel);
> > +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > +	int ret;
> > +
> > +	ret = mipi_dsi_dcs_set_display_off(dsi);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(ctx->dev,
> > +			      "Failed to turn off the display: %d\n", ret);
> > +
> > +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(ctx->dev,
> > +			      "Failed to enter sleep mode: %d\n", ret);
> > +
> > +	mipi_dsi_dcs_enter_sleep_mode(dsi);
> Does the display really need to enter sleep mode twice?
> 
> > +	return 0;
> > +}
> > +
> > +static int mantix_unprepare(struct drm_panel *panel)
> > +{
> > +	struct mantix *ctx = panel_to_mantix(panel);
> > +
> > +	regulator_disable(ctx->avdd);
> > +	regulator_disable(ctx->avee);
> > +	regulator_disable(ctx->vddi);
> 
> The order the regulators are disabled is not the reverse of the order
> thay are enabled. Unless this is on purpose it would be more logical and
> less confusing if unprepare is the reverse of prepare.
> 
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int mantix_prepare(struct drm_panel *panel)
> > +{
> > +	struct mantix *ctx = panel_to_mantix(panel);
> > +	int ret;
> > +
> > +	/* Focaltech FT8006P, section 7.3.1 and 7.3.4 */
> > +	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> > +	ret = regulator_enable(ctx->vddi);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(ctx->dev,
> > +			      "Failed to enable vddi supply: %d\n", ret);
> > +		return ret;
> > +	}
> > +	/* T1 + T2 */
> > +	usleep_range(8000, 10000);
> > +
> > +	ret = regulator_enable(ctx->avdd);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(ctx->dev,
> > +			      "Failed to enable avdd supply: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* T2d */
> > +	usleep_range(3500, 4000);
> > +	ret = regulator_enable(ctx->avee);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(ctx->dev,
> > +			      "Failed to enable avee supply: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* T3+T5 */
> > +	usleep_range(10000, 12000);
> > +
> > +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> > +	usleep_range(5150, 7000);
> > +
> > +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > +
> > +	/* T6 */
> > +	msleep(50);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_display_mode default_mode = {
> > +	.hdisplay    = 720,
> > +	.hsync_start = 720 + 45,
> > +	.hsync_end   = 720 + 45 + 14,
> > +	.htotal	     = 720 + 45 + 14 + 25,
> > +	.vdisplay    = 1440,
> > +	.vsync_start = 1440 + 130,
> > +	.vsync_end   = 1440 + 130 + 8,
> > +	.vtotal	     = 1440 + 130 + 8 + 106,
> > +	.clock	     = 85298,
> > +	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +	.width_mm    = 65,
> > +	.height_mm   = 130,
> > +};
> > +
> > +static int mantix_get_modes(struct drm_panel *panel,
> > +			    struct drm_connector *connector)
> > +{
> > +	struct mantix *ctx = panel_to_mantix(panel);
> > +	struct drm_display_mode *mode;
> > +
> > +	mode = drm_mode_duplicate(connector->dev, &default_mode);
> > +	if (!mode) {
> > +		DRM_DEV_ERROR(ctx->dev, "Failed to add mode %ux%u@%u\n",
> > +			      default_mode.hdisplay, default_mode.vdisplay,
> > +			      drm_mode_vrefresh(mode));
> > +		return -ENOMEM;
> > +	}
> > +
> > +	drm_mode_set_name(mode);
> > +
> > +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > +	connector->display_info.width_mm = mode->width_mm;
> > +	connector->display_info.height_mm = mode->height_mm;
> > +	drm_mode_probed_add(connector, mode);
> > +
> > +	return 1;
> > +}
> > +
> > +static const struct drm_panel_funcs mantix_drm_funcs = {
> > +	.disable   = mantix_disable,
> > +	.unprepare = mantix_unprepare,
> > +	.prepare   = mantix_prepare,
> > +	.enable	   = mantix_enable,
> > +	.get_modes = mantix_get_modes,
> > +};
> > +
> > +static int mantix_probe(struct mipi_dsi_device *dsi)
> > +{
> > +	struct device *dev = &dsi->dev;
> > +	struct mantix *ctx;
> > +	int ret;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(ctx->reset_gpio)) {
> > +		DRM_DEV_ERROR(dev, "cannot get reset gpio\n");
> > +		return PTR_ERR(ctx->reset_gpio);
> > +	}
> > +
> > +	mipi_dsi_set_drvdata(dsi, ctx);
> > +	ctx->dev = dev;
> > +
> > +	dsi->lanes = 4;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> > +		MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +
> > +	ctx->avdd = devm_regulator_get(dev, "avdd");
> > +	if (IS_ERR(ctx->avdd)) {
> > +		ret = PTR_ERR(ctx->avdd);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request avdd regulator: %d\n",
> > +				      ret);
> In these modern times linelegth of 100 is acceptable, so avoid
> linewrapping here and likewise below.
> 
> > +		return ret;
> > +	}
> > +	ctx->avee = devm_regulator_get(dev, "avee");
> > +	if (IS_ERR(ctx->avee)) {
> > +		ret = PTR_ERR(ctx->avee);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request avee regulator: %d\n",
> > +				      ret);
> > +		return ret;
> > +	}
> > +	ctx->vddi = devm_regulator_get(dev, "vddi");
> > +	if (IS_ERR(ctx->vddi)) {
> > +		ret = PTR_ERR(ctx->vddi);
> > +		if (ret != -EPROBE_DEFER)
> > +			DRM_DEV_ERROR(dev,
> > +				      "Failed to request vddi regulator: %d\n",
> > +				      ret);
> > +		return ret;
> > +	}
> > +
> > +	drm_panel_init(&ctx->panel, dev, &mantix_drm_funcs,
> > +		       DRM_MODE_CONNECTOR_DSI);
> > +
> > +	ret = drm_panel_of_backlight(&ctx->panel);
> > +	if (ret)
> > +		return ret;
> empty line?
> > +	drm_panel_add(&ctx->panel);
> > +
> > +	ret = mipi_dsi_attach(dsi);
> > +	if (ret < 0) {
> > +		DRM_DEV_ERROR(dev,
> > +			      "mipi_dsi_attach failed (%d). Is host ready?\n",
> > +			      ret);
> > +		drm_panel_remove(&ctx->panel);
> > +		return ret;
> > +	}
> > +
> > +	DRM_DEV_INFO(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> > +		     default_mode.hdisplay, default_mode.vdisplay,
> > +		     drm_mode_vrefresh(&default_mode),
> > +		     mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mantix_shutdown(struct mipi_dsi_device *dsi)
> > +{
> > +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> > +	int ret;
> > +
> > +	ret = drm_panel_unprepare(&ctx->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> > +			      ret);
> > +
> > +	ret = drm_panel_disable(&ctx->panel);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to disable panel: %d\n",
> > +			      ret);
> > +}
> In shutdown we usually ignore any errors.
> 
> > +
> > +static int mantix_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct mantix *ctx = mipi_dsi_get_drvdata(dsi);
> > +	int ret;
> > +
> > +	mantix_shutdown(dsi);
> > +
> > +	ret = mipi_dsi_detach(dsi);
> > +	if (ret < 0)
> > +		DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
> > +			      ret);
> Likewise in remove we ignore any errors.
>
> 
> > +
> > +	drm_panel_remove(&ctx->panel);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mantix_of_match[] = {
> > +	{ .compatible = "mantix,mlaf057we51-x" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mantix_of_match);
> > +
> > +static struct mipi_dsi_driver mantix_driver = {
> > +	.probe	= mantix_probe,
> > +	.remove = mantix_remove,
> > +	.shutdown = mantix_shutdown,
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.of_match_table = mantix_of_match,
> > +	},
> > +};
> > +module_mipi_dsi_driver(mantix_driver);
> > +
> > +MODULE_AUTHOR("Guido Günther <agx@sigxcpu.org>");
> > +MODULE_DESCRIPTION("DRM driver for Mantix MLAF057WE51-X MIPI DSI panel");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.26.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>