Re: [U-Boot] [PATCH v3 3/5] dm: led: move default state support in led uclass

Hi Patrick, sorry, for responding so late, I am in the middle of a vacation at the moment.

From: Patrick Delaunay [mailto:patrick.delaunay@st.com] Sent: Dienstag, 24. Juli 2018 10:59 Subject: [PATCH v3 3/5] dm: led: move default state support in led uclass

This patch save common LED property "default-state" value in post bind of LED uclass. The configuration for this default state is only performed when led_default_state() is called; It can be called in your board_init() or it could added in init_sequence_r[] in future.

Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Patrick Delaunay patrick.delaunay@st.com

---

Changes in v3: None Changes in v2: None

drivers/led/led-uclass.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/led/led_gpio.c | 8 ------- include/led.h | 23 +++++++++++++++++++++ 3 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 2f4d69e..141401d 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -8,6 +8,7 @@ #include <dm.h> #include <errno.h> #include <led.h> +#include <dm/device-internal.h> #include <dm/root.h> #include <dm/uclass-internal.h>

@@ -63,8 +64,61 @@ int led_set_period(struct udevice *dev, int period_ms) } #endif

+static int led_post_bind(struct udevice *dev) +{

•  struct led_uc_plat *uc_pdata;
  

•  const char *default_state;
  

•  uc_pdata = dev_get_uclass_platdata(dev);
  

•  /* common optional properties */
  

•  uc_pdata->default_state = LED_DEF_NO;
  

•  default_state = dev_read_string(dev, "default-state");
  

•  if (default_state) {
  

•          if (!strncmp(default_state, "on", 2))
  

•                  uc_pdata->default_state = LED_DEF_ON;
  

•          else if (!strncmp(default_state, "off", 3))
  

•                  uc_pdata->default_state = LED_DEF_OFF;
  

•          else if (!strncmp(default_state, "keep", 4))
  

•                  uc_pdata->default_state = LED_DEF_KEEP;
  

•  }
  

•  return 0;
  

+}

+int led_default_state(void) +{

•  struct udevice *dev;
  

•  struct uclass *uc;
  

•  struct led_uc_plat *uc_pdata;
  

•  int ret;
  

•  ret = uclass_get(UCLASS_LED, &uc);
  

•  if (ret)
  

•          return ret;
  

•  for (uclass_find_first_device(UCLASS_LED, &dev);
  

•       dev;
  

•       uclass_find_next_device(&dev)) {
  

•          uc_pdata = dev_get_uclass_platdata(dev);
  

•          if (!uc_pdata || uc_pdata->default_state == LED_DEF_NO)
  

•                  continue;
  

•          ret = device_probe(dev);
  

•          if (ret)
  

•                  return ret;
  

•          if (uc_pdata->default_state == LED_DEF_ON)
  

•                  led_set_state(dev, LEDST_ON);
  

•          else if (uc_pdata->default_state == LED_DEF_OFF)
  

•                  led_set_state(dev, LEDST_OFF);
  

•          printf("%s: default_state=%d\n",
  

•                 uc_pdata->label, uc_pdata->default_state);
  

Do you really want to keep this printf()?

•  }
  

•  return ret;
  

+}

UCLASS_DRIVER(led) = { .id = UCLASS_LED, .name = "led",

•  .post_bind      = led_post_bind,
  .per_device_platdata_auto_alloc_size = sizeof(struct led_uc_plat),
  

}; diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c index 533587d..93f6b91 100644 --- a/drivers/led/led_gpio.c +++ b/drivers/led/led_gpio.c @@ -57,7 +57,6 @@ static int led_gpio_probe(struct udevice *dev) { struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev); struct led_gpio_priv *priv = dev_get_priv(dev);

•  const char *default_state;
  int ret;
  
  /* Ignore the top-level LED node */
  

@@ -68,13 +67,6 @@ static int led_gpio_probe(struct udevice *dev) if (ret) return ret;

•  default_state = dev_read_string(dev, "default-state");
  

•  if (default_state) {
  

•          if (!strncmp(default_state, "on", 2))
  

•                  gpio_led_set_state(dev, LEDST_ON);
  

•          else if (!strncmp(default_state, "off", 3))
  

•                  gpio_led_set_state(dev, LEDST_OFF);
  

•  }
  

Is it necessary to move this code out of led_gpio_probe()? If possible I would keep it here and implement led_default_state() similar to mmc_initialize(->mmc_probe()). Than we could avoid enum led_def_state_t and the double evaluation of default_state.

  return 0;

}

diff --git a/include/led.h b/include/led.h index 940b97f..ff45f03 100644 --- a/include/led.h +++ b/include/led.h @@ -8,12 +8,27 @@ #define __LED_H

/**

• • enum led_default_state - The initial state of the LED.
• • see Documentation/devicetree/bindings/leds/common.txt
• */

+enum led_def_state_t {

•  LED_DEF_NO,
  

•  LED_DEF_ON,
  

•  LED_DEF_OFF,
  

•  LED_DEF_KEEP
  

+};

+/**

• struct led_uc_plat - Platform data the uclass stores about each device
• @label: LED label

• • @default_state* - The initial state of the LED.
• see Documentation/devicetree/bindings/leds/common.txt
• • • • set automatically on device bind by the uclass's '.post_bind' method.

*/ struct led_uc_plat { const char *label;

•  enum led_def_state_t default_state;
  

};

/** @@ -106,4 +121,12 @@ enum led_state_t led_get_state(struct udevice *dev); */ int led_set_period(struct udevice *dev, int period_ms);

+/**

• • led_default_state() - set the default state for all the LED
• • This enables all leds which have default state.
• */

+int led_default_state(void);

#endif

2.7.4

However, this whole v3 series was: Tested-by: Patrick Bruenn p.bruenn@beckhoff.com Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075