Re: [PATCH 2/3] drm: Add an hx8367d tinydrm driver.

[Eric Anholt]

Noralf TrÃnnes <noralf@xxxxxxxxxxx> writes:

> Den 24.10.2018 20.43, skrev Eric Anholt:
>> I want to sort out support for tinydrm in vc4, so I needed to get a
>> tinydrm-appropriate panel working and this is what I had on hand.
>> This is derived from a combination of ili9341.c from tinydrm and
>> fb_hx8357d.c from staging's fbtft.  The register header is copied
>> directly from staging's fbtft, on the assumption that we will delete
>> that copy later.
>>
>> Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>
>> ---
>>   MAINTAINERS                       |   7 +
>>   drivers/gpu/drm/tinydrm/Kconfig   |  11 ++
>>   drivers/gpu/drm/tinydrm/Makefile  |   1 +
>>   drivers/gpu/drm/tinydrm/hx8357d.c | 261 ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tinydrm/hx8357d.h |  71 ++++++++
>>   5 files changed, 351 insertions(+)
>>   create mode 100644 drivers/gpu/drm/tinydrm/hx8357d.c
>>   create mode 100644 drivers/gpu/drm/tinydrm/hx8357d.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 39c3f6682ace..e78971e20a11 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4623,6 +4623,13 @@ S:	Maintained
>>   F:	drivers/gpu/drm/tinydrm/ili9225.c
>>   F:	Documentation/devicetree/bindings/display/ilitek,ili9225.txt
>>   
>> +DRM DRIVER FOR HX8357D PANELS
>> +M:	Eric Anholt <eric@xxxxxxxxxx>
>> +T:	git git://anongit.freedesktop.org/drm/drm-misc
>> +S:	Maintained
>> +F:	drivers/gpu/drm/tinydrm/hx8357d.c
>> +F:	Documentation/devicetree/bindings/display/himax,hx8357d.txt
>> +
>>   DRM DRIVER FOR INTEL I810 VIDEO CARDS
>>   S:	Orphan / Obsolete
>>   F:	drivers/gpu/drm/i810/
>> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
>> index 16f4b5c91f1b..2c408ac1a900 100644
>> --- a/drivers/gpu/drm/tinydrm/Kconfig
>> +++ b/drivers/gpu/drm/tinydrm/Kconfig
>> @@ -10,6 +10,17 @@ menuconfig DRM_TINYDRM
>>   config TINYDRM_MIPI_DBI
>>   	tristate
>>   
>> +config TINYDRM_HX8357D
>> +	tristate "DRM support for HX8357D display panels"
>> +	depends on DRM_TINYDRM && SPI
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	select TINYDRM_MIPI_DBI
>> +	help
>> +	  DRM driver for the following HX8357D panels:
>> +	  * YX350HV15-T 3.5" 340x350 TFT (Adafruit 3.5")
>> +
>> +	  If M is selected the module will be called hx8357d.
>> +
>>   config TINYDRM_ILI9225
>>   	tristate "DRM support for ILI9225 display panels"
>>   	depends on DRM_TINYDRM && SPI
>> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
>> index 14d99080665a..f823066f7743 100644
>> --- a/drivers/gpu/drm/tinydrm/Makefile
>> +++ b/drivers/gpu/drm/tinydrm/Makefile
>> @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_TINYDRM)		+= core/
>>   obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
>>   
>>   # Displays
>> +obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
>>   obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
>>   obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
>>   obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>> diff --git a/drivers/gpu/drm/tinydrm/hx8357d.c b/drivers/gpu/drm/tinydrm/hx8357d.c
>> new file mode 100644
>> index 000000000000..51d4da624d57
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tinydrm/hx8357d.c
>> @@ -0,0 +1,261 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * DRM driver for the HX8357D LCD controller
>> + *
>> + * Copyright 2018 Broadcom
>> + * Copyright 2018 David Lechner <david@xxxxxxxxxxxxxx>
>> + * Copyright 2016 Noralf TrÃnnes
>> + * Copyright (C) 2015 Adafruit Industries
>> + * Copyright (C) 2013 Christian Vogelgsang
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_modeset_helper.h>
>> +#include <drm/tinydrm/mipi-dbi.h>
>> +#include <drm/tinydrm/tinydrm-helpers.h>
>> +#include <video/mipi_display.h>
>> +#include "hx8357d.h"
>
> I prefer to have the defines in the driver instead of an extra header file.
> The reason is that usually only a handful of defines are actually used,
> in this case it's 9.

Yeah.  Also, that license on the header is pretty sketchy.  I've written
my own defines based on reading the HX8357D spec and included it in the
driver instead.

>> +
>> +static const struct spi_device_id hx8357d_id[] = {
>> +	{ "hx8357d", 0 },
>
> Last time I tried, module autoloading didn't work unless this contains
> the last part of the compatible. In this case: "yx350hv15".
> Have you checked that autoloading does work?

I hadn't since I changed the compatible string to adafruit.  Thanks!

> Otherwise this looks good:
> Reviewed-by: Noralf TrÃnnes <noralf@xxxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature