From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9E175C43381 for ; Mon, 18 Mar 2019 17:59:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5CE0620863 for ; Mon, 18 Mar 2019 17:59:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=amarulasolutions.com header.i=@amarulasolutions.com header.b="prGuIM6a" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727163AbfCRR70 (ORCPT ); Mon, 18 Mar 2019 13:59:26 -0400 Received: from mail-it1-f194.google.com ([209.85.166.194]:52676 "EHLO mail-it1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726828AbfCRR7Y (ORCPT ); Mon, 18 Mar 2019 13:59:24 -0400 Received: by mail-it1-f194.google.com with SMTP id g17so21867384ita.2 for ; Mon, 18 Mar 2019 10:59:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mkC5F+mq8UN0edl2jZDGdQEyt0l3equ5V4OfGx0bko0=; b=prGuIM6a1IhuwcU45vSd+NAPJ5DL70GWJlKmp4ymCqmsbEl8cP/uwQTz4lr88o0l98 Z10ZXl9+kTU5al2B/5mOYkVlGdMphH4nuoaJtFyrCh/0G1907SKKr4om7yB9nY0NWbWy 49e7EPinVX2HPOW1yicN5fBp+XiVlB1Lf1mtM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mkC5F+mq8UN0edl2jZDGdQEyt0l3equ5V4OfGx0bko0=; b=rN8bKXBUsW3ZXJvSGE9TxKnFio6hO35XWuPajM0wbAhCKIVPNRkKIjzCOQzNEuxYsv dYRCCcVwL65H0ItnBA9Dw7u7oAtm8ujjdAzyPkgEr0U+ZcMYKDu21zVc6sq7R1p6HBoi gAnnfcJuC/qqKQ8LMoQ8YKI9NwU9IsFXvJt+Wba6EaqNNmfiGLm9eEAGesag8uQ4rrzU mG0YxexZakh3utzMDIbUAYAwY7p3QKKFkly+fPQvsMNW1PwyCzWhzKIl7E2IkaK5llNk riMi6C0FH8dtonA9HQ2ez0BHKa5xrse+QaaAzW36lwXWgLBVxgDpeUoJukXMXm6EmxNn wZiA== X-Gm-Message-State: APjAAAWnNJxmCzZweRN9gLguUsnSJ2V+avwt+Vec4HvlbkoVhL6JX1er fMZ/OBDPEUEMjpO1Gc1JOUnXrqb1HVMxC2kSuzCePeaf82w= X-Google-Smtp-Source: APXvYqx947RC1kYf/V9QRnnjcogiAp//n3KEZCDsboypYGQjwjg00owqGti7rpfv1gelQYnwdeNp4mUjgEREJ6UsIiU= X-Received: by 2002:a02:23cd:: with SMTP id u196mr11253877jau.103.1552931963150; Mon, 18 Mar 2019 10:59:23 -0700 (PDT) MIME-Version: 1.0 References: <20190315130825.9005-1-jagan@amarulasolutions.com> <20190315130825.9005-6-jagan@amarulasolutions.com> <20190315133304.vzwy3tbw27e6lum3@flea> In-Reply-To: <20190315133304.vzwy3tbw27e6lum3@flea> From: Jagan Teki Date: Mon, 18 Mar 2019 23:29:11 +0530 Message-ID: Subject: Re: [PATCH 5/6] drm/bridge: Add Chipone ICN6211 MIPI-DSI/RGB convertor bridge To: Maxime Ripard Cc: Andrzej Hajda , Laurent Pinchart , Chen-Yu Tsai , David Airlie , Daniel Vetter , Rob Herring , Mark Rutland , Michael Trimarchi , dri-devel , linux-kernel , devicetree , linux-arm-kernel , linux-sunxi , linux-amarula@amarulasolutions.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 15, 2019 at 7:03 PM Maxime Ripard wrote: > > On Fri, Mar 15, 2019 at 06:38:24PM +0530, Jagan Teki wrote: > > ICN6211 is MIPI-DSI/RGB converter bridge from chipone. > > It has a flexible configuration of MIPI DSI signal input > > and produce RGB565, RGB666, RGB888 output format. > > > > Add bridge driver for it. > > > > Signed-off-by: Jagan Teki > > --- > > MAINTAINERS | 6 + > > drivers/gpu/drm/bridge/Kconfig | 10 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/chipone-icn6211.c | 275 +++++++++++++++++++++++ > > 4 files changed, 292 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/chipone-icn6211.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 4de80222cffb..e529addd30f5 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -4897,6 +4897,12 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > > S: Maintained > > F: drivers/gpu/drm/bochs/ > > > > +DRM DRIVER FOR CHIPONE ICN6211 MIPI-DSI to RGB CONVERTOR BRIDGE > > +M: Jagan Teki > > +S: Maintained > > +F: drivers/gpu/drm/bridge/chipone-icn6211.c > > +F: Documentation/devicetree/bindings/display/bridge/chipone,icn6211.txt > > + > > DRM DRIVER FOR FARADAY TVE200 TV ENCODER > > M: Linus Walleij > > T: git git://anongit.freedesktop.org/drm/drm-misc > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index 8840f396a7b6..cd314572e4ed 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -36,6 +36,16 @@ config DRM_CDNS_DSI > > Support Cadence DPI to DSI bridge. This is an internal > > bridge and is meant to be directly embedded in a SoC. > > > > +config DRM_CHIPONE_ICN6211 > > + tristate "Chipone ICN6211 MIPI-DSI/RGB converter bridge" > > + depends on DRM && DRM_PANEL > > + depends on OF > > + select DRM_MIPI_DSI > > + help > > + ICN6211 is MIPI-DSI/RGB converter bridge from chipone. > > + It has a flexible configuration of MIPI DSI signal input > > + and produce RGB565, RGB666, RGB888 output format. > > + > > config DRM_DUMB_VGA_DAC > > tristate "Dumb VGA DAC Bridge support" > > depends on OF > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > index 4934fcf5a6f8..541fdccad10b 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -1,6 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o > > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o > > +obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o > > obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > > obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c > > new file mode 100644 > > index 000000000000..cd2f3505f845 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c > > @@ -0,0 +1,275 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2018 Amarula Solutions > > + * Author: Jagan Teki > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#define ICN6211_INIT_CMD_LEN 2 > > + > > +struct chipone { > > + struct device *dev; > > + struct drm_bridge bridge; > > + struct drm_connector connector; > > + struct drm_panel *panel; > > + > > + struct gpio_desc *reset; > > +}; > > + > > +static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct chipone, bridge); > > +} > > + > > +static inline > > +struct chipone *connector_to_chipone(struct drm_connector *connector) > > +{ > > + return container_of(connector, struct chipone, connector); > > +} > > + > > +struct icn6211_init_cmd { > > + u8 data[ICN6211_INIT_CMD_LEN]; > > +}; > > + > > +static const struct icn6211_init_cmd icn6211_init_cmds[] = { > > + { .data = { 0x7A, 0xC1 } }, > > + { .data = { 0x20, 0x20 } }, > > + { .data = { 0x21, 0xE0 } }, > > + { .data = { 0x22, 0x13 } }, > > + { .data = { 0x23, 0x28 } }, > > + { .data = { 0x24, 0x30 } }, > > + { .data = { 0x25, 0x28 } }, > > + { .data = { 0x26, 0x00 } }, > > + { .data = { 0x27, 0x0D } }, > > + { .data = { 0x28, 0x03 } }, > > + { .data = { 0x29, 0x1D } }, > > + { .data = { 0x34, 0x80 } }, > > + { .data = { 0x36, 0x28 } }, > > + { .data = { 0xB5, 0xA0 } }, > > + { .data = { 0x5C, 0xFF } }, > > + { .data = { 0x2A, 0x01 } }, > > + { .data = { 0x56, 0x92 } }, > > + { .data = { 0x6B, 0x71 } }, > > + { .data = { 0x69, 0x2B } }, > > + { .data = { 0x10, 0x40 } }, > > + { .data = { 0x11, 0x98 } }, > > + { .data = { 0xB6, 0x20 } }, > > + { .data = { 0x51, 0x20 } }, > > + { .data = { 0x09, 0x10 } }, > > +}; > > What are those commands supposed to be? Some of them at least look > pretty standard (and have proper functions): > MIPI_DCS_EXIT_INVERT_MODE, _SET_DISPLAY_ON, _SET_TEAR_OFF, etc. As I said in previous Allwinner MIPI-DSI fixes and new code support series. For this bridge we don't have programming guide or any datasheet. The initialization sequence is taken from BSP panel driver and analyzed based on other references mentioned here [1]. Some of the command sequence value are based on the timings other based on standard MIPI commands as you said. Do you need to add comments to describe the same or some other changes? > > > +static int chipone_get_modes(struct drm_connector *connector) > > +{ > > + struct chipone *icn = connector_to_chipone(connector); > > + > > + return drm_panel_get_modes(icn->panel); > > +} > > + > > +static const > > +struct drm_connector_helper_funcs chipone_connector_helper_funcs = { > > + .get_modes = chipone_get_modes, > > +}; > > + > > +static const struct drm_connector_funcs chipone_connector_funcs = { > > + .fill_modes = drm_helper_probe_single_connector_modes, > > + .destroy = drm_connector_cleanup, > > + .reset = drm_atomic_helper_connector_reset, > > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > +}; > > + > > +static void chipone_disable(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + int ret = drm_panel_disable(bridge_to_chipone(bridge)->panel); > > + > > + if (ret < 0) > > + DRM_DEV_ERROR(icn->dev, "error disabling panel (%d)\n", ret); > > +} > > + > > +static void chipone_post_disable(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + int ret; > > + > > + ret = drm_panel_unprepare(icn->panel); > > + if (ret < 0) > > + DRM_DEV_ERROR(icn->dev, "error unpreparing panel (%d)\n", ret); > > + > > + msleep(50); > > + > > + gpiod_set_value(icn->reset, 0); > > +} > > + > > +static void chipone_pre_enable(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev); > > + unsigned int i; > > + int ret; > > + > > + gpiod_set_value(icn->reset, 0); > > + msleep(20); > > + > > + gpiod_set_value(icn->reset, 1); > > + msleep(50); > > + > > + for (i = 0; i < ARRAY_SIZE(icn6211_init_cmds); i++) { > > + const struct icn6211_init_cmd *cmd = &icn6211_init_cmds[i]; > > + > > + ret = mipi_dsi_generic_write(dsi, cmd->data, > > + ICN6211_INIT_CMD_LEN); > > + if (ret < 0) { > > + DRM_DEV_ERROR(icn->dev, > > + "failed to write cmd %d: %d\n", i, ret); > > + return; > > + } > > + } > > + > > + ret = drm_panel_prepare(icn->panel); > > + if (ret < 0) > > + DRM_DEV_ERROR(icn->dev, "error preparing panel (%d)\n", ret); > > +} > > + > > +static void chipone_enable(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + int ret = drm_panel_enable(icn->panel); > > + > > + if (ret < 0) > > + DRM_DEV_ERROR(icn->dev, "error enabling panel (%d)\n", ret); > > +} > > + > > +static int chipone_attach(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + struct drm_device *drm = bridge->dev; > > + int ret; > > + > > + icn->connector.polled = DRM_CONNECTOR_POLL_HPD; > > + ret = drm_connector_init(drm, &icn->connector, > > + &chipone_connector_funcs, > > + DRM_MODE_CONNECTOR_Unknown); > > It should be a DRM_MODE_CONNECTOR_DPI connector. Since the output connector is RGB, I have used 'Unknown' since we follow similar connector type in sun4i_rgb.c So these RGB comes under parallel display interface we can have to use this DPI connector is it? > > > + if (ret) { > > + DRM_ERROR("Failed to initialize connector\n"); > > + return ret; > > + } > > + > > + drm_connector_helper_add(&icn->connector, > > + &chipone_connector_helper_funcs); > > + drm_connector_attach_encoder(&icn->connector, bridge->encoder); > > + drm_panel_attach(icn->panel, &icn->connector); > > + icn->connector.funcs->reset(&icn->connector); > > + drm_fb_helper_add_one_connector(drm->fb_helper, &icn->connector); > > What do you need that for? I took the reference code, sorry it is accidentally present will wind-up in next version. > > > + drm_connector_register(&icn->connector); > > + > > + return 0; > > +} > > + > > +static void chipone_detach(struct drm_bridge *bridge) > > +{ > > + struct chipone *icn = bridge_to_chipone(bridge); > > + struct drm_device *drm = bridge->dev; > > + > > + drm_connector_unregister(&icn->connector); > > + drm_fb_helper_remove_one_connector(drm->fb_helper, &icn->connector); > > + drm_panel_detach(icn->panel); > > + icn->panel = NULL; > > + drm_connector_put(&icn->connector); > > +} > > + > > +static const struct drm_bridge_funcs chipone_bridge_funcs = { > > + .disable = chipone_disable, > > + .post_disable = chipone_post_disable, > > + .enable = chipone_enable, > > + .pre_enable = chipone_pre_enable, > > + .attach = chipone_attach, > > + .detach = chipone_detach, > > +}; > > + > > +static int chipone_probe(struct mipi_dsi_device *dsi) > > +{ > > + struct device *dev = &dsi->dev; > > + struct chipone *icn; > > + int ret; > > + > > + icn = devm_kzalloc(dev, sizeof(struct chipone), GFP_KERNEL); > > + if (!icn) > > + return -ENOMEM; > > + > > + mipi_dsi_set_drvdata(dsi, icn); > > + > > + icn->dev = dev; > > + dsi->lanes = 4; > > + dsi->format = MIPI_DSI_FMT_RGB888; > > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE; > > + > > + icn->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(icn->reset)) { > > + DRM_DEV_ERROR(dev, "no reset GPIO pin provided\n"); > > + return PTR_ERR(icn->reset); > > + } > > + > > + ret = drm_of_find_panel_or_bridge(icn->dev->of_node, 1, 0, > > + &icn->panel, NULL); > > + if (ret && ret != -EPROBE_DEFER) { > > + DRM_DEV_ERROR(dev, "failed to find panel (ret = %d)\n", ret); > > + return ret; > > + } > > + > > + icn->bridge.funcs = &chipone_bridge_funcs; > > + icn->bridge.of_node = dev->of_node; > > + > > + drm_bridge_add(&icn->bridge); > > + > > + ret = mipi_dsi_attach(dsi); > > + if (ret < 0) { > > + drm_bridge_remove(&icn->bridge); > > + DRM_DEV_ERROR(dev, "failed to attach dsi (ret = %d)\n", ret); > > + } > > + > > + return ret; > > +} > > + > > +static int chipone_remove(struct mipi_dsi_device *dsi) > > +{ > > + struct chipone *icn = mipi_dsi_get_drvdata(dsi); > > + > > + mipi_dsi_detach(dsi); > > + drm_bridge_remove(&icn->bridge); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id chipone_of_match[] = { > > + { .compatible = "bananapi,icn6211" }, > > This should have your generic compatible listed Any idea why? usually it can be know attached device associated IC chip isn't it? I can see this similar approach in some panel driver which I commented in 4/5 https://patchwork.freedesktop.org/patch/292416/ [1] https://patchwork.kernel.org/patch/10617921/