[U-Boot] [PATCH 2/3] video_osd: Add ihs_video_out driver
Mario Six
mario.six at gdsys.cc
Wed Apr 11 06:18:47 UTC 2018
Hi Simon,
On Fri, Mar 30, 2018 at 12:42 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:39, Mario Six <mario.six at gdsys.cc> wrote:
>> Add a driver for IHS OSDs on IHS FPGAs.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>> 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, mario.six at gdsys.cc
>> + *
>> + * based on the gdsys osd driver, which is
>> + *
>> + * (C) Copyright 2010
>> + * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eibach at gdsys.de
>> + *
>> + * 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
More information about the U-Boot
mailing list