[RFC PATCH 4/7] realtek: add RTL8380 switch port LED driver
Olliver Schinagl
oliver at schinagl.nl
Fri Jul 29 05:58:34 PDT 2022
On 16-07-2022 21:09, Sander Vanheule wrote:
> RTL8380 switch SoCs can control up to 3×28 LEDs to display status of the
> switch's ethernet port. This driver allows to address these LEDs and
> provides direct control, blink offloading, and switch port status
> offloading.
>
> Since offloading of the generic netdev trigger does not exist, this
> driver provides a private trigger which achieves the same and is named
> "realtek-switchport". Each LED will have a file name "rtl_hw_trigger" in
> /sys/class/leds/$LED, where the requested trigger mode can be written
> to. If an unsupported mode is requested, this will be reported to the
> user. When the trigger is then activated, the LED will be added to a
> group of LEDs with the same trigger conditions. If it is not possible to
> add the LED to a group, the user must use an already existing trigger
> condition, or use direct LED control with a software trigger.
>
> Some common modes (i.e. values for "rtl_hw_trigger") are:
> - Link present, blink on activity: 1f
> - 100M/10M link, blink on activity: f
> - 1G link present: 10
>
> Signed-off-by: Sander Vanheule <sander at svanheule.net>
> ---
> .../drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 ++++++++++++++++++
> ...s-add-RTL8380-switch-port-LED-driver.patch | 64 +++
> target/linux/realtek/rtl838x/config-5.10 | 1 +
> 3 files changed, 435 insertions(+)
> create mode 100644 target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
> create mode 100644 target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-port-LED-driver.patch
>
> diff --git a/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
> new file mode 100644
> index 000000000000..0caf925989b6
> --- /dev/null
> +++ b/target/linux/realtek/files-5.10/drivers/pinctrl/pinctrl-rtl-switchcore.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/mfd/syscon.h>
for ARRAY_SIZE()
+#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "core.h"
> +#include "pinmux.h"
> +
> +/**
> + * struct rtl_swcore_mux_desc - switchcore pin group information
> + *
> + * Pins are frequently muxed between alternative functions, but the control
> + * bits for the muxes are scattered throught the switchcore's register space.
> + * Provide a regmap-based interface to flexibly manage these mux fields, which
> + * may vary in size and do not always provide a GPIO function.
> + *
> + * @name: name to identify the pin group
> + * @field_desc: description of the register field with mux control bits
> + * @functions: NULL terminated array of function names
> + * @pins: array of numbers of the pins in this group
> + * @npins: number of pins in this group
> + */
> +struct rtl_swcore_mux_desc {
> + const char *name;
> + struct reg_field field;
> + const unsigned int *pins;
> + unsigned int npins;
> +};
> +
> +#define SWITCHCORE_MUX(_name, _field, _pins) { \
> + .name = (_name), \
> + .field = _field, \
> + .pins = (_pins), \
> + .npins = ARRAY_SIZE(_pins), \
> + }
> +
> +/**
> + * struct rtl_swcore_mux_setting - stored mux setting
> + *
> + * @mux: pointer to the mux descriptor
> + * @value: value to write in the mux's register field to apply this setting
> + */
> +struct rtl_swcore_mux_setting {
> + const struct rtl_swcore_mux_desc *mux;
> + unsigned int value;
mux_setting? value is so ... undefined ;)
> +};
> +
> +/**
> + * struct rtl_swcore_function_desc - switchcore function information
> + *
> + * @name: name of this function
> + * @settings: list of mux settings that enable this function on said mux
> + * @nsettings: length of the @settings list
> + */
> +struct rtl_swcore_function_desc {
> + const char *name;
> + const struct rtl_swcore_mux_setting *settings;
> + unsigned int nsettings;
> +};
> +
> +#define SWITCHCORE_FUNCTION(_name, _settings) { \
> + .name = (_name), \
> + .settings = (_settings), \
> + .nsettings = ARRAY_SIZE(_settings), \
> + }
> +
> +struct rtl_swcore_config {
> + const struct pinctrl_pin_desc *pins;
> + unsigned int npins;
> + const struct rtl_swcore_function_desc *functions;
> + unsigned int nfunctions;
> + const struct rtl_swcore_mux_desc **groups;
> + unsigned int ngroups;
> +};
> +
> +struct rtl_swcore_pinctrl {
> + struct pinctrl_desc pdesc;
> + struct device *dev;
> + const struct rtl_swcore_config *config;
> + struct regmap_field **mux_fields;
> +};
> +
> +/*
> + * RTL838x chips come in LQFP packages with 216 pins. Pins are indexed
> + * counter-clockwise, starting with pin 1 at the bottom left.
> + * Map package pin {1..216} to pinctrl pin number {0..215}.
> + */
> +static const struct pinctrl_pin_desc rtl8380_swcore_pins[] = {
> + /* JTAG pins */
> + PINCTRL_PIN(27, "tck"),
> + PINCTRL_PIN(28, "tms"),
> + PINCTRL_PIN(29, "tdo"),
> + PINCTRL_PIN(30, "tdi"),
> + PINCTRL_PIN(31, "ntrst"),
maybe a new line to make grouping more clear
> + /* aux MDIO bus pins */
Do we have anything other then aux? I mean, these are (external) pins so
by definition auxiliary. So it's "just" our MDIO bus isn't it
> + PINCTRL_PIN(109, "aux-mdio"),
> + PINCTRL_PIN(110, "aux-mdc"),
> + /* system LED pin */
> + PINCTRL_PIN(112, "sys-led"),
> + /* UART1/SPI slave pins */
> + PINCTRL_PIN(115, "uart1-rx"),
> + PINCTRL_PIN(116, "uart1-tx"),
you currently name the pads/pins uart1-tx, since we don't have a
datasheet, is it worth to use something slightly more descriptive or
generic? I know different soc vendors do different things, some use the
pin name ("AB1"), some just use the gpioN name, some slash seperated
strings ("GPIO112/RG2TXD2/DDRV2") or also ("U8 GMAC0 TXC") some exactly
what you do (both underscore and hyphenized, though i'd use the hyphen
to connect words, sys-led; underscore to define function uart1_tx (or
uart1_tx-sp1_slave)? Anyway, there's a LOT of different flavors, and
_personally_ (but hard for us unless we desolder a SoC to do an analsys)
I personally a REALLY like the most decriptives ones ("pad name, func1
... funcn") as it makes it super clear what it is what uses it has, and
avoids easy confusion (why ist he spi pin named uart)
> +};
> +
> +static const unsigned int rtl8380_jtag_pins[] = {27, 28, 29, 30, 31};
> +static const unsigned int rtl8380_aux_mdio_pins[] = {109, 110};
> +static const unsigned int rtl8380_sys_led_pins[] = {112};
> +static const unsigned int rtl8380_uart1_pins[] = {115, 116};
> +
> +static const struct rtl_swcore_mux_desc rtl8380_mux_jtag =
> + SWITCHCORE_MUX("jtag", REG_FIELD(0x1000, 2, 3), rtl8380_jtag_pins);
> +
> +static const struct rtl_swcore_mux_desc rtl8380_mux_aux_mdio =
> + SWITCHCORE_MUX("aux-mdio", REG_FIELD(0xa0e0, 0, 0), rtl8380_aux_mdio_pins);
> +
> +static const struct rtl_swcore_mux_desc rtl8380_mux_sys_led =
> + SWITCHCORE_MUX("sys-led", REG_FIELD(0xa000, 15, 15), rtl8380_sys_led_pins);
> +
> +static const struct rtl_swcore_mux_desc rtl8380_mux_uart1 =
> + SWITCHCORE_MUX("uart1", REG_FIELD(0x1000, 4, 4), rtl8380_uart1_pins);
> +
> +static const struct rtl_swcore_mux_desc *rtl8380_groups[] = {
> + &rtl8380_mux_jtag,
> + &rtl8380_mux_aux_mdio,
> + &rtl8380_mux_sys_led,
> + &rtl8380_mux_uart1,
> +};
> +
> +static const struct rtl_swcore_mux_setting rtl8380_gpio_settings[] = {
> + {&rtl8380_mux_jtag, 2},
> + {&rtl8380_mux_aux_mdio, 0},
> + {&rtl8380_mux_sys_led, 0},
> +};
> +static const struct rtl_swcore_mux_setting rtl8380_aux_mdio_settings[] = {
> + {&rtl8380_mux_aux_mdio, 1},
> +};
> +static const struct rtl_swcore_mux_setting rtl8380_sys_led_settings[] = {
> + {&rtl8380_mux_sys_led, 1},
> +};
> +static const struct rtl_swcore_mux_setting rtl8380_uart1_settings[] = {
> + {&rtl8380_mux_uart1, 1},
> +};
> +static const struct rtl_swcore_mux_setting rtl8380_spi_slave_settings[] = {
> + {&rtl8380_mux_uart1, 0},
> +};
> +
> +static const struct rtl_swcore_function_desc rtl8380_functions[] = {
> + SWITCHCORE_FUNCTION("gpio", rtl8380_gpio_settings),
> + SWITCHCORE_FUNCTION("aux-mdio", rtl8380_aux_mdio_settings),
> + SWITCHCORE_FUNCTION("sys-led", rtl8380_sys_led_settings),
> + SWITCHCORE_FUNCTION("uart1", rtl8380_uart1_settings),
> + SWITCHCORE_FUNCTION("spi-slave", rtl8380_spi_slave_settings),
> +};
> +
> +static const struct rtl_swcore_config rtl8380_config = {
> + .pins = rtl8380_swcore_pins,
> + .npins = ARRAY_SIZE(rtl8380_swcore_pins),
> + .functions = rtl8380_functions,
> + .nfunctions = ARRAY_SIZE(rtl8380_functions),
> + .groups = rtl8380_groups,
> + .ngroups = ARRAY_SIZE(rtl8380_groups),
> +};
> +
> +/*
> + * RTL839x chips are in BGA packages with 26×26 positions. Board designs number
> + * these as 1..26 for the rows, and A..AF for the columns, with position A1 in
> + * the bottom left corner. Letters I, O, Q, S, X, and Z are skipped; presumably
> + * to avoid ambiguities.
> + * This gives a total of 676 positions. Note that not all positions will
> + * actually have a pad, and many pads will be used for power.
> + *
> + * Index pins using (ROW + 26×COL), where ROW and COL mapped as:
> + * - ROW: {1..26} -> {0..25}
> + * - COL: {A..AF} -> {0..25}
> + *
> + * Since there are no datasheets available, use a virtual pin range starting at
> + * 676 for pins with unknowns positions. When actual pin positions are found
> + * (if ever), these can the be mapped to their real values.
> + */
> +#define RTL8390_VPIN(num, name) PINCTRL_PIN(26 * 26 + (num), (name))
this is unused code though, right? (i get the RFC nature here)
> +
> +/* TODO RTL8390 */
> +
> +/* TODO RTL9300 */
> +
> +/* TODO RTL9310 */
> +
> +static int rtl_swcore_group_count(struct pinctrl_dev *pctldev)
> +{
> + struct rtl_swcore_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> + return priv->config->ngroups;
> +}
> +
> +static const char * rtl_swcore_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + struct rtl_swcore_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> + return priv->config->groups[selector]->name;
> +};
> +
> +static int rtl_swcore_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector, const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct rtl_swcore_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> +
> + *pins = priv->config->groups[selector]->pins;
> + *num_pins = priv->config->groups[selector]->npins;
> +
> + return 0;
> +}
> +
> +static int rtl_swcore_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int selector, unsigned int group)
> +{
> + struct rtl_swcore_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
> + const struct rtl_swcore_function_desc *function;
> + const struct rtl_swcore_mux_setting *setting;
you sure you do't want to scope anything in the for/if part?
> + const struct rtl_swcore_mux_desc *mux;
> + unsigned int i;
> +
> + dev_info(priv->dev, "requesting selector %d, group %d\n", selector, group);
> +
> + function = &priv->config->functions[selector];
> + mux = priv->config->groups[group];
> +
> + for (i = 0; i < function->nsettings; i++) {
> + setting = &function->settings[i];
> + if (setting->mux == mux) {
> + dev_info(priv->dev, "set mux %s to function %s (%d)\n",
> + mux->name, function->name, setting->value);
> + return regmap_field_write(priv->mux_fields[group], setting->value);
i'm generally not a fan of these 'deeply' hidden return statements;
maybe do a goto to jump after the enodev error?
> + }
> + }
> +
> + /* Should never hit this, unless something was misconfigured */
dev_err(dev, "something was miss-configured\n");
> + return -ENODEV;
> +}
> +
> +static const struct pinctrl_ops rtl_swcore_pinctrl_ops = {
> + .get_groups_count = rtl_swcore_group_count,
> + .get_group_name = rtl_swcore_group_name,
> + .get_group_pins = rtl_swcore_group_pins,
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> + .dt_free_map = pinconf_generic_dt_free_map,
> +};
> +
> +static const struct pinmux_ops rtl_swcore_pinmux_ops = {
> + .get_functions_count = pinmux_generic_get_function_count,
> + .get_function_name = pinmux_generic_get_function_name,
> + .get_function_groups = pinmux_generic_get_function_groups,
> + .set_mux = rtl_swcore_set_mux,
> + .strict = true,
> +};
> +
> +static int rtl_swcore_functions_init(struct pinctrl_dev *pctl, struct rtl_swcore_pinctrl *priv)
> +{
> + const struct rtl_swcore_function_desc *function;
> + unsigned int ngroups;
> + const char **groups;
> + unsigned int f_idx;
> + unsigned int g_idx;
> +
> + for (f_idx = 0; f_idx < priv->config->nfunctions; f_idx++) {
> + function = &priv->config->functions[f_idx];
> + ngroups = function->nsettings;
> +
> + dev_info(priv->dev, "found %d groups for function %s\n", ngroups, function->name);
shouldn't this be after the add? we THINK all of this will work; but not
until after the add we know we actually found them. Can the add function
even fail btw?
> +
> + groups = devm_kcalloc(priv->dev, ngroups, sizeof(*groups), GFP_KERNEL);
> + if (!groups)
> + return -ENOMEM;
> +
> + for (g_idx = 0; g_idx < ngroups; g_idx++)
> + groups[g_idx] = function->settings[g_idx].mux->name;
> +
> + pinmux_generic_add_function(pctl, function->name, groups, ngroups, NULL);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_rtl_swcore_pinctrl_match[] = {
> + {
> + .compatible = "realtek,rtl8380-pinctrl",
> + .data = &rtl8380_config,
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_rtl_swcore_pinctrl_match);
> +
> +static int rtl_swcore_pinctrl_probe(struct platform_device *pdev)
> +{
> + const struct rtl_swcore_config *config;
> + struct rtl_swcore_pinctrl *priv;
> + struct device *dev = &pdev->dev;
> + struct pinctrl_dev *pctldev;
> + struct regmap_field *field;
> + struct regmap *regmap;
> + int mux;
> + int err;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + config = of_device_get_match_data(dev);
> + if (!config)
> + return dev_err_probe(dev, -EINVAL, "no config\n");
+ return dev_err_probe(dev, -EINVAL, "unable to match compatible from devicetree\n");
> +
> + /* FIXME of_get_parent(dev_of_node(dev)) OR dev_of_node(dev->parent) */
is there even a functional difference between the two? I like what you
have now, as that doesn't access struct members
> + regmap = device_node_to_regmap(of_get_parent(dev_of_node(dev)));
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "failed to find parent regmap\n");
> +
> + priv->dev = dev;
> + priv->config = config;
> + priv->pdesc.name = "realtek-switchcore-pinctrl";
> + priv->pdesc.owner = THIS_MODULE;
> + priv->pdesc.pctlops = &rtl_swcore_pinctrl_ops;
> + priv->pdesc.pmxops = &rtl_swcore_pinmux_ops;
> + priv->pdesc.pins = config->pins;
> + priv->pdesc.npins = config->npins;
> +
> + priv->mux_fields = devm_kcalloc(dev, config->ngroups, sizeof(*priv->mux_fields),
> + GFP_KERNEL);
> + if (!priv->mux_fields)
> + return -ENOMEM;
> +
> + for (mux = 0; mux < config->ngroups; mux++) {
> + field = devm_regmap_field_alloc(dev, regmap, config->groups[mux]->field);
> + if (IS_ERR(field))
> + return PTR_ERR(field);
> +
> + priv->mux_fields[mux] = field;
> + }
> +
> + err = devm_pinctrl_register_and_init(dev, &priv->pdesc, priv, &pctldev);
> + if (err)
> + return dev_err_probe(dev, err, "failed to register\n");
> +
> + err = rtl_swcore_functions_init(pctldev, priv);
> + if (err)
> + return dev_err_probe(dev, err, "failed to generate function list\n");
> +
> + err = pinctrl_enable(pctldev);
> + if (err)
> + return dev_err_probe(dev, err, "failed to enable\n");
> +
> + return 0;
> +};
> +
> +static struct platform_driver rtl_swcore_pinctrl_driver = {
> + .driver = {
> + .name = "realtek-switchcore-pinctrl",
> + .of_match_table = of_rtl_swcore_pinctrl_match
> + },
> + .probe = rtl_swcore_pinctrl_probe,
> +};
> +module_platform_driver(rtl_swcore_pinctrl_driver);
> diff --git a/target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-port-LED-driver.patch b/target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-port-LED-driver.patch
> new file mode 100644
> index 000000000000..5da5a01cdb48
> --- /dev/null
> +++ b/target/linux/realtek/patches-5.10/202-leds-add-RTL8380-switch-port-LED-driver.patch
> @@ -0,0 +1,64 @@
> +From d71ec8184236356c50088b00b2417fb142e72bd9 Mon Sep 17 00:00:00 2001
> +From: Sander Vanheule <sander at svanheule.net>
> +Date: Sun, 10 Jul 2022 11:31:53 +0200
> +Subject: [PATCH 03/14] leds: add RTL8380 switch port LED driver
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +RTL8380 switch SoCs can control up to 3×28 LEDs to display status of the
> +switch's ethernet port. This driver allows to address these LEDs and
> +provides direct control, blink offloading, and switch port status
> +offloading.
> +
> +Since offloading of the generic netdev trigger does not exist, this
> +driver provides a private trigger which achieves the same and is named
> +"realtek-switchport". Each LED will have a file name "rtl_hw_trigger" in
> +/sys/class/leds/$LED, where the requested trigger mode can be written
> +to. If an unsupported mode is requested, this will be reported to the
> +user. When the trigger is then activated, the LED will be added to a
> +group of LEDs with the same trigger conditions. If it is not possible to
> +add the LED to a group, the user must use an already existing trigger
> +condition, or use direct LED control with a software trigger.
> +
> +Some common modes (i.e. values for "rtl_hw_trigger") are:
> + - Link present, blink on activity: 1f
> + - 100M/10M link, blink on activity: f
> + - 1G link present: 10
> +
> +Signed-off-by: Sander Vanheule <sander at svanheule.net>
> +---
> + drivers/leds/Kconfig | 9 +
> + drivers/leds/Makefile | 1 +
> + drivers/leds/leds-rtl-switch-port.c | 668 ++++++++++++++++++++++++++++
> + 3 files changed, 678 insertions(+)
> + create mode 100644 drivers/leds/leds-rtl-switch-port.c
> +
> +--- a/drivers/leds/Kconfig
> ++++ b/drivers/leds/Kconfig
> +@@ -594,6 +594,15 @@ config LEDS_REGULATOR
> + help
> + This option enables support for regulator driven LEDs.
> +
> ++config LEDS_RTL_SWITCH_PORT
> ++ tristate "Realtek SoC switch port LED support"
> ++ depends on LEDS_CLASS
> ++ depends on MFD_REALTEK_SWITCHCORE || COMPILE_TEST
> ++ select MFD_SYSCON
> ++ select REGMAP_MMIO
> ++ help
> ++ This option enables support for Realtek switch SoC port LEDs.
> ++
> + config LEDS_BD2802
> + tristate "LED driver for BD2802 RGB LED"
> + depends on LEDS_CLASS
> +--- a/drivers/leds/Makefile
> ++++ b/drivers/leds/Makefile
> +@@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_PM8058) += leds-pm805
> + obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
> + obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
> + obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
> ++obj-$(CONFIG_LEDS_RTL_SWITCH_PORT) += leds-rtl-switch-port.o
> + obj-$(CONFIG_LEDS_S3C24XX) += leds-s3c24xx.o
> + obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
> + obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
> diff --git a/target/linux/realtek/rtl838x/config-5.10 b/target/linux/realtek/rtl838x/config-5.10
> index 5c69832be41d..a28366a7fd03 100644
> --- a/target/linux/realtek/rtl838x/config-5.10
> +++ b/target/linux/realtek/rtl838x/config-5.10
> @@ -164,6 +164,7 @@ CONFIG_PGTABLE_LEVELS=2
> CONFIG_PHYLIB=y
> CONFIG_PHYLINK=y
> CONFIG_PINCTRL=y
> +CONFIG_PINCTRL_RTL_SWITCHCORE=y
> CONFIG_POWER_RESET=y
> CONFIG_POWER_RESET_GPIO_RESTART=y
> CONFIG_POWER_RESET_SYSCON=y
>
More information about the openwrt-devel
mailing list