Hi Simon, On Fri, Mar 30, 2018 at 12:42 AM, Simon Glass <[email protected]> wrote: > Hi Mario, > > On 28 March 2018 at 20:39, Mario Six <[email protected]> wrote: >> Add a driver for IHS OSDs on IHS FPGAs. >> >> Signed-off-by: Mario Six <[email protected]> >> --- >> drivers/misc/Kconfig | 6 +- >> drivers/video/Kconfig | 7 ++ >> drivers/video/Makefile | 1 + >> drivers/video/ihs_video_out.c | 277 >> ++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 290 insertions(+), 1 deletion(-) >> create mode 100644 drivers/video/ihs_video_out.c >> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig >> index d774569cbc..742fee3b76 100644 >> --- a/drivers/misc/Kconfig >> +++ b/drivers/misc/Kconfig >> @@ -263,5 +263,9 @@ config SYS_I2C_EEPROM_ADDR_OVERFLOW >> >> endif >> >> - >> +config IHS_VIDEO_OUT >> + bool "Enable IHS video out driver" >> + depends on MISC >> + help >> + Support for IHS video out. > > What is IHS? Please greatly expand this help. > >> endmenu >> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig >> index da60bbe692..40a881cf56 100644 >> --- a/drivers/video/Kconfig >> +++ b/drivers/video/Kconfig >> @@ -668,4 +668,11 @@ config OSD >> This supports drivers that provide a OSD (on-screen display), >> which >> is a (usually text-oriented) graphics buffer to show information >> on >> a display. >> + >> +config IHS_VIDEO_OUT >> + bool "Enable IHS video out driver" >> + depends on OSD >> + help >> + Support for IHS video out OSD. > > Same here > >> + >> endmenu >> diff --git a/drivers/video/Makefile b/drivers/video/Makefile >> index 209d5e3a75..0d633e03d4 100644 >> --- a/drivers/video/Makefile >> +++ b/drivers/video/Makefile >> @@ -59,6 +59,7 @@ obj-${CONFIG_VIDEO_ROCKCHIP} += rockchip/ >> obj-${CONFIG_VIDEO_STM32} += stm32/ >> >> obj-${CONFIG_OSD} += video_osd-uclass.o >> +obj-$(CONFIG_IHS_VIDEO_OUT) += ihs_video_out.o >> >> obj-y += bridge/ >> obj-y += sunxi/ >> diff --git a/drivers/video/ihs_video_out.c b/drivers/video/ihs_video_out.c >> new file mode 100644 >> index 0000000000..3efb343c02 >> --- /dev/null >> +++ b/drivers/video/ihs_video_out.c >> @@ -0,0 +1,277 @@ >> +/* >> + * (C) Copyright 2017 >> + * Mario Six, Guntermann & Drunck GmbH, [email protected] >> + * >> + * based on the gdsys osd driver, which is >> + * >> + * (C) Copyright 2010 >> + * Dirk Eibach, Guntermann & Drunck GmbH, [email protected] >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <display.h> >> +#include <dm.h> >> +#include <fpgamap.h> >> +#include <video_osd.h> >> +#include <asm/gpio.h> >> + >> +#include "../misc/gdsys_soc.h" >> +#include "../video/logicore_dp_tx.h" >> + >> +#define MAX_X_CHARS 53 >> +#define MAX_Y_CHARS 26 >> +#define MAX_VIDEOMEM_WIDTH (2 * 64) >> +#define MAX_VIDEOMEM_HEIGHT (2 * 32) >> + >> +enum { >> + REG_VERSION = 0x00, >> + REG_FEATURES = 0x02, >> + REG_CONTROL = 0x04, >> + REG_XY_SIZE = 0x06, >> + REG_XY_SCALE = 0x08, >> + REG_X_POS = 0x0A, >> + REG_Y_POS = 0x0C, >> +}; >> + >> +struct ihs_video_out_priv { > > This struct needs a comment describing the members > >> + int addr; >> + int osd_base; >> + int osd_buffer_base; >> + int osd_buffer_size; >> + uint base_width; >> + uint base_height; >> + uint res_x; >> + uint res_y; >> + int sync_src; >> + struct udevice *dp_tx; >> + struct udevice *clk_gen; >> +}; >> + >> +/** >> + * struct ihs_video_out_data - information about a IHS OSD instance >> + * >> + * @width Maximum width of the OSD screen in characters. >> + * @heigth Maximum height of the OSD screen in characters. >> + */ >> +struct ihs_video_out_data { >> + uint width; >> + uint height; >> +}; >> + >> +static const struct udevice_id ihs_video_out_ids[] = { >> + { .compatible = "gdsys,ihs_video_out" }, >> + { } >> +}; >> + >> +static int set_control(struct udevice *dev, u16 value) >> +{ >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + struct udevice *fpga; >> + >> + gdsys_soc_get_fpga(dev, &fpga); >> + >> + if (priv->sync_src) >> + value |= ((priv->sync_src & 0x7) << 8); >> + >> + fpgamap_write(fpga, priv->osd_base + REG_CONTROL, &value, >> + FPGAMAP_SIZE_16); >> + >> + return 0; >> +} >> + >> +static void print_info(struct udevice *dev) >> +{ > > Instead of printing the info, can you add a way to get the driver > info? See cpu.h get_desc() for how it does it. > >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + struct udevice *fpga; >> + u16 version; >> + u16 features; >> + >> + gdsys_soc_get_fpga(dev, &fpga); >> + >> + fpgamap_read(fpga, priv->osd_base + REG_VERSION, &version, >> + FPGAMAP_SIZE_16); >> + fpgamap_read(fpga, priv->osd_base + REG_FEATURES, &features, >> + FPGAMAP_SIZE_16); >> + >> + set_control(dev, 0x0049); >> + >> + priv->base_width = ((features & 0x3f00) >> 8) + 1; >> + priv->base_height = (features & 0x001f) + 1; >> + >> + printf("OSD-%s: Digital-OSD version %01d.%02d, %d x %d characters\n", >> + dev->name, version / 100, version % 100, >> + priv->base_width, priv->base_height); >> +} >> + >> +int ihs_video_out_get_data(struct udevice *dev, void *data) >> +{ >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + struct ihs_video_out_data *ihs_data = data; >> + >> + ihs_data->width = priv->base_width; >> + ihs_data->height = priv->base_height; > > I think you should make this operation return info in a known struct, > rather than an opaque thing. The width and height are generally > useful. See cpu_get_info() for example. > >> + >> + return 0; >> +} >> + >> +int ihs_video_out_set_mem(struct udevice *dev, uint x, uint y, u8 *buf, >> + size_t buflen, uint count) >> +{ >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + struct udevice *fpga; >> + uint offset; >> + uint k, l; >> + u16 data; >> + >> + gdsys_soc_get_fpga(dev, &fpga); >> + >> + for (l = 0; l < count; ++l) { >> + offset = y * priv->base_width + x + l * (buflen / 2); >> + >> + for (k = 0; k < buflen / 2; ++k) { >> + if (offset + k >= priv->base_width * >> priv->base_height) >> + return -ENXIO; > > Should this be E2BIG? > >> + >> + data = buf[2 * k + 1] + 256 * buf[2 * k]; >> + fpgamap_write(fpga, >> + priv->osd_buffer_base + 2 * (offset + >> k), >> + &data, FPGAMAP_SIZE_16); >> + } >> + } >> + >> + set_control(dev, 0x0049); >> + >> + return 0; >> +} >> + >> +int ihs_video_out_set_size(struct udevice *dev, uint x, uint y) >> +{ >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + struct udevice *fpga; >> + u16 data; >> + >> + gdsys_soc_get_fpga(dev, &fpga); >> + >> + if (!x || x > 64 || x > MAX_X_CHARS || >> + !y || y > 32 || y > MAX_Y_CHARS) { >> + return -EINVAL; >> + } >> + >> + data = ((x - 1) << 8) | (y - 1); >> + fpgamap_write(fpga, priv->osd_base + REG_XY_SIZE, &data, >> + FPGAMAP_SIZE_16); >> + data = 32767 * (priv->res_x - 12 * x) / 65535; >> + fpgamap_write(fpga, priv->osd_base + REG_X_POS, &data, >> + FPGAMAP_SIZE_16); >> + data = 32767 * (priv->res_y - 18 * y) / 65535; >> + fpgamap_write(fpga, priv->osd_base + REG_Y_POS, &data, >> + FPGAMAP_SIZE_16); >> + >> + return 0; >> +} >> + >> +int ihs_video_out_print(struct udevice *dev, uint x, uint y, ulong color, >> + char *text) >> +{ >> + int res; >> + u8 buffer[MAX_VIDEOMEM_WIDTH]; >> + uint k; >> + uint charcount = strlen(text); >> + uint len = min(charcount, (uint)(MAX_VIDEOMEM_WIDTH)); >> + >> + for (k = 0; k < len; ++k) { >> + buffer[2 * k] = text[k]; >> + buffer[2 * k + 1] = color; >> + } >> + >> + res = ihs_video_out_set_mem(dev, x, y, buffer, 2 * len, 1); >> + >> + if (res < 0) >> + return res; >> + >> + return 0; >> +} >> + >> +static const struct video_osd_ops ihs_video_out_ops = { >> + .get_data = ihs_video_out_get_data, >> + .set_mem = ihs_video_out_set_mem, >> + .set_size = ihs_video_out_set_size, >> + .print = ihs_video_out_print, >> +}; >> + >> +int ihs_video_out_probe(struct udevice *dev) >> +{ >> + struct ihs_video_out_priv *priv = dev_get_priv(dev); >> + int len = 0; >> + struct ofnode_phandle_args phandle_args; >> + char *mode; >> + >> + priv->addr = dev_read_u32_default(dev, "reg", -1); >> + >> + priv->osd_base = dev_read_u32_default(dev, "osd_base", -1); >> + > > Drop these extra blank lines - all three lines are doing a similar thing. > >> + priv->osd_buffer_base = dev_read_u32_default(dev, "osd_buffer_base", > > Property should use hyphens not underscore. Also is there a binding > file for this? > >> + -1); >> + >> + mode = (char *)dev_read_prop(dev, "mode", &len); >> + >> + if (!mode) { >> + printf("%s: Could not read mode property.\n", dev->name); > > debug() > >> + return -EINVAL; >> + } >> + >> + if (!strcmp(mode, "1024_768_60")) { >> + priv->sync_src = 2; >> + priv->res_x = 1024; >> + priv->res_y = 768; >> + } else if (!strcmp(mode, "720_400_70")) { >> + priv->sync_src = 1; >> + priv->res_x = 720; >> + priv->res_y = 400; >> + } else { >> + priv->sync_src = 0; >> + priv->res_x = 640; >> + priv->res_y = 480; >> + } >> + >> + print_info(dev); >> + >> + if (dev_read_phandle_with_args(dev, "clk_gen", NULL, 0, 0, >> + &phandle_args)) { >> + printf("%s: Could not get clk_gen node.\n", dev->name); > > Please change all to debug() > >> + return -EINVAL; >> + } >> + >> + if (uclass_get_device_by_ofnode(UCLASS_CLK, phandle_args.node, >> + &priv->clk_gen)) { >> + printf("%s: Could not get clk_gen dev.\n", dev->name); >> + return -EINVAL; >> + } >> + >> + if (dev_read_phandle_with_args(dev, "dp_tx", NULL, 0, 0, >> + &phandle_args)) { >> + printf("%s: Could not get dp_tx.\n", dev->name); >> + return -EINVAL; >> + } >> + >> + if (uclass_get_device_by_ofnode(UCLASS_DISPLAY, phandle_args.node, >> + &priv->dp_tx)) { >> + printf("%s: Could not get dp_tx dev.\n", dev->name); >> + return -EINVAL; >> + } >> + >> + display_power_on(priv->dp_tx, mode); >> + >> + return 0; >> +} >> + >> +U_BOOT_DRIVER(ihs_video_out_drv) = { >> + .name = "ihs_video_out_drv", >> + .id = UCLASS_VIDEO_OSD, >> + .ops = &ihs_video_out_ops, >> + .of_match = ihs_video_out_ids, >> + .probe = ihs_video_out_probe, >> + .priv_auto_alloc_size = sizeof(struct ihs_video_out_priv), >> +}; >> -- >> 2.16.1 > > Regards, > Simon >
I'll address all problems in v2. Thanks for reviewing! Best regards, Mario _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
