Hi Noralf.
On Sun, Jan 20, 2019 at 12:43:11PM +0100, Noralf Trønnes wrote:
> Further strip down tinydrm.ko and switch to drm_simple_connector_create().
It is nice how the tinydrm drivers goes from special drivers to small
drivers with a little added to the core.
Two minor comments below. With that addressed/processed it has:
Reviewed-by: Sam Ravnborg <s...@ravnborg.org>
>
> Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
> ---
> Documentation/gpu/tinydrm.rst | 3 -
> drivers/gpu/drm/tinydrm/core/Makefile | 2 +-
> drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 183 --------------------
> drivers/gpu/drm/tinydrm/mipi-dbi.c | 24 ++-
> drivers/gpu/drm/tinydrm/repaper.c | 16 +-
> drivers/gpu/drm/tinydrm/st7586.c | 19 +-
> include/drm/tinydrm/tinydrm.h | 9 -
> 7 files changed, 43 insertions(+), 213 deletions(-)
> delete mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
>
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 918f77c7de34..d1d546f3a664 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -391,7 +391,13 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi
> *mipi,
> {
> size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16);
> struct tinydrm_device *tdev = &mipi->tinydrm;
> + struct drm_connector *connector;
> + struct drm_device *drm;
> int ret;
> + static const uint64_t modifiers[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> + };
>
> if (!mipi->command)
> return -EINVAL;
> @@ -406,18 +412,22 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi
> *mipi,
> if (ret)
> return ret;
>
> - /* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */
> - ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL,
> - mipi_dbi_formats,
> - ARRAY_SIZE(mipi_dbi_formats), mode,
> - rotation);
> + drm = tdev->drm;
> +
> + connector = drm_simple_connector_create(drm,
> DRM_MODE_CONNECTOR_VIRTUAL, mode, rotation);
Line length too long.
This goes for all calls to drm_simple_connector_create() in this patch.
We are loosing the comment:
"TODO: Maybe add DRM_MODE_CONNECTOR_SPI"
On purpose?
> + if (IS_ERR(connector))
> + return PTR_ERR(connector);
> +
> + ret = drm_simple_display_pipe_init(drm, &tdev->pipe, pipe_funcs,
> + mipi_dbi_formats,
> ARRAY_SIZE(mipi_dbi_formats),
> + modifiers, connector);
> if (ret)
> return ret;
>
> drm_plane_enable_fb_damage_clips(&tdev->pipe.plane);
>
> - tdev->drm->mode_config.preferred_depth = 16;
> + drm_simple_connector_set_mode_config(connector);
> + drm->mode_config.preferred_depth = 16;
> mipi->rotation = rotation;
>
> drm_mode_config_reset(tdev->drm);
> diff --git a/drivers/gpu/drm/tinydrm/repaper.c
> b/drivers/gpu/drm/tinydrm/repaper.c
> index 72d30151ecd8..1d551744cc9b 100644
> --- a/drivers/gpu/drm/tinydrm/repaper.c
> +++ b/drivers/gpu/drm/tinydrm/repaper.c
> @@ -924,12 +924,14 @@ static int repaper_probe(struct spi_device *spi)
> const struct drm_display_mode *mode;
> const struct spi_device_id *spi_id;
> const struct of_device_id *match;
> + struct drm_connector *connector;
> struct device *dev = &spi->dev;
> struct tinydrm_device *tdev;
> enum repaper_model model;
> const char *thermal_zone;
> struct repaper_epd *epd;
> size_t line_buffer_size;
> + struct drm_device *drm;
> int ret;
>
> match = of_match_device(repaper_of_match, dev);
> @@ -1069,13 +1071,19 @@ static int repaper_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - ret = tinydrm_display_pipe_init(tdev, &repaper_pipe_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL,
> - repaper_formats,
> - ARRAY_SIZE(repaper_formats), mode, 0);
> + drm = tdev->drm;
> +
> + connector = drm_simple_connector_create(drm,
> DRM_MODE_CONNECTOR_VIRTUAL, mode, 0);
> + if (IS_ERR(connector))
> + return PTR_ERR(connector);
> +
> + ret = drm_simple_display_pipe_init(drm, &tdev->pipe,
> &repaper_pipe_funcs,
> + repaper_formats,
> ARRAY_SIZE(repaper_formats),
> + NULL, connector);
In mipi-dbi modifiers are specified, like done in tiny-core.
But this, and the other drivers, do not specify any modifiers.
Any specific reason why it is needed in one place but not the others?
Note: I do not really know the purpose, and the comment is alone triggered
because there was a difference in the calls to drm_simple_display_pipe_init()
> if (ret)
> return ret;
>
> + drm_simple_connector_set_mode_config(connector);
No need to set preferred_depth here?
(It is not in the original driver, so it seems to work without it)
> drm_mode_config_reset(tdev->drm);
> spi_set_drvdata(spi, tdev);
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
