Re: [PATCH v3] drm/panel: add s6e3ha2 AMOLED panel driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9077 bytes --]

On Tue, Jan 27, 2015 at 10:48:32AM +0900, Hyungwon Hwang wrote:
> From: Donghwa Lee <dh09.lee@samsung.com>
> 
> This patch adds MIPI-DSI based S6E3HA2 panel driver. This panel has
> 1440x2560 resolution in 5.7-inch physical panel.
> 
> Signed-off-by: Donghwa Lee <dh09.lee@samsung.com>
> Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Sangbae Lee <sangbae90.lee@samsung.com>
> ---
> Changes for v2:
> - Fix errata in documentation and source code comments
> Changes for v3:
> - Remove the term LCD to clarify the sort of this panel
> - Rename lcd-en-gpios to panel-en-gpios to clarify the sort of this panel
> - Fix errata in documentation and source code comments
>  .../devicetree/bindings/panel/samsung,s6e3ha2.txt  |  49 ++
>  drivers/gpu/drm/panel/Kconfig                      |   6 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-s6e3ha2.c              | 513 +++++++++++++++++++++
>  4 files changed, 569 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-s6e3ha2.c
> 
> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt
> new file mode 100644
> index 0000000..5210926
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3ha2.txt
> @@ -0,0 +1,49 @@
> +Samsung S6E3HA2 5.7" 1440x2560 AMOLED panel
> +
> +Required properties:
> +  - compatible: "samsung,s6e3ha2"
> +  - reg: the virtual channel number of a DSI peripheral
> +  - vdd3-supply: core voltage supply
> +  - vci-supply: voltage supply for analog circuits
> +  - reset-gpios: a GPIO spec for the reset pin
> +  - panel-en-gpios: a GPIO spec for the panel enable pin

Why not "enable-gpios"? That would be much more in line with the
reset-gpios property.

> +  - te-gpios: a GPIO spec for the tearing effect synchronization signal gpio pin

s/gpio/GPIO/

> +
> +Optional properties:
> +  - display-timings: timings for the connected panel as described by [1]
> +  - panel-width-mm: physical panel width [mm]
> +  - panel-height-mm: physical panel height [mm]

If these are optional, what happens if they aren't there? The panel is
not likely to work in that case.

Also I think these should be hard-coded in the driver, because they are
implied by the "compatible" string.

> +The device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in [2]. This
> +node should describe panel's video bus.

The example doesn't show how to use that, why is it necessary?

> diff --git a/drivers/gpu/drm/panel/panel-s6e3ha2.c b/drivers/gpu/drm/panel/panel-s6e3ha2.c
[...]
> +struct s6e3ha2 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +
> +	struct regulator_bulk_data supplies[2];
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *panel_en_gpio;
> +	struct videomode vm;
> +	u32 width_mm;
> +	u32 height_mm;
> +
> +	/* This field is tested by functions directly accessing DSI bus before
> +	 * transfer, transfer is skipped if it is set. In case of transfer
> +	 * failure or unexpected response the field is set to error value.
> +	 * Such construct allows to eliminate many checks in higher level
> +	 * functions.
> +	 */
> +	int error;

I hate myself for not NAK'ing the first patch that introduced this idiom
stronger. I think it's a really bad concept and you're not doing
yourself any favours by using it.

> +static void s6e3ha2_te_start_setting(struct s6e3ha2 *ctx)
> +{
> +	s6e3ha2_dcs_write_seq_static(ctx, 0xb9, 0x10, 0x09, 0xff, 0x00, 0x09);
> +}
> +
> +

Gratuituous blank line.

> +static void s6e3ha2_panel_init(struct s6e3ha2 *ctx)
> +{
> +	s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);

We have a standard function for this, please use it.

> +	/* common setting */
> +	s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON);

Same here.

> +	s6e3ha2_test_key_off_f0(ctx);
> +	s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);

And here.

> +static int s6e3ha2_unprepare(struct drm_panel *panel)
> +{
> +	struct s6e3ha2 *ctx = panel_to_s6e3ha2(panel);
> +
> +	s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
> +	s6e3ha2_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);

More standard functions.

> +	msleep(40);
> +
> +	s6e3ha2_clear_error(ctx);
> +
> +	return s6e3ha2_power_off(ctx);
> +}
> +
> +static int s6e3ha2_power_on(struct s6e3ha2 *ctx)
> +{
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(25);
> +
> +	gpiod_set_value(ctx->panel_en_gpio, 0);
> +	usleep_range(5000, 6000);
> +	gpiod_set_value(ctx->panel_en_gpio, 1);
> +
> +	gpiod_set_value(ctx->reset_gpio, 1);
> +	usleep_range(5000, 6000);
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +	usleep_range(5000, 6000);
> +	gpiod_set_value(ctx->reset_gpio, 1);

I would expect the value after power on to be 0 for reset. Perhaps you
need GPIO_ACTIVE_LOW in your device tree? Also why do you toggle thrice?
I would assume that putting the peripheral into reset and taking it out
again would be enough.

> +static int s6e3ha2_prepare(struct drm_panel *panel)
> +{
> +	struct s6e3ha2 *ctx = panel_to_s6e3ha2(panel);
> +	int ret;
> +
> +	ret = s6e3ha2_power_on(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	s6e3ha2_panel_init(ctx);
> +	if (ctx->error < 0)

This is one of the reasons why ctx->error is a bad idea. It's completely
unintuitive to check ctx->error here because nobody's actually setting
it in this context.

> +		s6e3ha2_unprepare(panel);

This is somewhat asymmetric. I would expect the s6e3ha2_panel_init() to
undo what it did on failure, so that you would only have to call
s6e3ha2_power_off() here and undo what you did in *this* function.

> +static int s6e3ha2_enable(struct drm_panel *panel)
> +{
> +	return 0;
> +}

This is really where you're supposed to turn on the backlight or
similar. Where does that happen?

> +static int s6e3ha2_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct s6e3ha2 *ctx = panel_to_s6e3ha2(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);
> +	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;
> +}

Like I said before, the mode is implied by the compatible value, so no
need to parse it from device tree.

> +static int s6e3ha2_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct s6e3ha2 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct s6e3ha2), GFP_KERNEL);

sizeof(*ctx) is much shorter.

> +	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, 1);

For consistency the above two lines should be separated by a blank line.

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

Same here.

> +	if (ret < 0) {
> +		dev_err(dev, "cannot configure panel-en-gpios %d\n", ret);
> +		return ret;
> +	}

You seem to be turning on the panel here. That's wrong because you're
supposed to wait for the display driver to tell you to turn it on via
->prepare() and ->enable().

> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = dev;
> +	ctx->panel.funcs = &s6e3ha2_drm_funcs;

I don't see a use for the _drm in here.

> +static struct of_device_id s6e3ha2_of_match[] = {

static const, please.

> +	{ .compatible = "samsung,s6e3ha2" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, s6e3ha2_of_match);
> +
> +static struct mipi_dsi_driver s6e3ha2_driver = {
> +	.probe = s6e3ha2_probe,
> +	.remove = s6e3ha2_remove,
> +	.driver = {
> +		.name = "panel_s6e3ha2",

Please use a - instead of an _ here, for consistency with other drivers.

> +		.owner = THIS_MODULE,

No need for this anymore.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel