[PATCH 1/2] dt-bindings: leds: Add Allwinner R329/D1 LED controller

[PATCH 1/2] dt-bindings: leds: Add Allwinner R329/D1 LED controller

[Samuel Holland]

The Allwinner R329 and D1 SoCs contain an LED controller designed to
drive a series of RGB LED pixels. It supports PIO and DMA transfers, and
has configurable timing and pixel format.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../leds/allwinner,sun50i-r329-ledc.yaml      | 141 ++++++++++++++++++
 1 file changed, 141 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml

diff --git a/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml b/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
new file mode 100644
index 000000000000..bf883944e911
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/allwinner,sun50i-r329-ledc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner sunxi LED Controller Bindings
+
+maintainers:
+  - Samuel Holland <samuel@sholland.org>
+
+description:
+  The LED controller found in Allwinner sunxi SoCs uses a one-wire serial
+  interface to drive up to 1024 RGB LEDs.
+
+properties:
+  compatible:
+    oneOf:
+      - const: allwinner,sun50i-r329-ledc
+      - items:
+          - enum:
+              - allwinner,sun20i-d1-ledc
+          - const: allwinner,sun50i-r329-ledc
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  clocks:
+    items:
+      - description: Bus clock
+      - description: Module clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: mod
+
+  resets:
+    maxItems: 1
+
+  dmas:
+    items:
+      - description: TX DMA channel
+
+  dma-names:
+    items:
+      - const: tx
+
+  interrupts:
+    maxItems: 1
+
+  vled-supply:
+    description: Regulator supplying power to external LEDs
+
+  format:
+    description: Pixel format (subpixel transmission order), default is "grb"
+    enum:
+      - "bgr"
+      - "brg"
+      - "gbr"
+      - "grb"
+      - "rbg"
+      - "rgb"
+
+  t0h-ns:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Length of high pulse when transmitting a "0" bit
+
+  t0l-ns:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Length of low pulse when transmitting a "0" bit
+
+  t1h-ns:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Length of high pulse when transmitting a "1" bit
+
+  t1h-ns:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Length of low pulse when transmitting a "1" bit
+
+  treset-ns:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Minimum delay between transmission frames
+
+patternProperties:
+  "^multi-led@[0-9a-f]+$":
+    type: object
+    $ref: leds-class-multicolor.yaml#
+    properties:
+      reg:
+        minimum: 0
+        maximum: 1023
+        description: Index of the LED in the series (must be contiguous)
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - dmas
+  - dma-names
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/leds/common.h>
+
+    ledc: led-controller@2008000 {
+      compatible = "allwinner,sun20i-d1-ledc",
+                   "allwinner,sun50i-r329-ledc";
+      reg = <0x2008000 0x400>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      clocks = <&ccu 12>, <&ccu 34>;
+      clock-names = "bus", "mod";
+      resets = <&ccu 12>;
+      dmas = <&dma 42>;
+      dma-names = "tx";
+      interrupts = <36 IRQ_TYPE_LEVEL_HIGH>;
+
+      multi-led@0 {
+        reg = <0x0>;
+        color = <LED_COLOR_ID_RGB>;
+        function = LED_FUNCTION_INDICATOR;
+      };
+    };
+
+...
-- 
2.31.1

[PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Samuel Holland]

Some Allwinner sunxi SoCs, starting with the R329, contain an LED
controller designed to drive RGB LED pixels. Add a driver for it using
the multicolor LED framework, and with LEDs defined in the device tree.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/leds/Kconfig      |   8 +
 drivers/leds/Makefile     |   1 +
 drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 571 insertions(+)
 create mode 100644 drivers/leds/leds-sunxi.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..559d2ca0a7f4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -297,6 +297,14 @@ config LEDS_SUNFIRE
 	  This option enables support for the Left, Middle, and Right
 	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
 
+config LEDS_SUNXI
+	tristate "LED support for Allwinner sunxi LED controller"
+	depends on LEDS_CLASS
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  This option enables support for the LED controller provided in
+	  some Allwinner sunxi SoCs.
+
 config LEDS_IPAQ_MICRO
 	tristate "LED Support for the Compaq iPAQ h3xxx"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c636ec069612..3395f188e5bf 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
 obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
+obj-$(CONFIG_LEDS_SUNXI)		+= leds-sunxi.o
 obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
 obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
 obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
