On Wed, 2019-12-18 at 13:41 +0800, CK Hu wrote:

Hi, Jitao:

On Mon, 2019-12-16 at 16:29 +0800, Jitao Shi wrote:

...

Add a property to control mipi tx drive current: "mipitx-current-drive"

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

.../devicetree/bindings/display/mediatek/mediatek,dsi.txt | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt index a19a6cc375ed..780201ddcd5c 100644 --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt @@ -33,6 +33,9 @@ Required properties:

• #clock-cells: must be <0>;
• #phy-cells: must be <0>.

+Optional properties: +- mipitx-current-drive: adjust driving current, should be 1 ~ 0xF

In "[PATCH 3/4] drm/mediatek: add the mipitx driving control" [1], I see that you actually control a register its name is MIPITX_VOLTAGE_SEL, so I guess this control the voltage. If mipi_tx has the ability to control the voltage, could we just treat mipi_tx as a regulator? For a regulator, regulator-min-microvolt and regulator-max-microvolt would limit the volt range and you could get it by of_get_regulator_init_data(). If it actually control the current, regulator-min-microamp and regulator-max-microamp could be used. I'm not expert on this, so please give me more information on this.

[1] http://lists.infradead.org/pipermail/linux-mediatek/2019-December/025638.htm...

Regards, CK

'MIPITX_VOLTAGE_SEL' is the whole 32bit name. mipitx-current-drive is just bit[9:6] RG_DSI_HSTX_LDO_REF_SEL,

Not for voltage.

Best Regards Jitao

...

Example:

mipi_tx0: mipi-dphy@10215000 { @@ -42,6 +45,7 @@ mipi_tx0: mipi-dphy@10215000 { clock-output-names = "mipi_tx0_pll"; #clock-cells = <0>; #phy-cells = <0>;

• mipitx-current-drive = <0x8>;

};

dsi0: dsi@1401b000 {

Hi, Jitao:

On Mon, 2019-12-16 at 16:29 +0800, Jitao Shi wrote:

Read calibration data from nvmem, and config mipitx impedance with calibration data to make sure their impedance are 100ohm.

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

drivers/gpu/drm/mediatek/mtk_mipi_tx.h | 1 + drivers/gpu/drm/mediatek/mtk_mt8183_mipi_tx.c | 63 +++++++++++++++++++ 2 files changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.h b/drivers/gpu/drm/mediatek/mtk_mipi_tx.h index eea44327fe9f..a1b6292145de 100644 --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.h +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.h @@ -28,6 +28,7 @@ struct mtk_mipi_tx { void __iomem *regs; u32 data_rate; u32 mipitx_drive;

• u32 rt_code[5]; const struct mtk_mipitx_data *driver_data; struct clk_hw pll_hw; struct clk *pll;

diff --git a/drivers/gpu/drm/mediatek/mtk_mt8183_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mt8183_mipi_tx.c index 124fdf95f1e5..f624516944bb 100644 --- a/drivers/gpu/drm/mediatek/mtk_mt8183_mipi_tx.c +++ b/drivers/gpu/drm/mediatek/mtk_mt8183_mipi_tx.c @@ -5,6 +5,8 @@ */

#include "mtk_mipi_tx.h" +#include <linux/nvmem-consumer.h> +#include <linux/slab.h>

#define MIPITX_LANE_CON 0x000c #define RG_DSI_CPHY_T1DRV_EN BIT(0) @@ -28,6 +30,7 @@ #define MIPITX_PLL_CON4 0x003c #define RG_DSI_PLL_IBIAS (3 << 10)

+#define MIPITX_D2P_RTCODE 0x0100 #define MIPITX_D2_SW_CTL_EN 0x0144 #define MIPITX_D0_SW_CTL_EN 0x0244 #define MIPITX_CK_CKMODE_EN 0x0328 @@ -108,6 +111,64 @@ static const struct clk_ops mtk_mipi_tx_pll_ops = { .recalc_rate = mtk_mipi_tx_pll_recalc_rate, };

+static int mtk_mipi_tx_config_calibration_data(struct mtk_mipi_tx *mipi_tx) +{

• u32 *buf = NULL;

Need not to assign NULL to buf, it would be assigned some value later.

• int i, j;
• struct nvmem_cell *cell;
• struct device *dev = mipi_tx->dev;
• size_t len;
• cell = nvmem_cell_get(dev, "calibration-data");
• if (IS_ERR(cell)) {

• dev_warn(dev, "nvmem_cell_get fail\n");
  

In [1], nvmem is optional property, so I think you should use dev_dbg() or dev_info().

[1] http://lists.infradead.org/pipermail/linux-mediatek/2019-December/025640.htm...

• return -EINVAL;
  

The caller does not process return value, so I think you don't need to return value.

• }
• buf = (u32 *)nvmem_cell_read(cell, &len);
• nvmem_cell_put(cell);
• if (IS_ERR(buf)) {

• dev_warn(dev, "can't get data\n");
  

• return -EINVAL;
  

• }
• if (len < 3 * sizeof(u32)) {

• dev_warn(dev, "invalid calibration data\n");
  

• kfree(buf);
  

• return -EINVAL;
  

• }
• mipi_tx->rt_code[0] = ((buf[0] >> 6 & 0x1F) << 5) |

You don't access rt_code[] out of this function, so I think this could be local variable.

• 	      (buf[0] >> 11 & 0x1F);
  

Lower case 0x1f.

• mipi_tx->rt_code[1] = ((buf[1] >> 27 & 0x1F) << 5) |

• 	      (buf[0] >> 1 & 0x1F);
  

• mipi_tx->rt_code[2] = ((buf[1] >> 17 & 0x1F) << 5) |

• 	      (buf[1] >> 22 & 0x1F);
  

• mipi_tx->rt_code[3] = ((buf[1] >> 7 & 0x1F) << 5) |

• 	      (buf[1] >> 12 & 0x1F);
  

• mipi_tx->rt_code[4] = ((buf[2] >> 27 & 0x1F) << 5) |

• 	      (buf[1] >> 2 & 0x1F);
  

Why not just save rt_code in nvmem and you don't need to translate here? If you need to do so, please add description for this.

• for (i = 0; i < 5; i++) {

• if ((mipi_tx->rt_code[i] & 0x1F) == 0)
  

• 	mipi_tx->rt_code[i] |= 0x10;
  

• if ((mipi_tx->rt_code[i] >> 5 & 0x1F) == 0)
  

• 	mipi_tx->rt_code[i] |= 0x10 << 5;
  

• for (j = 0; j < 10; j++) {
  

• 	mtk_mipi_tx_update_bits(mipi_tx,
  

• 		MIPITX_D2P_RTCODE * (i + 1) + j * 4,
  

• 		1, mipi_tx->rt_code[i] >> j & 1);
  

• }
  

There is only one statement in for-loop, so you could get rid of {}.

Regards, CK

• }
• kfree(buf);
• return 0;

+}

static void mtk_mipi_tx_power_on_signal(struct phy *phy) { struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy); @@ -130,6 +191,8 @@ static void mtk_mipi_tx_power_on_signal(struct phy *phy) RG_DSI_HSTX_LDO_REF_SEL, mipi_tx->mipitx_drive << 6);

• mtk_mipi_tx_config_calibration_data(mipi_tx);
• mtk_mipi_tx_set_bits(mipi_tx, MIPITX_CK_CKMODE_EN, DSI_CK_CKMODE_EN);

}