Re: [PATCH 1/3] drm/bridge/synopsys: Add MIPI DSI2 host controller bridge

["Heiko Stübner" <heiko@sntech.de>]

Hi,

Am Mittwoch, 6. November 2024, 14:54:39 CET schrieb neil.armstrong@linaro.org:
> > +#define UPDATE(v, h, l)			(((v) << (l)) & GENMASK((h), (l)))
> 
> I'm not super fan of this macro, overall I thinkg you should switch to
> regmap and make use of regmap_update_bits and drop dsi2_write/read wrappers
> to readl/writel.

Yep you're right. That macro acutally is just FIELD_PREP in disguise ;-)
So I've gone with that (and regmap).


> <snip>
> 
> > +
> > +static struct dw_mipi_dsi2 *
> > +__dw_mipi_dsi2_probe(struct platform_device *pdev,
> > +		     const struct dw_mipi_dsi2_plat_data *plat_data)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct reset_control *apb_rst;
> > +	struct dw_mipi_dsi2 *dsi2;
> > +	int ret;
> > +
> > +	dsi2 = devm_kzalloc(dev, sizeof(*dsi2), GFP_KERNEL);
> > +	if (!dsi2)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	dsi2->dev = dev;
> > +	dsi2->plat_data = plat_data;
> > +
> > +	if (!plat_data->phy_ops->init || !plat_data->phy_ops->get_lane_mbps ||
> > +	    !plat_data->phy_ops->get_timing)
> > +		return dev_err_ptr_probe(dev, -ENODEV, "Phy not properly configured\n");
> > +
> > +	if (!plat_data->base) {
> > +		dsi2->base = devm_platform_ioremap_resource(pdev, 0);
> > +		if (IS_ERR(dsi2->base))
> > +			return ERR_PTR(-ENODEV);
> > +	} else {
> > +		dsi2->base = plat_data->base;
> > +	}
> > +
> > +	dsi2->pclk = devm_clk_get(dev, "pclk");
> > +	if (IS_ERR(dsi2->pclk))
> > +		return dev_err_cast_probe(dev, dsi2->pclk, "Unable to get pclk\n");
> > +
> > +	dsi2->sys_clk = devm_clk_get(dev, "sys");
> > +	if (IS_ERR(dsi2->sys_clk))
> > +		return dev_err_cast_probe(dev, dsi2->sys_clk, "Unable to get sys_clk\n");
> > +
> > +	/*
> > +	 * Note that the reset was not defined in the initial device tree, so
> > +	 * we have to be prepared for it not being found.
> > +	 */
> > +	apb_rst = devm_reset_control_get_optional_exclusive(dev, "apb");
> > +	if (IS_ERR(apb_rst))
> > +		return dev_err_cast_probe(dev, apb_rst, "Unable to get reset control\n");
> > +
> > +	if (apb_rst) {
> > +		ret = clk_prepare_enable(dsi2->pclk);
> > +		if (ret) {
> > +			dev_err(dev, "%s: Failed to enable pclk\n", __func__);
> > +			return ERR_PTR(ret);
> > +		}
> > +
> > +		reset_control_assert(apb_rst);
> > +		usleep_range(10, 20);
> > +		reset_control_deassert(apb_rst);
> > +
> > +		clk_disable_unprepare(dsi2->pclk);
> > +	}
> > +
> > +	pm_runtime_enable(dev);
> > +
> > +	dsi2->dsi_host.ops = &dw_mipi_dsi2_host_ops;
> > +	dsi2->dsi_host.dev = dev;
> > +	ret = mipi_dsi_host_register(&dsi2->dsi_host);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register MIPI host: %d\n", ret);
> > +		pm_runtime_disable(dev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	dsi2->bridge.driver_private = dsi2;
> > +	dsi2->bridge.funcs = &dw_mipi_dsi2_bridge_funcs;
> > +	dsi2->bridge.of_node = pdev->dev.of_node;
> > +
> > +	return dsi2;
> > +}
> > +
> > +static void __dw_mipi_dsi2_remove(struct dw_mipi_dsi2 *dsi2)
> > +{
> > +	mipi_dsi_host_unregister(&dsi2->dsi_host);
> > +
> > +	pm_runtime_disable(dsi2->dev);
> > +}
> > +
> > +/*
> > + * Probe/remove API, used from platforms based on the DRM bridge API.
> > + */
> > +struct dw_mipi_dsi2 *
> > +dw_mipi_dsi2_probe(struct platform_device *pdev,
> > +		   const struct dw_mipi_dsi2_plat_data *plat_data)
> > +{
> > +	return __dw_mipi_dsi2_probe(pdev, plat_data);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_mipi_dsi2_probe);
> > +
> > +void dw_mipi_dsi2_remove(struct dw_mipi_dsi2 *dsi2)
> > +{
> > +	__dw_mipi_dsi2_remove(dsi2);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_mipi_dsi2_remove);
> 
> Since it's not use yet, you should probably drop those since it's dead
> code.

Though it is used. The Rockchip glue in patch 3 calls 
	dsi2->dmd = dw_mipi_dsi2_probe(pdev, &dsi2->pdata);
in its dw_mipi_dsi2_rockchip_probe() and
	dw_mipi_dsi2_remove(dsi2->dmd);
in its dw_mipi_dsi2_rockchip_remove() for the whole bridge setup

Similarly the below bind/unbind functions are called for the bridge-attach
from the component part of the Rockchip glue.

Which is the same way the dsi1 driver handles things right now.


> > +
> > +/*
> > + * Bind/unbind API, used from platforms based on the component framework.
> > + */
> > +int dw_mipi_dsi2_bind(struct dw_mipi_dsi2 *dsi2, struct drm_encoder *encoder)
> > +{
> > +	return drm_bridge_attach(encoder, &dsi2->bridge, NULL, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_mipi_dsi2_bind);
> > +
> > +void dw_mipi_dsi2_unbind(struct dw_mipi_dsi2 *dsi2)
> > +{
> > +}
> > +EXPORT_SYMBOL_GPL(dw_mipi_dsi2_unbind);

[...]

> > +struct dw_mipi_dsi2 *dw_mipi_dsi2_probe(struct platform_device *pdev,
> > +					const struct dw_mipi_dsi2_plat_data *plat_data);
> > +void dw_mipi_dsi2_remove(struct dw_mipi_dsi2 *dsi2);
> > +int dw_mipi_dsi2_bind(struct dw_mipi_dsi2 *dsi2, struct drm_encoder *encoder);
> > +void dw_mipi_dsi2_unbind(struct dw_mipi_dsi2 *dsi2);
> > +
> > +#endif /* __DW_MIPI_DSI2__ */
> 
> Overall the driver is very close to dw-mipi-dsi, si it's overall good!

that was the intention ... so that I don't introduce issues that were
already solved elsewhere ;-) .

Heiko