Re: [PATCH v2 5/8] led: led_cortina: Add CAxxx LED support

Hi Alex,

On Thu, 19 Mar 2020 at 18:57, 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 v3: None Changes in v2: None

MAINTAINERS | 2 + drivers/led/Kconfig | 8 ++ drivers/led/Makefile | 1 + drivers/led/led_cortina.c | 308 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 drivers/led/led_cortina.c

diff --git a/MAINTAINERS b/MAINTAINERS index b147faa..24a2655 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -183,6 +183,7 @@ F: drivers/serial/serial_cortina.c F: drivers/mmc/ca_dw_mmc.c F: drivers/i2c/i2c-cortina.c F: drivers/i2c/i2c-cortina.h +F: drivers/led/led_cortina.c

ARM/CZ.NIC TURRIS MOX SUPPORT M: Marek Behun marek.behun@nic.cz @@ -676,6 +677,7 @@ F: drivers/serial/serial_cortina.c F: drivers/mmc/ca_dw_mmc.c F: drivers/i2c/i2c-cortina.c F: drivers/i2c/i2c-cortina.h +F: drivers/led/led_cortina.c

MIPS MSCC M: Gregory CLEMENT gregory.clement@bootlin.com diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig index 6675934..cc87fbf 100644 --- a/drivers/led/Kconfig +++ b/drivers/led/Kconfig @@ -35,6 +35,14 @@ config LED_BCM6858 This option enables support for LEDs connected to the BCM6858 HW has blinking capabilities and up to 32 LEDs can be controlled.

+config LED_CORTINA

•   bool "LED Support for Cortina Access CAxxxx SoCs"
  

•   depends on LED && (CORTINA_PLATFORM)
  

•   help
  

•     This option enables support for LEDs connected to the Cortina
  

•     Access CAxxxx SOCs.
  

config LED_BLINK bool "Support LED blinking" depends on LED diff --git a/drivers/led/Makefile b/drivers/led/Makefile index 3654dd3..8e3ae7f 100644 --- a/drivers/led/Makefile +++ b/drivers/led/Makefile @@ -8,3 +8,4 @@ obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o obj-$(CONFIG_LED_BCM6858) += led_bcm6858.o obj-$(CONFIG_$(SPL_)LED_GPIO) += led_gpio.o +obj-$(CONFIG_LED_CORTINA) += led_cortina.o diff --git a/drivers/led/led_cortina.c b/drivers/led/led_cortina.c new file mode 100644 index 0000000..53435e8 --- /dev/null +++ b/drivers/led/led_cortina.c @@ -0,0 +1,308 @@ +// SPDX-License-Identifier: GPL-2.0+

+/*

• • Copyright (C) 2020 Cortina-Access
• • Author: Jway Lin jway.lin@cortina-access.com
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <led.h> +#include <asm/io.h> +#include <dm/lists.h> +#include <linux/compat.h>

+#define CORTINA_LED_NUM 16

+#define BIT(nr) (1UL << (nr))

That is already defined somewhere

+#define cortina_LED_CONTROL 0x00

Drop the cortina_ prefix if you can.. It should be upper case. Since this is local to the file it just adds code.

+#define cortina_LED_CONFIG_0 0x04 +#define cortina_LED_CONFIG_1 0x08 +#define cortina_LED_CONFIG_2 0x0c +#define cortina_LED_CONFIG_3 0x10 +#define cortina_LED_CONFIG_4 0x14 +#define cortina_LED_CONFIG_5 0x18 +#define cortina_LED_CONFIG_6 0x1c +#define cortina_LED_CONFIG_7 0x20 +#define cortina_LED_CONFIG_8 0x24 +#define cortina_LED_CONFIG_9 0x28 +#define cortina_LED_CONFIG_10 0x2c +#define cortina_LED_CONFIG_11 0x30 +#define cortina_LED_CONFIG_12 0x34 +#define cortina_LED_CONFIG_13 0x38 +#define cortina_LED_CONFIG_14 0x3c +#define cortina_LED_CONFIG_15 0x40

What are all those used for?

+#define cortina_LED_MAX_HW_BLINK 127 +#define cortina_LED_MAX_COUNT CORTINA_LED_NUM +#define cortina_LED_MAX_PORT 8

+/* LED_CONTROL fields */ +#define cortina_LED_BLINK_RATE1_OFFSET 0 +#define cortina_LED_BLINK_RATE1_MASK 0xFF

Lower-case hex

+#define cortina_LED_BLINK_RATE2_OFFSET 8 +#define cortina_LED_BLINK_RATE2_MASK 0xFF +#define cortina_LED_CLK_TEST BIT(16) +#define cortina_LED_CLK_POLARITY BIT(17) +#define cortina_LED_CLK_TEST_MODE BIT(16) +#define cortina_LED_CLK_TEST_RX_TEST BIT(30) +#define cortina_LED_CLK_TEST_TX_TEST BIT(31)

+/* LED_CONFIG fields */ +#define cortina_LED_EVENT_ON_OFFSET 0 +#define cortina_LED_EVENT_ON_MASK 0x7 +#define cortina_LED_EVENT_BLINK_OFFSET 3 +#define cortina_LED_EVENT_BLINK_MASK 0x7 +#define cortina_LED_EVENT_OFF_OFFSET 6 +#define cortina_LED_EVENT_OFF_MASK 0x7 +#define cortina_LED_OFF_ON_OFFSET 9 +#define cortina_LED_OFF_ON_MASK 0x3 +#define cortina_LED_PORT_OFFSET 11 +#define cortina_LED_PORT_MASK 0x7 +#define cortina_LED_OFF_VAL BIT(14) +#define cortina_LED_SW_EVENT BIT(15) +#define cortina_LED_BLINK_SEL BIT(16)

Need a struct comment

+struct cortina_led_cfg {

•   void __iomem *regs;
  

•   spinlock_t *lock;       /* protect LED resource access */
  

•   int idx;
  

•   bool active_low;
  

•   int off_event;
  

•   int blink_event;
  

•   int on_event;
  

•   int port;
  

•   int blink;
  

•   int enable;
  

+};

+struct cortina_led_top_cfg {

here too

•   void __iomem *regs;
  

•   u16 blink_rate1;
  

•   u16 blink_rate2;
  

+};

+static struct cortina_led_top_cfg glb_led_ctrl;

You are not allowed to use BSS in driver model. It's not clear why you have two structures as there are no comments. Can you not combine them?

+static void cortina_led_write(void __iomem *reg, unsigned long data) +{

•   writel(data, reg);
  

+}

+static unsigned long cortina_led_read(void __iomem *reg) +{

•   return readl(reg);
  

+}

+static enum led_state_t cortina_led_get_state(struct udevice *dev) +{

•   struct cortina_led_cfg *priv = dev_get_priv(dev);
  

•   enum led_state_t state = LEDST_OFF;
  

•   u32 val;
  

•   val = readl(priv->regs);
  

•   if (val & cortina_LED_SW_EVENT)
  

•           state = LEDST_ON;
  

•   return state;
  

+}

+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);
  

