Hi Jagan
The change to regmap looks nice. But two small comments below,
just some drive-by comments.
Sam
On Tue, Jan 24, 2023 at 12:16:47AM +0530, Jagan Teki wrote:
> To make debugging easier, switch the driver to use regmap
> from conventional io calls.
>
> Signed-off-by: Jagan Teki <ja...@edgeble.ai>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 81 ++++++++++++-------
> 1 file changed, 54 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 47bd69d5ac99..62a160af4047 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> #include <linux/reset.h>
>
> #include <video/mipi_display.h>
> @@ -242,7 +243,7 @@ struct dw_mipi_dsi {
> struct mipi_dsi_host dsi_host;
> struct drm_bridge *panel_bridge;
> struct device *dev;
> - void __iomem *base;
> + struct regmap *regmap;
>
> struct clk *pclk;
>
> @@ -301,12 +302,16 @@ static inline struct dw_mipi_dsi *bridge_to_dsi(struct
> drm_bridge *bridge)
>
> static inline void dsi_write(struct dw_mipi_dsi *dsi, u32 reg, u32 val)
> {
> - writel(val, dsi->base + reg);
> + regmap_write(dsi->regmap, reg, val);
> }
>
> static inline u32 dsi_read(struct dw_mipi_dsi *dsi, u32 reg)
> {
> - return readl(dsi->base + reg);
> + u32 val;
> +
> + regmap_read(dsi->regmap, reg, &val);
> +
> + return val;
> }
>
> static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
> @@ -332,6 +337,7 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host
> *host,
> if (IS_ERR(bridge))
> return PTR_ERR(bridge);
>
> + dev_info(host->dev, "Attached device %s\n", device->name);
> dsi->panel_bridge = bridge;
>
> drm_bridge_add(&dsi->bridge);
> @@ -400,9 +406,9 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct
> dw_mipi_dsi *dsi, u32 hdr_val)
> int ret;
> u32 val, mask;
>
> - ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> - val, !(val & GEN_CMD_FULL), 1000,
> - CMD_PKT_STATUS_TIMEOUT_US);
> + ret = regmap_read_poll_timeout(dsi->regmap, DSI_CMD_PKT_STATUS,
> + val, !(val & GEN_CMD_FULL), 1000,
> + CMD_PKT_STATUS_TIMEOUT_US);
> if (ret) {
> dev_err(dsi->dev, "failed to get available command FIFO\n");
> return ret;
> @@ -411,9 +417,9 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct
> dw_mipi_dsi *dsi, u32 hdr_val)
> dsi_write(dsi, DSI_GEN_HDR, hdr_val);
>
> mask = GEN_CMD_EMPTY | GEN_PLD_W_EMPTY;
> - ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> - val, (val & mask) == mask,
> - 1000, CMD_PKT_STATUS_TIMEOUT_US);
> + ret = regmap_read_poll_timeout(dsi->regmap, DSI_CMD_PKT_STATUS,
> + val, (val & mask) == mask,
> + 1000, CMD_PKT_STATUS_TIMEOUT_US);
> if (ret) {
> dev_err(dsi->dev, "failed to write command FIFO\n");
> return ret;
> @@ -443,9 +449,9 @@ static int dw_mipi_dsi_write(struct dw_mipi_dsi *dsi,
> len -= pld_data_bytes;
> }
>
> - ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> - val, !(val & GEN_PLD_W_FULL), 1000,
> - CMD_PKT_STATUS_TIMEOUT_US);
> + ret = regmap_read_poll_timeout(dsi->regmap, DSI_CMD_PKT_STATUS,
> + val, !(val & GEN_PLD_W_FULL),
> 1000,
> + CMD_PKT_STATUS_TIMEOUT_US);
> if (ret) {
> dev_err(dsi->dev,
> "failed to get available write payload FIFO\n");
> @@ -466,9 +472,9 @@ static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
> u32 val;
>
> /* Wait end of the read operation */
> - ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> - val, !(val & GEN_RD_CMD_BUSY),
> - 1000, CMD_PKT_STATUS_TIMEOUT_US);
> + ret = regmap_read_poll_timeout(dsi->regmap, DSI_CMD_PKT_STATUS,
> + val, !(val & GEN_RD_CMD_BUSY), 1000,
> + CMD_PKT_STATUS_TIMEOUT_US);
> if (ret) {
> dev_err(dsi->dev, "Timeout during read operation\n");
> return ret;
> @@ -476,9 +482,9 @@ static int dw_mipi_dsi_read(struct dw_mipi_dsi *dsi,
>
> for (i = 0; i < len; i += 4) {
> /* Read fifo must not be empty before all bytes are read */
> - ret = readl_poll_timeout(dsi->base + DSI_CMD_PKT_STATUS,
> - val, !(val & GEN_PLD_R_EMPTY),
> - 1000, CMD_PKT_STATUS_TIMEOUT_US);
> + ret = regmap_read_poll_timeout(dsi->regmap, DSI_CMD_PKT_STATUS,
> + val, !(val & GEN_PLD_R_EMPTY),
> 1000,
> + CMD_PKT_STATUS_TIMEOUT_US);
> if (ret) {
> dev_err(dsi->dev, "Read payload FIFO is empty\n");
> return ret;
> @@ -499,6 +505,9 @@ static ssize_t dw_mipi_dsi_host_transfer(struct
> mipi_dsi_host *host,
> struct mipi_dsi_packet packet;
> int ret, nb_bytes;
>
> + DRM_INFO("%x %x %x\n", msg->type,
> + ((((u8 *)msg->tx_buf)[0] << 8) >> 8),
> + ((((u8 *)msg->tx_buf)[1] << 16)) >> 16);
This looks like some debug left-over. If not, then please consider to
use drm_info or dev_info.
> ret = mipi_dsi_create_packet(&packet, msg);
> if (ret) {
> dev_err(dsi->dev, "failed to create packet: %d\n", ret);
> @@ -828,17 +837,18 @@ static void dw_mipi_dsi_dphy_enable(struct dw_mipi_dsi
> *dsi)
> u32 val;
> int ret;
>
> - dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
> + dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENABLECLK |
> PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
This change is not related to the regmap conversion and should be
separated out.
Sam
