Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Sat, Jan 30, 2021 at 10:10 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> Add driver for TI SN65DSI83 Single-Channel DSI to Single-Link LVDS bridge.
> The driver operates the chip over I2C bus. Currently the LVDS clock are
> always derived from DSI clock lane, which is the usual mode of operation,
> support for clock from external oscillator is not implemented, but it is
> easy to add if ever needed. Only RGB888 pixel format is implemented, the
> LVDS666 is not supported, but could be added if needed.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Philippe Schenker <philippe.schenker@xxxxxxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Cc: Valentin Raevsky <valentin@xxxxxxxxxxxxxx>
> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> ---
>  drivers/gpu/drm/bridge/Kconfig        |  10 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 535 ++++++++++++++++++++++++++
>  3 files changed, 546 insertions(+)

I shouldn't be the main reviewer because, as I've said elsewhere, I'm
a DRM noob and know almost nothing about LVDS or MIPI.  :-P  ...but
I've hacked a bunch on the sn65dsi86 driver and lots of others have
helped me with reviews, so I'll give it a shot...


>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi83.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index cc401786962a..5e4f674e3fdb 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -234,6 +234,16 @@ config DRM_TI_TFP410
>         help
>           Texas Instruments TFP410 DVI/HDMI Transmitter driver
>
> +config DRM_TI_SN65DSI83
> +       tristate "TI SN65DSI83 DSI to LVDS bridge"
> +       depends on OF
> +       select DRM_KMS_HELPER
> +       select REGMAP_I2C
> +       select DRM_PANEL
> +       select DRM_MIPI_DSI
> +       help
> +         Texas Instruments SN65DSI83 DSI to LVDS Bridge driver
> +
>  config DRM_TI_SN65DSI86
>         tristate "TI SN65DSI86 DSI to eDP bridge"
>         depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 21ea8fe09992..e11415db0a31 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> +obj-$(CONFIG_DRM_TI_SN65DSI83) += ti-sn65dsi83.o
>  obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>  obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> new file mode 100644
> index 000000000000..79c023ba3dce
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -0,0 +1,535 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Marek Vasut <marex@xxxxxxx>
> + *
> + * Based on previous work of:
> + * Valentin Raevsky <valentin@xxxxxxxxxxxxxx>
> + * Philippe Schenker <philippe.schenker@xxxxxxxxxxx>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of_graph.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +/* ID registers */
> +#define REG_ID(n)                              (0x00 + (n))
> +/* Reset and clock registers */
> +#define REG_RC_RESET                           0x09
> +#define  REG_RC_RESET_SOFT_RESET               BIT(0)
> +#define REG_RC_LVDS_PLL                                0x0a
> +#define  REG_RC_LVDS_PLL_PLL_EN_STAT           BIT(7)
> +#define  REG_RC_LVDS_PLL_LVDS_CLK_RANGE(n)     (((n) & 0x7) << 1)
> +#define  REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY       BIT(0)
> +#define REG_RC_DSI_CLK                         0x0b
> +#define  REG_RC_DSI_CLK_DSI_CLK_DIVIDER(n)     (((n) & 0x1f) << 3)
> +#define  REG_RC_DSI_CLK_REFCLK_MULTIPLIER(n)   ((n) & 0x3)
> +#define REG_RC_PLL_EN                          0x0d
> +#define  REG_RC_PLL_EN_PLL_EN                  BIT(0)
> +/* DSI registers */
> +#define REG_DSI_LANE                           0x10
> +#define  REG_DSI_LANE_RESERVED                 BIT(5)
> +#define  REG_DSI_LANE_CHA_DSI_LANES(n)         (((n) & 0x3) << 3)
> +#define  REG_DSI_LANE_SOT_ERR_TOL_DIS          BIT(0)
> +#define REG_DSI_EQ                             0x11
> +#define  REG_DSI_EQ_CHA_DSI_DATA_EQ(n)         (((n) & 0x3) << 6)
> +#define  REG_DSI_EQ_CHA_DSI_CLK_EQ(n)          (((n) & 0x3) << 2)
> +#define REG_DSI_CLK                            0x12
> +#define  REG_DSI_CLK_CHA_DSI_CLK_RANGE(n)      ((n) & 0xff)
> +/* LVDS registers */
> +#define REG_LVDS_FMT                           0x18
> +#define  REG_LVDS_FMT_DE_NEG_POLARITY          BIT(7)
> +#define  REG_LVDS_FMT_HS_NEG_POLARITY          BIT(6)
> +#define  REG_LVDS_FMT_VS_NEG_POLARITY          BIT(5)
> +#define  REG_LVDS_FMT_RESERVED                 BIT(4)  /* Set to 1 */
> +#define  REG_LVDS_FMT_CHA_24BPP_MODE           BIT(3)
> +#define  REG_LVDS_FMT_CHA_24BPP_FORMAT1                BIT(1)
> +#define REG_LVDS_VCOM                          0x19
> +#define  REG_LVDS_VCOM_CHA_LVDS_VOCM           BIT(6)
> +#define  REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(n)   (((n) & 0x3) << 2)
> +#define REG_LVDS_LANE                          0x1a
> +#define  REG_LVDS_LANE_CHA_REVERSE_LVDS                BIT(5)
> +#define  REG_LVDS_LANE_CHA_LVDS_TERM           BIT(1)
> +#define REG_LVDS_CM                            0x1b
> +#define  REG_LVDS_CM_CHA_LVDS_CM_ADJUST(n)     ((n) & 0x3)
> +/* Video registers */
> +#define REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW     0x20
> +#define REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH    0x21
> +#define REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW  0x24
> +#define REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH 0x25
> +#define REG_VID_CHA_SYNC_DELAY_LOW             0x28
> +#define REG_VID_CHA_SYNC_DELAY_HIGH            0x29
> +#define REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW      0x2c
> +#define REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH     0x2d
> +#define REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW      0x30
> +#define REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH     0x31
> +#define REG_VID_CHA_HORIZONTAL_BACK_PORCH      0x34
> +#define REG_VID_CHA_VERTICAL_BACK_PORCH                0x36
> +#define REG_VID_CHA_HORIZONTAL_FRONT_PORCH     0x38
> +#define REG_VID_CHA_VERTICAL_FRONT_PORCH       0x3a
> +#define REG_VID_CHA_TEST_PATTERN               0x3c
> +/* IRQ registers */
> +#define REG_IRQ_GLOBAL                         0xe0
> +#define  REG_IRQ_GLOBAL_IRQ_EN                 BIT(0)
> +#define REG_IRQ_EN                             0xe1
> +#define  REG_IRQ_EN_CHA_SYNCH_ERR_EN           BIT(7)
> +#define  REG_IRQ_EN_CHA_CRC_ERR_EN             BIT(6)
> +#define  REG_IRQ_EN_CHA_UNC_ECC_ERR_EN         BIT(5)
> +#define  REG_IRQ_EN_CHA_COR_ECC_ERR_EN         BIT(4)
> +#define  REG_IRQ_EN_CHA_LLP_ERR_EN             BIT(3)
> +#define  REG_IRQ_EN_CHA_SOT_BIT_ERR_EN         BIT(2)
> +#define  REG_IRQ_EN_CHA_PLL_UNLOCK_EN          BIT(0)
> +#define REG_IRQ_STAT                           0xe5
> +#define  REG_IRQ_STAT_CHA_SYNCH_ERR            BIT(7)
> +#define  REG_IRQ_STAT_CHA_CRC_ERR              BIT(6)
> +#define  REG_IRQ_STAT_CHA_UNC_ECC_ERR          BIT(5)
> +#define  REG_IRQ_STAT_CHA_COR_ECC_ERR          BIT(4)
> +#define  REG_IRQ_STAT_CHA_LLP_ERR              BIT(3)
> +#define  REG_IRQ_STAT_CHA_SOT_BIT_ERR          BIT(2)
> +#define  REG_IRQ_STAT_CHA_PLL_UNLOCK           BIT(0)
> +
> +struct sn65dsi83 {
> +       struct drm_bridge               bridge;
> +       struct drm_display_mode         mode;
> +       struct device                   *dev;
> +       struct regmap                   *regmap;
> +       struct device_node              *host_node;
> +       struct mipi_dsi_device          *dsi;
> +       struct drm_bridge               *panel_bridge;
> +       struct gpio_desc                *enable_gpio;
> +       int                             dsi_lanes;
> +};
> +
> +static const struct regmap_range sn65dsi83_readable_ranges[] = {
> +       regmap_reg_range(REG_ID(0), REG_ID(8)),
> +       regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_DSI_CLK),
> +       regmap_reg_range(REG_RC_PLL_EN, REG_RC_PLL_EN),
> +       regmap_reg_range(REG_DSI_LANE, REG_DSI_CLK),
> +       regmap_reg_range(REG_LVDS_FMT, REG_LVDS_CM),
> +       regmap_reg_range(REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> +                        REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> +                        REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH),
> +       regmap_reg_range(REG_VID_CHA_SYNC_DELAY_LOW,
> +                        REG_VID_CHA_SYNC_DELAY_HIGH),
> +       regmap_reg_range(REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> +                        REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> +                        REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> +                        REG_VID_CHA_HORIZONTAL_BACK_PORCH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_BACK_PORCH,
> +                        REG_VID_CHA_VERTICAL_BACK_PORCH),
> +       regmap_reg_range(REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> +                        REG_VID_CHA_HORIZONTAL_FRONT_PORCH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_FRONT_PORCH,
> +                        REG_VID_CHA_VERTICAL_FRONT_PORCH),
> +       regmap_reg_range(REG_VID_CHA_TEST_PATTERN, REG_VID_CHA_TEST_PATTERN),
> +       regmap_reg_range(REG_IRQ_GLOBAL, REG_IRQ_EN),
> +       regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> +};
> +
> +static const struct regmap_access_table sn65dsi83_readable_table = {
> +       .yes_ranges = sn65dsi83_readable_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(sn65dsi83_readable_ranges),
> +};
> +
> +static const struct regmap_range sn65dsi83_writeable_ranges[] = {
> +       regmap_reg_range(REG_RC_RESET, REG_RC_DSI_CLK),
> +       regmap_reg_range(REG_RC_PLL_EN, REG_RC_PLL_EN),
> +       regmap_reg_range(REG_DSI_LANE, REG_DSI_CLK),
> +       regmap_reg_range(REG_LVDS_FMT, REG_LVDS_CM),
> +       regmap_reg_range(REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> +                        REG_VID_CHA_ACTIVE_LINE_LENGTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> +                        REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH),
> +       regmap_reg_range(REG_VID_CHA_SYNC_DELAY_LOW,
> +                        REG_VID_CHA_SYNC_DELAY_HIGH),
> +       regmap_reg_range(REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> +                        REG_VID_CHA_HSYNC_PULSE_WIDTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> +                        REG_VID_CHA_VSYNC_PULSE_WIDTH_HIGH),
> +       regmap_reg_range(REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> +                        REG_VID_CHA_HORIZONTAL_BACK_PORCH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_BACK_PORCH,
> +                        REG_VID_CHA_VERTICAL_BACK_PORCH),
> +       regmap_reg_range(REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> +                        REG_VID_CHA_HORIZONTAL_FRONT_PORCH),
> +       regmap_reg_range(REG_VID_CHA_VERTICAL_FRONT_PORCH,
> +                        REG_VID_CHA_VERTICAL_FRONT_PORCH),
> +       regmap_reg_range(REG_VID_CHA_TEST_PATTERN, REG_VID_CHA_TEST_PATTERN),
> +       regmap_reg_range(REG_IRQ_GLOBAL, REG_IRQ_EN),
> +       regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> +};
> +
> +static const struct regmap_access_table sn65dsi83_writeable_table = {
> +       .yes_ranges = sn65dsi83_writeable_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(sn65dsi83_writeable_ranges),
> +};
> +
> +static const struct regmap_range sn65dsi83_volatile_ranges[] = {
> +       regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_LVDS_PLL),

Why is REG_RC_LVDS_PLL volatile?

> +       regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),

Do you need to list REG_RC_RESET as volatile?  Specifically you need
to make sure it's not cached...


> +};
> +
> +static const struct regmap_access_table sn65dsi83_volatile_table = {
> +       .yes_ranges = sn65dsi83_volatile_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(sn65dsi83_volatile_ranges),
> +};
> +
> +static const struct regmap_config sn65dsi83_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .rd_table = &sn65dsi83_readable_table,
> +       .wr_table = &sn65dsi83_writeable_table,
> +       .volatile_table = &sn65dsi83_volatile_table,
> +       .cache_type = REGCACHE_RBTREE,
> +       .max_register = REG_IRQ_STAT,
> +};

I'm curious how much the "readable" and "writable" sections get you.
In theory only the "volatile" should really matter, right?


> +
> +static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge)
> +{
> +       return container_of(bridge, struct sn65dsi83, bridge);
> +}
> +
> +static int sn65dsi83_attach(struct drm_bridge *bridge,
> +                           enum drm_bridge_attach_flags flags)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +       struct device *dev = ctx->dev;
> +       struct mipi_dsi_device *dsi;
> +       struct mipi_dsi_host *host;
> +       int ret = 0;
> +
> +       const struct mipi_dsi_device_info info = {
> +               .type = "sn65dsi83",
> +               .channel = 0,
> +               .node = NULL,
> +       };
> +
> +       host = of_find_mipi_dsi_host_by_node(ctx->host_node);
> +       if (!host) {
> +               dev_err(dev, "failed to find dsi host\n");

I notice that the sn65dsi86 driver usually uses DRM_ERROR instead of dev_err().


> +               return -EPROBE_DEFER;
> +       }
> +
> +       dsi = mipi_dsi_device_register_full(host, &info);
> +       if (IS_ERR(dsi)) {
> +               ret = PTR_ERR(dsi);
> +               dev_err(dev, "failed to create dsi device, ret=%i\n", ret);
> +               goto err_dsi_device;

Why not just "return ret" and get rid of the "err_dsi_device" label?


> +       }
> +
> +       ctx->dsi = dsi;
> +
> +       dsi->lanes = ctx->dsi_lanes;
> +       dsi->format = MIPI_DSI_FMT_RGB888;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
> +
> +       ret = mipi_dsi_attach(dsi);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to attach dsi to host\n");
> +               goto err_dsi_attach;
> +       }
> +
> +       return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
> +                                &ctx->bridge, flags);
> +
> +err_dsi_attach:
> +       mipi_dsi_device_unregister(dsi);
> +err_dsi_device:
> +       return ret;
> +}
> +
> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> +       /*
> +        * Reset the chip, pull EN line low for t_reset=10ms,
> +        * then high for t_en=1ms.
> +        */
> +       gpiod_set_value(ctx->enable_gpio, 0);

Why not use the "cansleep" version to give some flexibility?


> +       usleep_range(10000, 11000);

It seems like it would be worth it to read the enable_gpio first?  If
it was already 0 maybe you can skip the 10 ms delay?  I would imagine
that most of the time the bridge would already be disabled to start?


> +       gpiod_set_value(ctx->enable_gpio, 1);
> +       usleep_range(1000, 1100);

This fully resets the chip, right?  So you need to invalidate your regmap cache?


> +}
> +
> +static u8 sn65dsi83_get_lvds_range(struct sn65dsi83 *ctx)
> +{
> +       /*
> +        * The encoding of the LVDS_CLK_RANGE is as follows:
> +        * 000 - 25 MHz <= LVDS_CLK < 37.5 MHz
> +        * 001 - 37.5 MHz <= LVDS_CLK < 62.5 MHz
> +        * 010 - 62.5 MHz <= LVDS_CLK < 87.5 MHz
> +        * 011 - 87.5 MHz <= LVDS_CLK < 112.5 MHz
> +        * 100 - 112.5 MHz <= LVDS_CLK < 137.5 MHz
> +        * 101 - 137.5 MHz <= LVDS_CLK <= 154 MHz
> +        * which is a range of 12.5MHz..162.5MHz in 50MHz steps, except that
> +        * the ends of the ranges are clamped to the supported range. Since
> +        * sn65dsi83_mode_valid() already filters the valid modes and limits
> +        * the clock to 25..154 MHz, the range calculation can be simplified
> +        * as follows:
> +        */
> +       return (ctx->mode.clock - 12500) / 25000;
> +}
> +
> +static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx)
> +{
> +       /*
> +        * The encoding of the CHA_DSI_CLK_RANGE is as follows:
> +        * 0x00 through 0x07 - Reserved
> +        * 0x08 - 40 <= DSI_CLK < 45 MHz
> +        * 0x09 - 45 <= DSI_CLK < 50 MHz
> +        * ...
> +        * 0x63 - 495 <= DSI_CLK < 500 MHz
> +        * 0x64 - 500 MHz
> +        * 0x65 through 0xFF - Reserved
> +        * which is DSI clock in 5 MHz steps, clamped to 40..500 MHz.
> +        * The DSI clock are calculated as:
> +        *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
> +        * the 2 is there because the bus is DDR.
> +        */
> +       return clamp((unsigned int)ctx->mode.clock *
> +                    mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> +                    ctx->dsi_lanes / 2, 40000U, 500000U) / 5000U;
> +}
> +
> +static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> +{
> +       /* The divider is (DSI_CLK / LVDS_CLK) - 1, which really is: */
> +       return (mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> +               ctx->dsi_lanes / 2) - 1;
> +}
> +
> +static void sn65dsi83_enable(struct drm_bridge *bridge)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +       u16 val;
> +
> +       /* Clear reset, disable PLL */
> +       regmap_write(ctx->regmap, REG_RC_RESET, 0x00);

I don't think you need to clear reset, do you?  The doc says "This bit
automatically clears when set to 1 and returns 0s when read."  Maybe
was needed because you forgot to list this register as volatile?


> +       regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);

Probably you don't need this?  It's the default and in pre-enable you
just reset the chip.  Maybe it was needed since you don't flush the
cache in pre-enable?


> +       /* Reference clock derived from DSI link clock. */
> +       regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
> +               REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
> +               REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
> +       regmap_write(ctx->regmap, REG_DSI_CLK,
> +               REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
> +       regmap_write(ctx->regmap, REG_RC_DSI_CLK,
> +               REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
> +
> +       /* Set number of DSI lanes, keep reserved bits. */
> +       regmap_write(ctx->regmap, REG_DSI_LANE,
> +               REG_DSI_LANE_RESERVED |
> +               REG_DSI_LANE_CHA_DSI_LANES(~(ctx->dsi_lanes - 1)));
> +       /* No equalization. */
> +       regmap_write(ctx->regmap, REG_DSI_EQ, 0x00);
> +
> +       /* RGB888 is the only format supported so far. */
> +       regmap_write(ctx->regmap, REG_LVDS_FMT,
> +               (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC ?
> +                REG_LVDS_FMT_HS_NEG_POLARITY : 0) |
> +               (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC ?
> +                REG_LVDS_FMT_VS_NEG_POLARITY : 0) |
> +               REG_LVDS_FMT_RESERVED |
> +               REG_LVDS_FMT_CHA_24BPP_MODE);
> +       regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00);
> +       regmap_write(ctx->regmap, REG_LVDS_LANE, REG_LVDS_LANE_CHA_LVDS_TERM);
> +       regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
> +
> +       regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
> +                         &ctx->mode.hdisplay, 2);
> +       regmap_bulk_write(ctx->regmap, REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
> +                         &ctx->mode.vdisplay, 2);
> +       val = 32 + 1;   /* 32 + 1 pixel clock to ensure proper operation */
> +       regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &val, 2);
> +       val = ctx->mode.hsync_end - ctx->mode.hsync_start;
> +       regmap_bulk_write(ctx->regmap, REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW,
> +                         &val, 2);
> +       val = ctx->mode.vsync_end - ctx->mode.vsync_start;
> +       regmap_bulk_write(ctx->regmap, REG_VID_CHA_VSYNC_PULSE_WIDTH_LOW,
> +                         &val, 2);
> +       regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_BACK_PORCH,
> +                    ctx->mode.htotal - ctx->mode.hsync_end);
> +       regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_BACK_PORCH,
> +                    ctx->mode.vtotal - ctx->mode.vsync_end);
> +       regmap_write(ctx->regmap, REG_VID_CHA_HORIZONTAL_FRONT_PORCH,
> +                    ctx->mode.hsync_start - ctx->mode.hdisplay);
> +       regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
> +                    ctx->mode.vsync_start - ctx->mode.vdisplay);
> +       regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
> +
> +       /* Enable PLL */
> +       regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);

After you turn on the PLL, I think you should be waiting for
"PLL_EN_STAT" to show that the PLL is actually enabled and then
delaying 3ms, right?


> +       /* Trigger reset after CSR register update. */
> +       regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
> +}
> +
> +static void sn65dsi83_disable(struct drm_bridge *bridge)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> +       /* Clear reset, disable PLL */
> +       regmap_write(ctx->regmap, REG_RC_RESET, 0x00);

I don't think you need this--it self-clears.


> +       regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> +}
> +
> +static void sn65dsi83_post_disable(struct drm_bridge *bridge)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> +       /* Put the chip in reset, pull EN line low. */
> +       gpiod_set_value(ctx->enable_gpio, 0);
> +}
> +
> +static enum drm_mode_status
> +sn65dsi83_mode_valid(struct drm_bridge *bridge,
> +                    const struct drm_display_info *info,
> +                    const struct drm_display_mode *mode)
> +{
> +       /* LVDS output clock range 25..154 MHz */
> +       if (mode->clock < 25000)
> +               return MODE_CLOCK_LOW;
> +       if (mode->clock > 154000)
> +               return MODE_CLOCK_HIGH;
> +
> +       return MODE_OK;
> +}
> +
> +static void sn65dsi83_mode_set(struct drm_bridge *bridge,
> +                              const struct drm_display_mode *mode,
> +                              const struct drm_display_mode *adj)
> +{
> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +
> +       ctx->mode = *mode;

IIRC you should be using adj, not mode.  If something earlier in the
chain tweaked it then adj will be more correct.


> +}
> +
> +static const struct drm_bridge_funcs sn65dsi83_funcs = {
> +       .attach         = sn65dsi83_attach,
> +       .pre_enable     = sn65dsi83_pre_enable,
> +       .enable         = sn65dsi83_enable,
> +       .disable        = sn65dsi83_disable,
> +       .post_disable   = sn65dsi83_post_disable,
> +       .mode_valid     = sn65dsi83_mode_valid,
> +       .mode_set       = sn65dsi83_mode_set,
> +};
> +
> +static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx)
> +{
> +       struct drm_bridge *panel_bridge;
> +       struct device *dev = ctx->dev;
> +       struct device_node *endpoint;
> +       struct drm_panel *panel;
> +       int ret;
> +
> +       endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> +       ctx->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> +       ctx->host_node = of_graph_get_remote_port_parent(endpoint);
> +       of_node_put(endpoint);
> +       of_node_put(ctx->host_node);

It seems odd that you're storing "ctx->host_node" permanently in your
structures but you're also calling of_node_put() on it.  Are you
positive that's correct?  I think you need to do the of_node_put() in
the remove function and probably in the error handling of this
function (or use devm to handle it).


> +
> +       if (ctx->dsi_lanes < 0 || ctx->dsi_lanes > 4)
> +               return -EINVAL;

Should be dsi_lanes <= 0, right?



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux