Hi,
On Mon, Mar 09, 2020 at 10:53:04AM +0530, Harigovindan P wrote:
> Add support for Visionox panel driver.
>
> Signed-off-by: Harigovindan P <harigovi@xxxxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - Dropping redundant space in Kconfig(Sam Ravnborg).
> - Changing structure for include files(Sam Ravnborg).
> - Removing backlight related code and functions(Sam Ravnborg).
> - Removing repeated printing of error message(Sam Ravnborg).
> - Adding drm_connector as an argument for get_modes function.
> Changes in v3:
> - Adding arguments for drm_panel_init to support against mainline.
> Changes in v4:
> - Removing error messages from regulator_set_load.
> - Removing dev struct entry.
> - Removing checks.
> - Dropping empty comment lines.
> Changes in v5:
> - Removing unused struct member variables.
> - Removing blank lines.
> - Fixed indentation.
> - Invoking dsi_detach and panel_remove while early exiting from probe.
>
> drivers/gpu/drm/panel/Kconfig | 8 +
> drivers/gpu/drm/panel/Makefile | 1 +
> .../gpu/drm/panel/panel-visionox-rm69299.c | 315 ++++++++++++++++++
> 3 files changed, 324 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c
>
> ...
>
> diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
> new file mode 100644
> index 000000000000..2bd3af46d933
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
>
> ...
>
> +static int visionox_35597_power_on(struct visionox_rm69299 *ctx)
> +{
s/35597/rm69299/ ?
> +static const struct rm69299_config rm69299_dir = {
> + .width_mm = 74,
> + .height_mm = 131,
> + .dm = &visionox_rm69299_1080x2248_60hz,
> +};
Are there actually variants of the panel with different sizes? So far the
driver supports a single type of panel, so I would say struct rm69299_config
is not needed. It can be added later if the driver ever gets support for
other panel variants. For now just assign the values directly in
'visionox_rm69299_get_modes'.
> +static int visionox_rm69299_probe(struct mipi_dsi_device *dsi)
> +{
> + struct device *dev = &dsi->dev;
> + struct visionox_rm69299 *ctx;
> + int ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + mipi_dsi_set_drvdata(dsi, ctx);
> +
> + ctx->supplies[0].supply = "vdda";
> + ctx->supplies[1].supply = "vdd3p3";
> +
> + ret = devm_regulator_bulk_get(ctx->panel.dev, ARRAY_SIZE(ctx->supplies),
> + ctx->supplies);
nit: alignment is odd, either align with a tab after 'devm_regulator_bulk_get'
or with 'ctx->panel.dev'.
> + if (ret < 0)
> + return ret;
> +
> + ctx->reset_gpio = devm_gpiod_get(ctx->panel.dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ctx->reset_gpio)) {
> + DRM_DEV_ERROR(dev, "cannot get reset gpio %ld\n",
> + PTR_ERR(ctx->reset_gpio));
> + return PTR_ERR(ctx->reset_gpio);
> + }
> +
> + drm_panel_init(&ctx->panel, dev, &visionox_rm69299_drm_funcs,
> + DRM_MODE_CONNECTOR_DSI);
> + ctx->panel.dev = dev;
> + ctx->panel.funcs = &visionox_rm69299_drm_funcs;
> + drm_panel_add(&ctx->panel);
> +
> + dsi->lanes = 4;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |
> + MIPI_DSI_CLOCK_NON_CONTINUOUS;
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "dsi attach failed ret = %d\n", ret);
> + goto err_dsi_attach;
> + }
> +
> + ret = regulator_set_load(ctx->supplies[0].consumer, 32000);
> + if (ret) {
> + mipi_dsi_detach(dsi);
> + drm_panel_remove(&ctx->panel);
that's technically correct, but since you have the 'goto' above and do the
same unwinding for the other 'regulator_set_load' call below it would be
better to use a 'goto' here (and below) too. Actually the 'goto' above only
makes sense if 'goto' is also used for the other cases.
> + return ret;
> + }
> +
> + ret = regulator_set_load(ctx->supplies[1].consumer, 13200);
> + if (ret) {
> + mipi_dsi_detach(dsi);
> + drm_panel_remove(&ctx->panel);
> + return ret;
> + }
> +
> + return 0;
> +
> +err_dsi_attach:
> + drm_panel_remove(&ctx->panel);
> + return ret;
> +}
> +
> +static int visionox_rm69299_remove(struct mipi_dsi_device *dsi)
> +{
> + struct visionox_rm69299 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> + mipi_dsi_detach(ctx->dsi);
> + mipi_dsi_device_unregister(ctx->dsi);
> +
> + drm_panel_remove(&ctx->panel);
> + return 0;
> +}
> +
> +static const struct of_device_id visionox_rm69299_of_match[] = {
> + {
> + .compatible = "visionox,rm69299-1080p-display",
> + .data = &rm69299_dir,
> + },
> +};
> +MODULE_DEVICE_TABLE(of, visionox_rm69299_of_match);
> +
> +static struct mipi_dsi_driver visionox_rm69299_driver = {
> + .driver = {
> + .name = "panel-visionox-rm69299",
> + .of_match_table = visionox_rm69299_of_match,
> + },
> + .probe = visionox_rm69299_probe,
> + .remove = visionox_rm69299_remove,
> +};
> +module_mipi_dsi_driver(visionox_rm69299_driver);
> +
> +MODULE_DESCRIPTION("VISIONOX RM69299 DSI Panel Driver");
As commented on v4, this should be 'Visionox'.
