Hello Minkyu,

Thank you for review.

On 01/06/2014 12:38 PM, Minkyu Kang wrote:
On 04/01/14 01:43, Przemyslaw Marczak wrote:
This simple LCD menu allows run one of download mode on device
without writing on console or for fast and easy upgrade.
This feature check user keys combination at boot:
- power key + volume up - download menu
- power key + volume down - thor mode (without menu)

New configs:
- CONFIG_LCD_MENU
- CONFIG_LCD_MENU_BOARD
which depends on: CONFIG_MISC_INIT_R

For proper effect this feature needs following definitions:

Power key:
- KEY_PWR_PMIC_NAME - (string) pmic which supports power key check

Register address:
- KEY_PWR_STATUS_REG
- KEY_PWR_INTERRUPT_REG

Register power key mask:
- KEY_PWR_STATUS_MASK
- KEY_PWR_INTERRUPT_MASK

Gpio numbers:
- KEY_PWR_INTERRUPT_MASK
- KEY_VOL_DOWN_GPIO

Signed-off-by: Przemyslaw Marczak <p.marc...@samsung.com>

---
Changes v2:
- remove keys.h  - definitions should be in boards headers
- add misc.h
- code cleanup
- extend commit msg by more informations

Changes v3:
- none

  board/samsung/common/misc.c |  340 +++++++++++++++++++++++++++++++++++++++++++
  board/samsung/common/misc.h |   18 +++
  2 files changed, 358 insertions(+)
  create mode 100644 board/samsung/common/misc.h

diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
index 3a91d62..93f766c 100644
--- a/board/samsung/common/misc.c
+++ b/board/samsung/common/misc.c
@@ -8,6 +8,341 @@
  #include <common.h>
  #include <lcd.h>
  #include <libtizen.h>
+#include <errno.h>
+#include <version.h>
+#include <asm/sizes.h>
+#include <asm/arch/cpu.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+#include <linux/input.h>
+#include <lcd.h>
+#include <libtizen.h>
+#include <power/pmic.h>
+#include <mmc.h>
+#include "misc.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#ifdef CONFIG_LCD_MENU
+static int power_key_pressed(int reg)

u32 reg?

+{
+       struct pmic *pmic = pmic_get(KEY_PWR_PMIC_NAME);
+       u32 status = 0;

= 0 do not need.
please do not use initial value, if possible.

+       u32 mask;

please check return value of pmic_get

        pmic = pmic_get(KEY_PWR_PMIC_NAME);
        if !(pmic)
                return 0;

+
+       if (pmic_probe(pmic))
+               return 0;
+
+       if (!pmic) {
+               printf("%s: Not found\n", KEY_PWR_PMIC_NAME);
+               return 0;
+       }
+
+       if (reg == KEY_PWR_STATUS_REG)
+               mask = KEY_PWR_STATUS_MASK;
+       else
+               mask = KEY_PWR_INTERRUPT_MASK;
+
+       if (pmic_reg_read(pmic, reg, &status))
+               return 0;
+
+       return !!(status & mask);
+}
+
+static int key_pressed(int key)
+{
+       int value = 0;

please do not use initial value, if possible.
instead, please add "value = 0" at default statement.

+
+       switch (key) {
+       case KEY_POWER:
+               value = power_key_pressed(KEY_PWR_INTERRUPT_REG);
+               break;
+       case KEY_VOLUMEUP:
+               value = !gpio_get_value(KEY_VOL_UP_GPIO);
+               break;
+       case KEY_VOLUMEDOWN:
+               value = !gpio_get_value(KEY_VOL_DOWN_GPIO);
+               break;
+       default:
+               break;
+       }
+
+       return value;
+}
+
+static int check_keys(void)
+{
+       int keys = 0;
+
+       if (key_pressed(KEY_POWER))
+               keys += KEY_POWER;
+       if (key_pressed(KEY_VOLUMEUP))
+               keys += KEY_VOLUMEUP;
+       if (key_pressed(KEY_VOLUMEDOWN))
+               keys += KEY_VOLUMEDOWN;
+
+       return keys;
+}
+
+/*
+ * 0 BOOT_MODE_INFO
+ * 1 BOOT_MODE_THOR
+ * 2 BOOT_MODE_UMS
+ * 3 BOOT_MODE_DFU
+ * 4 BOOT_MODE_EXIT
+ */
+static char *
+mode_name[BOOT_MODE_EXIT + 1] = {"DEVICE",

please move the "DEVICE" to next line.

+                               "THOR",
+                               "UMS",
+                               "DFU",
+                               "EXIT"};

please move the brace to next line.

+
+static char *
+mode_info[BOOT_MODE_EXIT + 1] = {"info",
+                                "downloader",
+                                "mass storage",
+                                "firmware update",
+                                "and run normal boot"};

ditto.

+
+static char *
+mode_cmd[BOOT_MODE_EXIT + 1] = {"",
+                               "thor 0 mmc 0",
+                               "ums 0 mmc 0",
+                               "dfu 0 mmc 0",
+                               ""};

ditto.

+
+static void display_board_info(void)
+{

#ifdef CONFIG_GENERIC_MMC
+       struct mmc *mmc = find_mmc_device(0);
#endif

+       vidinfo_t *vid = &panel_info;
+
+       lcd_position_cursor(4, 4);
+
+       lcd_printf("%s\n\t", U_BOOT_VERSION);
+       lcd_puts("\n\t\tBoard Info:\n");
+#ifdef CONFIG_SYS_BOARD
+       lcd_printf("\tBoard name: %s\n", CONFIG_SYS_BOARD);
+#endif
+#ifdef CONFIG_REVISION_TAG
+       lcd_printf("\tBoard rev: %u\n", get_board_rev());
+#endif
+       lcd_printf("\tDRAM banks: %u\n", CONFIG_NR_DRAM_BANKS);
+       lcd_printf("\tDRAM size: %u MB\n", gd->ram_size / SZ_1M);
+

#ifdef CONFIG_GENERIC_MMC
+       if (mmc)
+               lcd_printf("\teMMC size: %llu MB\n", mmc->capacity / SZ_1M);
#endif

maybe, you can access mmc->capacity after do mmc_init.


I will add mmc_init for sure here. But in most cases mmc is initialized at misc_init_r stage because of environment initialization.

+
+       if (vid)
+               lcd_printf("\tDisplay resolution: %u x % u\n",
+                          vid->vl_col, vid->vl_row);
+
+       lcd_printf("\tDisplay BPP: %u\n", 1 << vid->vl_bpix);
+}
+
+static int mode_leave_menu(int mode)
+{
+       int mode_supported = 1;
+       int leave = 0;
+       char *exit_option;
+       char *exit_boot = "boot";
+       char *exit_back = "back";
+
+       switch (mode) {
+       case BOOT_MODE_INFO:
+#if !defined(CONFIG_LCD_MENU_BOARD)
+               mode_supported = 0;
+#endif
+               break;
+       case BOOT_MODE_THOR:
+#if !defined(CONFIG_CMD_THOR_DOWNLOAD)
+               mode_supported = 0;
+#endif
+               break;
+       case BOOT_MODE_UMS:
+#if !defined(CONFIG_CMD_USB_MASS_STORAGE)
+               mode_supported = 0;
+#endif
+               break;
+       case BOOT_MODE_DFU:
+#if !defined(CONFIG_CMD_DFU)
+               mode_supported = 0;
+#endif
+               break;
+       case BOOT_MODE_EXIT:
+               leave = 1;
+               goto exit;

why don't you return directly at here.
goto statement is used here only.


Ok, will be changed.

+       default:

then, mode_supported = 0; maybe?

+               break;
+       }
+
+       lcd_clear();
+
+       if (mode_supported) {
+               if (mode) {
+                       lcd_printf("\n\n\t%s %s\n", mode_name[mode],
+                                                   mode_info[mode]);
+                       lcd_puts("\n\tDo not turn off device before finish!\n");
+
+                       run_command(mode_cmd[mode], 0);

do not need to check a return value?


You're right,  I will add checking here.

+                       printf("Command finished\n");
+                       lcd_clear();
+                       lcd_printf("\n\n\t%s finished\n", mode_name[mode]);
+                       exit_option = exit_boot;
+                       leave = 1;
+               } else {
+                       display_board_info();
+                       exit_option = exit_back;
+                       leave = 0;
+               }
+       } else {
+               lcd_puts("\n\n\tThis mode is not supported.\n");
+               exit_option = exit_back;
+               leave = 0;
+       }
+
+       lcd_printf("\n\n\tPress POWER KEY to %s\n", exit_option);
+
+       /* Wait for PWR key */
+       while (!key_pressed(KEY_POWER))
+               udelay(1000);
+exit:
+       lcd_clear();
+       return leave;
+}
+
+static void display_download_menu(int mode)
+{
+       char *menu_name = "Download Mode Menu";
+       char *indicator = "[=>]";
+       char *blank = "[  ]";

Do we need those three variable?


Actually we don't. Will be removed.


+       char *selection[BOOT_MODE_EXIT + 1];
+       int i;
+
+       for (i = 0; i <= BOOT_MODE_EXIT; i++)
+               selection[i] = blank;
+
+       selection[mode] = indicator;
+
+       lcd_clear();
+       lcd_printf("\n\t\t%s\n", menu_name);
+
+       for (i = 0; i <= BOOT_MODE_EXIT; i++)
+               lcd_printf("\t%s  %s - %s\n\n", selection[i],
+                                               mode_name[i],
+                                               mode_info[i]);
+}
+
+static void download_menu(void)
+{
+       int mode = 0;
+       int last_mode = 0;
+       int run;
+       int key;
+
+       display_download_menu(mode);
+
+       while (1) {
+               run = 0;
+
+               if (mode != last_mode)
+                       display_download_menu(mode);
+
+               last_mode = mode;
+               udelay(100000);
+
+               key = check_keys();
+               switch (key) {
+               case KEY_POWER:
+                       run = 1;
+                       break;
+               case KEY_VOLUMEUP:
+                       if (mode > 0)
+                               mode--;
+                       break;
+               case KEY_VOLUMEDOWN:
+                       if (mode < BOOT_MODE_EXIT)
+                               mode++;
+                       break;
+               default:
+                       break;
+               }
+
+               if (run) {
+                       if (mode_leave_menu(mode))
+                               break;
+
+                       display_download_menu(mode);
+               }
+       }
+
+       lcd_clear();
+}
+
+static void display_mode_info(void)
+{
+       lcd_position_cursor(4, 4);
+       lcd_printf("%s\n", U_BOOT_VERSION);
+       lcd_puts("\nDownload Mode Menu\n");
+#ifdef CONFIG_SYS_BOARD
+       lcd_printf("Board name: %s\n", CONFIG_SYS_BOARD);
+#endif
+       lcd_printf("Press POWER KEY to display MENU options.");
+}
+
+static int boot_menu(void)
+{
+       int key = 0;
+       int timeout = 10;
+
+       display_mode_info();
+
+       while (timeout--) {
+               lcd_printf("\rNormal boot will start in: %d seconds.", timeout);
+               udelay(1000000);

I think.. user input can be ignored because too long delay.
1 second? it's too long.
and how about use mdelay instead?


User input can't be ignored because key_pressed(KEY_POWER) is using pmic interrupt register. It is no matter when user press the power key, if he does then he will wait at most 1 second.

+
+               key = key_pressed(KEY_POWER);
+               if (key)
+                       break;
+       }
+
+       lcd_clear();
+
+       /* If PWR pressed - show download menu */
+       if (key) {
+               printf("Power pressed - go to download menu\n");
+               udelay(1000000);

delay for 1 second? is it need?
You're right It is unneeded here.


+               download_menu();
+               printf("Download mode exit.\n");
+       }
+
+       return 0;
+}
+
+static void check_boot_mode(void)
+{
+       int pwr_key;
+
+       pwr_key = power_key_pressed(KEY_PWR_STATUS_REG);
+       if (!pwr_key)
+               return;
+
+       /* Clear PWR button Rising edge interrupt status flag */
+       power_key_pressed(KEY_PWR_INTERRUPT_REG);
+
+       if (key_pressed(KEY_VOLUMEUP))
+               boot_menu();
+       else if (key_pressed(KEY_VOLUMEDOWN))
+               mode_leave_menu(BOOT_MODE_THOR);
+}
+
+static void keys_init(void)
+{
+       /* Set direction to input */
+       gpio_direction_input(KEY_VOL_UP_GPIO);
+       gpio_direction_input(KEY_VOL_DOWN_GPIO);
+}
+#endif /* CONFIG_LCD_MENU */

  #ifdef CONFIG_CMD_BMP
  static void draw_logo(void)
@@ -50,9 +385,14 @@ static void draw_logo(void)
  /* Common for Samsung boards */
  int misc_init_r(void)
  {
+#ifdef CONFIG_LCD_MENU
+       keys_init();
+       check_boot_mode();
+#endif
  #ifdef CONFIG_CMD_BMP
        if (panel_info.logo_on)
                draw_logo();
  #endif
+
        return 0;
  }
diff --git a/board/samsung/common/misc.h b/board/samsung/common/misc.h
new file mode 100644
index 0000000..f583552
--- /dev/null
+++ b/board/samsung/common/misc.h
@@ -0,0 +1,18 @@
+#ifndef __SAMSUNG_COMMON_MISC_H__
+#define __SAMSUNG_COMMON_MISC_H__
+
+#ifdef CONFIG_LCD_MENU
+enum {
+       BOOT_MODE_INFO,
+       BOOT_MODE_THOR,
+       BOOT_MODE_UMS,
+       BOOT_MODE_DFU,
+       BOOT_MODE_EXIT,
+};
+
+#ifdef CONFIG_REVISION_TAG
+u32 get_board_rev(void);
+#endif
+#endif /* CONFIG_LCD_MENU */
+
+#endif /* __SAMSUNG_COMMON_MISC_H__ */
\ No newline at end of file


Thanks,
Minkyu Kang.


Comments will be applied to next version.
Regards
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to