On Tue, Jan 27, 2015 at 10:48:32AM +0900, Hyungwon Hwang wrote:
> From: Donghwa Lee <dh09.lee@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Hyungwon Hwang <human.hwang@xxxxxxxxxxx>
> Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> Cc: Sangbae Lee <sangbae90.lee@xxxxxxxxxxx>
> ---
> 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:
pgpMmZgW0vqVR.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
