On 04/07/2024 10:48, Manikandan Muralidharan wrote:
> Add the Microchip's DSI controller wrapper driver that uses
> the Synopsys DesignWare MIPI DSI host controller bridge.
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@xxxxxxxxxxxxx>
> ---
...
> +
> +#define HSTT(_maxfreq, _c_lp2hs, _c_hs2lp, _d_lp2hs, _d_hs2lp) \
> +{ \
> + .maxfreq = _maxfreq, \
> + .timing = { \
> + .clk_lp2hs = _c_lp2hs, \
> + .clk_hs2lp = _c_hs2lp, \
> + .data_lp2hs = _d_lp2hs, \
> + .data_hs2lp = _d_hs2lp, \
> + } \
> +}
> +
> +struct hstt hstt_table[] = {
So more globals? No.
> + HSTT(90, 32, 20, 26, 13),
> + HSTT(100, 35, 23, 28, 14),
> + HSTT(110, 32, 22, 26, 13),
> + HSTT(130, 31, 20, 27, 13),
> + HSTT(140, 33, 22, 26, 14),
> + HSTT(150, 33, 21, 26, 14),
> + HSTT(170, 32, 20, 27, 13),
> + HSTT(180, 36, 23, 30, 15),
> + HSTT(200, 40, 22, 33, 15),
> + HSTT(220, 40, 22, 33, 15),
> + HSTT(240, 44, 24, 36, 16),
> + HSTT(250, 48, 24, 38, 17),
> + HSTT(270, 48, 24, 38, 17),
> + HSTT(300, 50, 27, 41, 18),
> + HSTT(330, 56, 28, 45, 18),
> + HSTT(360, 59, 28, 48, 19),
> + HSTT(400, 61, 30, 50, 20),
> + HSTT(450, 67, 31, 55, 21),
> + HSTT(500, 73, 31, 59, 22),
> + HSTT(550, 79, 36, 63, 24),
> + HSTT(600, 83, 37, 68, 25),
> + HSTT(650, 90, 38, 73, 27),
> + HSTT(700, 95, 40, 77, 28),
> + HSTT(750, 102, 40, 84, 28),
> + HSTT(800, 106, 42, 87, 30),
> + HSTT(850, 113, 44, 93, 31),
> + HSTT(900, 118, 47, 98, 32),
> + HSTT(950, 124, 47, 102, 34),
> + HSTT(1000, 130, 49, 107, 35),
> +};
> +
...
> +
> +static void dw_mipi_dsi_mchp_power_on(void *priv_data)
> +{
> + struct dw_mipi_dsi_mchp *dsi = priv_data;
> +
> + /* Enable the DSI wrapper */
> + dsi_write(dsi, DSI_PWR_UP, HOST_PWRUP);
> +}
> +
> +static void dw_mipi_dsi_mchp_power_off(void *priv_data)
> +{
> + struct dw_mipi_dsi_mchp *dsi = priv_data;
> +
> + /* Disable the DSI wrapper */
> + dsi_write(dsi, DSI_PWR_UP, HOST_RESET);
> +}
> +
> +struct dw_mipi_dsi_phy_ops dw_mipi_dsi_mchp_phy_ops = {
Why this is not static?
Why this is not const?
> + .init = dw_mipi_dsi_mchp_init,
> + .power_on = dw_mipi_dsi_mchp_power_on,
> + .power_off = dw_mipi_dsi_mchp_power_off,
> + .get_lane_mbps = dw_mipi_dsi_mchp_get_lane_mbps,
> + .get_timing = dw_mipi_dsi_mchp_get_timing,
> +};
> +
> +static int dw_mipi_dsi_mchp_probe(struct platform_device *pdev)
> +{
> + struct dw_mipi_dsi_mchp *dsi;
> + struct resource *res;
> + struct regmap *sfr;
> + const struct dw_mipi_dsi_mchp_chip_data *cdata;
> + int ret;
> +
> + dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
> + if (!dsi)
> + return -ENOMEM;
> +
> + dsi->dev = &pdev->dev;
> + cdata = of_device_get_match_data(dsi->dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + dsi->base = devm_ioremap_resource(&pdev->dev, res);
There is a helper for these two.
> + if (IS_ERR(dsi->base)) {
> + ret = PTR_ERR(dsi->base);
> + dev_err(dsi->dev, "Unable to get DSI Base address: %d\n", ret);
return dev_err_probe
> + return ret;
> + }
> +
> + dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
> + if (IS_ERR(dsi->pclk)) {
> + ret = PTR_ERR(dsi->pclk);
> + dev_err(dsi->dev, "Unable to get pclk: %d\n", ret);
return dev_err_probe
You are upstreaming some old code, aren't you?
> + return ret;
> + }
> +
> + dsi->pllref_clk = devm_clk_get(&pdev->dev, "refclk");
> + if (IS_ERR(dsi->pllref_clk)) {
> + ret = PTR_ERR(dsi->pllref_clk);
> + dev_err(dsi->dev, "Unable to get DSI PHY PLL reference clock: %d\n",
return dev_err_probe
> + ret);
> + return ret;
> + }
> +
> + clk_set_rate(dsi->pllref_clk, DSI_PLL_REF_CLK);
> + if (clk_get_rate(dsi->pllref_clk) != DSI_PLL_REF_CLK) {
> + dev_err(dsi->dev, "Failed to set DSI PHY PLL reference clock\n");
> + return -EINVAL;
> + }
> +
> + ret = clk_prepare_enable(dsi->pllref_clk);
Enable clock later, so your error paths will be simpler.
> + if (ret) {
> + dev_err(dsi->dev, "Failed to enable DSI PHY PLL reference clock: %d\n",
> + ret);
> + return ret;
> + }
> +
> + sfr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "microchip,sfr");
> + if (IS_ERR_OR_NULL(sfr)) {
NULL? Can it be NULL?
> + ret = PTR_ERR(sfr);
> + dev_err(dsi->dev, "Failed to get handle on Special Function Register: %d\n",
> + ret);
ret = dev_err_probe
> + goto err_dsi_probe;
> + }
> + /* Select DSI in SFR's ISS Configuration Register */
> + ret = regmap_write(sfr, SFR_ISS_CFG, ISS_CFG_DSI_MODE);
> + if (ret) {
> + dev_err(dsi->dev, "Failed to enable DSI in SFR ISS configuration register: %d\n",
> + ret);
> + goto err_dsi_probe;
> + }
Best regards,
Krzysztof
