Re: [PATCH v8 1/2] led: led_cortina: Add CAxxx LED support

Hi Alex,

On Thu, 21 May 2020 at 18:43, Alex Nemirovsky alex.nemirovsky@cortina-access.com wrote:

From: Jway Lin jway.lin@cortina-access.com

Add Cortina Access LED controller support for CAxxxx SOCs

Signed-off-by: Jway Lin jway.lin@cortina-access.com Signed-off-by: Alex Nemirovsky alex.nemirovsky@cortina-access.com CC: Simon Glass sjg@chromium.org

---

Changes in v8:

• No code change
• Split out individual driver from Cortina Package 2 patch series

to help streamline acceptence into master

Changes in v7:

• rename OFFSET to SHIFT from macros
• add additinal struct comments
• Reading the DT should really happen in the ofdata_to_platdata method

Changes in v4:

• remove unused macros
• remove cortina prefix from macros
• remove use BSS variable
• further cleanup to meet code style guidelines
• add additinal struct comments
• rename DT blink rate symbol

MAINTAINERS | 8 +- drivers/led/Kconfig | 8 ++ drivers/led/Makefile | 1 + drivers/led/led_cortina.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 321 insertions(+), 1 deletion(-) create mode 100644 drivers/led/led_cortina.c

Reviewed-by: Simon Glass sjg@chromium.org

One comment below

[..]

diff --git a/drivers/led/led_cortina.c b/drivers/led/led_cortina.c new file mode 100644 index 0000000..a6d9159 --- /dev/null +++ b/drivers/led/led_cortina.c @@ -0,0 +1,305 @@ +// SPDX-License-Identifier: GPL-2.0+

+/*

[..]

+static int cortina_led_set_state(struct udevice *dev, enum led_state_t state) +{

•   u32 val;
  

•   struct cortina_led_cfg *priv = dev_get_priv(dev);
  

•   val = readl(priv->regs);
  

•   val &= ~(LED_OFF_ON_MASK << LED_OFF_ON_SHIFT);
  

•   switch (state) {
  

•   case LEDST_OFF:
  

•           val &= ~LED_SW_EVENT;
  

•           val |= CA_LED_OFF << LED_OFF_ON_SHIFT;
  

•           cortina_led_write(priv->regs, val);
  

•           break;
  

•   case LEDST_ON:
  

•           val |= LED_SW_EVENT;
  

•           val |= CA_LED_ON << LED_OFF_ON_SHIFT;
  

•           cortina_led_write(priv->regs, val);
  

•           break;
  

•   case LEDST_TOGGLE:
  

•           if (cortina_led_get_state(dev) == LEDST_OFF)
  

•                   return cortina_led_set_state(dev, LEDST_ON);
  

•           else
  

•                   return cortina_led_set_state(dev, LEDST_OFF);
  

•           break;
  

+#ifdef CONFIG_LED_BLINK

•   case LEDST_BLINK:
  

•           val &= ~LED_SW_EVENT;
  

•           val |= CA_LED_OFF << LED_OFF_ON_SHIFT;
  

•           val |= LED_EVENT_BLINK_MASK << LED_EVENT_BLINK_SHIFT;
  

•           cortina_led_write(priv->regs, val);
  

•           break;
  

+#endif

Try to void #ifdef. You should be able to do this:

case CONFIG_LED_BLINK: if (IS_ENABLED(CONFIG_LED_BLINK)) { val code ... break; } /* no break */

•   default:
  

•           return -EINVAL;
  

•   }
  

•   return 0;
  

+}

+static const struct led_ops cortina_led_ops = {

•   .get_state = cortina_led_get_state,
  

•   .set_state = cortina_led_set_state,
  

+};

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

•   struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev);
  

•   /* Top-level LED node */
  

•   if (!uc_plat->label) {
  

•           struct cortina_led_platdata *plt = dev_get_platdata(dev);
  

•           plt->rate1 =
  

•                   dev_read_u32_default(dev, "Cortina,blink-rate1", 256);
  

•           plt->rate2 =
  

•                   dev_read_u32_default(dev, "Cortina,blink-rate2", 512);
  

•           plt->ctrl_regs = dev_remap_addr(dev);
  

•   } else {
  

•           struct cortina_led_cfg *priv = dev_get_priv(dev);
  

•           priv->regs = dev_remap_addr(dev_get_parent(dev));
  

•           priv->pin = dev_read_u32_default(dev, "pin", LED_MAX_COUNT);
  

•           priv->blink_sel = dev_read_u32_default(dev, "blink-sel", 0);
  

•           priv->off_event = dev_read_u32_default(dev, "off-event", 0);
  

•           priv->blink_event = dev_read_u32_default(dev, "blink-event", 0);
  

•           priv->on_event = dev_read_u32_default(dev, "on-event", 0);
  

•           priv->port = dev_read_u32_default(dev, "port", 0);
  

•           if (dev_read_bool(dev, "active-low"))
  

•                   priv->active_low = true;
  

•           else
  

•                   priv->active_low = false;
  

•   }
  

•   return 0;
  

+}

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

•   struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev);
  

•   /* Top-level LED node */
  

•   if (!uc_plat->label) {
  

•           struct cortina_led_platdata *platdata = dev_get_platdata(dev);
  

•           u32 reg_value, val;
  

•           u16 rate1, rate2;
  

•           if (!platdata->ctrl_regs)
  

•                   return -EINVAL;
  

•           reg_value = 0;
  

•           reg_value |= LED_CLK_POLARITY;
  

•           rate1 = platdata->rate1;
  

•           rate2 = platdata->rate2;
  

•           val = rate1 / 16 - 1;
  

•           rate1 = val > LED_MAX_HW_BLINK ?
  

•                                   LED_MAX_HW_BLINK : val;
  

•           reg_value |= (rate1 & LED_BLINK_RATE1_MASK) <<
  

•                                   LED_BLINK_RATE1_SHIFT;
  

•           val = rate2 / 16 - 1;
  

•           rate2 = val > LED_MAX_HW_BLINK ?
  

•                                   LED_MAX_HW_BLINK : val;
  

•           reg_value |= (rate2 & LED_BLINK_RATE2_MASK) <<
  

•                                   LED_BLINK_RATE2_SHIFT;
  

•           cortina_led_write(platdata->ctrl_regs, reg_value);
  

•   } else {
  

•           struct cortina_led_cfg *priv = dev_get_priv(dev);
  

•           void __iomem *regs;
  

•           u32 val, port, off_event, blink_event, on_event;
  

•           regs = priv->regs;
  

•           if (!regs)
  

•                   return -EINVAL;
  

•           if (priv->pin >= LED_MAX_COUNT)
  

•                   return -EINVAL;
  

•           priv->regs = regs + 4 + priv->pin * 4;
  

•           val = cortina_led_read(priv->regs);
  

•           if (priv->active_low)
  

•                   val |= LED_OFF_VAL;
  

•           else
  

•                   val &= ~LED_OFF_VAL;
  

•           if (priv->blink_sel == 0)
  

•                   val &= ~LED_BLINK_SEL;
  

•           else if (priv->blink_sel == 1)
  

•                   val |= LED_BLINK_SEL;
  

•           off_event = priv->off_event;
  

•           val &= ~(LED_EVENT_OFF_MASK << LED_EVENT_OFF_SHIFT);
  

•           if (off_event != 0)
  

•                   val |= BIT(off_event) << LED_EVENT_OFF_SHIFT;
  

•           blink_event =  priv->blink_event;
  

•           val &= ~(LED_EVENT_BLINK_MASK << LED_EVENT_BLINK_SHIFT);
  

•           if (blink_event != 0)
  

•                   val |= BIT(blink_event) << LED_EVENT_BLINK_SHIFT;
  

•           on_event = priv->on_event;
  

•           val &= ~(LED_EVENT_ON_MASK << LED_EVENT_ON_SHIFT);
  

•           if (on_event != 0)
  

•                   val |= BIT(on_event) << LED_EVENT_ON_SHIFT;
  

•           port = priv->port;
  

•           val &= ~(LED_PORT_MASK << LED_PORT_SHIFT);
  

•           val |= port << LED_PORT_SHIFT;
  

•           /* force off */
  

•           val &= ~(LED_OFF_ON_MASK << LED_OFF_ON_SHIFT);
  

•           val |= CA_LED_OFF << LED_OFF_ON_SHIFT;
  

•           cortina_led_write(priv->regs, val);
  

•   }
  

•   return 0;
  

+}

+static int cortina_led_bind(struct udevice *parent) +{

•   ofnode node;
  

•   dev_for_each_subnode(node, parent) {
  

•           struct led_uc_plat *uc_plat;
  

•           struct udevice *dev;
  

•           const char *label;
  

•           int ret;
  

•           label = ofnode_read_string(node, "label");
  

•           if (!label) {
  

•                   debug("%s: node %s has no label\n", __func__,
  

•                         ofnode_get_name(node));
  

•                   return -EINVAL;
  

•           }
  

•           ret = device_bind_driver_to_node(parent, "ca-leds",
  

•                                            ofnode_get_name(node),
  

•                                            node, &dev);
  

•           if (ret)
  

•                   return ret;
  

•           uc_plat = dev_get_uclass_platdata(dev);
  

•           uc_plat->label = label;
  

•   }
  

•   return 0;
  

+}

+static const struct udevice_id ca_led_ids[] = {

•   { .compatible = "cortina,ca-leds" },
  

•   { /* sentinel */ }
  

+};

+U_BOOT_DRIVER(cortina_led) = {

•   .name = "ca-leds",
  

•   .id = UCLASS_LED,
  

•   .of_match = ca_led_ids,
  

•   .ofdata_to_platdata = ca_led_ofdata_to_platdata,
  

•   .bind = cortina_led_bind,
  

•   .probe = cortina_led_probe,
  

•   .platdata_auto_alloc_size = sizeof(struct cortina_led_platdata),
  

•   .priv_auto_alloc_size = sizeof(struct cortina_led_cfg),
  

•   .ops = &cortina_led_ops,
  

+};

2.7.4