Hi Alex,
On Thu, 19 Mar 2020 at 18:57, Alex Nemirovsky
<alex.nemirov...@cortina-access.com> wrote:
>
> From: Jway Lin <jway....@cortina-access.com>
>
> Add Cortina Access LED controller support for CAxxxx SOCs
>
> Signed-off-by: Jway Lin <jway....@cortina-access.com>
> Signed-off-by: Alex Nemirovsky <alex.nemirov...@cortina-access.com>
> CC: Simon Glass <s...@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.be...@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.clem...@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....@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
