Hi Harigovindan

Thanks for the follow-up. There are still a few things to improve. See below.

Sam

On Fri, Mar 06, 2020 at 04:06:27PM +0530, Harigovindan P wrote:

Add bindings for visionox rm69299 panel.

Signed-off-by: Harigovindan P harigovi@codeaurora.org

Changes in v2:

• Removed unwanted properties from description.
• Creating source files without execute permissions(Rob Herring).

Changes in v3:

• Changing txt file into yaml

Changes in v4:

• Updating license identifier.
• Moving yaml file inside panel directory.
• Removing pinctrl entries.
• Adding documentation for reset-gpios.

.../display/panel/visionox,rm69299.yaml | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml new file mode 100644 index 000000000000..93cae431207c --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml @@ -0,0 +1,85 @@ +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/visionox,rm69299.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Visionox model RM69299 Panels Device Tree Bindings.

+maintainers:

• • Harigovindan P harigovi@codeaurora.org

+description: |

• This binding is for display panels using a Visionox RM692999 panel.

+allOf:

• • $ref: panel-common.yaml#

+patternProperties:

• "^(panel|panel-dsi)@[0-9]$":
• type: object

This looks wrong. None of the other panels needs to specify a patternProperties, and neither do we need to do so for this panel.

• properties:

•  compatible:
  

•    const: visionox,rm69299-1080p-display
  

The compatible is what we use to match this binding.

•  reg:
  

•    maxItems: 1
  

•  vdda-supply:
  

•    description:
  

•      Phandle of the regulator that provides the vdda supply voltage.
  

•  vdd3p3-supply:
  

•    description:
  

•      Phandle of the regulator that provides the vdd3p3 supply voltage.
  

•  ports:
  

•    type: object
  

•    description:
  

•      A node containing DSI input & output port nodes with endpoint
  

•      definitions as documented in
  

•      Documentation/devicetree/bindings/media/video-interfaces.txt
  

•      Documentation/devicetree/bindings/graph.txt
  

I think you miss as " |" after "description:" Again, see other panel bindings.

•    properties:
  

•      port@0:
  

Please just use port: true

port is already documented in panel-common.yaml.

•        type: object
  

•        description:
  

•          DSI input port node.
  

•  reset-gpios:
  

reset-gpios is part of panel-common. So specify only: reset-gpios: true

No description, as it is in panel-common.

•    description:
  

•      a GPIO spec for the reset pin.
  

+required:

• • compatible
• • reg
• • vdda-supply
• • vdd3p3-supply
• • reset-gpios

+additionalProperties: false

+examples: +- |

• dsi@ae94000 {

No @ae94000 - this will warn with upcoming dt_binding_check improvements.

•    panel@0 {
  

•        compatible = "visionox,rm69299-1080p-display";
  

•        vdda-supply = <&src_pp1800_l8c>;
  

•        vdd3p3-supply = <&src_pp2800_l18a>;
  

•        reset-gpios = <&pm6150l_gpio 3 0>;
  

reg property is required, but missing in the example.

•        ports {
  

•                #address-cells = <1>;
  

•                #size-cells = <0>;
  

•                port@0 {
  

•                        reg = <0>;
  

•                        panel0_in: endpoint {
  

•                            remote-endpoint = <&dsi0_out>;
  

•                        };
  

•                };
  

•        };
  

•    };
  

• };

+...

-- 2.25.1

---

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

Hi,

On Fri, Mar 06, 2020 at 04:06:28PM +0530, Harigovindan P wrote:

Add support for Visionox panel driver.

Signed-off-by: Harigovindan P harigovi@codeaurora.org

Changes in v2:

• Dropping redundant space in Kconfig(Sam Ravnborg).
• Changing structure for include files(Sam Ravnborg).
• Removing backlight related code and functions(Sam Ravnborg).
• Removing repeated printing of error message(Sam Ravnborg).
• Adding drm_connector as an argument for get_modes function.

