Message-ID: <20141210140334.GE23558@ulmo.nvidia.com>
Date:	Wed, 10 Dec 2014 15:03:39 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Liu Ying <Ying.Liu@...escale.com>
Cc:	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	p.zabel@...gutronix.de, shawn.guo@...aro.org,
	kernel@...gutronix.de, linux@....linux.org.uk,
	mturquette@...aro.org, airlied@...ux.ie
Subject: Re: [PATCH RFC 10/15] drm: panel: Add support for Himax HX8369A MIPI
 DSI panel

On Wed, Dec 10, 2014 at 04:37:23PM +0800, Liu Ying wrote:
> This patch adds support for Himax HX8369A MIPI DSI panel.
> 
> Signed-off-by: Liu Ying <Ying.Liu@...escale.com>
> ---
>  .../devicetree/bindings/panel/himax,hx8369a.txt    |  86 +++
>  drivers/gpu/drm/panel/Kconfig                      |   6 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-hx8369a.c              | 627 +++++++++++++++++++++
>  4 files changed, 720 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/himax,hx8369a.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-hx8369a.c
> 
> diff --git a/Documentation/devicetree/bindings/panel/himax,hx8369a.txt b/Documentation/devicetree/bindings/panel/himax,hx8369a.txt
> new file mode 100644
> index 0000000..6fe251e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/himax,hx8369a.txt
> @@ -0,0 +1,86 @@
> +Himax HX8369A WVGA 16.7M color TFT single chip driver with internal GRAM
> +
> +Himax HX8369A is a WVGA resolution driving controller.
> +It is designed to provide a single chip solution that combines a source
> +driver and power supply circuits to drive a TFT dot matrix LCD with
> +480RGBx864 dots at the maximum.
> +
> +The HX8369A supports several interface modes, including MPU MIPI DBI Type
> +A/B mode, MIPI DPI/DBI Type C mode, MIPI DSI video mode, MIPI DSI command
> +mode and MDDI mode. The interface mode is selected by the external hardware
> +pins BS[3:0].
> +
> +Currently, only the MIPI DSI video mode is supported.
> +
> +Required properties:
> +  - compatible: "himax,hx8369a-dsi"
> +  - reg: the virtual channel number of a DSI peripheral
> +  - reset-gpios: a GPIO spec for the reset pin
> +  - data-lanes: the data lane number of a DSI peripheral

This is implied by the compatible already.

> +  - display-timings: timings for the connected panel as described by [1]

Also implied by the compatible value.

> +  - bs: the interface mode number described by the following table
> +        --------------------------
> +       | DBI_TYPE_A_8BIT     |  0 |
> +       | DBI_TYPE_A_9BIT     |  1 |
> +       | DBI_TYPE_A_16BIT    |  2 |
> +       | DBI_TYPE_A_18BIT    |  3 |
> +       | DBI_TYPE_B_8BIT     |  4 |
> +       | DBI_TYPE_B_9BIT     |  5 |
> +       | DBI_TYPE_B_16BIT    |  6 |
> +       | DBI_TYPE_B_18BIT    |  7 |
> +       | DSI_CMD_MODE        |  8 |
> +       | DBI_TYPE_B_24BIT    |  9 |
> +       | DSI_VIDEO_MODE      | 10 |
> +       | MDDI                | 11 |
> +       | DPI_DBI_TYPE_C_OPT1 | 12 |
> +       | DPI_DBI_TYPE_C_OPT2 | 13 |
> +       | DPI_DBI_TYPE_C_OPT3 | 14 |
> +        --------------------------

Can this not be inferred by the driver? If it's a DSI driver can't it
select between DSI_VIDEO_MODE or DSI_CMD_MODE based on its capabilities?
That is, if the panel driver can setup command mode, shouldn't it be
using command mode in that case? And use DSI_VIDEO_MODE otherwise?

> +Optional properties:
> +  - power-on-delay: delay after turning regulators on [ms]
> +  - reset-delay: delay after reset sequence [ms]

Surely these are constant for this panel?

> +  - panel-width-mm: physical panel width [mm]
> +  - panel-height-mm: physical panel height [mm]

These are also implied by compatible.

