Hi Simon,
On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Mario,
>
> On 14 July 2017 at 05:55, Mario Six <mario....@gdsys.cc> wrote:
>> Add command to query information from and write text to IHS OSDs.
>>
>> Signed-off-by: Mario Six <mario....@gdsys.cc>
>> ---
>>
>> cmd/Kconfig | 6 +++
>> cmd/Makefile | 1 +
>> cmd/ihs_osd.c | 167
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 174 insertions(+)
>> create mode 100644 cmd/ihs_osd.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 6425c425d6..b632049022 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -933,6 +933,12 @@ config CMD_DISPLAY
>> displayed on a simple board-specific display. Implement
>> display_putc() to use it.
>>
>> +config CMD_IHS_OSD
>> + bool "ihs osd"
>> + help
>> + Enable the 'osd' command which allows to query information from and
>> + write text data to a gdsys OSD.
>> +
>> config CMD_LED
>> bool "led"
>> default y if LED
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index 243f9f45d4..c30511982b 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_CMD_FS_GENERIC) += fs.o
>> obj-$(CONFIG_CMD_FUSE) += fuse.o
>> obj-$(CONFIG_CMD_GETTIME) += gettime.o
>> obj-$(CONFIG_CMD_GPIO) += gpio.o
>> +obj-$(CONFIG_CMD_IHS_OSD) += ihs_osd.o
>> obj-$(CONFIG_CMD_I2C) += i2c.o
>> obj-$(CONFIG_CMD_IOTRACE) += iotrace.o
>> obj-$(CONFIG_CMD_HASH) += hash.o
>> diff --git a/cmd/ihs_osd.c b/cmd/ihs_osd.c
>> new file mode 100644
>> index 0000000000..01ef3eee83
>> --- /dev/null
>> +++ b/cmd/ihs_osd.c
>> @@ -0,0 +1,167 @@
>> +/*
>> + * (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, eib...@gdsys.de
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <dm/uclass-internal.h>
>> +#include <i2c.h>
>> +#include <ihs_video_out.h>
>> +#include <malloc.h>
>> +
>> +#define MAX_VIDEOMEM_WIDTH 64
>> +#define MAX_VIDEOMEM_HEIGHT 32
>> +#define MAX_X_CHARS 53
>> +#define MAX_Y_CHARS 26
>
>
>> +
>> +size_t hexstr_to_u16_array(char *hexstr, u16 *array, size_t arrsize)
>> +{
>> + size_t pos;
>> +
>> + for (pos = 0; pos < arrsize; ++pos) {
>> + char substr[5];
>> +
>> + memcpy(substr, hexstr, 4);
>> + substr[4] = 0;
>> + *array = simple_strtoul(substr, NULL, 16);
>> +
>> + hexstr += 4;
>> + array++;
>> + if (*hexstr == 0)
>> + break;
>> + }
>> +
>> + return pos;
>> +}
>> +
>> +int do_osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + struct udevice *dev;
>> + uint x, y;
>> + uint count;
>> + char *hexstr;
>> + u16 buffer[MAX_VIDEOMEM_WIDTH];
>> + size_t buflen;
>> +
>> + if ((argc < 4) || (strlen(argv[3]) % 4)) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + x = simple_strtoul(argv[1], NULL, 16);
>> + y = simple_strtoul(argv[2], NULL, 16);
>> + hexstr = argv[3];
>> + count = (argc > 4) ? simple_strtoul(argv[4], NULL, 16) : 1;
>> +
>> + buflen = hexstr_to_u16_array(hexstr, buffer, MAX_VIDEOMEM_WIDTH);
>> +
>> + for (uclass_find_first_device(UCLASS_IHS_VIDEO_OUT, &dev);
>> + dev;
>> + uclass_find_next_device(&dev)) {
>
> Why write to all devices? Perhaps you should have the concept of a
> current device in this file?
>
We always write to all possible OSDs on our boards. But for a generic OSD
uclass this doesn't really make sense, I realize.
This does create a problem with backwards compatibility, though. See below.
>> + uint k;
>> +
>> + for (k = 0; k < count; ++k)
>> + video_out_set_mem(dev, x + k * buflen, y, buffer,
>> + buflen);
>> +
>> + video_out_set_control(dev, 0x0049);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int do_osd_print(cmd_tbl_t *cmdtp, int flag, int argc,
>> + char * const argv[])
>> +{
>> + struct udevice *dev;
>> + u16 buffer[MAX_VIDEOMEM_WIDTH];
>> + uint x, y, charcount, len;
>> + u8 color;
>> + uint k;
>> + char *text;
>> +
>> + if (argc < 5) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + x = simple_strtoul(argv[1], NULL, 16);
>> + y = simple_strtoul(argv[2], NULL, 16);
>> + color = simple_strtoul(argv[3], NULL, 16);
>> + text = argv[4];
>> + charcount = strlen(text);
>> + len = min(charcount, (uint)MAX_VIDEOMEM_WIDTH);
>> +
>> + for (uclass_find_first_device(UCLASS_IHS_VIDEO_OUT, &dev);
>> + dev;
>> + uclass_find_next_device(&dev)) {
>> + int res;
>> +
>> + for (k = 0; k < len; ++k)
>> + buffer[k] = (text[k] << 8) | color;
>
> This is specific to your device. If you are making a generic device
> you should have something like video_osd_set_char(...k, color).
>
> Then other drivers can implement it.
>
OK, will implement a generic interface in v2 if the backwards compatibility
problem can be solved, see below.
>> +
>> + res = video_out_set_mem(dev, x, y, buffer, len);
>> +
>> + if (res < 0)
>> + return res;
>> +
>> + video_out_set_control(dev, 0x0049);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int do_osd_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + struct udevice *dev;
>> + uint x, y;
>> +
>> + if (argc < 3) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + x = simple_strtoul(argv[1], NULL, 16);
>> + y = simple_strtoul(argv[2], NULL, 16);
>> +
>> + if (!x || (x > 64) || (x > MAX_X_CHARS) ||
>> + !y || (y > 32) || (y > MAX_Y_CHARS)) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + for (uclass_find_first_device(UCLASS_IHS_VIDEO_OUT, &dev);
>> + dev;
>> + uclass_find_next_device(&dev))
>> + video_out_set_size(dev, ((x - 1) << 8) | (y - 1),
>> + 32767 * (640 - 12 * x) / 65535,
>> + 32767 * (480 - 18 * x) / 65535);
>
> Again this logic should be in the driver, not the command.
>
Dito, will change in v2, if the backwards compatibility problem can be solved,
see below.
>> +
>> + return 0;
>> +}
>> +
>> +U_BOOT_CMD(
>> + osdw, 5, 0, do_osd_write,
>> + "write 16-bit hex encoded buffer to osd memory",
>> + "osd write [pos_x] [pos_y] [buffer] [count] - write 16-bit hex
>> encoded buffer to osd memory\n"
>> +);
>> +
>> +U_BOOT_CMD(
>> + osdp, 5, 0, do_osd_print,
>> + "write ASCII buffer to osd memory",
>> + "osd print [pos_x] [pos_y] [color] [text] - write ASCII buffer to
>> osd memory\n"
>> +);
>> +
>> +U_BOOT_CMD(
>> + osdsize, 3, 0, do_osd_size,
>> + "set OSD XY size in characters",
>> + "osd size [size_x] [size_y] - set OSD XY size in characters\n"
>> +);
>> --
>> 2.11.0
>>
>
> I think this should be an 'osd' command with sub-commands.
>
That's how I first implemented it. Then I realized that these commands have to
be backwards compatible to the ones in boards/gdsys/common/osd.c. These demand
that data is queried from/written to all OSD instances, and they define three
commands instead of one with three subcommands.
How can I keep backwards compatibility intact while still implementing a
generic interface for OSDs?
> Regards,
> Simon
Best regards,
Mario
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