diff --git a/drivers/leds/leds-sunxi.c b/drivers/leds/leds-sunxi.c
new file mode 100644
index 000000000000..6569d64e0c76
--- /dev/null
+++ b/drivers/leds/leds-sunxi.c
@@ -0,0 +1,562 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2021 Samuel Holland <samuel@sholland.org>
+//
+// Partly based on drivers/leds/leds-turris-omnia.c, which is:
+//     Copyright (c) 2020 by Marek Behún <kabel@kernel.org>
+//
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+
+#define LEDC_CTRL_REG			0x0000
+#define LEDC_CTRL_REG_DATA_LENGTH		(0x1fff << 16)
+#define LEDC_CTRL_REG_RGB_MODE			(0x7 << 6)
+#define LEDC_CTRL_REG_LEDC_EN			BIT(0)
+#define LEDC_T01_TIMING_CTRL_REG	0x0004
+#define LEDC_T01_TIMING_CTRL_REG_T1H		(0x3f << 21)
+#define LEDC_T01_TIMING_CTRL_REG_T1L		(0x1f << 16)
+#define LEDC_T01_TIMING_CTRL_REG_T0H		(0x1f << 6)
+#define LEDC_T01_TIMING_CTRL_REG_T0L		(0x3f << 0)
+#define LEDC_RESET_TIMING_CTRL_REG	0x000c
+#define LEDC_RESET_TIMING_CTRL_REG_LED_NUM	(0x3ff << 0)
+#define LEDC_DATA_REG			0x0014
+#define LEDC_DMA_CTRL_REG		0x0018
+#define LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL	(0x1f << 0)
+#define LEDC_INT_CTRL_REG		0x001c
+#define LEDC_INT_CTRL_REG_GLOBAL_INT_EN		BIT(5)
+#define LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN	BIT(1)
+#define LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN	BIT(0)
+#define LEDC_INT_STS_REG		0x0020
+#define LEDC_INT_STS_REG_FIFO_CPUREQ_INT	BIT(1)
+#define LEDC_INT_STS_REG_TRANS_FINISH_INT	BIT(0)
+
+#define LEDC_FIFO_DEPTH			32
+#define LEDC_MAX_LEDS			1024
+
+#define LEDS_TO_BYTES(n)		((n) * sizeof(u32))
+
+struct sunxi_ledc_led {
+	struct led_classdev_mc mc_cdev;
+	struct mc_subled subled_info[3];
+};
+#define to_ledc_led(mc) container_of(mc, struct sunxi_ledc_led, mc_cdev)
+
+struct sunxi_ledc_timing {
+	u32 t0h_ns;
+	u32 t0l_ns;
+	u32 t1h_ns;
+	u32 t1l_ns;
+	u32 treset_ns;
+};
+
+struct sunxi_ledc {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *bus_clk;
+	struct clk *mod_clk;
+	struct reset_control *reset;
+	struct regulator *vled;
+
+	u32 *buffer;
+	struct dma_chan *dma_chan;
+	dma_addr_t dma_handle;
+	int pio_length;
+	int pio_offset;
+
+	spinlock_t lock;
+	int next_length;
+	bool xfer_active;
+
+	u32 format;
+	struct sunxi_ledc_timing timing;
+
+	int num_leds;
+	struct sunxi_ledc_led leds[];
+};
+
+static int sunxi_ledc_dma_xfer(struct sunxi_ledc *priv, int length)
+{
+	struct dma_async_tx_descriptor *desc;
+	dma_cookie_t cookie;
+
+	desc = dmaengine_prep_slave_single(priv->dma_chan, priv->dma_handle,
+					   LEDS_TO_BYTES(length),
+					   DMA_MEM_TO_DEV, 0);
+	if (!desc)
+		return -ENOMEM;
+
+	cookie = dmaengine_submit(desc);
+	if (dma_submit_error(cookie))
+		return -EIO;
+
+	dma_async_issue_pending(priv->dma_chan);
+
+	return 0;
+}
+
+static void sunxi_ledc_pio_xfer(struct sunxi_ledc *priv, int length)
+{
+	u32 burst, offset, val;
+
+	if (length) {
+		/* New transfer (FIFO is empty). */
+		offset = 0;
+		burst  = min(length, LEDC_FIFO_DEPTH);
+	} else {
+		/* Existing transfer (FIFO is half-full). */
+		length = priv->pio_length;
+		offset = priv->pio_offset;
+		burst  = min(length, LEDC_FIFO_DEPTH / 2);
+	}
+
+	writesl(priv->base + LEDC_DATA_REG, priv->buffer + offset, burst);
+
+	if (burst < length) {
+		priv->pio_length = length - burst;
+		priv->pio_offset = offset + burst;
+
+		if (!offset) {
+			val = readl(priv->base + LEDC_INT_CTRL_REG);
+			val |= LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
+			writel(val, priv->base + LEDC_INT_CTRL_REG);
+		}
+	} else {
+		/* Disable the request IRQ once all data is written. */
+		val = readl(priv->base + LEDC_INT_CTRL_REG);
+		val &= ~LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
+		writel(val, priv->base + LEDC_INT_CTRL_REG);
+	}
+}
+
+static void sunxi_ledc_start_xfer(struct sunxi_ledc *priv, int length)
+{
+	u32 val;
+
+	dev_dbg(priv->dev, "Updating %d LEDs\n", length);
+
+	val = readl(priv->base + LEDC_CTRL_REG);
+	val &= ~LEDC_CTRL_REG_DATA_LENGTH;
+	val |= length << 16 | LEDC_CTRL_REG_LEDC_EN;
+	writel(val, priv->base + LEDC_CTRL_REG);
+
+	if (length > LEDC_FIFO_DEPTH) {
+		int ret = sunxi_ledc_dma_xfer(priv, length);
+
+		if (!ret)
+			return;
+
+		dev_warn(priv->dev, "Failed to set up DMA: %d\n", ret);
+	}
+
+	sunxi_ledc_pio_xfer(priv, length);
+}
+
+static irqreturn_t sunxi_ledc_irq(int irq, void *dev_id)
+{
+	struct sunxi_ledc *priv = dev_id;
+	u32 val;
+
+	val = readl(priv->base + LEDC_INT_STS_REG);
+
+	if (val & LEDC_INT_STS_REG_TRANS_FINISH_INT) {
+		int next_length;
+
+		/* Start the next transfer if needed. */
+		spin_lock(&priv->lock);
+		next_length = priv->next_length;
+		if (next_length)
+			priv->next_length = 0;
+		else
+			priv->xfer_active = false;
+		spin_unlock(&priv->lock);
+
+		if (next_length)
+			sunxi_ledc_start_xfer(priv, next_length);
+	} else if (val & LEDC_INT_STS_REG_FIFO_CPUREQ_INT) {
+		/* Continue the current transfer. */
+		sunxi_ledc_pio_xfer(priv, 0);
+	}
+
+	writel(val, priv->base + LEDC_INT_STS_REG);
+
+	return IRQ_HANDLED;
+}
+
+static void sunxi_ledc_brightness_set(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct sunxi_ledc *priv = dev_get_drvdata(cdev->dev->parent);
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct sunxi_ledc_led *led = to_ledc_led(mc_cdev);
+	int addr = led - priv->leds;
+	unsigned long flags;
+	bool xfer_active;
+	int next_length;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	priv->buffer[addr] = led->subled_info[0].brightness << 16 |
+			     led->subled_info[1].brightness <<  8 |
+			     led->subled_info[2].brightness;
+
+	dev_dbg(priv->dev, "LED %d -> #%06x\n", addr, priv->buffer[addr]);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	next_length = max(priv->next_length, addr + 1);
+	xfer_active = priv->xfer_active;
+	if (xfer_active)
+		priv->next_length = next_length;
+	else
+		priv->xfer_active = true;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (!xfer_active)
+		sunxi_ledc_start_xfer(priv, next_length);
+}
+
+static const char *const sunxi_ledc_formats[] = {
+	"rgb",
+	"rbg",
+	"grb",
+	"gbr",
+	"brg",
+	"bgr",
+};
+
+static int sunxi_ledc_parse_format(const struct device_node *np,
+				    struct sunxi_ledc *priv)
+{
+	const char *format = "grb";
+	u32 i;
+
+	of_property_read_string(np, "format", &format);
+
+	for (i = 0; i < ARRAY_SIZE(sunxi_ledc_formats); ++i) {
+		if (!strcmp(format, sunxi_ledc_formats[i])) {
+			priv->format = i;
+			return 0;
+		}
+	}
+
+	dev_err(priv->dev, "Bad pixel format '%s'\n", format);
+
+	return -EINVAL;
+}
+
+static void sunxi_ledc_set_format(struct sunxi_ledc *priv)
+{
+	u32 val;
+
+	val = readl(priv->base + LEDC_CTRL_REG);
+	val &= ~LEDC_CTRL_REG_RGB_MODE;
+	val |= priv->format << 6;
+	writel(val, priv->base + LEDC_CTRL_REG);
+}
+
+static const struct sunxi_ledc_timing sunxi_ledc_default_timing = {
+	.t0h_ns = 336,
+	.t0l_ns = 840,
+	.t1h_ns = 882,
+	.t1l_ns = 294,
+	.treset_ns = 300000,
+};
+
+static int sunxi_ledc_parse_timing(const struct device_node *np,
+				    struct sunxi_ledc *priv)
+{
+	struct sunxi_ledc_timing *timing = &priv->timing;
+
+	*timing = sunxi_ledc_default_timing;
+
+	of_property_read_u32(np, "t0h-ns", &timing->t0h_ns);
+	of_property_read_u32(np, "t0l-ns", &timing->t0l_ns);
+	of_property_read_u32(np, "t1h-ns", &timing->t1h_ns);
+	of_property_read_u32(np, "t1l-ns", &timing->t1l_ns);
+	of_property_read_u32(np, "treset-ns", &timing->treset_ns);
+
+	return 0;
+}
+
+static void sunxi_ledc_set_timing(struct sunxi_ledc *priv)
+{
+	const struct sunxi_ledc_timing *timing = &priv->timing;
+	unsigned long mod_freq = clk_get_rate(priv->mod_clk);
+	u32 cycle_ns = NSEC_PER_SEC / mod_freq;
+	u32 val;
+
+	val = (timing->t1h_ns / cycle_ns) << 21 |
+	      (timing->t1l_ns / cycle_ns) << 16 |
+	      (timing->t0h_ns / cycle_ns) <<  6 |
+	      (timing->t0l_ns / cycle_ns);
+	writel(val, priv->base + LEDC_T01_TIMING_CTRL_REG);
+
+	val = (timing->treset_ns / cycle_ns) << 16 |
+	      (priv->num_leds - 1);
+	writel(val, priv->base + LEDC_RESET_TIMING_CTRL_REG);
+}
+
+static int sunxi_ledc_resume(struct device *dev)
+{
+	struct sunxi_ledc *priv = dev_get_drvdata(dev);
+	u32 val;
+	int ret;
+
+	ret = regulator_enable(priv->vled);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(priv->reset);
+	if (ret)
+		goto err_disable_regulator;
+
+	ret = clk_prepare_enable(priv->bus_clk);
+	if (ret)
+		goto err_assert_reset;
+
+	ret = clk_prepare_enable(priv->mod_clk);
+	if (ret)
+		goto err_disable_bus_clk;
+
+	sunxi_ledc_set_format(priv);
+	sunxi_ledc_set_timing(priv);
+
+	/* The trigger level must be at least the burst length. */
+	val = readl(priv->base + LEDC_DMA_CTRL_REG);
+	val &= ~LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL;
+	val |= LEDC_FIFO_DEPTH / 2;
+	writel(val, priv->base + LEDC_DMA_CTRL_REG);
+
+	val = LEDC_INT_CTRL_REG_GLOBAL_INT_EN |
+	      LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN;
+	writel(val, priv->base + LEDC_INT_CTRL_REG);
+
+	return 0;
+
+err_disable_bus_clk:
+	clk_disable_unprepare(priv->bus_clk);
+err_assert_reset:
+	reset_control_assert(priv->reset);
+err_disable_regulator:
+	regulator_disable(priv->vled);
+
+	return ret;
+}
+
+static int sunxi_ledc_suspend(struct device *dev)
+{
+	struct sunxi_ledc *priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->mod_clk);
+	clk_disable_unprepare(priv->bus_clk);
+	reset_control_assert(priv->reset);
+	regulator_disable(priv->vled);
+
+	return 0;
+}
+
+static void sunxi_ledc_dma_cleanup(void *data)
+{
+	struct sunxi_ledc *priv = data;
+	struct device *dma_dev = dmaengine_get_dma_device(priv->dma_chan);
+
+	if (priv->buffer)
+		dma_free_wc(dma_dev, LEDS_TO_BYTES(priv->num_leds),
+			    priv->buffer, priv->dma_handle);
+	dma_release_channel(priv->dma_chan);
+}
+
+static int sunxi_ledc_probe(struct platform_device *pdev)
+{
+	const struct device_node *np = pdev->dev.of_node;
+	struct dma_slave_config dma_cfg = {};
+	struct led_init_data init_data = {};
+	struct device *dev = &pdev->dev;
+	struct device_node *child;
+	struct sunxi_ledc *priv;
+	struct resource *mem;
+	int count, irq, ret;
+
+	count = of_get_available_child_count(np);
+	if (!count)
+		return -ENODEV;
+	if (count > LEDC_MAX_LEDS) {
+		dev_err(dev, "Too many LEDs! (max is %d)\n", LEDC_MAX_LEDS);
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->num_leds = count;
+	spin_lock_init(&priv->lock);
+	dev_set_drvdata(dev, priv);
+
+	ret = sunxi_ledc_parse_format(np, priv);
+	if (ret)
+		return ret;
+
+	ret = sunxi_ledc_parse_timing(np, priv);
+	if (ret)
+		return ret;
+
+	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->bus_clk = devm_clk_get(dev, "bus");
+	if (IS_ERR(priv->bus_clk))
+		return PTR_ERR(priv->bus_clk);
+
+	priv->mod_clk = devm_clk_get(dev, "mod");
+	if (IS_ERR(priv->mod_clk))
+		return PTR_ERR(priv->mod_clk);
+
+	priv->reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return PTR_ERR(priv->reset);
+
+	priv->vled = devm_regulator_get(dev, "vled");
+	if (IS_ERR(priv->vled))
+		return PTR_ERR(priv->vled);
+
+	priv->dma_chan = dma_request_chan(dev, "tx");
+	if (IS_ERR(priv->dma_chan))
+		return PTR_ERR(priv->dma_chan);
+
+	ret = devm_add_action_or_reset(dev, sunxi_ledc_dma_cleanup, priv);
+	if (ret)
+		return ret;
+
+	dma_cfg.dst_addr	= mem->start + LEDC_DATA_REG;
+	dma_cfg.dst_addr_width	= DMA_SLAVE_BUSWIDTH_4_BYTES;
+	dma_cfg.dst_maxburst	= LEDC_FIFO_DEPTH / 2;
+	ret = dmaengine_slave_config(priv->dma_chan, &dma_cfg);
+	if (ret)
+		return ret;
+
+	priv->buffer = dma_alloc_wc(dmaengine_get_dma_device(priv->dma_chan),
+				    LEDS_TO_BYTES(priv->num_leds),
+				    &priv->dma_handle, GFP_KERNEL);
+	if (!priv->buffer)
+		return -ENOMEM;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, sunxi_ledc_irq, 0, dev_name(dev), priv);
+	if (ret)
+		return ret;
+
+	ret = sunxi_ledc_resume(dev);
+	if (ret)
+		return ret;
+
+	for_each_available_child_of_node(np, child) {
+		struct sunxi_ledc_led *led;
+		struct led_classdev *cdev;
+		u32 addr, color;
+
+		ret = of_property_read_u32(child, "reg", &addr);
+		if (ret || addr >= count) {
+			dev_err(dev, "LED 'reg' values must be from 0 to %d\n",
+				priv->num_leds - 1);
+			ret = -EINVAL;
+			goto err_put_child;
+		}
+
+		ret = of_property_read_u32(child, "color", &color);
+		if (ret || color != LED_COLOR_ID_RGB) {
+			dev_err(dev, "LED 'color' must be LED_COLOR_ID_RGB\n");
+			ret = -EINVAL;
+			goto err_put_child;
+		}
+
+		led = &priv->leds[addr];
+
+		led->subled_info[0].color_index = LED_COLOR_ID_RED;
+		led->subled_info[0].channel = 0;
+		led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+		led->subled_info[1].channel = 1;
+		led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+		led->subled_info[2].channel = 2;
+
+		led->mc_cdev.num_colors = ARRAY_SIZE(led->subled_info);
+		led->mc_cdev.subled_info = led->subled_info;
+
+		cdev = &led->mc_cdev.led_cdev;
+		cdev->max_brightness = U8_MAX;
+		cdev->brightness_set = sunxi_ledc_brightness_set;
+
+		init_data.fwnode = of_fwnode_handle(child);
+
+		ret = devm_led_classdev_multicolor_register_ext(dev,
+								&led->mc_cdev,
+								&init_data);
+		if (ret) {
+			dev_err(dev, "Failed to register LED %u: %d\n", addr, ret);
+			goto err_put_child;
+		}
+	}
+
+	dev_info(dev, "Registered %d LEDs\n", priv->num_leds);
+
+	return 0;
+
+err_put_child:
+	of_node_put(child);
+	sunxi_ledc_suspend(&pdev->dev);
+
+	return ret;
+}
+
+int sunxi_ledc_remove(struct platform_device *pdev)
+{
+	sunxi_ledc_suspend(&pdev->dev);
+
+	return 0;
+}
+
+void sunxi_ledc_shutdown(struct platform_device *pdev)
+{
+	sunxi_ledc_suspend(&pdev->dev);
+}
+
+static const struct of_device_id sunxi_ledc_of_match[] = {
+	{ .compatible = "allwinner,sun50i-r329-ledc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunxi_ledc_of_match);
+
+SIMPLE_DEV_PM_OPS(sunxi_ledc_pm, sunxi_ledc_suspend, sunxi_ledc_resume);
+
+static struct platform_driver sunxi_ledc_driver = {
+	.probe		= sunxi_ledc_probe,
+	.remove		= sunxi_ledc_remove,
+	.shutdown	= sunxi_ledc_shutdown,
+	.driver		= {
+		.name		= "sunxi-ledc",
+		.of_match_table	= sunxi_ledc_of_match,
+		.pm		= pm_ptr(&sunxi_ledc_pm),
+	},
+};
+module_platform_driver(sunxi_ledc_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Allwinner sunxi LED controller driver");
+MODULE_LICENSE("GPL");
-- 
2.31.1

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

Hi Samuel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-linux-leds/for-next]
[also build test WARNING on next-20210902]
[cannot apply to robh/for-next sunxi/sunxi/for-next v5.14]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Samuel-Holland/dt-bindings-leds-Add-Allwinner-R329-D1-LED-controller/20210903-074537
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5f2d46e769847b3716683bc93cf7568f7e994044
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Samuel-Holland/dt-bindings-leds-Add-Allwinner-R329-D1-LED-controller/20210903-074537
        git checkout 5f2d46e769847b3716683bc93cf7568f7e994044
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-sunxi.c:528:5: warning: no previous prototype for 'sunxi_ledc_remove' [-Wmissing-prototypes]
     528 | int sunxi_ledc_remove(struct platform_device *pdev)
         |     ^~~~~~~~~~~~~~~~~
>> drivers/leds/leds-sunxi.c:535:6: warning: no previous prototype for 'sunxi_ledc_shutdown' [-Wmissing-prototypes]
     535 | void sunxi_ledc_shutdown(struct platform_device *pdev)
         |      ^~~~~~~~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
   Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
   Selected by
   - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
   - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC


vim +/sunxi_ledc_remove +528 drivers/leds/leds-sunxi.c

   527	
 > 528	int sunxi_ledc_remove(struct platform_device *pdev)
   529	{
   530		sunxi_ledc_suspend(&pdev->dev);
   531	
   532		return 0;
   533	}
   534	
 > 535	void sunxi_ledc_shutdown(struct platform_device *pdev)
   536	{
   537		sunxi_ledc_suspend(&pdev->dev);
   538	}
   539	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55005 bytes --]

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3720 bytes --]

Hi Samuel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-linux-leds/for-next]
[also build test ERROR on next-20210903]
[cannot apply to robh/for-next sunxi/sunxi/for-next v5.14]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Samuel-Holland/dt-bindings-leds-Add-Allwinner-R329-D1-LED-controller/20210903-074537
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: alpha-randconfig-r005-20210903 (attached as .config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5f2d46e769847b3716683bc93cf7568f7e994044
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Samuel-Holland/dt-bindings-leds-Add-Allwinner-R329-D1-LED-controller/20210903-074537
        git checkout 5f2d46e769847b3716683bc93cf7568f7e994044
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/leds/leds-sunxi.c: In function 'sunxi_ledc_pio_xfer':
>> drivers/leds/leds-sunxi.c:125:9: error: implicit declaration of function 'writesl'; did you mean 'writel'? [-Werror=implicit-function-declaration]
     125 |         writesl(priv->base + LEDC_DATA_REG, priv->buffer + offset, burst);
         |         ^~~~~~~
         |         writel
   drivers/leds/leds-sunxi.c: At top level:
   drivers/leds/leds-sunxi.c:528:5: warning: no previous prototype for 'sunxi_ledc_remove' [-Wmissing-prototypes]
     528 | int sunxi_ledc_remove(struct platform_device *pdev)
         |     ^~~~~~~~~~~~~~~~~
   drivers/leds/leds-sunxi.c:535:6: warning: no previous prototype for 'sunxi_ledc_shutdown' [-Wmissing-prototypes]
     535 | void sunxi_ledc_shutdown(struct platform_device *pdev)
         |      ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +125 drivers/leds/leds-sunxi.c

   109	
   110	static void sunxi_ledc_pio_xfer(struct sunxi_ledc *priv, int length)
   111	{
   112		u32 burst, offset, val;
   113	
   114		if (length) {
   115			/* New transfer (FIFO is empty). */
   116			offset = 0;
   117			burst  = min(length, LEDC_FIFO_DEPTH);
   118		} else {
   119			/* Existing transfer (FIFO is half-full). */
   120			length = priv->pio_length;
   121			offset = priv->pio_offset;
   122			burst  = min(length, LEDC_FIFO_DEPTH / 2);
   123		}
   124	
 > 125		writesl(priv->base + LEDC_DATA_REG, priv->buffer + offset, burst);
   126	
   127		if (burst < length) {
   128			priv->pio_length = length - burst;
   129			priv->pio_offset = offset + burst;
   130	
   131			if (!offset) {
   132				val = readl(priv->base + LEDC_INT_CTRL_REG);
   133				val |= LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
   134				writel(val, priv->base + LEDC_INT_CTRL_REG);
   135			}
   136		} else {
   137			/* Disable the request IRQ once all data is written. */
   138			val = readl(priv->base + LEDC_INT_CTRL_REG);
   139			val &= ~LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
   140			writel(val, priv->base + LEDC_INT_CTRL_REG);
   141		}
   142	}
   143	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32369 bytes --]

Re: [PATCH 1/2] dt-bindings: leds: Add Allwinner R329/D1 LED controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4903 bytes --]

Hi,

On Thu, Sep 02, 2021 at 06:42:27PM -0500, Samuel Holland wrote:
> The Allwinner R329 and D1 SoCs contain an LED controller designed to
> drive a series of RGB LED pixels. It supports PIO and DMA transfers, and
> has configurable timing and pixel format.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../leds/allwinner,sun50i-r329-ledc.yaml      | 141 ++++++++++++++++++
>  1 file changed, 141 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml b/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
> new file mode 100644
> index 000000000000..bf883944e911
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
> @@ -0,0 +1,141 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/allwinner,sun50i-r329-ledc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner sunxi LED Controller Bindings

sunxi is ambiguous, it implies that it works with every Allwinner SoC,
but actually supports only one. Please mention R329 everywhere you used
sunxi.

> +
> +maintainers:
> +  - Samuel Holland <samuel@sholland.org>
> +
> +description:
> +  The LED controller found in Allwinner sunxi SoCs uses a one-wire serial
> +  interface to drive up to 1024 RGB LEDs.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: allwinner,sun50i-r329-ledc
> +      - items:
> +          - enum:
> +              - allwinner,sun20i-d1-ledc
> +          - const: allwinner,sun50i-r329-ledc
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: Bus clock
> +      - description: Module clock
> +
> +  clock-names:
> +    items:
> +      - const: bus
> +      - const: mod
> +
> +  resets:
> +    maxItems: 1
> +
> +  dmas:
> +    items:
> +      - description: TX DMA channel
> +
> +  dma-names:
> +    items:
> +      - const: tx

You don't need the items: here since you have a single item.

> +  interrupts:
> +    maxItems: 1
> +
> +  vled-supply:
> +    description: Regulator supplying power to external LEDs
> +
> +  format:
> +    description: Pixel format (subpixel transmission order), default is "grb"
> +    enum:
> +      - "bgr"
> +      - "brg"
> +      - "gbr"
> +      - "grb"
> +      - "rbg"
> +      - "rgb"
> +
> +  t0h-ns:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Length of high pulse when transmitting a "0" bit

Every property with an -ns suffix is already a uint32 here:

https://github.com/devicetree-org/dt-schema/blob/master/schemas/property-units.yaml#L54

Just make sure you have a single item

Looks good otherwise, thanks

Maxime

