Hi Michal,
On Mon, 14 Dec 2020 at 00:30, Michal Simek <michal.si...@xilinx.com> wrote:
>
>
>
> On 12. 12. 20 16:35, Simon Glass wrote:
> > Hi Michal,
> >
> > On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.si...@xilinx.com> wrote:
> >>
> >> Add support for the WiseChip Semiconductor Inc. (UG-6028GDEBF02) display
> >> using the SEPS525 (Syncoam) LCD Controller. Syncoam Seps525 PM-Oled is RGB
> >> 160x128 display. This driver has been tested through zynq-spi driver.
> >>
> >> ZynqMP> load mmc 1 100000 rainbow.bmp
> >> 61562 bytes read in 20 ms (2.9 MiB/s)
> >> ZynqMP> bmp info 100000
> >> Image size : 160 x 128
> >> Bits per pixel: 24
> >> Compression : 0
> >> ZynqMP> bmp display 100000
> >> ZynqMP> setenv stdout vidconsole
> >>
> >> Signed-off-by: Michal Simek <michal.si...@xilinx.com>
> >> ---
> >>
> >> MAINTAINERS | 1 +
> >> drivers/video/Kconfig | 6 +
> >> drivers/video/Makefile | 1 +
> >> drivers/video/seps525.c | 327 ++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 335 insertions(+)
> >> create mode 100644 drivers/video/seps525.c
> >
> > Reviewed-by: Simon Glass <s...@chromium.org>
> >
> > nits below
> >
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 2f908843da72..178a43f2b46e 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -589,6 +589,7 @@ F: drivers/spi/zynq_qspi.c
> >> F: drivers/spi/zynq_spi.c
> >> F: drivers/timer/cadence-ttc.c
> >> F: drivers/usb/host/ehci-zynq.c
> >> +F: drivers/video/seps525.c
> >> F: drivers/watchdog/cdns_wdt.c
> >> F: include/zynqmppl.h
> >> F: include/zynqmp_firmware.h
> >> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> >> index 998271b9b628..b354709890b6 100644
> >> --- a/drivers/video/Kconfig
> >> +++ b/drivers/video/Kconfig
> >> @@ -979,4 +979,10 @@ config VIDEO_VCXK
> >> This enables VCXK driver which can be used with VC2K, VC4K
> >> and VC8K devices on various boards from BuS Elektronik GmbH.
> >>
> >> +config SEPS525
> >> + bool "SEPS525"
> >> + help
> >> + Enable support for the Syncoam PM-OLED display driver (RGB
> >> 160x128).
> >> + Currently driver is supporting only SPI interface.
> >> +
> >> endmenu
> >> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> >> index 67a492a2d60d..6e4e35c58de7 100644
> >> --- a/drivers/video/Makefile
> >> +++ b/drivers/video/Makefile
> >> @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_SIMPLE) += simplefb.o
> >> obj-$(CONFIG_VIDEO_TEGRA20) += tegra.o
> >> obj-$(CONFIG_VIDEO_VCXK) += bus_vcxk.o
> >> obj-$(CONFIG_VIDEO_VESA) += vesa.o
> >> +obj-$(CONFIG_SEPS525) += seps525.o
> >>
> >> obj-y += bridge/
> >> obj-y += sunxi/
> >> diff --git a/drivers/video/seps525.c b/drivers/video/seps525.c
> >> new file mode 100644
> >> index 000000000000..b4b99b61fb2f
> >> --- /dev/null
> >> +++ b/drivers/video/seps525.c
> >> @@ -0,0 +1,327 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * FB driver for the WiseChip Semiconductor Inc. (UG-6028GDEBF02) display
> >> + * using the SEPS525 (Syncoam) LCD Controller
> >> + *
> >> + * Copyright (C) 2020 Xilinx Inc.
> >> + */
> >> +
> >> +#include <common.h>
> >> +#include <command.h>
> >> +#include <cpu_func.h>
> >> +#include <dm.h>
> >> +#include <errno.h>
> >> +#include <spi.h>
> >> +#include <video.h>
> >> +#include <asm/gpio.h>
> >> +#include <dm/device_compat.h>
> >> +#include <linux/delay.h>
> >> +
> >> +#define WIDTH 160
> >> +#define HEIGHT 128
> >> +
> >> +#define SEPS525_INDEX 0x00
> >> +#define SEPS525_STATUS_RD 0x01
> >> +#define SEPS525_OSC_CTL 0x02
> >> +#define SEPS525_IREF 0x80
> >> +#define SEPS525_CLOCK_DIV 0x03
> >> +#define SEPS525_REDUCE_CURRENT 0x04
> >> +#define SEPS525_SOFT_RST 0x05
> >> +#define SEPS525_DISP_ONOFF 0x06
> >> +#define SEPS525_PRECHARGE_TIME_R 0x08
> >> +#define SEPS525_PRECHARGE_TIME_G 0x09
> >> +#define SEPS525_PRECHARGE_TIME_B 0x0A
> >> +#define SEPS525_PRECHARGE_CURRENT_R 0x0B
> >> +#define SEPS525_PRECHARGE_CURRENT_G 0x0C
> >> +#define SEPS525_PRECHARGE_CURRENT_B 0x0D
> >> +#define SEPS525_DRIVING_CURRENT_R 0x10
> >> +#define SEPS525_DRIVING_CURRENT_G 0x11
> >> +#define SEPS525_DRIVING_CURRENT_B 0x12
> >> +#define SEPS525_DISPLAYMODE_SET 0x13
> >> +#define SEPS525_RGBIF 0x14
> >> +#define SEPS525_RGB_POL 0x15
> >> +#define SEPS525_MEMORY_WRITEMODE 0x16
> >> +#define SEPS525_MX1_ADDR 0x17
> >> +#define SEPS525_MX2_ADDR 0x18
> >> +#define SEPS525_MY1_ADDR 0x19
> >> +#define SEPS525_MY2_ADDR 0x1A
> >> +#define SEPS525_MEMORY_ACCESS_POINTER_X 0x20
> >> +#define SEPS525_MEMORY_ACCESS_POINTER_Y 0x21
> >> +#define SEPS525_DDRAM_DATA_ACCESS_PORT 0x22
> >> +#define SEPS525_GRAY_SCALE_TABLE_INDEX 0x50
> >> +#define SEPS525_GRAY_SCALE_TABLE_DATA 0x51
> >> +#define SEPS525_DUTY 0x28
> >> +#define SEPS525_DSL 0x29
> >> +#define SEPS525_D1_DDRAM_FAC 0x2E
> >> +#define SEPS525_D1_DDRAM_FAR 0x2F
> >> +#define SEPS525_D2_DDRAM_SAC 0x31
> >> +#define SEPS525_D2_DDRAM_SAR 0x32
> >> +#define SEPS525_SCR1_FX1 0x33
> >> +#define SEPS525_SCR1_FX2 0x34
> >> +#define SEPS525_SCR1_FY1 0x35
> >> +#define SEPS525_SCR1_FY2 0x36
> >> +#define SEPS525_SCR2_SX1 0x37
> >> +#define SEPS525_SCR2_SX2 0x38
> >> +#define SEPS525_SCR2_SY1 0x39
> >> +#define SEPS525_SCR2_SY2 0x3A
> >> +#define SEPS525_SCREEN_SAVER_CONTEROL 0x3B
> >> +#define SEPS525_SS_SLEEP_TIMER 0x3C
> >> +#define SEPS525_SCREEN_SAVER_MODE 0x3D
> >> +#define SEPS525_SS_SCR1_FU 0x3E
> >> +#define SEPS525_SS_SCR1_MXY 0x3F
> >> +#define SEPS525_SS_SCR2_FU 0x40
> >> +#define SEPS525_SS_SCR2_MXY 0x41
> >> +#define SEPS525_MOVING_DIRECTION 0x42
> >> +#define SEPS525_SS_SCR2_SX1 0x47
> >> +#define SEPS525_SS_SCR2_SX2 0x48
> >> +#define SEPS525_SS_SCR2_SY1 0x49
> >> +#define SEPS525_SS_SCR2_SY2 0x4A
> >> +
> >> +/* SEPS525_DISPLAYMODE_SET */
> >> +#define MODE_SWAP_BGR BIT(7)
> >> +#define MODE_SM BIT(6)
> >> +#define MODE_RD BIT(5)
> >> +#define MODE_CD BIT(4)
> >> +
> >> +struct seps525_priv {
> >
> > comments for this
>
> done.
>
> >
> >> + struct gpio_desc reset_gpio;
> >> + struct gpio_desc dc_gpio;
> >> + struct udevice *dev;
> >> +};
> >> +
> >> +static int seps525_spi_write_cmd(struct udevice *dev, u8 reg)
> >
> > u8 reg seems silly as registers are 32/64 bit and this may require
> > masking - just use uint?
>
> This is SPI based controller and there are only 0x4a addresses. That's
> why u8 should be fine. And 8bit interface is also used.
Just for the record, I'm making a different point. Functions should
generally use natural sizes so the compiler doesn't have to mask
registers. It depends on how you call the code, static/extern, etc.
but it can add to code size. Given that args are passed in registers
there is no space benefit, so I just don't understand the desire to
use an u8 arg.
>
> >
> >> +{
> >> + struct seps525_priv *priv = dev_get_priv(dev);
> >> + unsigned long flags = SPI_XFER_BEGIN;
> >> + u8 buf8 = reg;
> >> + int ret;
> >> +
> >> + ret = dm_gpio_set_value(&priv->dc_gpio, 0);
> >> + if (ret) {
> >> + dev_dbg(dev, "Failed to handle dc\n");
> >> + return ret;
> >> + }
> >> +
> >> + flags |= SPI_XFER_END;
> >
> > Up to you, but you don't really use this variable in these two functions.
>
> Fixed this and just get rid of this variable.
>
> Thanks,
> Michal
Regards,
Simon