Changes in v3:

• Adding arguments for drm_panel_init to support against mainline.

Changes in v4:

• Removing error messages from regulator_set_load.
• Removing dev struct entry.
• Removing checks.
• Dropping empty comment lines.

drivers/gpu/drm/panel/Kconfig | 8 + drivers/gpu/drm/panel/Makefile | 1 + .../gpu/drm/panel/panel-visionox-rm69299.c | 322 ++++++++++++++++++ 3 files changed, 331 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index ae44ac2ec106..7b696f304a99 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -389,6 +389,14 @@ config DRM_PANEL_TRULY_NT35597_WQXGA Say Y here if you want to enable support for Truly NT35597 WQXGA Dual DSI Video Mode panel

+config DRM_PANEL_VISIONOX_RM69299

• tristate "Visionox RM69299"
• depends on OF
• depends on DRM_MIPI_DSI
• help

•  Say Y here if you want to enable support for Visionox
  

•  RM69299  DSI Video Mode panel.
  

config DRM_PANEL_XINPENG_XPP055C272 tristate "Xinpeng XPP055C272 panel driver" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 7c4d3c581fd4..9f11d067a6b2 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -41,4 +41,5 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += panel-tpo-td028ttec1.o obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o +obj-$(CONFIG_DRM_PANEL_VISIONOX_RM69299) += panel-visionox-rm69299.o obj-$(CONFIG_DRM_PANEL_XINPENG_XPP055C272) += panel-xinpeng-xpp055c272.o diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c new file mode 100644 index 000000000000..eb15dca15398 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c @@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2019, The Linux Foundation. All rights reserved.
• */

+#include <linux/delay.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>

+#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>

+struct rm69299_config {

• unsigned long width_mm;
• unsigned long height_mm;
• const char *panel_name;
• u32 num_on_cmds;

not used, remove

• const struct drm_display_mode *dm;

+};

+struct visionox_rm69299 {

• struct drm_panel panel;
• struct regulator_bulk_data supplies[2];
• struct gpio_desc *reset_gpio;
• struct mipi_dsi_device *dsi;

nit: the use of blank lines in this struct seems a bit arbitrary, just remove them?

• const struct rm69299_config *config;
• bool prepared;
• bool enabled;

+};

+static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel) +{

• return container_of(panel, struct visionox_rm69299, panel);

+}

+static int visionox_35597_power_on(struct visionox_rm69299 *ctx) +{

• int ret;
• ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
• if (ret < 0)

• return ret;
  

• /*

• * Reset sequence of visionox panel requires the panel to be
  

• * out of reset for 10ms, followed by being held in reset
  

• * for 10ms and then out again
  

• */
  

• gpiod_set_value(ctx->reset_gpio, 1);
• usleep_range(10000, 20000);
• gpiod_set_value(ctx->reset_gpio, 0);
• usleep_range(10000, 20000);
• gpiod_set_value(ctx->reset_gpio, 1);
• usleep_range(10000, 20000);
• return 0;

+}

+static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx) +{

• gpiod_set_value(ctx->reset_gpio, 0);
• return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);

+}

+static int visionox_rm69299_unprepare(struct drm_panel *panel) +{

• struct visionox_rm69299 *ctx = panel_to_ctx(panel);
• int ret;
• ctx->dsi->mode_flags = 0;
• ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
• if (ret < 0)

• DRM_DEV_ERROR(ctx->panel.dev, "set_display_off cmd failed ret = %d\n", ret);
  

• /* 120ms delay required here as per DCS spec */
• msleep(120);
• ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• 	"enter_sleep cmd failed ret = %d\n", ret);
  

• }
• ret = visionox_rm69299_power_off(ctx);
• ctx->prepared = false;
• return ret;

+}

+static int visionox_rm69299_prepare(struct drm_panel *panel) +{

• struct visionox_rm69299 *ctx = panel_to_ctx(panel);
• int ret;
• const struct rm69299_config *config;
• if (ctx->prepared)

• return 0;
  

• ret = visionox_35597_power_on(ctx);
• if (ret < 0)

• return ret;
  

• ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
• config = ctx->config;
• ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xfe, 0x00 }, 2);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• "cmd set tx 0 failed, ret = %d\n",
  

• ret);
  

The above lines should be indented, e.g. by adding a tab or aligning it after the parenthesis of DRM_DEV_ERROR(). 'ret' shouldn't be in an separate line unless the line would exceed 80 characters otherwise.

• goto power_off;
  

• }
• ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xc2, 0x08 }, 2);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• "cmd set tx 1 failed, ret = %d\n",
  

• ret);
  

fix indentation

• goto power_off;
  

• }
• ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0x35, 0x00 }, 2);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• "cmd set tx 2 failed, ret = %d\n",
  

• ret);
  

fix indentation

• goto power_off;
  

• }
• ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0x51, 0xff }, 2);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• "cmd set tx 3 failed, ret = %d\n",
  

• ret);
  

fix indentation

• goto power_off;
  

• }
• ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_EXIT_SLEEP_MODE, NULL, 0);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• 	"exit_sleep_mode cmd failed ret = %d\n",
  

• 	ret);
  

no separate line needed for 'ret'.

• goto power_off;
  

• }
• /* Per DSI spec wait 120ms after sending exit sleep DCS command */
• msleep(120);
• ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_ON, NULL, 0);
• if (ret < 0) {

• DRM_DEV_ERROR(ctx->panel.dev,
  

• 	"set_display_on cmd failed ret = %d\n", ret);
  

make the others DRM_DEV_ERROR calls look like this :)

• goto power_off;
  

• }
• /* Per DSI spec wait 120ms after sending set_display_on DCS command */
• msleep(120);
• ctx->prepared = true;
• return 0;

+power_off:

• return visionox_rm69299_power_off(ctx);

I think you want to return 'ret' here, otherwise _prepare() appears to be successful, unless _power_off() fails.

+}

+static int visionox_rm69299_get_modes(struct drm_panel *panel,

• 		       struct drm_connector *connector)
  

+{

• struct visionox_rm69299 *ctx = panel_to_ctx(panel);
• struct drm_display_mode *mode;
• const struct rm69299_config *config;
• config = ctx->config;

nit: could be done in the declaration.

• mode = drm_mode_create(connector->dev);
• if (!mode) {

• DRM_DEV_ERROR(ctx->dev,
  

• 	"failed to create a new display mode\n");
  

• return 0;
  

• }
• connector->display_info.width_mm = config->width_mm;
• connector->display_info.height_mm = config->height_mm;
• drm_mode_copy(mode, config->dm);
• mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
• drm_mode_probed_add(connector, mode);
• return 1;

+}

+static const struct drm_panel_funcs visionox_rm69299_drm_funcs = {

• .unprepare = visionox_rm69299_unprepare,
• .prepare = visionox_rm69299_prepare,
• .get_modes = visionox_rm69299_get_modes,

+};

+static const struct drm_display_mode qcom_sc7180_mtp_1080p_mode = {

The QCOM SC7180 MTP is a board that uses this panel in a certain mode (though in theory it could support multiple modes I guess). I don't thihks the board should be references here. Instead the name could be something like visionox_rm69299_1080x2248_60hz.

• .name = "1080x2248",
• .clock = 158695,
• .hdisplay = 1080,
• .hsync_start = 1080 + 26,
• .hsync_end = 1080 + 26 + 2,
• .htotal = 1080 + 26 + 2 + 36,
• .vdisplay = 2248,
• .vsync_start = 2248 + 56,
• .vsync_end = 2248 + 56 + 4,
• .vtotal = 2248 + 56 + 4 + 4,
• .vrefresh = 60,
• .flags = 0,

+};

