Re: [PATCH 07/20] drm/bridge: tc358775: use regmap instead of open coded access functions

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

 



Hello Michael,

Thank you for the patch!

On Mon, May 6, 2024 at 3:35 PM Michael Walle <mwalle@xxxxxxxxxx> wrote:
>
> The DSI bridge also supports access via DSI in-band reads and writes.
> Prepare the driver for that by converting all the access functions to
> regmap. This also have the advantage that it will make tracing and
> debugging easier and we can use all the bit manipulation helpers from
> regmap.
>
> Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 150 +++++++++++++++++---------------------
>  1 file changed, 68 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> index 7ae86e8d4c72..b7f15164e655 100644
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -16,6 +16,7 @@
>  #include <linux/media-bus-format.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
> @@ -238,7 +239,7 @@ enum tc3587x5_type {
>  };
>
>  struct tc_data {
> -       struct i2c_client       *i2c;

The 'i2c' node is removed here,

> +       struct regmap           *regmap;
>         struct device           *dev;
>
>         struct drm_bridge       bridge;
> @@ -309,42 +310,6 @@ static void tc_bridge_post_disable(struct drm_bridge *bridge)
>         usleep_range(10000, 11000);
>  }
>
> -static void d2l_read(struct i2c_client *i2c, u16 addr, u32 *val)
> -{
> -       int ret;
> -       u8 buf_addr[2];
> -
> -       put_unaligned_be16(addr, buf_addr);
> -       ret = i2c_master_send(i2c, buf_addr, sizeof(buf_addr));
> -       if (ret < 0)
> -               goto fail;
> -
> -       ret = i2c_master_recv(i2c, (u8 *)val, sizeof(*val));
> -       if (ret < 0)
> -               goto fail;
> -
> -       pr_debug("d2l: I2C : addr:%04x value:%08x\n", addr, *val);
> -       return;
> -
> -fail:
> -       dev_err(&i2c->dev, "Error %d reading from subaddress 0x%x\n",
> -               ret, addr);
> -}
> -
> -static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
> -{
> -       u8 data[6];
> -       int ret;
> -
> -       put_unaligned_be16(addr, data);
> -       put_unaligned_le32(val, data + 2);
> -
> -       ret = i2c_master_send(i2c, data, ARRAY_SIZE(data));
> -       if (ret < 0)
> -               dev_err(&i2c->dev, "Error %d writing to subaddress 0x%x\n",
> -                       ret, addr);
> -}
> -
>  /* helper function to access bus_formats */
>  static struct drm_connector *get_connector(struct drm_encoder *encoder)
>  {
> @@ -358,12 +323,33 @@ static struct drm_connector *get_connector(struct drm_encoder *encoder)
>         return NULL;
>  }
>
> +static const struct reg_sequence tc_lvmux_vesa24[] = {
> +       { LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3) },
> +       { LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0) },
> +       { LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7) },
> +       { LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0) },
> +       { LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2) },
> +       { LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0) },
> +       { LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6) },
> +};
> +
> +/* JEIDA-24/JEIDA-18 have the same mapping */
> +static const struct reg_sequence tc_lvmux_jeida18_24[] = {
> +       { LV_MX0003, LV_MX(LVI_R2, LVI_R3, LVI_R4, LVI_R5) },
> +       { LV_MX0407, LV_MX(LVI_R6, LVI_R1, LVI_R7, LVI_G2) },
> +       { LV_MX0811, LV_MX(LVI_G3, LVI_G4, LVI_G0, LVI_G1) },
> +       { LV_MX1215, LV_MX(LVI_G5, LVI_G6, LVI_G7, LVI_B2) },
> +       { LV_MX1619, LV_MX(LVI_B0, LVI_B1, LVI_B3, LVI_B4) },
> +       { LV_MX2023, LV_MX(LVI_B5, LVI_B6, LVI_B7, LVI_L0) },
> +       { LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R0) },
> +};
> +
>  static void tc_bridge_enable(struct drm_bridge *bridge)
>  {
>         struct tc_data *tc = bridge_to_tc(bridge);
>         u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2;
>         u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2;
> -       u32 val = 0;
> +       unsigned int val = 0;
>         u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
>         struct drm_display_mode *mode;
>         struct drm_connector *connector = get_connector(bridge->encoder);
> @@ -386,28 +372,29 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         htime2 = (hfront_porch << 16) + hactive;
>         vtime2 = (vfront_porch << 16) + vactive;
>
> -       d2l_read(tc->i2c, IDREG, &val);
> +       regmap_read(tc->regmap, IDREG, &val);
>
>         dev_info(tc->dev, "DSI2LVDS Chip ID.%02x Revision ID. %02x **\n",
>                  (val >> 8) & 0xFF, val & 0xFF);
>
> -       d2l_write(tc->i2c, SYSRST, SYS_RST_REG | SYS_RST_DSIRX | SYS_RST_BM |
> -                 SYS_RST_LCD | SYS_RST_I2CM);
> +       regmap_write(tc->regmap, SYSRST,
> +                    SYS_RST_REG | SYS_RST_DSIRX | SYS_RST_BM | SYS_RST_LCD |
> +                    SYS_RST_I2CM);
>         usleep_range(30000, 40000);
>
> -       d2l_write(tc->i2c, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> -       d2l_write(tc->i2c, PPI_LPTXTIMECNT, LPX_PERIOD);
> -       d2l_write(tc->i2c, PPI_D0S_CLRSIPOCOUNT, 3);
> -       d2l_write(tc->i2c, PPI_D1S_CLRSIPOCOUNT, 3);
> -       d2l_write(tc->i2c, PPI_D2S_CLRSIPOCOUNT, 3);
> -       d2l_write(tc->i2c, PPI_D3S_CLRSIPOCOUNT, 3);
> +       regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> +       regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
> +       regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
> +       regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
> +       regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
> +       regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);
>
>         val = ((L0EN << tc->num_dsi_lanes) - L0EN) | DSI_CLEN_BIT;
> -       d2l_write(tc->i2c, PPI_LANEENABLE, val);
> -       d2l_write(tc->i2c, DSI_LANEENABLE, val);
> +       regmap_write(tc->regmap, PPI_LANEENABLE, val);
> +       regmap_write(tc->regmap, DSI_LANEENABLE, val);
>
> -       d2l_write(tc->i2c, PPI_STARTPPI, PPI_START_FUNCTION);
> -       d2l_write(tc->i2c, DSI_STARTDSI, DSI_RX_START);
> +       regmap_write(tc->regmap, PPI_STARTPPI, PPI_START_FUNCTION);
> +       regmap_write(tc->regmap, DSI_STARTDSI, DSI_RX_START);
>
>         /* Video event mode vs pulse mode bit, does not exist for tc358775 */
>         if (tc->type == TC358765)
> @@ -431,42 +418,28 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         vsdelay = (clkdiv * (t1 + t3) / byteclk) - hback_porch - hsync_len - hactive;
>
>         val |= TC358775_VPCTRL_VSDELAY(vsdelay);
> -       d2l_write(tc->i2c, VPCTRL, val);
> +       regmap_write(tc->regmap, VPCTRL, val);
>
> -       d2l_write(tc->i2c, HTIM1, htime1);
> -       d2l_write(tc->i2c, VTIM1, vtime1);
> -       d2l_write(tc->i2c, HTIM2, htime2);
> -       d2l_write(tc->i2c, VTIM2, vtime2);
> +       regmap_write(tc->regmap, HTIM1, htime1);
> +       regmap_write(tc->regmap, VTIM1, vtime1);
> +       regmap_write(tc->regmap, HTIM2, htime2);
> +       regmap_write(tc->regmap, VTIM2, vtime2);
>
> -       d2l_write(tc->i2c, VFUEN, VFUEN_EN);
> -       d2l_write(tc->i2c, SYSRST, SYS_RST_LCD);
> -       d2l_write(tc->i2c, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
> +       regmap_write(tc->regmap, VFUEN, VFUEN_EN);
> +       regmap_write(tc->regmap, SYSRST, SYS_RST_LCD);
> +       regmap_write(tc->regmap, LVPHY0, LV_PHY0_PRBS_ON(4) | LV_PHY0_ND(6));
>
>         dev_dbg(tc->dev, "bus_formats %04x bpc %d\n",
>                 connector->display_info.bus_formats[0],
>                 tc->bpc);
> -       if (connector->display_info.bus_formats[0] ==
> -               MEDIA_BUS_FMT_RGB888_1X7X4_SPWG) {
> -               /* VESA-24 */
> -               d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R0, LVI_R1, LVI_R2, LVI_R3));
> -               d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R4, LVI_R7, LVI_R5, LVI_G0));
> -               d2l_write(tc->i2c, LV_MX0811, LV_MX(LVI_G1, LVI_G2, LVI_G6, LVI_G7));
> -               d2l_write(tc->i2c, LV_MX1215, LV_MX(LVI_G3, LVI_G4, LVI_G5, LVI_B0));
> -               d2l_write(tc->i2c, LV_MX1619, LV_MX(LVI_B6, LVI_B7, LVI_B1, LVI_B2));
> -               d2l_write(tc->i2c, LV_MX2023, LV_MX(LVI_B3, LVI_B4, LVI_B5, LVI_L0));
> -               d2l_write(tc->i2c, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R6));
> -       } else {
> -               /* JEIDA-18 and JEIDA-24 */
> -               d2l_write(tc->i2c, LV_MX0003, LV_MX(LVI_R2, LVI_R3, LVI_R4, LVI_R5));
> -               d2l_write(tc->i2c, LV_MX0407, LV_MX(LVI_R6, LVI_R1, LVI_R7, LVI_G2));
> -               d2l_write(tc->i2c, LV_MX0811, LV_MX(LVI_G3, LVI_G4, LVI_G0, LVI_G1));
> -               d2l_write(tc->i2c, LV_MX1215, LV_MX(LVI_G5, LVI_G6, LVI_G7, LVI_B2));
> -               d2l_write(tc->i2c, LV_MX1619, LV_MX(LVI_B0, LVI_B1, LVI_B3, LVI_B4));
> -               d2l_write(tc->i2c, LV_MX2023, LV_MX(LVI_B5, LVI_B6, LVI_B7, LVI_L0));
> -               d2l_write(tc->i2c, LV_MX2427, LV_MX(LVI_HS, LVI_VS, LVI_DE, LVI_R0));
> -       }
> +       if (connector->display_info.bus_formats[0] == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG)
> +               regmap_multi_reg_write(tc->regmap, tc_lvmux_vesa24,
> +                                      ARRAY_SIZE(tc_lvmux_vesa24));
> +       else
> +               regmap_multi_reg_write(tc->regmap, tc_lvmux_jeida18_24,
> +                                      ARRAY_SIZE(tc_lvmux_jeida18_24));
>
> -       d2l_write(tc->i2c, VFUEN, VFUEN_EN);
> +       regmap_write(tc->regmap, VFUEN, VFUEN_EN);
>
>         val = LVCFG_LVEN_BIT;
>         if (tc->lvds_link == DUAL_LINK) {
> @@ -475,7 +448,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         } else {
>                 val |= TC358775_LVCFG_PCLKDIV(DIVIDE_BY_3);
>         }
> -       d2l_write(tc->i2c, LVCFG, val);
> +       regmap_write(tc->regmap, LVCFG, val);
>  }
>
>  /*
> @@ -617,7 +590,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>
>  static int tc_attach_host(struct tc_data *tc)
>  {
> -       struct device *dev = &tc->i2c->dev;
> +       struct device *dev = tc->dev;
>         struct mipi_dsi_host *host;
>         struct mipi_dsi_device *dsi;
>         int ret;
> @@ -665,6 +638,14 @@ static int tc_attach_host(struct tc_data *tc)
>         return 0;
>  }
>
> +static const struct regmap_config tc358775_regmap_config = {
> +       .reg_bits = 16,
> +       .val_bits = 32,
> +       .max_register = 0xffff,
> +       .reg_format_endian = REGMAP_ENDIAN_BIG,
> +       .val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};
> +
>  static int tc_probe(struct i2c_client *client)
>  {
>         struct device *dev = &client->dev;
> @@ -679,6 +660,11 @@ static int tc_probe(struct i2c_client *client)
>         tc->i2c = client;

so the assignment above is no longer correct and should
be also removed. Otherwise, this code will not compile.

>         tc->type = (enum tc3587x5_type)(unsigned long)of_device_get_match_data(dev);
>
> +       tc->regmap = devm_regmap_init_i2c(client, &tc358775_regmap_config);
> +       if (IS_ERR(tc->regmap))
> +               return dev_err_probe(dev, PTR_ERR(tc->regmap),
> +                                    "regmap i2c init failed\n");
> +
>         tc->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
>                                                   TC358775_LVDS_OUT0, 0);
>         if (IS_ERR(tc->panel_bridge))
>
> --
> 2.39.2
>

Kind regards
Daniel Semkowicz




[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