Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series

[Krzysztof Kozlowski <krzk@kernel.org>]

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@microchip.com>
> ---


...

> +
> +#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