> +  t0l-ns:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Length of low pulse when transmitting a "0" bit
> +
> +  t1h-ns:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Length of high pulse when transmitting a "1" bit
> +
> +  t1h-ns:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Length of low pulse when transmitting a "1" bit
> +
> +  treset-ns:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Minimum delay between transmission frames
> +
> +patternProperties:
> +  "^multi-led@[0-9a-f]+$":
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 1023
> +        description: Index of the LED in the series (must be contiguous)
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +  - dmas
> +  - dma-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/leds/common.h>
> +
> +    ledc: led-controller@2008000 {
> +      compatible = "allwinner,sun20i-d1-ledc",
> +                   "allwinner,sun50i-r329-ledc";
> +      reg = <0x2008000 0x400>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      clocks = <&ccu 12>, <&ccu 34>;
> +      clock-names = "bus", "mod";
> +      resets = <&ccu 12>;
> +      dmas = <&dma 42>;
> +      dma-names = "tx";
> +      interrupts = <36 IRQ_TYPE_LEVEL_HIGH>;
> +
> +      multi-led@0 {
> +        reg = <0x0>;
> +        color = <LED_COLOR_ID_RGB>;
> +        function = LED_FUNCTION_INDICATOR;
> +      };
> +    };
> +
> +...
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 16055 bytes --]

On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:
> Some Allwinner sunxi SoCs, starting with the R329, contain an LED
> controller designed to drive RGB LED pixels. Add a driver for it using
> the multicolor LED framework, and with LEDs defined in the device tree.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/leds/Kconfig      |   8 +
>  drivers/leds/Makefile     |   1 +
>  drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 571 insertions(+)
>  create mode 100644 drivers/leds/leds-sunxi.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..559d2ca0a7f4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -297,6 +297,14 @@ config LEDS_SUNFIRE
>  	  This option enables support for the Left, Middle, and Right
>  	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
>  
> +config LEDS_SUNXI
> +	tristate "LED support for Allwinner sunxi LED controller"
> +	depends on LEDS_CLASS
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  This option enables support for the LED controller provided in
> +	  some Allwinner sunxi SoCs.
> +

Same comment for the name

