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 command to query information from and write text to IHS OSDs.
>>
>> Signed-off-by: Mario Six <mario....@gdsys.cc>
>> ---
>> cmd/Kconfig | 16 +++
>> cmd/Makefile | 1 +
>> cmd/osd.c | 366
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 383 insertions(+)
>> create mode 100644 cmd/osd.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 136836d146..0d60051960 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -846,6 +846,22 @@ config CMD_ONENAND
>> and erasing blocks. It allso provides a way to show and change
>> bad blocks, and test the device.
>>
>> +config CMD_OSD
>> + bool "osd"
>> + help
>> + Enable the 'osd' command which allows to query information from and
>> + write text data to a OSD.
>
> Please expand help. E.g. what is an OSD?
>
>> +
>> +if CMD_OSD
>> +
>> +config GDSYS_LEGACY_OSD_CMDS
>> + bool "Use legacy gdsys-specific commands"
>> + help
>> + Use the 'osdw', 'osdp', and 'osdsize' legacy commands required by
>> + gdsys devices.
>> +
>> +endif
>> +
>> config CMD_PART
>> bool "part"
>> select PARTITION_UUIDS
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index 9a358e4801..d3f7522700 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -61,6 +61,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_OSD) += osd.o
>> obj-$(CONFIG_CMD_I2C) += i2c.o
>> obj-$(CONFIG_CMD_IOTRACE) += iotrace.o
>> obj-$(CONFIG_CMD_HASH) += hash.o
>> diff --git a/cmd/osd.c b/cmd/osd.c
>> new file mode 100644
>> index 0000000000..bbabfc3c54
>> --- /dev/null
>> +++ b/cmd/osd.c
>> @@ -0,0 +1,366 @@
>> +/*
>> + * (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 <video_osd.h>
>> +#include <malloc.h>
>> +
>> +#ifndef CONFIG_GDSYS_LEGACY_OSD_CMDS
>> +static struct udevice *osd_cur;
>> +#endif
>> +
>> +void hexstr_to_u8_array(char *hexstr, u8 *array, size_t arrsize)
>> +{
>> + size_t pos;
>> +
>> + for (pos = 0; pos < arrsize; ++pos) {
>> + char substr[3];
>> +
>> + memcpy(substr, hexstr, 2);
>> + substr[2] = 0;
>> + *array = simple_strtoul(substr, NULL, 16);
>> +
>> + hexstr += 2;
>> + array++;
>> + if (*hexstr == 0)
>> + break;
>> + }
>> +}
>
> Feels like we have a function like this already in U-Boot?
>
If I haven't completely overlooked it, there isn't. The Linux kernel has
hexdump.c, which we could (partially?) import for this purpose. It seems pretty
useful in general.
>> +
>> +#ifdef CONFIG_GDSYS_LEGACY_OSD_CMDS
>> +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;
>> + u8 *buffer;
>> + size_t buflen;
>> +
>> + if (argc < 4 || (strlen(argv[3])) % 2) {
>> + 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 = strlen(hexstr) / 2;
>> + buffer = malloc(buflen);
>> + hexstr_to_u8_array(hexstr, buffer, buflen);
>> +
>> + for (uclass_first_device(UCLASS_VIDEO_OSD, &dev);
>> + dev;
>> + uclass_next_device(&dev))
>> + if (video_osd_set_mem(dev, x, y, buffer, buflen, count))
>> + printf("Could not write to video mem on osd %s\n",
>> + dev->name);
>
> It seems odd to write it on all devices. If you want to do this, it
> should be implemented in the uclass I think.
>
> Also you ignore errors here.
>
> Most commands allow you to select a particular device to work with.
> You have this feature below, so why not use it here?
>
This is actually the case if CONFIG_GDSYS_LEGACY_OSD_CMDS is not defined (which
is probably not that well-suited in the cmd/Kconfig now that I think about it;
I'll move that). This is because this command should be backwards-compatible
with our current OSD commands (which write to all available OSDs by default).
If CONFIG_GDSYS_LEGACY_OSD_CMDS is not defined, the commands work on a
by-device basis, and you can select the one you want to work with.
> Same below.
>
>> +
>> + free(buffer);
>> +
>> + return 0;
>> +}
>> +
>> +static int do_osd_print(cmd_tbl_t *cmdtp, int flag, int argc,
>> + char * const argv[])
>> +{
>> + struct udevice *dev;
>> + uint x, y;
>> + u8 color;
>> + 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];
>> +
>> + for (uclass_first_device(UCLASS_VIDEO_OSD, &dev);
>> + dev;
>> + uclass_next_device(&dev)) {
>> + if (video_osd_print(dev, x, y, color, text))
>> + printf("Could not print string to osd %s\n",
>> dev->name);
>> + }
>> +
>> + 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);
>> +
>> + for (uclass_first_device(UCLASS_VIDEO_OSD, &dev);
>> + dev;
>> + uclass_next_device(&dev)) {
>> + if (video_osd_set_size(dev, x, y))
>> + printf("Could not set size on osd %s\n", dev->name);
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +int do_osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + uint x, y;
>> + uint count;
>> + char *hexstr;
>> + u8 *buffer;
>> + size_t buflen;
>> +
>> + if (argc < 4 || (strlen(argv[3]) % 2)) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + if (!osd_cur) {
>> + puts("No osd selected\n");
>> + return -ENODEV;
>> + }
>> +
>> + 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 = strlen(hexstr) / 2;
>> + buffer = malloc(buflen);
>> + hexstr_to_u8_array(hexstr, buffer, buflen);
>> +
>> + if (video_out_set_mem(osd_cur, x, y, buffer, buflen, count))
>> + printf("Could not write to video mem on osd %s\n",
>> + osd_cur->name);
>> +
>> + free(buffer);
>> +
>> + return 0;
>> +}
>> +
>> +int do_osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + uint x, y;
>> + u8 color;
>> + char *text;
>> +
>> + if (argc < 5) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + if (!osd_cur) {
>> + puts("No osd selected\n");
>> + return -ENODEV;
>> + }
>> +
>> + x = simple_strtoul(argv[1], NULL, 16);
>> + y = simple_strtoul(argv[2], NULL, 16);
>> + color = simple_strtoul(argv[3], NULL, 16);
>> + text = argv[4];
>> +
>> + if (video_out_print(osd_cur, x, y, color, text))
>> + printf("Could not print string to osd %s\n", osd_cur->name);
>> +
>> + return 0;
>> +}
>> +
>> +int do_osd_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + uint x, y;
>> +
>> + if (argc < 3) {
>> + cmd_usage(cmdtp);
>> + return 1;
>> + }
>> +
>> + if (!osd_cur) {
>> + puts("No osd selected\n");
>> + return -ENODEV;
>> + }
>> +
>> + x = simple_strtoul(argv[1], NULL, 16);
>> + y = simple_strtoul(argv[2], NULL, 16);
>> +
>> + if (video_out_set_size(osd_cur, x, y))
>> + printf("Could not set size on osd %s\n", osd_cur->name);
>> +
>> + return 0;
>> +}
>> +
>> +static void show_osd(struct udevice *osd)
>> +{
>> + printf("OSD %d:\t%s", osd->req_seq, osd->name);
>> + if (device_active(osd))
>> + printf(" (active %d)", osd->seq);
>> + printf("\n");
>> +}
>> +
>> +static int do_show_osd(cmd_tbl_t *cmdtp, int flag, int argc,
>> + char * const argv[])
>> +{
>> + struct udevice *osd;
>> +
>> + if (argc == 1) {
>> + /* show all OSDs */
>> + struct uclass *uc;
>> + int ret;
>> +
>> + ret = uclass_get(UCLASS_VIDEO_OSD, &uc);
>> + if (ret)
>> + return CMD_RET_FAILURE;
>> + uclass_foreach_dev(osd, uc)
>> + show_osd(osd);
>> + } else {
>> + int i, ret;
>> +
>> + /* show specific OSD */
>> + i = simple_strtoul(argv[1], NULL, 10);
>> +
>> + ret = uclass_get_device_by_seq(UCLASS_IHS_FPGA, i, &osd);
>> + if (ret) {
>> + printf("Invalid osd %d: err=%d\n", i, ret);
>> + return CMD_RET_FAILURE;
>> + }
>> + show_osd(osd);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int cmd_osd_set_osd_num(unsigned int osdnum)
>> +{
>> + struct udevice *osd;
>> + int ret;
>> +
>> + ret = uclass_get_device_by_seq(UCLASS_IHS_VIDEO_OUT, osdnum, &osd);
>> + if (ret) {
>> + debug("%s: No OSD %d\n", __func__, osdnum);
>> + return ret;
>> + }
>> + osd_cur = osd;
>> +
>> + return 0;
>> +}
>> +
>> +static int osd_get_osd_cur(struct udevice **osdp)
>> +{
>> + if (!osd_cur) {
>> + puts("No osd selected\n");
>> + return -ENODEV;
>> + }
>> + *osdp = osd_cur;
>> +
>> + return 0;
>> +}
>> +
>> +static int do_osd_num(cmd_tbl_t *cmdtp, int flag, int argc,
>> + char * const argv[])
>> +{
>> + int ret = 0;
>> + int osd_no;
>> +
>> + if (argc == 1) {
>> + /* querying current setting */
>> + struct udevice *osd;
>> +
>> + if (!osd_get_osd_cur(&osd))
>> + osd_no = osd->seq;
>> + else
>> + osd_no = -1;
>> + printf("Current osd is %d\n", osd_no);
>> + } else {
>> + osd_no = simple_strtoul(argv[1], NULL, 10);
>> + printf("Setting osd to %d\n", osd_no);
>> +
>> + ret = cmd_osd_set_osd_num(osd_no);
>> +
>> + if (ret)
>> + printf("Failure changing osd number (%d)\n", ret);
>> + }
>> +
>> + return ret ? CMD_RET_FAILURE : 0;
>> +}
>> +
>> +static cmd_tbl_t cmd_osd_sub[] = {
>> + U_BOOT_CMD_MKENT(show, 1, 1, do_show_osd, "", ""),
>> + U_BOOT_CMD_MKENT(dev, 1, 1, do_osd_num, "", ""),
>> + U_BOOT_CMD_MKENT(write, 4, 1, do_osd_write, "", ""),
>> + U_BOOT_CMD_MKENT(print, 4, 1, do_osd_print, "", ""),
>> + U_BOOT_CMD_MKENT(size, 2, 1, do_osd_size, "", ""),
>> +};
>> +
>> +static int do_osd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + cmd_tbl_t *c;
>> +
>> + if (argc < 2)
>> + return CMD_RET_USAGE;
>> +
>> + /* Strip off leading 'osd' command argument */
>> + argc--;
>> + argv++;
>> +
>> + c = find_cmd_tbl(argv[0], &cmd_osd_sub[0], ARRAY_SIZE(cmd_osd_sub));
>> +
>> + if (c)
>> + return c->cmd(cmdtp, flag, argc, argv);
>> + else
>> + return CMD_RET_USAGE;
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_GDSYS_LEGACY_OSD_CMDS
>> +U_BOOT_CMD(
>> + osdw, 5, 0, do_osd_write,
>> + "write 16-bit hex encoded buffer to osd memory",
>> + "osdw [pos_x] [pos_y] [buffer] [count] - write 8-bit hex encoded
>> buffer to osd memory\n"
>> +);
>> +
>> +U_BOOT_CMD(
>> + osdp, 5, 0, do_osd_print,
>> + "write ASCII buffer to osd memory",
>> + "osdp [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",
>> + "osdsize [size_x] [size_y] - set OSD XY size in characters\n"
>> +);
>> +#else
>> +static char osd_help_text[] =
>> + "show - show OSD info\n"
>> + "osd dev [dev] - show or set current OSD\n"
>> + "write [pos_x] [pos_y] [buffer] [count] - write 8-bit hex encoded
>> buffer to osd memory\n"
>> + "print [pos_x] [pos_y] [color] [text] - write ASCII buffer to osd
>> memory\n"
>
> What is colour?
>
>> + "size [size_x] [size_y] - set OSD XY size in characters\n";
>> +
>> +U_BOOT_CMD(
>> + osd, 6, 1, do_osd,
>> + "OSD sub-system",
>> + osd_help_text
>
> Is there a way to list osd devices?
>
If CONFIG_GDSYS_LEGACY_OSD_CMDS is not defined, "osd show" will list
all available OSD devices.
>> +);
>> +#endif
>> --
>> 2.16.1
>>
>
> Regards,
> Simon
Everything else will be addressed in v2. Thanks for reviewing!
Best regards,
Mario
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
