Re: [PATCH 2/2] drm: tiny: Add st7735s driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 06/09/2023 18:22, Stefan x Nilsson wrote:
> Add a driver for Sitronix st7735s display controller, as well as a
> Winstar wf0096atyaa3dnn0 0.96" 80x160 TFT panel.
> 
> The driver code is very similar to st7735r, but with adaptations for
> the pipe_enable function. There is also optional support to specify
> a power regulator for the display.
> 
> Signed-off-by: Stefan x Nilsson <stefan.x.nilsson@axis.com>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/gpu/drm/tiny/Kconfig   |  14 +++
>  drivers/gpu/drm/tiny/Makefile  |   1 +
>  drivers/gpu/drm/tiny/st7735s.c | 264 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 280 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c00b2b9086f2..f24295d691e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6733,6 +6733,7 @@ M:	Stefan x Nilsson <stefan.x.nilsson@axis.com>
>  S:	Maintained
>  T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	Documentation/devicetree/bindings/display/sitronix,st7735s.yaml
> +F:	drivers/gpu/drm/tiny/st7735s.c
>  
>  DRM DRIVER FOR SOLOMON SSD130X OLED DISPLAYS
>  M:	Javier Martinez Canillas <javierm@redhat.com>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index f6889f649bc1..2917f5412ddd 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -212,3 +212,17 @@ config TINYDRM_ST7735R
>  	  * Okaya RH128128T 1.44" 128x128 TFT
>  
>  	  If M is selected the module will be called st7735r.
> +
> +config TINYDRM_ST7735S
> +	tristate "DRM support for Sitronix ST7735S display panels"
> +	depends on DRM && SPI
> +	select DRM_KMS_HELPER
> +	select DRM_GEM_DMA_HELPER
> +	select DRM_MIPI_DBI
> +	select BACKLIGHT_CLASS_DEVICE
> +	help
> +	  DRM driver for Sitronix ST7735S with one of the following
> +	  LCDs:
> +	  * Winstar WF0096ATYAA3DNN0 0.96" 80x160 Color TFT
> +
> +	  If M is selected the module will be called st7735s.
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 76dde89a044b..2e805c5b6f16 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>  obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
>  obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
>  obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
> +obj-$(CONFIG_TINYDRM_ST7735S)		+= st7735s.o
> diff --git a/drivers/gpu/drm/tiny/st7735s.c b/drivers/gpu/drm/tiny/st7735s.c
> new file mode 100644
> index 000000000000..42290f4128db
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/st7735s.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * DRM driver for display panels connected to a Sitronix ST7735S
> + * display controller in SPI mode.
> + *
> + * Copyright (C) 2023 Axis Communications AB
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/dma-buf.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +
> +#define ST7735S_FRMCTR1		0xb1
> +#define ST7735S_FRMCTR2		0xb2
> +#define ST7735S_FRMCTR3		0xb3
> +#define ST7735S_INVCTR		0xb4
> +#define ST7735S_PWCTR1		0xc0
> +#define ST7735S_PWCTR2		0xc1
> +#define ST7735S_PWCTR3		0xc2
> +#define ST7735S_PWCTR4		0xc3
> +#define ST7735S_PWCTR5		0xc4
> +#define ST7735S_VMCTR1		0xc5
> +#define ST7735S_GAMCTRP1	0xe0
> +#define ST7735S_GAMCTRN1	0xe1
> +
> +#define ST7735S_MY	BIT(7)
> +#define ST7735S_MX	BIT(6)
> +#define ST7735S_MV	BIT(5)
> +#define ST7735S_RGB	BIT(3)

So you just duplicated st7735r. No need for new driver. Integrate your
work. Probably also no need for new bindings file (although this I did
not check)...

Best regards,
Krzysztof