* [PATCH 0/3] video: seps525: Add new driver for seps525 OLED display
@ 2020-12-03 9:12 Michal Simek
2020-12-03 9:12 ` [PATCH 1/3] video: Introduce video_sync call Michal Simek
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Michal Simek @ 2020-12-03 9:12 UTC (permalink / raw)
To: u-boot
Hi,
This driver is connected via spi on one ZynqMP board. Only 8bit SPI
connection is supported now. Spi zynq driver was used for testing this
driver.
We have tested load image via BMP command and also using it as console as
is visible from log in the last patch.
Thanks,
Michal
Michal Simek (2):
video: Introduce video_sync call
video: seps525: Add seps525 SPI driver
Vikhyat Goyal (1):
video: seps525: Add dt binding description
MAINTAINERS | 2 +
.../video/syncoam,seps525.txt | 24 ++
drivers/video/Kconfig | 6 +
drivers/video/Makefile | 1 +
drivers/video/seps525.c | 327 ++++++++++++++++++
drivers/video/video-uclass.c | 5 +
include/video.h | 13 +
7 files changed, 378 insertions(+)
create mode 100644 doc/device-tree-bindings/video/syncoam,seps525.txt
create mode 100644 drivers/video/seps525.c
--
2.29.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] video: Introduce video_sync call
2020-12-03 9:12 [PATCH 0/3] video: seps525: Add new driver for seps525 OLED display Michal Simek
@ 2020-12-03 9:12 ` Michal Simek
2020-12-12 15:35 ` Simon Glass
2020-12-03 9:13 ` [PATCH 2/3] video: seps525: Add dt binding description Michal Simek
2020-12-03 9:13 ` [PATCH 3/3] video: seps525: Add seps525 SPI driver Michal Simek
2 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-12-03 9:12 UTC (permalink / raw)
To: u-boot
Some drivers like LCD connected via SPI requires explicit sync function
which copy framebuffer content over SPI to controller to display.
This hook doesn't exist yet that's why introduce it via video operations.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Simon: Please review this. I didn't find existing way how this can be done
that's why I am introducing this hook. Also maybe name can be named a
little bit differently. That's why waiting for better suggestion.
---
drivers/video/video-uclass.c | 5 +++++
include/video.h | 13 +++++++++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index 650891e49dd0..ba52a6c7125b 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert)
/* Flush video activity to the caches */
void video_sync(struct udevice *vid, bool force)
{
+ struct video_ops *ops = video_get_ops(vid);
+
+ if (ops && ops->video_sync)
+ (void)ops->video_sync(vid);
+
/*
* flush_dcache_range() is declared in common.h but it seems that some
* architectures do not actually implement it. Is there a way to find
diff --git a/include/video.h b/include/video.h
index 9d09d2409af6..acac3f6b3c8d 100644
--- a/include/video.h
+++ b/include/video.h
@@ -115,7 +115,20 @@ struct video_priv {
};
/* Placeholder - there are no video operations at present */
+/**
+ * struct video_ops - structure for keeping video operations
+ */
struct video_ops {
+ /**
+ * video_sync - Synchronize FB with device
+ *
+ * Some device like SPI based LCD displays needs synchronization when
+ * data in an FB is available. For these devices implement video_sync
+ * hook to call a sync function
+ *
+ * @vid: Video device udevice structure
+ */
+ int (*video_sync)(struct udevice *vid);
};
#define video_get_ops(dev) ((struct video_ops *)(dev)->driver->ops)
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] video: seps525: Add dt binding description
2020-12-03 9:12 [PATCH 0/3] video: seps525: Add new driver for seps525 OLED display Michal Simek
2020-12-03 9:12 ` [PATCH 1/3] video: Introduce video_sync call Michal Simek
@ 2020-12-03 9:13 ` Michal Simek
2020-12-12 15:35 ` Simon Glass
2020-12-03 9:13 ` [PATCH 3/3] video: seps525: Add seps525 SPI driver Michal Simek
2 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-12-03 9:13 UTC (permalink / raw)
To: u-boot
From: Vikhyat Goyal <vikhyat.goyal@xilinx.com>
Added dt binding for seps525 display driver.
Signed-off-by: Vikhyat Goyal <vikhyat.goyal@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
MAINTAINERS | 1 +
.../video/syncoam,seps525.txt | 24 +++++++++++++++++++
2 files changed, 25 insertions(+)
create mode 100644 doc/device-tree-bindings/video/syncoam,seps525.txt
diff --git a/MAINTAINERS b/MAINTAINERS
index 874cf2c0e576..2f908843da72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -547,6 +547,7 @@ S: Maintained
T: git https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
F: arch/arm/mach-zynq/
F: doc/board/xilinx/
+F: doc/device-tree-bindings/video/syncoam,seps525.txt
F: drivers/clk/clk_zynq.c
F: drivers/fpga/zynqpl.c
F: drivers/gpio/zynq_gpio.c
diff --git a/doc/device-tree-bindings/video/syncoam,seps525.txt b/doc/device-tree-bindings/video/syncoam,seps525.txt
new file mode 100644
index 000000000000..e1e0db9d71fb
--- /dev/null
+++ b/doc/device-tree-bindings/video/syncoam,seps525.txt
@@ -0,0 +1,24 @@
+spi based seps525 framebuffer display driver
+
+Driver for seps525 display controller (in spi mode), This binding supports selection
+of spi chipselect, spi max frequency, gpio to drive dc and reset pin of seps525
+controller and spi transaction bit length.
+
+Required properties:
+- compatible: "syncoam,seps525"
+- reg: Specifies the chip-select the seps525 is connected to on the spi bus
+- reset-gpios: gpio connected to reset pin of seps525 controller.
+- dc-gpios: gpio connected to dc pin of seps525 controller:
+- buswidth: bitlength of each spi transaction
+
+Example:
+ displayspi at 0 {
+ compatible = "syncoam,seps525";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+ spi-cpol;
+ spi-cpha;
+ buswidth = <8>;
+ reset-gpios = <&gpio 0x1c GPIO_ACTIVE_LOW>;
+ dc-gpios = <&gpio 0x1b GPIO_ACTIVE_HIGH>;
+ };
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] video: seps525: Add seps525 SPI driver
2020-12-03 9:12 [PATCH 0/3] video: seps525: Add new driver for seps525 OLED display Michal Simek
2020-12-03 9:12 ` [PATCH 1/3] video: Introduce video_sync call Michal Simek
2020-12-03 9:13 ` [PATCH 2/3] video: seps525: Add dt binding description Michal Simek
@ 2020-12-03 9:13 ` Michal Simek
2020-12-12 15:35 ` Simon Glass
2 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-12-03 9:13 UTC (permalink / raw)
To: u-boot
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.simek@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
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 {
+ struct gpio_desc reset_gpio;
+ struct gpio_desc dc_gpio;
+ struct udevice *dev;
+};
+
+static int seps525_spi_write_cmd(struct udevice *dev, u8 reg)
+{
+ 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;
+
+ ret = dm_spi_xfer(dev, 8, &buf8, NULL, flags);
+ if (ret)
+ dev_dbg(dev, "Failed to write command\n");
+
+ return ret;
+}
+
+static int seps525_spi_write_data(struct udevice *dev, u8 val)
+{
+ struct seps525_priv *priv = dev_get_priv(dev);
+ unsigned long flags = SPI_XFER_BEGIN;
+ u8 buf8 = val;
+ int ret;
+
+ ret = dm_gpio_set_value(&priv->dc_gpio, 1);
+ if (ret) {
+ dev_dbg(dev, "Failed to handle dc\n");
+ return ret;
+ }
+
+ flags |= SPI_XFER_END;
+
+ ret = dm_spi_xfer(dev, 8, &buf8, NULL, flags);
+ if (ret)
+ dev_dbg(dev, "Failed to write data\n");
+
+ return ret;
+}
+
+static void seps525_spi_write(struct udevice *dev, u8 reg, u8 val)
+{
+ (void)seps525_spi_write_cmd(dev, reg);
+ (void)seps525_spi_write_data(dev, val);
+}
+
+static int seps525_display_init(struct udevice *dev)
+{
+ /* Disable Oscillator Power Down */
+ seps525_spi_write(dev, SEPS525_REDUCE_CURRENT, 0x03);
+ mdelay(5);
+
+ /* Set Normal Driving Current */
+ seps525_spi_write(dev, SEPS525_REDUCE_CURRENT, 0x00);
+ mdelay(5);
+
+ seps525_spi_write(dev, SEPS525_SCREEN_SAVER_CONTEROL, 0x00);
+ /* Set EXPORT1 Pin at Internal Clock */
+ seps525_spi_write(dev, SEPS525_OSC_CTL, 0x01);
+ /* Set Clock as 120 Frames/Sec */
+ seps525_spi_write(dev, SEPS525_CLOCK_DIV, 0x90);
+ /* Set Reference Voltage Controlled by External Resister */
+ seps525_spi_write(dev, SEPS525_IREF, 0x01);
+
+ /* precharge time R G B */
+ seps525_spi_write(dev, SEPS525_PRECHARGE_TIME_R, 0x04);
+ seps525_spi_write(dev, SEPS525_PRECHARGE_TIME_G, 0x05);
+ seps525_spi_write(dev, SEPS525_PRECHARGE_TIME_B, 0x05);
+
+ /* precharge current R G B (uA) */
+ seps525_spi_write(dev, SEPS525_PRECHARGE_CURRENT_R, 0x9D);
+ seps525_spi_write(dev, SEPS525_PRECHARGE_CURRENT_G, 0x8C);
+ seps525_spi_write(dev, SEPS525_PRECHARGE_CURRENT_B, 0x57);
+
+ /* driving current R G B (uA) */
+ seps525_spi_write(dev, SEPS525_DRIVING_CURRENT_R, 0x56);
+ seps525_spi_write(dev, SEPS525_DRIVING_CURRENT_G, 0x4D);
+ seps525_spi_write(dev, SEPS525_DRIVING_CURRENT_B, 0x46);
+ /* Set Color Sequence */
+ seps525_spi_write(dev, SEPS525_DISPLAYMODE_SET, 0x00);
+ /* Set MCU Interface Mode */
+ seps525_spi_write(dev, SEPS525_RGBIF, 0x01);
+ /* Set Memory Write Mode */
+ seps525_spi_write(dev, SEPS525_MEMORY_WRITEMODE, 0x66);
+ /* 1/128 Duty (0x0F~0x7F) */
+ seps525_spi_write(dev, SEPS525_DUTY, 0x7F);
+ /* Set Mapping RAM Display Start Line (0x00~0x7F) */
+ seps525_spi_write(dev, SEPS525_DSL, 0x00);
+ /* Display On (0x00/0x01) */
+ seps525_spi_write(dev, SEPS525_DISP_ONOFF, 0x01);
+ /* Set All Internal Register Value as Normal Mode */
+ seps525_spi_write(dev, SEPS525_SOFT_RST, 0x00);
+ /* Set RGB Interface Polarity as Active Low */
+ seps525_spi_write(dev, SEPS525_RGB_POL, 0x00);
+
+ /* Enable access for data */
+ (void)seps525_spi_write_cmd(dev, SEPS525_DDRAM_DATA_ACCESS_PORT);
+
+ return 0;
+}
+
+static int seps525_spi_startup(struct udevice *dev)
+{
+ struct seps525_priv *priv = dev_get_priv(dev);
+ int ret;
+
+ ret = dm_gpio_set_value(&priv->reset_gpio, 1);
+ if (ret)
+ return ret;
+
+ ret = dm_gpio_set_value(&priv->reset_gpio, 0);
+ if (ret)
+ return ret;
+
+ ret = dm_spi_claim_bus(dev);
+ if (ret) {
+ dev_err(dev, "Failed to claim SPI bus: %d\n", ret);
+ return ret;
+ }
+
+ ret = seps525_display_init(dev);
+ if (ret)
+ return ret;
+
+ dm_spi_release_bus(dev);
+
+ return 0;
+}
+
+static int seps525_sync(struct udevice *vid)
+{
+ struct video_priv *uc_priv = dev_get_uclass_priv(vid);
+ struct seps525_priv *priv = dev_get_priv(vid);
+ struct udevice *dev = priv->dev;
+ int i, ret;
+ u8 data1, data2;
+ u8 *start = uc_priv->fb;
+
+ ret = dm_spi_claim_bus(dev);
+ if (ret) {
+ dev_err(dev, "Failed to claim SPI bus: %d\n", ret);
+ return ret;
+ }
+
+ /* start position X,Y */
+ seps525_spi_write(dev, SEPS525_MEMORY_ACCESS_POINTER_X, 0);
+ seps525_spi_write(dev, SEPS525_MEMORY_ACCESS_POINTER_Y, 0);
+
+ /* Enable access for data */
+ (void)seps525_spi_write_cmd(dev, SEPS525_DDRAM_DATA_ACCESS_PORT);
+
+ for (i = 0; i < (uc_priv->xsize * uc_priv->ysize); i++) {
+ data2 = *start++;
+ data1 = *start++;
+ (void)seps525_spi_write_data(dev, data1);
+ (void)seps525_spi_write_data(dev, data2);
+ }
+
+ dm_spi_release_bus(dev);
+
+ return 0;
+}
+
+static int seps525_probe(struct udevice *dev)
+{
+ struct video_priv *uc_priv = dev_get_uclass_priv(dev);
+ struct seps525_priv *priv = dev_get_priv(dev);
+ u32 buswidth;
+ int ret;
+
+ buswidth = dev_read_u32_default(dev, "buswidth", 0);
+ if (buswidth != 8) {
+ dev_err(dev, "Only 8bit buswidth is supported now");
+ return -EINVAL;
+ }
+
+ ret = gpio_request_by_name(dev, "reset-gpios", 0,
+ &priv->reset_gpio, GPIOD_IS_OUT);
+ if (ret) {
+ dev_err(dev, "missing reset GPIO\n");
+ return ret;
+ }
+
+ ret = gpio_request_by_name(dev, "dc-gpios", 0,
+ &priv->dc_gpio, GPIOD_IS_OUT);
+ if (ret) {
+ dev_err(dev, "missing dc GPIO\n");
+ return ret;
+ }
+
+ uc_priv->bpix = VIDEO_BPP16;
+ uc_priv->xsize = WIDTH;
+ uc_priv->ysize = HEIGHT;
+ uc_priv->rot = 0;
+
+ priv->dev = dev;
+
+ ret = seps525_spi_startup(dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int seps525_bind(struct udevice *dev)
+{
+ struct video_uc_platdata *plat = dev_get_uclass_platdata(dev);
+
+ plat->size = WIDTH * HEIGHT * 16;
+
+ return 0;
+}
+
+static const struct video_ops seps525_ops = {
+ .video_sync = seps525_sync,
+};
+
+static const struct udevice_id seps525_ids[] = {
+ { .compatible = "syncoam,seps525" },
+ { }
+};
+
+U_BOOT_DRIVER(seps525_video) = {
+ .name = "seps525_video",
+ .id = UCLASS_VIDEO,
+ .of_match = seps525_ids,
+ .ops = &seps525_ops,
+ .platdata_auto_alloc_size = sizeof(struct video_uc_platdata),
+ .bind = seps525_bind,
+ .probe = seps525_probe,
+ .priv_auto_alloc_size = sizeof(struct seps525_priv),
+};
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/3] video: Introduce video_sync call
2020-12-03 9:12 ` [PATCH 1/3] video: Introduce video_sync call Michal Simek
@ 2020-12-12 15:35 ` Simon Glass
2020-12-14 8:39 ` Michal Simek
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2020-12-12 15:35 UTC (permalink / raw)
To: u-boot
Hi Michal,
On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Some drivers like LCD connected via SPI requires explicit sync function
> which copy framebuffer content over SPI to controller to display.
> This hook doesn't exist yet that's why introduce it via video operations.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Simon: Please review this. I didn't find existing way how this can be done
> that's why I am introducing this hook. Also maybe name can be named a
> little bit differently. That's why waiting for better suggestion.
> ---
> drivers/video/video-uclass.c | 5 +++++
> include/video.h | 13 +++++++++++++
> 2 files changed, 18 insertions(+)
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index 650891e49dd0..ba52a6c7125b 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert)
> /* Flush video activity to the caches */
> void video_sync(struct udevice *vid, bool force)
> {
> + struct video_ops *ops = video_get_ops(vid);
> +
> + if (ops && ops->video_sync)
> + (void)ops->video_sync(vid);
We should update video_sync() to return an error.
> +
> /*
> * flush_dcache_range() is declared in common.h but it seems that some
> * architectures do not actually implement it. Is there a way to find
> diff --git a/include/video.h b/include/video.h
> index 9d09d2409af6..acac3f6b3c8d 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -115,7 +115,20 @@ struct video_priv {
> };
>
> /* Placeholder - there are no video operations at present */
Need to update comment
> +/**
> + * struct video_ops - structure for keeping video operations
> + */
> struct video_ops {
> + /**
> + * video_sync - Synchronize FB with device
> + *
> + * Some device like SPI based LCD displays needs synchronization when
> + * data in an FB is available. For these devices implement video_sync
> + * hook to call a sync function
> + *
> + * @vid: Video device udevice structure
@return
> + */
> + int (*video_sync)(struct udevice *vid);
> };
>
> #define video_get_ops(dev) ((struct video_ops *)(dev)->driver->ops)
> --
> 2.29.2
>
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] video: seps525: Add dt binding description
2020-12-03 9:13 ` [PATCH 2/3] video: seps525: Add dt binding description Michal Simek
@ 2020-12-12 15:35 ` Simon Glass
0 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2020-12-12 15:35 UTC (permalink / raw)
To: u-boot
On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote:
>
> From: Vikhyat Goyal <vikhyat.goyal@xilinx.com>
>
> Added dt binding for seps525 display driver.
>
> Signed-off-by: Vikhyat Goyal <vikhyat.goyal@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> MAINTAINERS | 1 +
> .../video/syncoam,seps525.txt | 24 +++++++++++++++++++
> 2 files changed, 25 insertions(+)
> create mode 100644 doc/device-tree-bindings/video/syncoam,seps525.txt
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] video: seps525: Add seps525 SPI driver
2020-12-03 9:13 ` [PATCH 3/3] video: seps525: Add seps525 SPI driver Michal Simek
@ 2020-12-12 15:35 ` Simon Glass
2020-12-14 7:30 ` Michal Simek
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2020-12-12 15:35 UTC (permalink / raw)
To: u-boot
Hi Michal,
On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@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.simek@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 <sjg@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
> + 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?
> +{
> + 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.
> +
> + ret = dm_spi_xfer(dev, 8, &buf8, NULL, flags);
> + if (ret)
> + dev_dbg(dev, "Failed to write command\n");
> +
> + return ret;
> +}
> +
> +static int seps525_spi_write_data(struct udevice *dev, u8 val)
> +{
> + struct seps525_priv *priv = dev_get_priv(dev);
> + unsigned long flags = SPI_XFER_BEGIN;
> + u8 buf8 = val;
> + int ret;
> +
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] video: seps525: Add seps525 SPI driver
2020-12-12 15:35 ` Simon Glass
@ 2020-12-14 7:30 ` Michal Simek
2020-12-15 3:55 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-12-14 7:30 UTC (permalink / raw)
To: u-boot
On 12. 12. 20 16:35, Simon Glass wrote:
> Hi Michal,
>
> On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@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.simek@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 <sjg@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.
>
>> +{
>> + 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] video: Introduce video_sync call
2020-12-12 15:35 ` Simon Glass
@ 2020-12-14 8:39 ` Michal Simek
2020-12-15 3:55 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-12-14 8:39 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 12. 12. 20 16:35, Simon Glass wrote:
> Hi Michal,
>
> On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Some drivers like LCD connected via SPI requires explicit sync function
>> which copy framebuffer content over SPI to controller to display.
>> This hook doesn't exist yet that's why introduce it via video operations.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Simon: Please review this. I didn't find existing way how this can be done
>> that's why I am introducing this hook. Also maybe name can be named a
>> little bit differently. That's why waiting for better suggestion.
>> ---
>> drivers/video/video-uclass.c | 5 +++++
>> include/video.h | 13 +++++++++++++
>> 2 files changed, 18 insertions(+)
>
>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>> index 650891e49dd0..ba52a6c7125b 100644
>> --- a/drivers/video/video-uclass.c
>> +++ b/drivers/video/video-uclass.c
>> @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert)
>> /* Flush video activity to the caches */
>> void video_sync(struct udevice *vid, bool force)
>> {
>> + struct video_ops *ops = video_get_ops(vid);
>> +
>> + if (ops && ops->video_sync)
>> + (void)ops->video_sync(vid);
>
> We should update video_sync() to return an error.
I sent v2 with fixing this.
>
>> +
>> /*
>> * flush_dcache_range() is declared in common.h but it seems that some
>> * architectures do not actually implement it. Is there a way to find
>> diff --git a/include/video.h b/include/video.h
>> index 9d09d2409af6..acac3f6b3c8d 100644
>> --- a/include/video.h
>> +++ b/include/video.h
>> @@ -115,7 +115,20 @@ struct video_priv {
>> };
>>
>> /* Placeholder - there are no video operations at present */
>
> Need to update comment
Removed.
>
>> +/**
>> + * struct video_ops - structure for keeping video operations
>> + */
>> struct video_ops {
>> + /**
>> + * video_sync - Synchronize FB with device
>> + *
>> + * Some device like SPI based LCD displays needs synchronization when
>> + * data in an FB is available. For these devices implement video_sync
>> + * hook to call a sync function
>> + *
>> + * @vid: Video device udevice structure
>
> @return
I have looked at kernel-doc format and completely redo this to pass
kernel-doc script.
There are a lot of issues reported by that script. Would be good to run
it by patman automatically to be more visible for everybody.
Thanks,
Michal
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] video: Introduce video_sync call
2020-12-14 8:39 ` Michal Simek
@ 2020-12-15 3:55 ` Simon Glass
0 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2020-12-15 3:55 UTC (permalink / raw)
To: u-boot
Hi Michal,
On Mon, 14 Dec 2020 at 01:39, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi Simon,
>
> On 12. 12. 20 16:35, Simon Glass wrote:
> > Hi Michal,
> >
> > On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> Some drivers like LCD connected via SPI requires explicit sync function
> >> which copy framebuffer content over SPI to controller to display.
> >> This hook doesn't exist yet that's why introduce it via video operations.
> >>
> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> >> ---
> >>
> >> Simon: Please review this. I didn't find existing way how this can be done
> >> that's why I am introducing this hook. Also maybe name can be named a
> >> little bit differently. That's why waiting for better suggestion.
> >> ---
> >> drivers/video/video-uclass.c | 5 +++++
> >> include/video.h | 13 +++++++++++++
> >> 2 files changed, 18 insertions(+)
> >
> >> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> >> index 650891e49dd0..ba52a6c7125b 100644
> >> --- a/drivers/video/video-uclass.c
> >> +++ b/drivers/video/video-uclass.c
> >> @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert)
> >> /* Flush video activity to the caches */
> >> void video_sync(struct udevice *vid, bool force)
> >> {
> >> + struct video_ops *ops = video_get_ops(vid);
> >> +
> >> + if (ops && ops->video_sync)
> >> + (void)ops->video_sync(vid);
> >
> > We should update video_sync() to return an error.
>
> I sent v2 with fixing this.
>
> >
> >> +
> >> /*
> >> * flush_dcache_range() is declared in common.h but it seems that some
> >> * architectures do not actually implement it. Is there a way to find
> >> diff --git a/include/video.h b/include/video.h
> >> index 9d09d2409af6..acac3f6b3c8d 100644
> >> --- a/include/video.h
> >> +++ b/include/video.h
> >> @@ -115,7 +115,20 @@ struct video_priv {
> >> };
> >>
> >> /* Placeholder - there are no video operations at present */
> >
> > Need to update comment
>
> Removed.
>
> >
> >> +/**
> >> + * struct video_ops - structure for keeping video operations
> >> + */
> >> struct video_ops {
> >> + /**
> >> + * video_sync - Synchronize FB with device
> >> + *
> >> + * Some device like SPI based LCD displays needs synchronization when
> >> + * data in an FB is available. For these devices implement video_sync
> >> + * hook to call a sync function
> >> + *
> >> + * @vid: Video device udevice structure
> >
> > @return
>
> I have looked at kernel-doc format and completely redo this to pass
> kernel-doc script.
> There are a lot of issues reported by that script. Would be good to run
> it by patman automatically to be more visible for everybody.
Yes that would be nice.
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] video: seps525: Add seps525 SPI driver
2020-12-14 7:30 ` Michal Simek
@ 2020-12-15 3:55 ` Simon Glass
2020-12-15 14:20 ` Michal Simek
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2020-12-15 3:55 UTC (permalink / raw)
To: u-boot
Hi Michal,
On Mon, 14 Dec 2020 at 00:30, Michal Simek <michal.simek@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.simek@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.simek@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 <sjg@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] video: seps525: Add seps525 SPI driver
2020-12-15 3:55 ` Simon Glass
@ 2020-12-15 14:20 ` Michal Simek
0 siblings, 0 replies; 12+ messages in thread
From: Michal Simek @ 2020-12-15 14:20 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 15. 12. 20 4:55, Simon Glass wrote:
> Hi Michal,
>
> On Mon, 14 Dec 2020 at 00:30, Michal Simek <michal.simek@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.simek@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.simek@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 <sjg@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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-12-15 14:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-03 9:12 [PATCH 0/3] video: seps525: Add new driver for seps525 OLED display Michal Simek
2020-12-03 9:12 ` [PATCH 1/3] video: Introduce video_sync call Michal Simek
2020-12-12 15:35 ` Simon Glass
2020-12-14 8:39 ` Michal Simek
2020-12-15 3:55 ` Simon Glass
2020-12-03 9:13 ` [PATCH 2/3] video: seps525: Add dt binding description Michal Simek
2020-12-12 15:35 ` Simon Glass
2020-12-03 9:13 ` [PATCH 3/3] video: seps525: Add seps525 SPI driver Michal Simek
2020-12-12 15:35 ` Simon Glass
2020-12-14 7:30 ` Michal Simek
2020-12-15 3:55 ` Simon Glass
2020-12-15 14:20 ` Michal Simek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox