Message ID | 20211119054044.16286-1-samuel@sholland.org |
---|---|
State | New |
Headers | show |
Series | [v3,1/2] dt-bindings: leds: Add Allwinner R329/D1 LED controller | expand |
Hi, On Thu, Nov 18, 2021 at 11:40:43PM -0600, Samuel Holland wrote: > +static const struct sun50i_r329_ledc_timing sun50i_r329_ledc_default_timing = { > + .t0h_ns = 336, > + .t0l_ns = 840, > + .t1h_ns = 882, > + .t1l_ns = 294, > + .treset_ns = 300000, > +}; This should be mentioned in the binding as well (using the default keyword) > +static int sun50i_r329_ledc_parse_timing(const struct device_node *np, > + struct sun50i_r329_ledc *priv) > +{ > + struct sun50i_r329_ledc_timing *timing = &priv->timing; > + > + *timing = sun50i_r329_ledc_default_timing; > + > + of_property_read_u32(np, "allwinner,t0h-ns", &timing->t0h_ns); > + of_property_read_u32(np, "allwinner,t0l-ns", &timing->t0l_ns); > + of_property_read_u32(np, "allwinner,t1h-ns", &timing->t1h_ns); > + of_property_read_u32(np, "allwinner,t1l-ns", &timing->t1l_ns); > + of_property_read_u32(np, "allwinner,treset-ns", &timing->treset_ns); > + > + return 0; > +} > + > +static void sun50i_r329_ledc_set_timing(struct sun50i_r329_ledc *priv) > +{ > + const struct sun50i_r329_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 sun50i_r329_ledc_resume(struct device *dev) > +{ > + struct sun50i_r329_ledc *priv = dev_get_drvdata(dev); > + u32 val; > + int ret; > + > + ret = reset_control_deassert(priv->reset); > + if (ret) > + return ret; > + > + 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; > + > + sun50i_r329_ledc_set_format(priv); > + sun50i_r329_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); > + > + return ret; > +} > + > +static int sun50i_r329_ledc_suspend(struct device *dev) > +{ > + struct sun50i_r329_ledc *priv = dev_get_drvdata(dev); > + > + clk_disable_unprepare(priv->mod_clk); > + clk_disable_unprepare(priv->bus_clk); > + reset_control_assert(priv->reset); > + > + return 0; > +} > + > +static void sun50i_r329_ledc_dma_cleanup(void *data) > +{ > + struct sun50i_r329_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 sun50i_r329_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 sun50i_r329_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 = sun50i_r329_ledc_parse_format(np, priv); > + if (ret) > + return ret; > + > + ret = sun50i_r329_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->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, sun50i_r329_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, sun50i_r329_ledc_irq, > + 0, dev_name(dev), priv); > + if (ret) > + return ret; > + > + ret = sun50i_r329_ledc_resume(dev); > + if (ret) > + return ret; You seem to fill the runtime_pm hooks, but only call them directly and never enable runtime_pm on that device, is that intentional? Maxime
On 11/23/21 10:39 AM, Maxime Ripard wrote: > On Fri, Nov 19, 2021 at 05:57:01PM -0600, Samuel Holland wrote: >> Hi Maxime, >> >> On 11/19/21 2:28 AM, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, Nov 18, 2021 at 11:40:43PM -0600, Samuel Holland wrote: >>>> +static const struct sun50i_r329_ledc_timing sun50i_r329_ledc_default_timing = { >>>> + .t0h_ns = 336, >>>> + .t0l_ns = 840, >>>> + .t1h_ns = 882, >>>> + .t1l_ns = 294, >>>> + .treset_ns = 300000, >>>> +}; >>> >>> This should be mentioned in the binding as well (using the default keyword) >> >> Ok, I'll do this for v4. >> >>>> +static int sun50i_r329_ledc_parse_timing(const struct device_node *np, >>>> + struct sun50i_r329_ledc *priv) >>>> +{ >>>> + struct sun50i_r329_ledc_timing *timing = &priv->timing; >>>> + >>>> + *timing = sun50i_r329_ledc_default_timing; >>>> + >>>> + of_property_read_u32(np, "allwinner,t0h-ns", &timing->t0h_ns); >>>> + of_property_read_u32(np, "allwinner,t0l-ns", &timing->t0l_ns); >>>> + of_property_read_u32(np, "allwinner,t1h-ns", &timing->t1h_ns); >>>> + of_property_read_u32(np, "allwinner,t1l-ns", &timing->t1l_ns); >>>> + of_property_read_u32(np, "allwinner,treset-ns", &timing->treset_ns); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void sun50i_r329_ledc_set_timing(struct sun50i_r329_ledc *priv) >>>> +{ >>>> + const struct sun50i_r329_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 sun50i_r329_ledc_resume(struct device *dev) >>>> +{ >>>> + struct sun50i_r329_ledc *priv = dev_get_drvdata(dev); >>>> + u32 val; >>>> + int ret; >>>> + >>>> + ret = reset_control_deassert(priv->reset); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + 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; >>>> + >>>> + sun50i_r329_ledc_set_format(priv); >>>> + sun50i_r329_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); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int sun50i_r329_ledc_suspend(struct device *dev) >>>> +{ >>>> + struct sun50i_r329_ledc *priv = dev_get_drvdata(dev); >>>> + >>>> + clk_disable_unprepare(priv->mod_clk); >>>> + clk_disable_unprepare(priv->bus_clk); >>>> + reset_control_assert(priv->reset); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void sun50i_r329_ledc_dma_cleanup(void *data) >>>> +{ >>>> + struct sun50i_r329_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 sun50i_r329_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 sun50i_r329_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 = sun50i_r329_ledc_parse_format(np, priv); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = sun50i_r329_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->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, sun50i_r329_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, sun50i_r329_ledc_irq, >>>> + 0, dev_name(dev), priv); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = sun50i_r329_ledc_resume(dev); >>>> + if (ret) >>>> + return ret; >>> >>> You seem to fill the runtime_pm hooks, but only call them directly and >>> never enable runtime_pm on that device, is that intentional? >> >> Yes. I did not want to delay the initial version by adding runtime PM >> (and debugging the refcounts) when the driver already works now. >> However, I had runtime/system PM in mind while writing the driver. >> >> If you think it is too confusing, I could rename the functions to >> something like sun50i_r329_ledc_hw_init / sun50i_r329_ledc_hw_exit. > > It's not really the functions themselves that are confusing but rather > that you set them as runtime_pm hooks. I do not set these functions as runtime PM hooks. SIMPLE_DEV_PM_OPS only sets the system PM hooks, for "suspend to RAM and hibernation." Maybe you are thinking of SET_RUNTIME_PM_OPS, which I do not use? Regards, Samuel > If you plan on submitting it later on, I guess the easiest would just be > to remove the sun50i_r329_ledc_pm structure, and then you'll just have > to introduce it back with a call to pm_runtime_enable when it's ready > > Maxime >
On Thu, Nov 25, 2021 at 09:26:15AM -0600, Samuel Holland wrote: > >>>> + ret = sun50i_r329_ledc_resume(dev); > >>>> + if (ret) > >>>> + return ret; > >>> > >>> You seem to fill the runtime_pm hooks, but only call them directly and > >>> never enable runtime_pm on that device, is that intentional? > >> > >> Yes. I did not want to delay the initial version by adding runtime PM > >> (and debugging the refcounts) when the driver already works now. > >> However, I had runtime/system PM in mind while writing the driver. > >> > >> If you think it is too confusing, I could rename the functions to > >> something like sun50i_r329_ledc_hw_init / sun50i_r329_ledc_hw_exit. > > > > It's not really the functions themselves that are confusing but rather > > that you set them as runtime_pm hooks. > > I do not set these functions as runtime PM hooks. SIMPLE_DEV_PM_OPS only sets > the system PM hooks, for "suspend to RAM and hibernation." Maybe you are > thinking of SET_RUNTIME_PM_OPS, which I do not use? Ah, right, it's all good then, sorry for the noise Maxime
On Thu, 18 Nov 2021 23:40:42 -0600, 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> > --- > > Changes in v3: > - Removed quotes from enumeration values > - Added vendor prefix to timing/format properties > - Renamed "format" property to "pixel-format" for clarity > - Dropped "vled-supply" as it is unrelated to the controller hardware > > Changes in v2: > - Fixed typo leading to duplicate t1h-ns property > - Removed "items" layer in definition of dmas/dma-names > - Replaced uint32 type reference with maxItems in timing properties > > .../leds/allwinner,sun50i-r329-ledc.yaml | 137 ++++++++++++++++++ > 1 file changed, 137 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
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..f8ec5fc6e0ba --- /dev/null +++ b/Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml @@ -0,0 +1,137 @@ +# 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 R329 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: + maxItems: 1 + description: TX DMA channel + + dma-names: + const: tx + + interrupts: + maxItems: 1 + + allwinner,pixel-format: + description: Pixel format (subpixel transmission order), default is "grb" + enum: + - bgr + - brg + - gbr + - grb + - rbg + - rgb + + allwinner,t0h-ns: + maxItems: 1 + description: Length of high pulse when transmitting a "0" bit + + allwinner,t0l-ns: + maxItems: 1 + description: Length of low pulse when transmitting a "0" bit + + allwinner,t1h-ns: + maxItems: 1 + description: Length of high pulse when transmitting a "1" bit + + allwinner,t1l-ns: + maxItems: 1 + description: Length of low pulse when transmitting a "1" bit + + allwinner,treset-ns: + maxItems: 1 + 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; + }; + }; + +...
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> --- Changes in v3: - Removed quotes from enumeration values - Added vendor prefix to timing/format properties - Renamed "format" property to "pixel-format" for clarity - Dropped "vled-supply" as it is unrelated to the controller hardware Changes in v2: - Fixed typo leading to duplicate t1h-ns property - Removed "items" layer in definition of dmas/dma-names - Replaced uint32 type reference with maxItems in timing properties .../leds/allwinner,sun50i-r329-ledc.yaml | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-r329-ledc.yaml