Hi Simon,
On 15. 12. 20 4:55, Simon Glass wrote:
> 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.
I can't see any size difference for arm64 but not a problem to make it
32bit width. I found one more issue with cls that's why I have fixed
this in v3 and add one more patch there too.
Thanks,
Michal