•   switch (state) {
  

•   case LEDST_OFF:
  

•           val &= ~cortina_LED_SW_EVENT;
  

•           val &= ~(cortina_LED_OFF_ON_MASK << cortina_LED_OFF_ON_OFFSET);
  

•           val |= 0x3 << cortina_LED_OFF_ON_OFFSET;
  

•           cortina_led_write(priv->regs, val);
  

•           break;
  

•   case LEDST_ON:
  

•           val |= cortina_LED_SW_EVENT;
  

•           val &= ~(cortina_LED_OFF_ON_MASK << cortina_LED_OFF_ON_OFFSET);
  

•           val |= 0x1 << cortina_LED_OFF_ON_OFFSET;
  

•           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;
  

•   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 cortina_led_probe(struct udevice *dev) +{

•   struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev);
  

•   void __iomem *regs;
  

•   u32 reg_value, val;
  

•   u16 rate1, rate2;
  

•   struct cortina_led_cfg *priv = dev_get_priv(dev);
  

•   unsigned int pin;
  

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

•   /* Top-level LED node */
  

•   if (!uc_plat->label) {
  

•           regs = dev_remap_addr(dev);
  

•   if (!regs)
  

•           return -EINVAL;
  

•   glb_led_ctrl.regs = regs;
  

•   reg_value = 0;
  

•   reg_value |= cortina_LED_CLK_POLARITY;
  

•   rate1 = dev_read_u32_default(dev, "cortina, blink_rate1", 256);
  

•   rate2 = dev_read_u32_default(dev, "cortina, blink_rate2", 512);
  

Strictly speaking you should read your platdata into a plat struct in your ofdata_to_platdata method.

Also do you have a binding file for this? I think we sholld use hyphen for properties instead of underscore.

•   val = rate1 / 16 - 1;
  

•   glb_led_ctrl.blink_rate1 = val > cortina_LED_MAX_HW_BLINK ?
  

•                       cortina_LED_MAX_HW_BLINK : val;
  

•   reg_value |= (glb_led_ctrl.blink_rate1 & cortina_LED_BLINK_RATE1_MASK)
  

•                   << cortina_LED_BLINK_RATE1_OFFSET;
  

•   val = rate2 / 16 - 1;
  

•   glb_led_ctrl.blink_rate2 = val > cortina_LED_MAX_HW_BLINK ?
  

•                           cortina_LED_MAX_HW_BLINK : val;
  

•   reg_value |= (glb_led_ctrl.blink_rate2 & cortina_LED_BLINK_RATE2_MASK)
  

•                   << cortina_LED_BLINK_RATE2_OFFSET;
  

•   cortina_led_write(glb_led_ctrl.regs, reg_value);
  

•   } else {
  

•           regs = dev_remap_addr(dev_get_parent(dev));
  

•   if (!regs)
  

•           return -EINVAL;
  

•   pin = dev_read_u32_default(dev, "pin", cortina_LED_MAX_COUNT);
  

•   if (pin >= cortina_LED_MAX_COUNT)
  

•           return -EINVAL;
  

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

•   val = cortina_led_read(priv->regs);
  

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

•           priv->active_low = true;
  

•           val |= cortina_LED_OFF_VAL;
  

•   } else {
  

•           priv->active_low = false;
  

•           val &= ~cortina_LED_OFF_VAL;
  

•   }
  

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

•   if (blink == 0) {
  

•           priv->blink = 0;
  

•           val &= ~cortina_LED_BLINK_SEL;
  

•   } else if (blink == 1) {
  

•           priv->blink = 1;
  

•           val |= cortina_LED_BLINK_SEL;
  

•   }
  

•   //Todo : cortina_led_config();
  

•   off_event = dev_read_u32_default(dev, "off-event", 3);
  

•   val &= ~(cortina_LED_EVENT_OFF_MASK << cortina_LED_EVENT_OFF_OFFSET);
  

•   if (off_event != 3) {
  

•           priv->off_event = off_event;
  

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

•   }
  

•   blink_event = dev_read_u32_default(dev, "blink-event", 3);
  

•   val &= ~(cortina_LED_EVENT_BLINK_MASK <<
  

•            cortina_LED_EVENT_BLINK_OFFSET);
  

•   if (blink_event != 3) {
  

•           priv->blink_event = blink_event;
  

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

•   }
  

•   on_event = dev_read_u32_default(dev, "on-event", 3);
  

•   val &= ~(cortina_LED_EVENT_ON_MASK << cortina_LED_EVENT_ON_OFFSET);
  

•   if (on_event != 3) {
  

•           priv->on_event = on_event;
  

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

•   }
  

•   port = dev_read_u32_default(dev, "port", 0);
  

•   priv->port = port;
  

•   val &= ~(cortina_LED_PORT_MASK << cortina_LED_PORT_OFFSET);
  

•   val |= port << cortina_LED_PORT_OFFSET;
  

•   priv->enable = 0;
  

•   /* force off */
  

•   val &= ~(cortina_LED_OFF_ON_MASK << cortina_LED_OFF_ON_OFFSET);
  

•   val |= 0x3 << cortina_LED_OFF_ON_OFFSET;
  

•   cortina_led_write(priv->regs, val);
  

•   }
  

funny indent?

•   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);
  

Does device_bind_ofnode() help here?

•           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,
  

•   .bind = cortina_led_bind,
  

•   .probe = cortina_led_probe,
  

•   .priv_auto_alloc_size = sizeof(struct cortina_led_cfg),
  

•   .ops = &cortina_led_ops,
  

+};

2.7.4

Regards, Simon