Hi Simon,

On Fri, Mar 30, 2018 at 12:42 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:39, Mario Six <mario....@gdsys.cc> wrote:
>> Add a driver for IHS OSDs on IHS FPGAs.
>>
>> Signed-off-by: Mario Six <mario....@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....@gdsys.cc
>> + *
>> + * based on the gdsys osd driver, which is
>> + *
>> + * (C) Copyright 2010
>> + * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eib...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to