Marcus Folkesson <marcus.folkes...@gmail.com> writes:
Hello Marcus,
I tried to apply your patches to drm-misc and found some issues, so I will
have to ask you to do a final re-spin. Sorry about that...
> Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
> The controller has a SPI, I2C and 8bit parallel interface, this
> driver is for the I2C interface only.
>
> Reviewed-by: Thomas Zimmermann <tzimmrm...@suse.de>
> Reviewed-by: Javier Martinez Canillas <javi...@redhat.com>
> Signed-off-by: Marcus Folkesson <marcus.folkes...@gmail.com>
> ---
> drivers/gpu/drm/tiny/Kconfig | 11 +
> drivers/gpu/drm/tiny/Makefile | 1 +
> drivers/gpu/drm/tiny/st7571-i2c.c | 1007
> +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1019 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index
> 94cbdb1337c07f1628a33599a7130369b9d59d98..e4a55482e3bcd3f6851df1d322a14cbe1f96adfb
> 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>
> If M is selected the module will be called st7586.
>
> +config DRM_ST7571_I2C
> + tristate "DRM support for Sitronix ST7571 display panels (I2C)"
> + depends on DRM && I2C && MMU
> + select DRM_GEM_SHMEM_HELPER
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> + DRM driver for Sitronix ST7571 panels controlled over I2C.
> +
> + if M is selected the module will be called st7571-i2c.
> +
checkpatch here complains about:
WARNING: please write a help paragraph that fully describes the config symbol
with at least 4 lines
#144: FILE: drivers/gpu/drm/tiny/Kconfig:215:
+config DRM_ST7571_I2C
+ tristate "DRM support for Sitronix ST7571 display panels (I2C)"
+ depends on DRM && I2C && MMU
+ select DRM_GEM_SHMEM_HELPER
+ select DRM_KMS_HELPER
+ select REGMAP_I2C
+ help
+ DRM driver for Sitronix ST7571 panels controlled over I2C.
but honestly I think is just silly and your explanation is good enough so
you could ignore it if you want.
> config TINYDRM_ST7735R
> tristate "DRM support for Sitronix ST7715R/ST7735R display panels"
> depends on DRM && SPI
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index
> 60816d2eb4ff93b87228ed8eadd60a0a33a1144b..eab7568c92c880cfdf7c2f0b9c4bfac4685dbe95
> 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_GM12U320) += gm12u320.o
> obj-$(CONFIG_DRM_OFDRM) += ofdrm.o
> obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
> obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o
> +obj-$(CONFIG_DRM_ST7571_I2C) += st7571-i2c.o
this chunk doesn't apply on top of the drm-misc/drm-next branch [1] due
the simpledrm driver being moved out of the tiny directory. Please do a
rebase on top of that branch when posting a v6.
[1]: https://gitlab.freedesktop.org/drm/misc/kernel/-/tree/drm-misc-next
[...]
> +++ b/drivers/gpu/drm/tiny/st7571-i2c.c
Please run ./scripts/checkpatch.pl --strict -f --
drivers/gpu/drm/tiny/st7571-i2c.c
since checkpatch complains about some issues. Other than some style nits, it
has some
good points IMO. In particular:
[...]
> +
> +static int st7571_fb_update_rect_grayscale(struct drm_framebuffer *fb,
> struct drm_rect *rect)
> +{
> + struct st7571_device *st7571 = drm_to_st7571(fb->dev);
> + uint32_t format = fb->format->format;
CHECK: Prefer kernel type 'u32' over 'uint32_t'
#523: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:348:
+ uint32_t format = fb->format->format;
[...]
> +
> +static const uint64_t st7571_primary_plane_fmtmods[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> +};
CHECK: Prefer kernel type 'u64' over 'uint64_t'
#611: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:436:
+static const uint64_t st7571_primary_plane_fmtmods[] = {
[...]
> +static void st7571_reset(struct st7571_device *st7571)
> +{
> + gpiod_set_value_cansleep(st7571->reset, 1);
> + udelay(20);
CHECK: usleep_range is preferred over udelay; see function description of
usleep_range() and udelay().
#993: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:818:
+ udelay(20);
Specially since this is not in atomic context AFAIK. The Delay and sleep
mechanisms [2] doc has a good explanation about the different functions
provided by the Linux kernel for this.
Feel free to keep my R-B tag if you decide that some of the checkpatch
suggestions are applicable. The most important bit for me is the rebase,
to allow your patch to be cleanly applied on the drm-misc-next branch.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