+static const struct rm69299_config rm69299_dir = {

• .width_mm = 74,
• .height_mm = 131,
• .panel_name = "qcom_sc7180_mtp_1080p_panel",

ditto, the fact that the MTP board uses this panel is irrelevant in this driver. Actually 'panel_name' isn't used besides this assignment, just remove it from the struct.

• .dm = &qcom_sc7180_mtp_1080p_mode,

+};

+static int visionox_rm69299_probe(struct mipi_dsi_device *dsi) +{

• struct device *dev = &dsi->dev;
• struct visionox_rm69299 *ctx;
• int ret;
• ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);

nit: remove empty line between allocation and check?

• if (!ctx)

• return -ENOMEM;
  

• mipi_dsi_set_drvdata(dsi, ctx);
• ctx->supplies[0].supply = "vdda";
• ctx->supplies[1].supply = "vdd3p3";
• ret = devm_regulator_bulk_get(ctx->panel.dev, ARRAY_SIZE(ctx->supplies),

• 						ctx->supplies);
  

• if (ret < 0)

• return ret;
  

• ctx->reset_gpio = devm_gpiod_get(ctx->panel.dev, "reset", GPIOD_OUT_LOW);
• if (IS_ERR(ctx->reset_gpio)) {

• DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",
  

• 			PTR_ERR(ctx->reset_gpio));
  

The indentation looks a bit funny. Either use a tab wrt DRM_DEV_ERROR() or align with 'dev'.

• return PTR_ERR(ctx->reset_gpio);
  

• }
• drm_panel_init(&ctx->panel, dev, &visionox_rm69299_drm_funcs,

•        DRM_MODE_CONNECTOR_DSI);
  

• ctx->panel.dev = dev;
• ctx->panel.funcs = &visionox_rm69299_drm_funcs;
• drm_panel_add(&ctx->panel);
• dsi->lanes = 4;
• dsi->format = MIPI_DSI_FMT_RGB888;
• dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |

• MIPI_DSI_CLOCK_NON_CONTINUOUS;
  

• ret = mipi_dsi_attach(dsi);
• if (ret < 0) {

• DRM_DEV_ERROR(dev,
  

• 	"dsi attach failed ret = %d\n", ret);
  

• goto err_dsi_attach;
  

• }
• ret = regulator_set_load(ctx->supplies[0].consumer, 32000);
• if (ret)

• return ret;
  

need to call mipi_dsi_detach() and drm_panel_remove(), add label below.

• ret = regulator_set_load(ctx->supplies[1].consumer, 13200);
• if (ret)

• return ret;
  

ditto

• return 0;

+err_dsi_attach:

• drm_panel_remove(&ctx->panel);
• return ret;

+}

+static int visionox_rm69299_remove(struct mipi_dsi_device *dsi) +{

• struct visionox_rm69299 *ctx = mipi_dsi_get_drvdata(dsi);
• mipi_dsi_detach(ctx->dsi);
• mipi_dsi_device_unregister(ctx->dsi);
• drm_panel_remove(&ctx->panel);
• return 0;

+}

+static const struct of_device_id visionox_rm69299_of_match[] = {

• {

• .compatible = "visionox,rm69299-1080p-display",
  

• .data = &rm69299_dir,
  

• },

+}; +MODULE_DEVICE_TABLE(of, visionox_rm69299_of_match);

+static struct mipi_dsi_driver visionox_rm69299_driver = {

• .driver = {

• .name = "panel-visionox-rm69299",
  

• .of_match_table = visionox_rm69299_of_match,
  

• },
• .probe = visionox_rm69299_probe,
• .remove = visionox_rm69299_remove,

+}; +module_mipi_dsi_driver(visionox_rm69299_driver);

+MODULE_DESCRIPTION("VISIONOX RM69299 DSI Panel Driver");

nit: Visionox