>  config LEDS_IPAQ_MICRO
>  	tristate "LED Support for the Compaq iPAQ h3xxx"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index c636ec069612..3395f188e5bf 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
>  obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
>  obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
>  obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
> +obj-$(CONFIG_LEDS_SUNXI)		+= leds-sunxi.o
>  obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
>  obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
>  obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
> diff --git a/drivers/leds/leds-sunxi.c b/drivers/leds/leds-sunxi.c
> new file mode 100644
> index 000000000000..6569d64e0c76
> --- /dev/null
> +++ b/drivers/leds/leds-sunxi.c
> @@ -0,0 +1,562 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2021 Samuel Holland <samuel@sholland.org>
> +//
> +// Partly based on drivers/leds/leds-turris-omnia.c, which is:
> +//     Copyright (c) 2020 by Marek Behún <kabel@kernel.org>
> +//
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/spinlock.h>
> +
> +#define LEDC_CTRL_REG			0x0000
> +#define LEDC_CTRL_REG_DATA_LENGTH		(0x1fff << 16)
> +#define LEDC_CTRL_REG_RGB_MODE			(0x7 << 6)
> +#define LEDC_CTRL_REG_LEDC_EN			BIT(0)
> +#define LEDC_T01_TIMING_CTRL_REG	0x0004
> +#define LEDC_T01_TIMING_CTRL_REG_T1H		(0x3f << 21)
> +#define LEDC_T01_TIMING_CTRL_REG_T1L		(0x1f << 16)
> +#define LEDC_T01_TIMING_CTRL_REG_T0H		(0x1f << 6)
> +#define LEDC_T01_TIMING_CTRL_REG_T0L		(0x3f << 0)
> +#define LEDC_RESET_TIMING_CTRL_REG	0x000c
> +#define LEDC_RESET_TIMING_CTRL_REG_LED_NUM	(0x3ff << 0)
> +#define LEDC_DATA_REG			0x0014
> +#define LEDC_DMA_CTRL_REG		0x0018
> +#define LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL	(0x1f << 0)
> +#define LEDC_INT_CTRL_REG		0x001c
> +#define LEDC_INT_CTRL_REG_GLOBAL_INT_EN		BIT(5)
> +#define LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN	BIT(1)
> +#define LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN	BIT(0)
> +#define LEDC_INT_STS_REG		0x0020
> +#define LEDC_INT_STS_REG_FIFO_CPUREQ_INT	BIT(1)
> +#define LEDC_INT_STS_REG_TRANS_FINISH_INT	BIT(0)
> +
> +#define LEDC_FIFO_DEPTH			32
> +#define LEDC_MAX_LEDS			1024
> +
> +#define LEDS_TO_BYTES(n)		((n) * sizeof(u32))
> +
> +struct sunxi_ledc_led {
> +	struct led_classdev_mc mc_cdev;
> +	struct mc_subled subled_info[3];
> +};
> +#define to_ledc_led(mc) container_of(mc, struct sunxi_ledc_led, mc_cdev)
> +
> +struct sunxi_ledc_timing {
> +	u32 t0h_ns;
> +	u32 t0l_ns;
> +	u32 t1h_ns;
> +	u32 t1l_ns;
> +	u32 treset_ns;
> +};
> +
> +struct sunxi_ledc {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk *bus_clk;
> +	struct clk *mod_clk;
> +	struct reset_control *reset;
> +	struct regulator *vled;
> +
> +	u32 *buffer;
> +	struct dma_chan *dma_chan;
> +	dma_addr_t dma_handle;
> +	int pio_length;
> +	int pio_offset;
> +
> +	spinlock_t lock;
> +	int next_length;
> +	bool xfer_active;
> +
> +	u32 format;
> +	struct sunxi_ledc_timing timing;
> +
> +	int num_leds;
> +	struct sunxi_ledc_led leds[];
> +};
> +
> +static int sunxi_ledc_dma_xfer(struct sunxi_ledc *priv, int length)
> +{
> +	struct dma_async_tx_descriptor *desc;
> +	dma_cookie_t cookie;
> +
> +	desc = dmaengine_prep_slave_single(priv->dma_chan, priv->dma_handle,
> +					   LEDS_TO_BYTES(length),
> +					   DMA_MEM_TO_DEV, 0);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	cookie = dmaengine_submit(desc);
> +	if (dma_submit_error(cookie))
> +		return -EIO;
> +
> +	dma_async_issue_pending(priv->dma_chan);
> +
> +	return 0;
> +}
> +
> +static void sunxi_ledc_pio_xfer(struct sunxi_ledc *priv, int length)
> +{
> +	u32 burst, offset, val;
> +
> +	if (length) {
> +		/* New transfer (FIFO is empty). */
> +		offset = 0;
> +		burst  = min(length, LEDC_FIFO_DEPTH);
> +	} else {
> +		/* Existing transfer (FIFO is half-full). */
> +		length = priv->pio_length;
> +		offset = priv->pio_offset;
> +		burst  = min(length, LEDC_FIFO_DEPTH / 2);
> +	}
> +
> +	writesl(priv->base + LEDC_DATA_REG, priv->buffer + offset, burst);
> +
> +	if (burst < length) {
> +		priv->pio_length = length - burst;
> +		priv->pio_offset = offset + burst;
> +
> +		if (!offset) {
> +			val = readl(priv->base + LEDC_INT_CTRL_REG);
> +			val |= LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
> +			writel(val, priv->base + LEDC_INT_CTRL_REG);
> +		}
> +	} else {
> +		/* Disable the request IRQ once all data is written. */
> +		val = readl(priv->base + LEDC_INT_CTRL_REG);
> +		val &= ~LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
> +		writel(val, priv->base + LEDC_INT_CTRL_REG);
> +	}
> +}
> +
> +static void sunxi_ledc_start_xfer(struct sunxi_ledc *priv, int length)
> +{
> +	u32 val;
> +
> +	dev_dbg(priv->dev, "Updating %d LEDs\n", length);
> +
> +	val = readl(priv->base + LEDC_CTRL_REG);
> +	val &= ~LEDC_CTRL_REG_DATA_LENGTH;
> +	val |= length << 16 | LEDC_CTRL_REG_LEDC_EN;
> +	writel(val, priv->base + LEDC_CTRL_REG);
> +
> +	if (length > LEDC_FIFO_DEPTH) {
> +		int ret = sunxi_ledc_dma_xfer(priv, length);
> +
> +		if (!ret)
> +			return;
> +
> +		dev_warn(priv->dev, "Failed to set up DMA: %d\n", ret);
> +	}
> +
> +	sunxi_ledc_pio_xfer(priv, length);
> +}
> +
> +static irqreturn_t sunxi_ledc_irq(int irq, void *dev_id)
> +{
> +	struct sunxi_ledc *priv = dev_id;
> +	u32 val;
> +
> +	val = readl(priv->base + LEDC_INT_STS_REG);
> +
> +	if (val & LEDC_INT_STS_REG_TRANS_FINISH_INT) {
> +		int next_length;
> +
> +		/* Start the next transfer if needed. */
> +		spin_lock(&priv->lock);
> +		next_length = priv->next_length;
> +		if (next_length)
> +			priv->next_length = 0;
> +		else
> +			priv->xfer_active = false;
> +		spin_unlock(&priv->lock);
> +
> +		if (next_length)
> +			sunxi_ledc_start_xfer(priv, next_length);
> +	} else if (val & LEDC_INT_STS_REG_FIFO_CPUREQ_INT) {
> +		/* Continue the current transfer. */
> +		sunxi_ledc_pio_xfer(priv, 0);
> +	}
> +
> +	writel(val, priv->base + LEDC_INT_STS_REG);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void sunxi_ledc_brightness_set(struct led_classdev *cdev,
> +				       enum led_brightness brightness)
> +{
> +	struct sunxi_ledc *priv = dev_get_drvdata(cdev->dev->parent);
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct sunxi_ledc_led *led = to_ledc_led(mc_cdev);
> +	int addr = led - priv->leds;
> +	unsigned long flags;
> +	bool xfer_active;
> +	int next_length;
> +
> +	led_mc_calc_color_components(mc_cdev, brightness);
> +
> +	priv->buffer[addr] = led->subled_info[0].brightness << 16 |
> +			     led->subled_info[1].brightness <<  8 |
> +			     led->subled_info[2].brightness;
> +
> +	dev_dbg(priv->dev, "LED %d -> #%06x\n", addr, priv->buffer[addr]);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	next_length = max(priv->next_length, addr + 1);
> +	xfer_active = priv->xfer_active;
> +	if (xfer_active)
> +		priv->next_length = next_length;
> +	else
> +		priv->xfer_active = true;
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	if (!xfer_active)
> +		sunxi_ledc_start_xfer(priv, next_length);
> +}
> +
> +static const char *const sunxi_ledc_formats[] = {
> +	"rgb",
> +	"rbg",
> +	"grb",
> +	"gbr",
> +	"brg",
> +	"bgr",
> +};
> +
> +static int sunxi_ledc_parse_format(const struct device_node *np,
> +				    struct sunxi_ledc *priv)
> +{
> +	const char *format = "grb";
> +	u32 i;
> +
> +	of_property_read_string(np, "format", &format);
> +
> +	for (i = 0; i < ARRAY_SIZE(sunxi_ledc_formats); ++i) {
> +		if (!strcmp(format, sunxi_ledc_formats[i])) {
> +			priv->format = i;
> +			return 0;
> +		}
> +	}
> +
> +	dev_err(priv->dev, "Bad pixel format '%s'\n", format);
> +
> +	return -EINVAL;
> +}
> +
> +static void sunxi_ledc_set_format(struct sunxi_ledc *priv)
> +{
> +	u32 val;
> +
> +	val = readl(priv->base + LEDC_CTRL_REG);
> +	val &= ~LEDC_CTRL_REG_RGB_MODE;
> +	val |= priv->format << 6;
> +	writel(val, priv->base + LEDC_CTRL_REG);
> +}
> +
> +static const struct sunxi_ledc_timing sunxi_ledc_default_timing = {
> +	.t0h_ns = 336,
> +	.t0l_ns = 840,
> +	.t1h_ns = 882,
> +	.t1l_ns = 294,
> +	.treset_ns = 300000,
> +};
> +
> +static int sunxi_ledc_parse_timing(const struct device_node *np,
> +				    struct sunxi_ledc *priv)
> +{
> +	struct sunxi_ledc_timing *timing = &priv->timing;
> +
> +	*timing = sunxi_ledc_default_timing;
> +
> +	of_property_read_u32(np, "t0h-ns", &timing->t0h_ns);
> +	of_property_read_u32(np, "t0l-ns", &timing->t0l_ns);
> +	of_property_read_u32(np, "t1h-ns", &timing->t1h_ns);
> +	of_property_read_u32(np, "t1l-ns", &timing->t1l_ns);
> +	of_property_read_u32(np, "treset-ns", &timing->treset_ns);
> +
> +	return 0;
> +}
> +
> +static void sunxi_ledc_set_timing(struct sunxi_ledc *priv)
> +{
> +	const struct sunxi_ledc_timing *timing = &priv->timing;
> +	unsigned long mod_freq = clk_get_rate(priv->mod_clk);
> +	u32 cycle_ns = NSEC_PER_SEC / mod_freq;
> +	u32 val;
> +
> +	val = (timing->t1h_ns / cycle_ns) << 21 |
> +	      (timing->t1l_ns / cycle_ns) << 16 |
> +	      (timing->t0h_ns / cycle_ns) <<  6 |
> +	      (timing->t0l_ns / cycle_ns);
> +	writel(val, priv->base + LEDC_T01_TIMING_CTRL_REG);
> +
> +	val = (timing->treset_ns / cycle_ns) << 16 |
> +	      (priv->num_leds - 1);
> +	writel(val, priv->base + LEDC_RESET_TIMING_CTRL_REG);
> +}
> +
> +static int sunxi_ledc_resume(struct device *dev)
> +{
> +	struct sunxi_ledc *priv = dev_get_drvdata(dev);
> +	u32 val;
> +	int ret;
> +
> +	ret = regulator_enable(priv->vled);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(priv->reset);
> +	if (ret)
> +		goto err_disable_regulator;
> +
> +	ret = clk_prepare_enable(priv->bus_clk);
> +	if (ret)
> +		goto err_assert_reset;
> +
> +	ret = clk_prepare_enable(priv->mod_clk);
> +	if (ret)
> +		goto err_disable_bus_clk;
> +
> +	sunxi_ledc_set_format(priv);
> +	sunxi_ledc_set_timing(priv);
> +
> +	/* The trigger level must be at least the burst length. */
> +	val = readl(priv->base + LEDC_DMA_CTRL_REG);
> +	val &= ~LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL;
> +	val |= LEDC_FIFO_DEPTH / 2;
> +	writel(val, priv->base + LEDC_DMA_CTRL_REG);
> +
> +	val = LEDC_INT_CTRL_REG_GLOBAL_INT_EN |
> +	      LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN;
> +	writel(val, priv->base + LEDC_INT_CTRL_REG);
> +
> +	return 0;
> +
> +err_disable_bus_clk:
> +	clk_disable_unprepare(priv->bus_clk);
> +err_assert_reset:
> +	reset_control_assert(priv->reset);
> +err_disable_regulator:
> +	regulator_disable(priv->vled);
> +
> +	return ret;
> +}
> +
> +static int sunxi_ledc_suspend(struct device *dev)
> +{
> +	struct sunxi_ledc *priv = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(priv->mod_clk);
> +	clk_disable_unprepare(priv->bus_clk);
> +	reset_control_assert(priv->reset);
> +	regulator_disable(priv->vled);
> +
> +	return 0;
> +}
> +
> +static void sunxi_ledc_dma_cleanup(void *data)
> +{
> +	struct sunxi_ledc *priv = data;
> +	struct device *dma_dev = dmaengine_get_dma_device(priv->dma_chan);
> +
> +	if (priv->buffer)
> +		dma_free_wc(dma_dev, LEDS_TO_BYTES(priv->num_leds),
> +			    priv->buffer, priv->dma_handle);
> +	dma_release_channel(priv->dma_chan);
> +}
> +
> +static int sunxi_ledc_probe(struct platform_device *pdev)
> +{
> +	const struct device_node *np = pdev->dev.of_node;
> +	struct dma_slave_config dma_cfg = {};
> +	struct led_init_data init_data = {};
> +	struct device *dev = &pdev->dev;
> +	struct device_node *child;
> +	struct sunxi_ledc *priv;
> +	struct resource *mem;
> +	int count, irq, ret;
> +
> +	count = of_get_available_child_count(np);
> +	if (!count)
> +		return -ENODEV;
> +	if (count > LEDC_MAX_LEDS) {
> +		dev_err(dev, "Too many LEDs! (max is %d)\n", LEDC_MAX_LEDS);
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->num_leds = count;
> +	spin_lock_init(&priv->lock);
> +	dev_set_drvdata(dev, priv);
> +
> +	ret = sunxi_ledc_parse_format(np, priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = sunxi_ledc_parse_timing(np, priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->bus_clk = devm_clk_get(dev, "bus");
> +	if (IS_ERR(priv->bus_clk))
> +		return PTR_ERR(priv->bus_clk);
> +
> +	priv->mod_clk = devm_clk_get(dev, "mod");
> +	if (IS_ERR(priv->mod_clk))
> +		return PTR_ERR(priv->mod_clk);
> +
> +	priv->reset = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return PTR_ERR(priv->reset);
> +
> +	priv->vled = devm_regulator_get(dev, "vled");
> +	if (IS_ERR(priv->vled))
> +		return PTR_ERR(priv->vled);
> +
> +	priv->dma_chan = dma_request_chan(dev, "tx");
> +	if (IS_ERR(priv->dma_chan))
> +		return PTR_ERR(priv->dma_chan);
> +
> +	ret = devm_add_action_or_reset(dev, sunxi_ledc_dma_cleanup, priv);
> +	if (ret)
> +		return ret;
> +
> +	dma_cfg.dst_addr	= mem->start + LEDC_DATA_REG;
> +	dma_cfg.dst_addr_width	= DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	dma_cfg.dst_maxburst	= LEDC_FIFO_DEPTH / 2;
> +	ret = dmaengine_slave_config(priv->dma_chan, &dma_cfg);
> +	if (ret)
> +		return ret;
> +
> +	priv->buffer = dma_alloc_wc(dmaengine_get_dma_device(priv->dma_chan),
> +				    LEDS_TO_BYTES(priv->num_leds),
> +				    &priv->dma_handle, GFP_KERNEL);
> +	if (!priv->buffer)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, sunxi_ledc_irq, 0, dev_name(dev), priv);
> +	if (ret)
> +		return ret;

Starting here, you can get interrupts. I couldn't spot anything wrong in
the handler, but would that cause any trouble if an interrupt was coming
right here?

> +	ret = sunxi_ledc_resume(dev);
> +	if (ret)
> +		return ret;

It's not clear to me what is done here. I assume from the rest of the
driver that you want to use runtime_pm, but it's never enabled for that
driver and the active count will be off here

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: leds: Add Allwinner R329/D1 LED controller

[Rob Herring]

On Thu, 02 Sep 2021 18:42:27 -0500, Samuel Holland wrote:
> The Allwinner R329 and D1 SoCs contain an LED controller designed to
> drive a series of RGB LED pixels. It supports PIO and DMA transfers, and
> has configurable timing and pixel format.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../leds/allwinner,sun50i-r329-ledc.yaml      | 141 ++++++++++++++++++
>  1 file changed, 141 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml:83:3: [error] duplication of key "t1h-ns" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 122, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 132, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 722, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 446, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 264, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 295, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 17, column 3
found duplicate key "t1h-ns" with value "{}" (original value: "{}")
  in "<unicode string>", line 83, column 3

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.example.dts] Error 1
Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 25, in check_doc
    testtree = dtschema.load(filename, line_number=line_number)
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 623, in load
    return yaml.load(f.read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 122, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 132, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 722, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 446, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 264, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 295, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 17, column 3
found duplicate key "t1h-ns" with value "{}" (original value: "{}")
  in "<unicode string>", line 83, column 3

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 67, in <module>
    ret = check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 30, in check_doc
    print(filename + ":", exc.path[-1], exc.message, file=sys.stderr)
AttributeError: 'DuplicateKeyError' object has no attribute 'path'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1523964

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Samuel Holland]

Hi,

On 9/3/21 5:36 AM, Maxime Ripard wrote:
> On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:
>> Some Allwinner sunxi SoCs, starting with the R329, contain an LED
>> controller designed to drive RGB LED pixels. Add a driver for it using
>> the multicolor LED framework, and with LEDs defined in the device tree.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/leds/Kconfig      |   8 +
>>  drivers/leds/Makefile     |   1 +
>>  drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 571 insertions(+)
>>  create mode 100644 drivers/leds/leds-sunxi.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ed800f5da7d8..559d2ca0a7f4 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -297,6 +297,14 @@ config LEDS_SUNFIRE
>>  	  This option enables support for the Left, Middle, and Right
>>  	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
>>  
>> +config LEDS_SUNXI
>> +	tristate "LED support for Allwinner sunxi LED controller"
>> +	depends on LEDS_CLASS
>> +	depends on ARCH_SUNXI || COMPILE_TEST
>> +	help
>> +	  This option enables support for the LED controller provided in
>> +	  some Allwinner sunxi SoCs.
>> +
> 
> Same comment for the name

Are you concerned about the help text only, or do you also want me to rename the
Kconfig symbol?

I am happy to change the help text to something like: "This option enables
support for the LED controller provided in the Allwinner R329 and D1 SoCs."

But I don't know of any satisfying way to rename the Kconfig symbol. There is no
general category name for "R329 and D1."

>>  config LEDS_IPAQ_MICRO
>>  	tristate "LED Support for the Compaq iPAQ h3xxx"
>>  	depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index c636ec069612..3395f188e5bf 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
>>  obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
>>  obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
>>  obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
>> +obj-$(CONFIG_LEDS_SUNXI)		+= leds-sunxi.o
>>  obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
>>  obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
>>  obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
>> diff --git a/drivers/leds/leds-sunxi.c b/drivers/leds/leds-sunxi.c
>> new file mode 100644
>> index 000000000000..6569d64e0c76
>> --- /dev/null
>> +++ b/drivers/leds/leds-sunxi.c
>> @@ -0,0 +1,562 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (c) 2021 Samuel Holland <samuel@sholland.org>
>> +//
>> +// Partly based on drivers/leds/leds-turris-omnia.c, which is:
>> +//     Copyright (c) 2020 by Marek Behún <kabel@kernel.org>
>> +//
>> +
>> +#include <linux/clk.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define LEDC_CTRL_REG			0x0000
>> +#define LEDC_CTRL_REG_DATA_LENGTH		(0x1fff << 16)
>> +#define LEDC_CTRL_REG_RGB_MODE			(0x7 << 6)
>> +#define LEDC_CTRL_REG_LEDC_EN			BIT(0)
>> +#define LEDC_T01_TIMING_CTRL_REG	0x0004
>> +#define LEDC_T01_TIMING_CTRL_REG_T1H		(0x3f << 21)
>> +#define LEDC_T01_TIMING_CTRL_REG_T1L		(0x1f << 16)
>> +#define LEDC_T01_TIMING_CTRL_REG_T0H		(0x1f << 6)
>> +#define LEDC_T01_TIMING_CTRL_REG_T0L		(0x3f << 0)
>> +#define LEDC_RESET_TIMING_CTRL_REG	0x000c
>> +#define LEDC_RESET_TIMING_CTRL_REG_LED_NUM	(0x3ff << 0)
>> +#define LEDC_DATA_REG			0x0014
>> +#define LEDC_DMA_CTRL_REG		0x0018
>> +#define LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL	(0x1f << 0)
>> +#define LEDC_INT_CTRL_REG		0x001c
>> +#define LEDC_INT_CTRL_REG_GLOBAL_INT_EN		BIT(5)
>> +#define LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN	BIT(1)
>> +#define LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN	BIT(0)
>> +#define LEDC_INT_STS_REG		0x0020
>> +#define LEDC_INT_STS_REG_FIFO_CPUREQ_INT	BIT(1)
>> +#define LEDC_INT_STS_REG_TRANS_FINISH_INT	BIT(0)
>> +
>> +#define LEDC_FIFO_DEPTH			32
>> +#define LEDC_MAX_LEDS			1024
>> +
>> +#define LEDS_TO_BYTES(n)		((n) * sizeof(u32))
>> +
>> +struct sunxi_ledc_led {
>> +	struct led_classdev_mc mc_cdev;
>> +	struct mc_subled subled_info[3];
>> +};
>> +#define to_ledc_led(mc) container_of(mc, struct sunxi_ledc_led, mc_cdev)
>> +
>> +struct sunxi_ledc_timing {
>> +	u32 t0h_ns;
>> +	u32 t0l_ns;
>> +	u32 t1h_ns;
>> +	u32 t1l_ns;
>> +	u32 treset_ns;
>> +};
>> +
>> +struct sunxi_ledc {
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	struct clk *bus_clk;
>> +	struct clk *mod_clk;
>> +	struct reset_control *reset;
>> +	struct regulator *vled;
>> +
>> +	u32 *buffer;
>> +	struct dma_chan *dma_chan;
>> +	dma_addr_t dma_handle;
>> +	int pio_length;
>> +	int pio_offset;
>> +
>> +	spinlock_t lock;
>> +	int next_length;
>> +	bool xfer_active;
>> +
>> +	u32 format;
>> +	struct sunxi_ledc_timing timing;
>> +
>> +	int num_leds;
>> +	struct sunxi_ledc_led leds[];
>> +};
>> +
>> +static int sunxi_ledc_dma_xfer(struct sunxi_ledc *priv, int length)
>> +{
>> +	struct dma_async_tx_descriptor *desc;
>> +	dma_cookie_t cookie;
>> +
>> +	desc = dmaengine_prep_slave_single(priv->dma_chan, priv->dma_handle,
>> +					   LEDS_TO_BYTES(length),
>> +					   DMA_MEM_TO_DEV, 0);
>> +	if (!desc)
>> +		return -ENOMEM;
>> +
>> +	cookie = dmaengine_submit(desc);
>> +	if (dma_submit_error(cookie))
>> +		return -EIO;
>> +
>> +	dma_async_issue_pending(priv->dma_chan);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sunxi_ledc_pio_xfer(struct sunxi_ledc *priv, int length)
>> +{
>> +	u32 burst, offset, val;
>> +
>> +	if (length) {
>> +		/* New transfer (FIFO is empty). */
>> +		offset = 0;
>> +		burst  = min(length, LEDC_FIFO_DEPTH);
>> +	} else {
>> +		/* Existing transfer (FIFO is half-full). */
>> +		length = priv->pio_length;
>> +		offset = priv->pio_offset;
>> +		burst  = min(length, LEDC_FIFO_DEPTH / 2);
>> +	}
>> +
>> +	writesl(priv->base + LEDC_DATA_REG, priv->buffer + offset, burst);
>> +
>> +	if (burst < length) {
>> +		priv->pio_length = length - burst;
>> +		priv->pio_offset = offset + burst;
>> +
>> +		if (!offset) {
>> +			val = readl(priv->base + LEDC_INT_CTRL_REG);
>> +			val |= LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
>> +			writel(val, priv->base + LEDC_INT_CTRL_REG);
>> +		}
>> +	} else {
>> +		/* Disable the request IRQ once all data is written. */
>> +		val = readl(priv->base + LEDC_INT_CTRL_REG);
>> +		val &= ~LEDC_INT_CTRL_REG_FIFO_CPUREQ_INT_EN;
>> +		writel(val, priv->base + LEDC_INT_CTRL_REG);
>> +	}
>> +}
>> +
>> +static void sunxi_ledc_start_xfer(struct sunxi_ledc *priv, int length)
>> +{
>> +	u32 val;
>> +
>> +	dev_dbg(priv->dev, "Updating %d LEDs\n", length);
>> +
>> +	val = readl(priv->base + LEDC_CTRL_REG);
>> +	val &= ~LEDC_CTRL_REG_DATA_LENGTH;
>> +	val |= length << 16 | LEDC_CTRL_REG_LEDC_EN;
>> +	writel(val, priv->base + LEDC_CTRL_REG);
>> +
>> +	if (length > LEDC_FIFO_DEPTH) {
>> +		int ret = sunxi_ledc_dma_xfer(priv, length);
>> +
>> +		if (!ret)
>> +			return;
>> +
>> +		dev_warn(priv->dev, "Failed to set up DMA: %d\n", ret);
>> +	}
>> +
>> +	sunxi_ledc_pio_xfer(priv, length);
>> +}
>> +
>> +static irqreturn_t sunxi_ledc_irq(int irq, void *dev_id)
>> +{
>> +	struct sunxi_ledc *priv = dev_id;
>> +	u32 val;
>> +
>> +	val = readl(priv->base + LEDC_INT_STS_REG);
>> +
>> +	if (val & LEDC_INT_STS_REG_TRANS_FINISH_INT) {
>> +		int next_length;
>> +
>> +		/* Start the next transfer if needed. */
>> +		spin_lock(&priv->lock);
>> +		next_length = priv->next_length;
>> +		if (next_length)
>> +			priv->next_length = 0;
>> +		else
>> +			priv->xfer_active = false;
>> +		spin_unlock(&priv->lock);
>> +
>> +		if (next_length)
>> +			sunxi_ledc_start_xfer(priv, next_length);
>> +	} else if (val & LEDC_INT_STS_REG_FIFO_CPUREQ_INT) {
>> +		/* Continue the current transfer. */
>> +		sunxi_ledc_pio_xfer(priv, 0);
>> +	}
>> +
>> +	writel(val, priv->base + LEDC_INT_STS_REG);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void sunxi_ledc_brightness_set(struct led_classdev *cdev,
>> +				       enum led_brightness brightness)
>> +{
>> +	struct sunxi_ledc *priv = dev_get_drvdata(cdev->dev->parent);
>> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>> +	struct sunxi_ledc_led *led = to_ledc_led(mc_cdev);
>> +	int addr = led - priv->leds;
>> +	unsigned long flags;
>> +	bool xfer_active;
>> +	int next_length;
>> +
>> +	led_mc_calc_color_components(mc_cdev, brightness);
>> +
>> +	priv->buffer[addr] = led->subled_info[0].brightness << 16 |
>> +			     led->subled_info[1].brightness <<  8 |
>> +			     led->subled_info[2].brightness;
>> +
>> +	dev_dbg(priv->dev, "LED %d -> #%06x\n", addr, priv->buffer[addr]);
>> +
>> +	spin_lock_irqsave(&priv->lock, flags);
>> +	next_length = max(priv->next_length, addr + 1);
>> +	xfer_active = priv->xfer_active;
>> +	if (xfer_active)
>> +		priv->next_length = next_length;
>> +	else
>> +		priv->xfer_active = true;
>> +	spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +	if (!xfer_active)
>> +		sunxi_ledc_start_xfer(priv, next_length);
>> +}
>> +
>> +static const char *const sunxi_ledc_formats[] = {
>> +	"rgb",
>> +	"rbg",
>> +	"grb",
>> +	"gbr",
>> +	"brg",
>> +	"bgr",
>> +};
>> +
>> +static int sunxi_ledc_parse_format(const struct device_node *np,
>> +				    struct sunxi_ledc *priv)
>> +{
>> +	const char *format = "grb";
>> +	u32 i;
>> +
>> +	of_property_read_string(np, "format", &format);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sunxi_ledc_formats); ++i) {
>> +		if (!strcmp(format, sunxi_ledc_formats[i])) {
>> +			priv->format = i;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	dev_err(priv->dev, "Bad pixel format '%s'\n", format);
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static void sunxi_ledc_set_format(struct sunxi_ledc *priv)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(priv->base + LEDC_CTRL_REG);
>> +	val &= ~LEDC_CTRL_REG_RGB_MODE;
>> +	val |= priv->format << 6;
>> +	writel(val, priv->base + LEDC_CTRL_REG);
>> +}
>> +
>> +static const struct sunxi_ledc_timing sunxi_ledc_default_timing = {
>> +	.t0h_ns = 336,
>> +	.t0l_ns = 840,
>> +	.t1h_ns = 882,
>> +	.t1l_ns = 294,
>> +	.treset_ns = 300000,
>> +};
>> +
>> +static int sunxi_ledc_parse_timing(const struct device_node *np,
>> +				    struct sunxi_ledc *priv)
>> +{
>> +	struct sunxi_ledc_timing *timing = &priv->timing;
>> +
>> +	*timing = sunxi_ledc_default_timing;
>> +
>> +	of_property_read_u32(np, "t0h-ns", &timing->t0h_ns);
>> +	of_property_read_u32(np, "t0l-ns", &timing->t0l_ns);
>> +	of_property_read_u32(np, "t1h-ns", &timing->t1h_ns);
>> +	of_property_read_u32(np, "t1l-ns", &timing->t1l_ns);
>> +	of_property_read_u32(np, "treset-ns", &timing->treset_ns);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sunxi_ledc_set_timing(struct sunxi_ledc *priv)
>> +{
>> +	const struct sunxi_ledc_timing *timing = &priv->timing;
>> +	unsigned long mod_freq = clk_get_rate(priv->mod_clk);
>> +	u32 cycle_ns = NSEC_PER_SEC / mod_freq;
>> +	u32 val;
>> +
>> +	val = (timing->t1h_ns / cycle_ns) << 21 |
>> +	      (timing->t1l_ns / cycle_ns) << 16 |
>> +	      (timing->t0h_ns / cycle_ns) <<  6 |
>> +	      (timing->t0l_ns / cycle_ns);
>> +	writel(val, priv->base + LEDC_T01_TIMING_CTRL_REG);
>> +
>> +	val = (timing->treset_ns / cycle_ns) << 16 |
>> +	      (priv->num_leds - 1);
>> +	writel(val, priv->base + LEDC_RESET_TIMING_CTRL_REG);
>> +}
>> +
>> +static int sunxi_ledc_resume(struct device *dev)
>> +{
>> +	struct sunxi_ledc *priv = dev_get_drvdata(dev);
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regulator_enable(priv->vled);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(priv->reset);
>> +	if (ret)
>> +		goto err_disable_regulator;
>> +
>> +	ret = clk_prepare_enable(priv->bus_clk);
>> +	if (ret)
>> +		goto err_assert_reset;
>> +
>> +	ret = clk_prepare_enable(priv->mod_clk);
>> +	if (ret)
>> +		goto err_disable_bus_clk;
>> +
>> +	sunxi_ledc_set_format(priv);
>> +	sunxi_ledc_set_timing(priv);
>> +
>> +	/* The trigger level must be at least the burst length. */
>> +	val = readl(priv->base + LEDC_DMA_CTRL_REG);
>> +	val &= ~LEDC_DMA_CTRL_REG_FIFO_TRIG_LEVEL;
>> +	val |= LEDC_FIFO_DEPTH / 2;
>> +	writel(val, priv->base + LEDC_DMA_CTRL_REG);
>> +
>> +	val = LEDC_INT_CTRL_REG_GLOBAL_INT_EN |
>> +	      LEDC_INT_CTRL_REG_TRANS_FINISH_INT_EN;
>> +	writel(val, priv->base + LEDC_INT_CTRL_REG);
>> +
>> +	return 0;
>> +
>> +err_disable_bus_clk:
>> +	clk_disable_unprepare(priv->bus_clk);
>> +err_assert_reset:
>> +	reset_control_assert(priv->reset);
>> +err_disable_regulator:
>> +	regulator_disable(priv->vled);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_ledc_suspend(struct device *dev)
>> +{
>> +	struct sunxi_ledc *priv = dev_get_drvdata(dev);
>> +
>> +	clk_disable_unprepare(priv->mod_clk);
>> +	clk_disable_unprepare(priv->bus_clk);
>> +	reset_control_assert(priv->reset);
>> +	regulator_disable(priv->vled);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sunxi_ledc_dma_cleanup(void *data)
>> +{
>> +	struct sunxi_ledc *priv = data;
>> +	struct device *dma_dev = dmaengine_get_dma_device(priv->dma_chan);
>> +
>> +	if (priv->buffer)
>> +		dma_free_wc(dma_dev, LEDS_TO_BYTES(priv->num_leds),
>> +			    priv->buffer, priv->dma_handle);
>> +	dma_release_channel(priv->dma_chan);
>> +}
>> +
>> +static int sunxi_ledc_probe(struct platform_device *pdev)
>> +{
>> +	const struct device_node *np = pdev->dev.of_node;
>> +	struct dma_slave_config dma_cfg = {};
>> +	struct led_init_data init_data = {};
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *child;
>> +	struct sunxi_ledc *priv;
>> +	struct resource *mem;
>> +	int count, irq, ret;
>> +
>> +	count = of_get_available_child_count(np);
>> +	if (!count)
>> +		return -ENODEV;
>> +	if (count > LEDC_MAX_LEDS) {
>> +		dev_err(dev, "Too many LEDs! (max is %d)\n", LEDC_MAX_LEDS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	priv = devm_kzalloc(dev, struct_size(priv, leds, count), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = dev;
>> +	priv->num_leds = count;
>> +	spin_lock_init(&priv->lock);
>> +	dev_set_drvdata(dev, priv);
>> +
>> +	ret = sunxi_ledc_parse_format(np, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sunxi_ledc_parse_timing(np, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	priv->bus_clk = devm_clk_get(dev, "bus");
>> +	if (IS_ERR(priv->bus_clk))
>> +		return PTR_ERR(priv->bus_clk);
>> +
>> +	priv->mod_clk = devm_clk_get(dev, "mod");
>> +	if (IS_ERR(priv->mod_clk))
>> +		return PTR_ERR(priv->mod_clk);
>> +
>> +	priv->reset = devm_reset_control_get_exclusive(dev, NULL);
>> +	if (IS_ERR(priv->reset))
>> +		return PTR_ERR(priv->reset);
>> +
>> +	priv->vled = devm_regulator_get(dev, "vled");
>> +	if (IS_ERR(priv->vled))
>> +		return PTR_ERR(priv->vled);
>> +
>> +	priv->dma_chan = dma_request_chan(dev, "tx");
>> +	if (IS_ERR(priv->dma_chan))
>> +		return PTR_ERR(priv->dma_chan);
>> +
>> +	ret = devm_add_action_or_reset(dev, sunxi_ledc_dma_cleanup, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dma_cfg.dst_addr	= mem->start + LEDC_DATA_REG;
>> +	dma_cfg.dst_addr_width	= DMA_SLAVE_BUSWIDTH_4_BYTES;
>> +	dma_cfg.dst_maxburst	= LEDC_FIFO_DEPTH / 2;
>> +	ret = dmaengine_slave_config(priv->dma_chan, &dma_cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->buffer = dma_alloc_wc(dmaengine_get_dma_device(priv->dma_chan),
>> +				    LEDS_TO_BYTES(priv->num_leds),
>> +				    &priv->dma_handle, GFP_KERNEL);
>> +	if (!priv->buffer)
>> +		return -ENOMEM;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_irq(dev, irq, sunxi_ledc_irq, 0, dev_name(dev), priv);
>> +	if (ret)
>> +		return ret;
> 
> Starting here, you can get interrupts. I couldn't spot anything wrong in
> the handler, but would that cause any trouble if an interrupt was coming
> right here?

It should be fine. The data request IRQ will be disabled if it is received
because priv->pio_length == 0, and the other IRQs are oneshots and harmless.

>> +	ret = sunxi_ledc_resume(dev);
>> +	if (ret)
>> +		return ret;
> 
> It's not clear to me what is done here. I assume from the rest of the
> driver that you want to use runtime_pm, but it's never enabled for that
> driver and the active count will be off here

There is no runtime PM here, only system PM. So I have just reused the hardware
init procedure from system resume. If I was to add runtime PM, then yes, this
call would not belong here.

Regards,
Samuel

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Maxime Ripard]

Hi,

On Sun, Sep 05, 2021 at 11:17:19PM -0500, Samuel Holland wrote:
> Hi,
> 
> On 9/3/21 5:36 AM, Maxime Ripard wrote:
> > On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:
> >> Some Allwinner sunxi SoCs, starting with the R329, contain an LED
> >> controller designed to drive RGB LED pixels. Add a driver for it using
> >> the multicolor LED framework, and with LEDs defined in the device tree.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  drivers/leds/Kconfig      |   8 +
> >>  drivers/leds/Makefile     |   1 +
> >>  drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 571 insertions(+)
> >>  create mode 100644 drivers/leds/leds-sunxi.c
> >>
> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >> index ed800f5da7d8..559d2ca0a7f4 100644
> >> --- a/drivers/leds/Kconfig
> >> +++ b/drivers/leds/Kconfig
> >> @@ -297,6 +297,14 @@ config LEDS_SUNFIRE
> >>  	  This option enables support for the Left, Middle, and Right
> >>  	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
> >>  
> >> +config LEDS_SUNXI
> >> +	tristate "LED support for Allwinner sunxi LED controller"
> >> +	depends on LEDS_CLASS
> >> +	depends on ARCH_SUNXI || COMPILE_TEST
> >> +	help
> >> +	  This option enables support for the LED controller provided in
> >> +	  some Allwinner sunxi SoCs.
> >> +
> > 
> > Same comment for the name
> 
> Are you concerned about the help text only, or do you also want me to rename the
> Kconfig symbol?

The driver, the driver symbols and the Kconfig symbol would be nice

> I am happy to change the help text to something like: "This option enables
> support for the LED controller provided in the Allwinner R329 and D1 SoCs."
> 
> But I don't know of any satisfying way to rename the Kconfig symbol. There is no
> general category name for "R329 and D1."

Yeah, this is not ideal, but the issue is that nothing is telling us
whether or not it will support *only* the R329 and D1. Chances are it's
going to be featured in a number of other SoCs in the future, so if we
were to have the entire list of supported SoCs in the Kconfig symbol and
driver name, we'd have to always change them everytime a new SoC support
is introduced.

It would be a pain, and it's pretty much guaranteed that someone is
going to forget at some point. To mitigate this, we took the approach to
use the same semantic than the DT compatible: the driver name doesn't
really define the list of all the SoCs supported but matches every SoC
(more or less) compatible with that SoC.

If you want to have the entire list in the Kconfig help though, I don't
see anything wrong with that. Even if it goes unmaintained, it wouldn't
really be a big deal.

Maxime

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Samuel Holland]

On 9/9/21 6:36 AM, Maxime Ripard wrote:
> Hi,
> 
> On Sun, Sep 05, 2021 at 11:17:19PM -0500, Samuel Holland wrote:
>> Hi,
>>
>> On 9/3/21 5:36 AM, Maxime Ripard wrote:
>>> On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:
>>>> Some Allwinner sunxi SoCs, starting with the R329, contain an LED
>>>> controller designed to drive RGB LED pixels. Add a driver for it using
>>>> the multicolor LED framework, and with LEDs defined in the device tree.
>>>>
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>  drivers/leds/Kconfig      |   8 +
>>>>  drivers/leds/Makefile     |   1 +
>>>>  drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 571 insertions(+)
>>>>  create mode 100644 drivers/leds/leds-sunxi.c
>>>>
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index ed800f5da7d8..559d2ca0a7f4 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -297,6 +297,14 @@ config LEDS_SUNFIRE
>>>>  	  This option enables support for the Left, Middle, and Right
>>>>  	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
>>>>  
>>>> +config LEDS_SUNXI
>>>> +	tristate "LED support for Allwinner sunxi LED controller"
>>>> +	depends on LEDS_CLASS
>>>> +	depends on ARCH_SUNXI || COMPILE_TEST
>>>> +	help
>>>> +	  This option enables support for the LED controller provided in
>>>> +	  some Allwinner sunxi SoCs.
>>>> +
>>>
>>> Same comment for the name
>>
>> Are you concerned about the help text only, or do you also want me to rename the
>> Kconfig symbol?
> 
> The driver, the driver symbols and the Kconfig symbol would be nice
> 
>> I am happy to change the help text to something like: "This option enables
>> support for the LED controller provided in the Allwinner R329 and D1 SoCs."
>>
>> But I don't know of any satisfying way to rename the Kconfig symbol. There is no
>> general category name for "R329 and D1."
> 
> Yeah, this is not ideal, but the issue is that nothing is telling us
> whether or not it will support *only* the R329 and D1. Chances are it's
> going to be featured in a number of other SoCs in the future, so if we
> were to have the entire list of supported SoCs in the Kconfig symbol and
> driver name, we'd have to always change them everytime a new SoC support
> is introduced.

This is why I named it LEDS_SUNXI: until and unless a "v2" hardware
block shows up, this is the LED controller in all sunxi SoCs [that have
an LED controller at all]. And at that point, naming the new driver
LEDS_SUNXI_V2 makes more sense to me than trying to guess the pattern
for SoC support, where there likely is no pattern.

> It would be a pain, and it's pretty much guaranteed that someone is
> going to forget at some point. To mitigate this, we took the approach to
> use the same semantic than the DT compatible: the driver name doesn't
> really define the list of all the SoCs supported but matches every SoC
> (more or less) compatible with that SoC.

Ok, but this still doesn't tell me what you expect the driver to be
named. Again, there is no general name for "every SoC (more or less)
compatible with R329".

We tried to guess the pattern around the time H6 came out, and named a
bunch of things "sun50i" (like the IOMMU driver) that are now showing up
on sun8i (A50) and sun20i (D1) SoCs. And it turns out most of the
changes were not even new for H6 (sun50iw6), but already present in the
A63 (sun50iw3).

I'm in sort of the same situation here. I know the hardware exists on
the R329 (sun50iw11); and from looking at the A100 (sun50iw10) pinctrl
driver, I know some LED controller exists there as well. But I don't
have a manual for the A100 to verify that LED controller is compatible.
So I don't even know if R329 is the first supported SoC.

To be clear: do you want me to name the driver "sun50i_r329_ledc"?

Or maybe "sun50i_a100_ledc", since that came first and it is most likely
compatible?

Regards,
Samuel

> If you want to have the entire list in the Kconfig help though, I don't
> see anything wrong with that. Even if it goes unmaintained, it wouldn't
> really be a big deal.
> 
> Maxime
> 

Re: [PATCH 2/2] leds: sunxi: New driver for the R329/D1 LED controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4753 bytes --]

On Thu, Sep 09, 2021 at 08:54:28AM -0500, Samuel Holland wrote:
> On 9/9/21 6:36 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sun, Sep 05, 2021 at 11:17:19PM -0500, Samuel Holland wrote:
> >> Hi,
> >>
> >> On 9/3/21 5:36 AM, Maxime Ripard wrote:
> >>> On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:
> >>>> Some Allwinner sunxi SoCs, starting with the R329, contain an LED
> >>>> controller designed to drive RGB LED pixels. Add a driver for it using
> >>>> the multicolor LED framework, and with LEDs defined in the device tree.
> >>>>
> >>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >>>> ---
> >>>>  drivers/leds/Kconfig      |   8 +
> >>>>  drivers/leds/Makefile     |   1 +
> >>>>  drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 571 insertions(+)
> >>>>  create mode 100644 drivers/leds/leds-sunxi.c
> >>>>
> >>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >>>> index ed800f5da7d8..559d2ca0a7f4 100644
> >>>> --- a/drivers/leds/Kconfig
> >>>> +++ b/drivers/leds/Kconfig
> >>>> @@ -297,6 +297,14 @@ config LEDS_SUNFIRE
> >>>>  	  This option enables support for the Left, Middle, and Right
> >>>>  	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
> >>>>  
> >>>> +config LEDS_SUNXI
> >>>> +	tristate "LED support for Allwinner sunxi LED controller"
> >>>> +	depends on LEDS_CLASS
> >>>> +	depends on ARCH_SUNXI || COMPILE_TEST
> >>>> +	help
> >>>> +	  This option enables support for the LED controller provided in
> >>>> +	  some Allwinner sunxi SoCs.
> >>>> +
> >>>
> >>> Same comment for the name
> >>
> >> Are you concerned about the help text only, or do you also want me to rename the
> >> Kconfig symbol?
> > 
> > The driver, the driver symbols and the Kconfig symbol would be nice
> > 
> >> I am happy to change the help text to something like: "This option enables
> >> support for the LED controller provided in the Allwinner R329 and D1 SoCs."
> >>
> >> But I don't know of any satisfying way to rename the Kconfig symbol. There is no
> >> general category name for "R329 and D1."
> > 
> > Yeah, this is not ideal, but the issue is that nothing is telling us
> > whether or not it will support *only* the R329 and D1. Chances are it's
> > going to be featured in a number of other SoCs in the future, so if we
> > were to have the entire list of supported SoCs in the Kconfig symbol and
> > driver name, we'd have to always change them everytime a new SoC support
> > is introduced.
> 
> This is why I named it LEDS_SUNXI: until and unless a "v2" hardware
> block shows up, this is the LED controller in all sunxi SoCs [that have
> an LED controller at all]. And at that point, naming the new driver
> LEDS_SUNXI_V2 makes more sense to me than trying to guess the pattern
> for SoC support, where there likely is no pattern.

sunxi and sunxi_v2 doesn't mean anything though. The original A10 is
sunxi, yet it's very far from being supported by that driver. At least
putting the first SoC name has some sort of relationship to the hardware
supported.

> > It would be a pain, and it's pretty much guaranteed that someone is
> > going to forget at some point. To mitigate this, we took the approach to
> > use the same semantic than the DT compatible: the driver name doesn't
> > really define the list of all the SoCs supported but matches every SoC
> > (more or less) compatible with that SoC.
> 
> Ok, but this still doesn't tell me what you expect the driver to be
> named. Again, there is no general name for "every SoC (more or less)
> compatible with R329".

leds-sun50i-r329?

> We tried to guess the pattern around the time H6 came out, and named a
> bunch of things "sun50i" (like the IOMMU driver) that are now showing up
> on sun8i (A50) and sun20i (D1) SoCs.

I'm not sure what's wrong with that? We name and sort things in
chronological order, and there's new SoCs from multiple families in
parallel.

> And it turns out most of the changes were not even new for H6
> (sun50iw6), but already present in the A63 (sun50iw3).

That's indeed a bit unfortunate, but it's not worth changing at this
point.

> I'm in sort of the same situation here. I know the hardware exists on
> the R329 (sun50iw11); and from looking at the A100 (sun50iw10) pinctrl
> driver, I know some LED controller exists there as well. But I don't
> have a manual for the A100 to verify that LED controller is compatible.
> So I don't even know if R329 is the first supported SoC.

Let's just stick to something we know for sure and name it after the
r329

> To be clear: do you want me to name the driver "sun50i_r329_ledc"?

Yes

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]