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