Re: [PATCH 2/2] drm/tiny: Add driver for the sharp LS027B7DH01 Memory LCD

[Mehdi Djait <mehdi.djait.k@gmail.com>]

Hi Thomas, 

Thank you for the review.

On Wed, Nov 29, 2023 at 05:21:19PM +0100, Thomas Zimmermann wrote:
> Hi,
> 
> thanks for the patches.
> 
> Am 29.11.23 um 15:29 schrieb Mehdi Djait:
> > Introduce a DRM driver for the sharp LS027B7DH01 Memory LCD.
> > 
> > LS027B7DH01 is a 2.7" 400x240 monochrome display connected to a SPI bus.
> > The drivers implements the Multiple Lines Data Update Mode.
> > External COM inversion is enabled using a PWM signal as input.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > ---
> >   MAINTAINERS                              |   7 +
> >   drivers/gpu/drm/tiny/Kconfig             |   8 +
> >   drivers/gpu/drm/tiny/Makefile            |   1 +
> >   drivers/gpu/drm/tiny/sharp-ls027b7dh01.c | 411 +++++++++++++++++++++++
> >   4 files changed, 427 insertions(+)
> >   create mode 100644 drivers/gpu/drm/tiny/sharp-ls027b7dh01.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 012df8ccf34e..fb859698bd3d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6832,6 +6832,13 @@ S:	Maintained
> >   F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> >   F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> > +DRM DRIVER FOR SHARP LS027B7DH01 Memory LCD
> > +M:	Mehdi Djait <mehdi.djait@bootlin.com>
> > +S:	Maintained
> > +T:	git git://anongit.freedesktop.org/drm/drm-misc
> > +F:	Documentation/devicetree/bindings/display/sharp,ls027b7dh01.yaml
> > +F:	drivers/gpu/drm/tiny/sharp-ls027b7dh01.c
> > +
> >   DRM DRIVER FOR SITRONIX ST7586 PANELS
> >   M:	David Lechner <david@lechnology.com>
> >   S:	Maintained
> > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> > index f6889f649bc1..a2ade06403ca 100644
> > --- a/drivers/gpu/drm/tiny/Kconfig
> > +++ b/drivers/gpu/drm/tiny/Kconfig
> > @@ -186,6 +186,14 @@ config TINYDRM_REPAPER
> >   	  If M is selected the module will be called repaper.
> > +config TINYDRM_SHARP_LS027B7DH01
> > +	tristate "DRM support for SHARP LS027B7DH01 display"
> > +	depends on DRM && SPI
> > +	select DRM_KMS_HELPER
> > +	select DRM_GEM_DMA_HELPER
> > +	help
> > +	  DRM driver for the SHARP LS027B7DD01 LCD display.
> > +
> >   config TINYDRM_ST7586
> >   	tristate "DRM support for Sitronix ST7586 display panels"
> >   	depends on DRM && SPI
> > diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> > index 76dde89a044b..b05df3afb231 100644
> > --- a/drivers/gpu/drm/tiny/Makefile
> > +++ b/drivers/gpu/drm/tiny/Makefile
> > @@ -14,5 +14,6 @@ obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
> >   obj-$(CONFIG_TINYDRM_ILI9486)		+= ili9486.o
> >   obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
> >   obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
> > +obj-$(CONFIG_TINYDRM_SHARP_LS027B7DH01)	+= sharp-ls027b7dh01.o
> >   obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
> >   obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
> > diff --git a/drivers/gpu/drm/tiny/sharp-ls027b7dh01.c b/drivers/gpu/drm/tiny/sharp-ls027b7dh01.c
> > new file mode 100644
> > index 000000000000..2f58865a5c78
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tiny/sharp-ls027b7dh01.c
> > @@ -0,0 +1,411 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sharp LS027B7DH01 Memory Display Driver
> > + *
> > + * Copyright (C) 2023 Andrew D'Angelo
> > + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> > + *
> > + * The Sharp Memory LCD requires an alternating signal to prevent the buildup of
> > + * a DC bias that would result in a Display that no longer can be updated. Two
> > + * modes for the generation of this signal are supported:
> > + *
> > + * Software, EXTMODE = Low: toggling the BIT(1) of the Command and writing it at
> > + * least once a second to the display.
> > + *
> > + * Hardware, EXTMODE = High: the alternating signal should be supplied on the
> > + * EXTCOMIN pin.
> > + *
> > + * In this driver the Hardware mode is implemented with a PWM signal.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pwm.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_damage_helper.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_fbdev_generic.h>
> > +#include <drm/drm_fb_dma_helper.h>
> > +#include <drm/drm_format_helper.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem_atomic_helper.h>
> > +#include <drm/drm_gem_dma_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> > +
> > +#define CMD_WRITE BIT(0)
> > +#define CMD_CLEAR BIT(2)
> > +
> > +struct sharp_ls027b7dh01 {
> > +	struct spi_device *spi;
> > +
> > +	struct drm_device drm;
> > +	struct drm_connector connector;
> > +	struct drm_simple_display_pipe pipe;
> 
> Could you please replace the simple pipe and its helpers with regular
> DRMhelpers. It should no tbe used in new drivers. It's an unnecessary
> indirection. Replacing is simple: copy the content of the data structure and
> its helpers into the driver. Maybe clean up the result, if necessary.


Could you please further explain where to copy the helper functions or give me
an example driver ? This is my first DRM driver and grepping did not help me much.

(Sorry for the delayed response)

--
Kind Regards
Mehdi Djait