> +Example:
> +	panel@0 {
> +		compatible = "himax,hx8369a-dsi";
> +		reg = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_mipi_panel>;
> +		reset-gpios = <&gpio6 11 GPIO_ACTIVE_LOW>;
> +		reset-delay = <120>;
> +		bs2-gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>;
> +		data-lanes = <2>;
> +		panel-width-mm = <45>;
> +		panel-height-mm = <76>;
> +		bs = <10>;
> +		status = "okay";

status = "okay" is the default, so it probably shouldn't be in this
example.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 024e98e..f1a5b58 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -40,4 +40,10 @@ config DRM_PANEL_SHARP_LQ101R1SX01
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called panel-sharp-lq101r1sx01.
>  
> +config DRM_PANEL_HX8369A
> +	tristate "HX8369A panel"
> +	depends on OF
> +	select DRM_MIPI_DSI
> +	select VIDEOMODE_HELPERS
> +

This should be sorted alphabetically. I think it would also be a good
idea to use a HIMAX prefix here, just to reduce the potential for name
clashes.

> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 4b2a043..d6768ca 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> +obj-$(CONFIG_DRM_PANEL_HX8369A) += panel-hx8369a.o

Needs to be sorted alphabetically, too.

> diff --git a/drivers/gpu/drm/panel/panel-hx8369a.c b/drivers/gpu/drm/panel/panel-hx8369a.c
[...]
> +#include <drm/drmP.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
> +
> +#define SETCLUMN_ADDR	0x2a
> +#define SETPAGE_ADDR 	0x2b
> +#define SETPIXEL_FMT 	0x3a

There are standard DCS functions for these now.

> +#define WRDISBV	     	0x51
> +#define WRCTRLD	     	0x53
> +#define WRCABC	     	0x55
> +#define SETPOWER     	0xb1
> +#define SETDISP	     	0xb2
> +#define SETCYC	     	0xb4
> +#define SETVCOM	     	0xb6
> +#define SETEXTC	     	0xb9
> +#define SETMIPI	     	0xba
> +#define SETPANEL     	0xcc
> +#define SETGIP	     	0xd5
> +#define SETGAMMA     	0xe0

Watch your indentation here and above. Use tabs and spaces more
consistently.

> +static bool hx8369a_is_dsi_interface(struct hx8369a *ctx) {

Coding style: opening { goes on a line by itself.

> +	if (ctx->mpu_interface == HX8369A_DSI_VIDEO_MODE ||
> +	    ctx->mpu_interface == HX8369A_DSI_CMD_MODE)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void hx8369a_dcs_write(struct hx8369a *ctx, const void *data, size_t len)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	ssize_t ret;
> +
> +	ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
> +	if (ret < 0)
> +		dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
> +			data);
> +}

You really shouldn't have based this on the Samsung drivers. You really
should do proper error handling here. These error messages are going to
be completely cryptic and very hard for people to look up in the driver
as opposed to a simple specific error message like:

	dev_err(ctx->dev, "failed to do XYZ: %d\n", err);

which will immediately let you look up the right location without a need
to decode the hexdump and look for the correct array.

> +#define hx8369a_dcs_write_seq(ctx, seq...) \
> +({\
> +	const u8 d[] = { seq };\
> +	BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack");\
> +	hx8369a_dcs_write(ctx, d, ARRAY_SIZE(d));\
> +})
> +
> +#define hx8369a_dcs_write_seq_static(ctx, seq...) \
> +({\
> +	static const u8 d[] = { seq };\
> +	hx8369a_dcs_write(ctx, d, ARRAY_SIZE(d));\
> +})
> +
> +static void hx8369a_dsi_set_display_related_register(struct hx8369a *ctx)
> +{
> +	u8 sec_p = (ctx->res_sel << 4) | 0x03;
> +
> +	hx8369a_dcs_write_seq(ctx, SETDISP, 0x00, sec_p, 0x03,
> +		0x03, 0x70, 0x00, 0xff, 0x00, 0x00, 0x00, 0x00,
> +		0x03, 0x03, 0x00, 0x01);
> +}

This and all of the other initialization functions below completely
ignore any errors. Other than spamming the kernel log the user won't get
any indication of anything going wrong.

> +static void hx8369a_dsi_set_interface_pixel_fomat(struct hx8369a *ctx)
> +{
[...]
> +}
> +
> +static void hx8369a_dsi_set_column_address(struct hx8369a *ctx)
> +{
[...]
> +}
> +
> +static void hx8369a_dsi_set_page_address(struct hx8369a *ctx)
> +{
[...]
> +}

We have standard functions for these, please use them.

> +static void hx8369a_dsi_set_extension_command(struct hx8369a *ctx)
> +{
> +	hx8369a_dcs_write_seq_static(ctx, SETEXTC, 0xff, 0x83, 0x69);
> +}

What does this do exactly? It seems to be more than setting an extension
command.

> +static void hx8369a_dsi_set_maximum_return_packet_size(struct hx8369a *ctx,
> +						       u16 size)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	ret = mipi_dsi_set_maximum_return_packet_size(dsi, size);
> +	if (ret < 0)
> +		dev_err(ctx->dev,
> +			"error %d setting maximum return packet size to %d\n",
> +			ret, size);
> +}
> +
> +static int hx8369a_dsi_set_sequence(struct hx8369a *ctx)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	hx8369a_dsi_set_extension_command(ctx);
> +	hx8369a_dsi_set_maximum_return_packet_size(ctx, 4);
> +	hx8369a_dsi_panel_init(ctx);
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0) {
> +		dev_err(ctx->dev, "failed to exit sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret < 0) {
> +		dev_err(ctx->dev, "failed to set display on: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

> +static int hx8369a_dsi_disable(struct drm_panel *panel)
> +{
> +	return 0;
> +}
[...]
> +static int hx8369a_dsi_enable(struct drm_panel *panel)
> +{
> +	return 0;
> +}

Doesn't this usually come with a backlight attached that you want to
control here?

> +static int hx8369a_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct hx8369a *ctx = panel_to_hx8369a(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_create(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("failed to create a new display mode\n");
> +		return 0;
> +	}
> +
> +	drm_display_mode_from_videomode(&ctx->vm, mode);

Like I've said before, the driver should hardcode the mode because it is
implied by the compatible value.

> +	mode->width_mm = ctx->width_mm;
> +	mode->height_mm = ctx->height_mm;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs hx8369a_dsi_drm_funcs = {
> +	.disable = hx8369a_dsi_disable,
> +	.unprepare = hx8369a_dsi_unprepare,
> +	.prepare = hx8369a_dsi_prepare,
> +	.enable = hx8369a_dsi_enable,
> +	.get_modes = hx8369a_get_modes,
> +};

> +static int hx8369a_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct hx8369a *ctx;
> +	int ret, i;
> +	char bs[4];
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct hx8369a), GFP_KERNEL);

I'd prefer sizeof(*ctx).

> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset");
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		dev_err(dev, "cannot get reset-gpios %ld\n",
> +			PTR_ERR(ctx->reset_gpio));
> +		return PTR_ERR(ctx->reset_gpio);
> +	}
> +	ret = gpiod_direction_output(ctx->reset_gpio, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot configure reset-gpios %d\n", ret);
> +		return ret;
> +	}

I think you're supposed to combine this into something like:

	ret = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

nowadays.

> +
> +	for (i = 0; i < 4; i++) {
> +		snprintf(bs, sizeof(bs), "bs%d", i);
> +		ctx->bs_gpio[i] = devm_gpiod_get(dev, bs);
> +		if (IS_ERR(ctx->bs_gpio[i]))
> +			continue;
> +
> +		ret = gpiod_direction_output(ctx->bs_gpio[i], 1);
> +		if (ret < 0) {
> +			dev_err(dev, "cannot configure bs%d-gpio %d\n", i, ret);
> +			return ret;
> +		}

Similarly to the above:

	ret = devm_gpiod_get(dev, bs, GPIOD_OUT_HIGH);

> +static int __init hx8369a_init(void)
> +{
> +	int err;
> +
> +	err = mipi_dsi_driver_register(&hx8369a_dsi_driver);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +module_init(hx8369a_init);
> +
> +static void __exit hx8369a_exit(void)
> +{
> +	mipi_dsi_driver_unregister(&hx8369a_dsi_driver);
> +}
> +module_exit(hx8369a_exit);

Why can't this be module_mipi_dsi_driver(&hx8369a_dsi_driver)?

> +
> +MODULE_DESCRIPTION("Himax HX8369A panel driver");
> +MODULE_LICENSE("GPL v2");

Missing MODULE_AUTHOR()?

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists