Hi Neil,
some high-level comments from me below.
On Fri, Jan 7, 2022 at 3:58 PM Neil Armstrong narmstrong@baylibre.com wrote: [...]
+/* MIPI DSI Relative REGISTERs Definitions */ +/* For MIPI_DSI_TOP_CNTL */ +#define BIT_DPI_COLOR_MODE 20 +#define BIT_IN_COLOR_MODE 16 +#define BIT_CHROMA_SUBSAMPLE 14 +#define BIT_COMP2_SEL 12 +#define BIT_COMP1_SEL 10 +#define BIT_COMP0_SEL 8 +#define BIT_DE_POL 6 +#define BIT_HSYNC_POL 5 +#define BIT_VSYNC_POL 4 +#define BIT_DPICOLORM 3 +#define BIT_DPISHUTDN 2 +#define BIT_EDPITE_INTR_PULSE 1 +#define BIT_ERR_INTR_PULSE 0
Why not use BIT() and GENMASK() for these and prefixing them with MIPI_DSI_TOP_CNTL_? That would make them consistent with other parts of the meson sub-driver.
[...]
+static void meson_dw_mipi_dsi_hw_init(struct meson_dw_mipi_dsi *mipi_dsi) +{
writel_relaxed((1 << 4) | (1 << 5) | (0 << 6),mipi_dsi->base + MIPI_DSI_TOP_CNTL);
please use the macros from above
writel_bits_relaxed(0xf, 0xf, mipi_dsi->base + MIPI_DSI_TOP_SW_RESET);writel_bits_relaxed(0xf, 0, mipi_dsi->base + MIPI_DSI_TOP_SW_RESET);
[...]
phy_power_on(mipi_dsi->phy);
Please propagate the error code here. Also shouldn't this go to a new dw_mipi_dsi_phy_power_on() as the PHY driver uses the updated settings from phy_configure only in it's .power_on callback?
[...]
phy_configure(mipi_dsi->phy, &mipi_dsi->phy_opts);
please propagate the error code here as the PHY driver has some explicit code to return an error in it's .phy_configure callback
[...]
phy_init(mipi_dsi->phy);
please propagate the error code here
[...]
phy_exit(mipi_dsi->phy);
please propagate the error code here
[...]
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);mipi_dsi->base = devm_ioremap_resource(&pdev->dev, res);
other parts of the meson DRM driver have been converted to use devm_platform_ioremap_resource() I suggest updating this as well to simplify the code here
[...]
mipi_dsi->phy = devm_phy_get(&pdev->dev, "dphy");if (IS_ERR(mipi_dsi->phy)) {ret = PTR_ERR(mipi_dsi->phy);dev_err(&pdev->dev, "failed to get mipi dphy: %d\n", ret);return ret;
you can simplify this with: return dev_err_probe(&pdev->dev, PTR_ERR(mipi_dsi->phy, "failed to get mipi dphy\n");
[...]
mipi_dsi->px_clk = devm_clk_get(&pdev->dev, "px_clk");if (IS_ERR(mipi_dsi->px_clk)) {dev_err(&pdev->dev, "Unable to get PLL clk\n");return PTR_ERR(mipi_dsi->px_clk);
you can simplify this with: return dev_err_probe(&pdev->dev, PTR_ERR(mipi_dsi->px_clk, "Unable to get PLL clk\n"); Also should it say s/PLL clk/px clock/?
[...]
top_rst = devm_reset_control_get_exclusive(&pdev->dev, "top");if (IS_ERR(top_rst)) {ret = PTR_ERR(top_rst);if (ret != -EPROBE_DEFER)dev_err(&pdev->dev, "Unable to get reset control: %d\n", ret);return ret;
you can simplify this with: return dev_err_probe(&pdev->dev, PTR_ERR(top_rst, "Unable to get reset control\n");
[...]
mipi_dsi->dmd = dw_mipi_dsi_probe(pdev, &mipi_dsi->pdata);if (IS_ERR(mipi_dsi->dmd)) {ret = PTR_ERR(mipi_dsi->dmd);if (ret != -EPROBE_DEFER)dev_err(&pdev->dev,"Failed to probe dw_mipi_dsi: %d\n", ret);
you can simplify this with: dev_err_probe(&pdev->dev, ret, "Failed to probe dw_mipi_dsi\n");
Best regards, Martin