Hi,
Am Mittwoch, 6. November 2024, 14:54:39 CET schrieb neil.armstr...@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
