Hi Adrian,
Thanks for the patch. This is nice consolidation work.
I'm Ccing Heiko for the Rockchip part.
See below for some comments.
On Mon, 2019-12-02 at 21:33 +0200, AdrianAdrian Ratiu wrote:
> Convert the common bridge code and the two rockchip & stm drivers
> which currently use it to the regmap API in anticipation for further
> changes to make it more generic and add older DSI host controller
> support as found on i.mx6 based devices.
>
> The regmap becomes an internal state of the bridge. No functional
> changes other than requiring the platform drivers to use the
> pre-configured regmap supplied by the bridge after its probe() call
> instead of ioremp'ing the registers themselves.
>
> In subsequent commits the bridge will become able to detect the
> DSI host core version and init the regmap with different register
> layouts. The platform drivers will continue to use the regmap without
> modifications or worrying about the specific layout in use (in other
> words the layout is abstracted away via the regmap).
>
> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Reviewed-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 215 ++++++++++--------
> .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 17 +-
At least for Rockchip, I'd rather see this done in two
steps: first some regmap infrastructure introduced,
and then in a follow-up patch, the rockchip driver
moved to it.
It's safer, and better from a bisection POV, and from
a first look it seems doable.
> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 34 ++-
It would be good to do try the same for STM. It's also
simpler to review that way.
> include/drm/bridge/dw_mipi_dsi.h | 2 +-
> 4 files changed, 145 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index b6e793bb653c..6cb57807f3f9 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> #include <linux/reset.h>
>
> #include <video/mipi_display.h>
> @@ -226,7 +227,7 @@ struct dw_mipi_dsi {
> struct mipi_dsi_host dsi_host;
> struct drm_bridge *panel_bridge;
> struct device *dev;
> - void __iomem *base;
> + struct regmap *regs;
>
You have the regmap here...
>
> struct clk *pclk;
>
[..]
> @@ -954,7 +952,6 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
> }
>
> dsi->dev = dev;
> - dsi->pdata.base = dsi->base;
> dsi->pdata.max_data_lanes = dsi->cdata->max_data_lanes;
> dsi->pdata.phy_ops = &dw_mipi_dsi_rockchip_phy_ops;
> dsi->pdata.host_ops = &dw_mipi_dsi_rockchip_host_ops;
> @@ -970,6 +967,8 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
> goto err_clkdisable;
> }
>
> + dsi->regs = dsi->pdata.regs;
> +
... and this goes for both STM and Rockchip: I don't think you need neither
the struct dw_mipi_dsi_plat_data.regs nor the
structdw_mipi_dsi_{rockchip, stm}.regs. You should be able to
just access the regmap via the struct dw_mipi_dsi.
[..]
>
> err_dsi_probe:
> @@ -474,7 +472,7 @@ static struct platform_driver dw_mipi_dsi_stm_driver = {
> .remove = dw_mipi_dsi_stm_remove,
> .driver = {
> .of_match_table = dw_mipi_dsi_stm_dt_ids,
> - .name = "stm32-display-dsi",
> + .name = DRIVER_NAME,
Unrelated change, please drop it.
> .pm = &dw_mipi_dsi_stm_pm_ops,
> },
> };
Thanks,
Ezequiel
