[RFC,3/7] realtek: add pinctrl for Realtek maple

Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
RTL838x has been tested, but functionality should be near identical.

Since control bits for different muxes are scattered throughout the
switch core's register space, pin control features are provided by a
table of regmap fields. This should be flexible enough to allow
extension to other SoC generations providing similar features.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 .../drivers/leds/leds-rtl-switch-port.c       | 668 ++++++++++++++++++
 ...inctrl-add-pinctrl-for-Realtek-maple.patch |  51 ++
 target/linux/realtek/rtl838x/config-5.10      |   1 +
 3 files changed, 720 insertions(+)
 create mode 100644 target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
 create mode 100644 target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch

On Sat, 2022-07-16 at 21:09 +0200, Sander Vanheule wrote:
> Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
> RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
> RTL838x has been tested, but functionality should be near identical.
> 
> Since control bits for different muxes are scattered throughout the
> switch core's register space, pin control features are provided by a
> table of regmap fields. This should be flexible enough to allow
> extension to other SoC generations providing similar features.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  .../drivers/leds/leds-rtl-switch-port.c       | 668 ++++++++++++++++++
>  ...inctrl-add-pinctrl-for-Realtek-maple.patch |  51 ++
>  target/linux/realtek/rtl838x/config-5.10      |   1 +
>  3 files changed, 720 insertions(+)
>  create mode 100644 target/linux/realtek/files-5.10/drivers/leds/leds-rtl-
> switch-port.c
>  create mode 100644 target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-
> for-Realtek-maple.patch

It appears I have mixed up the contents and description of the pinctrl and leds
patches. The end result of applying both will be the same, but applying only the
first won't work. Sorry the trouble.

Best,
Sander

On 16-07-2022 21:09, Sander Vanheule wrote:
> Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
> RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
> RTL838x has been tested, but functionality should be near identical.
>
> Since control bits for different muxes are scattered throughout the
> switch core's register space, pin control features are provided by a
> table of regmap fields. This should be flexible enough to allow
> extension to other SoC generations providing similar features.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>   .../drivers/leds/leds-rtl-switch-port.c       | 668 ++++++++++++++++++
>   ...inctrl-add-pinctrl-for-Realtek-maple.patch |  51 ++
>   target/linux/realtek/rtl838x/config-5.10      |   1 +
>   3 files changed, 720 insertions(+)
>   create mode 100644 target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
>   create mode 100644 target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch
>
> diff --git a/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
> new file mode 100644
> index 000000000000..76dfede7ba15
> --- /dev/null
> +++ b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
> @@ -0,0 +1,668 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
Probably want bitops.h for BIT()

+#include <linux/bitops.h>

> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * Realtek switch port LED
> + *
> + * The switch ASIC can control up to three LEDs per phy, based on a number of
> + * matching conditions. Alternatively, each individual LED output can also be
> + * configured for manual control.
> + */

Is this an internal define? `REALTEK_PORT_LED_TRIGGER_NONE` (etc) maybe 
better? (or LEDTRIG if it really must be shorter)

It wasn't until much later that I figured out that this was the 
abbreviation for port trigger *doh*

> +#define PTRG_NONE		0
> +#define PTRG_ACT_RX		BIT(0)
> +#define PTRG_ACT_TX		BIT(1)
> +#define PTRG_ACT		PTRG_ACT_RX | PTRG_ACT_TX
> +#define PTRG_LINK_10		BIT(2)
> +#define PTRG_LINK_100		BIT(3)
> +#define PTRG_LINK_1000		BIT(4)
> +
> +struct led_port_blink_mode {
> +	u16 interval; /* Toggle interval in ms */
> +	u8 mode; /* ASIC mode bits */
> +};
> +
> +#define REALTEK_PORT_LED_BLINK_COUNT	6
> +struct led_port_modes {
realtek_led_port_modes?
> +	u8 off;
> +	u8 on;
> +	struct led_port_blink_mode blink[REALTEK_PORT_LED_BLINK_COUNT];

ah here's the other list. So would it make sense to re-use this same 
structure for the sys-led, or is that over-optimizing things ...

Why though are on/off and the blink entries separated?  Isn't 'off' a 
interval of 'inviite (max-int)' and on a blink rate of 0? While you may 
have to do some logic later, it keeps the interface more consistent? 
idk; just some food for thought.

> +};
> +
> +struct led_port_group {
> +	struct regmap_field *setting;
> +	unsigned int size;
> +	/* bitmap to keep track of associated ports */
Can you explain a bit what's supposed to be here? right now its just an 
it pointer, but appearantly it will point at some bitmap? can we not 
make a struct representing this bitmap and then use that instead?
> +	unsigned long *ports;
> +};
> +
> +struct switch_port_led_config;

can we do this forward declaration the other way around? You forward 
declare it here because need the TYPE for the cfg pointer.

Later however, you need it to indicate the argument of a function 
pointer. I think the type declaration ways heavier and makes it thus 
'nicer' to do it the other way around. though that does indeed mean you 
have to do 2 forard declarations for your two structs, but would still 
be cleaner imo

> +
> +struct switch_port_led_ctrl {
> +	struct device *dev;
> +	struct regmap *map;
> +	const struct switch_port_led_config *cfg;
> +	struct mutex lock;
> +	struct led_port_group *groups;
> +};
> +
> +struct switch_port_led {
> +	struct led_classdev led;
> +	struct switch_port_led_ctrl *ctrl;
> +	unsigned int port;
> +	unsigned int index;
> +	u32 trigger_flags;
> +	struct led_port_group *current_group;
> +};
> +
> +struct switch_port_led_config {
> +	unsigned int port_count;
> +	unsigned int port_led_count;
> +	unsigned int group_count;
> +	const struct led_port_modes *modes;
> +	/* Set the number of LEDs per port */
> +	int (*port_led_init)(struct switch_port_led_ctrl *ctrl,
> +		unsigned int led_count);
what is the significance for both these comments (above and below)? the 
comment states to set the leds per port; but the function is called 
led_init? Below the function is called set enabled, so that's pretty 
obvious that you set the port enabled switch port led?
> +	/* Enable or disable all LEDs for a certain port */
> +	int (*set_port_enabled)(struct switch_port_led *led, bool enabled);
> +	int (*set_hw_managed)(struct switch_port_led *led, bool hw_managed);
> +	int (*write_mode)(struct switch_port_led *led, unsigned int mode);
> +	int (*read_mode)(struct switch_port_led *led);
> +	int (*trigger_xlate)(u32 trigger);
> +	struct led_port_group *(*map_group)(struct switch_port_led *led, u32 trigger);
> +	struct reg_field group_settings[];
> +};
> +
> +static struct led_port_group *switch_port_led_get_group(
> +	struct switch_port_led *pled, unsigned int group)
> +{
> +	return &pled->ctrl->groups[group * pled->ctrl->cfg->port_led_count + pled->index];
also in this patch; don't fear parenthesis :) they are our friend ;)
> +}
> +
> +/*
> + * SoC specific implementation for RTL8380 series (Maple)
Maybe 'x' the 0 here?

+ * SoC specific implementation for RTL838x series (Maple)

> + */
> +#define RTL8380_REG_LED_MODE_SEL		0x1004
do we have any idea what the SDK names these registers? Usually they 
match the (secret) datasheet ...
> +#define RTL8380_REG_LED_GLB_CTRL		0xa000
> +#define RTL8380_REG_LED_MODE_CTRL		0xa004
> +#define RTL8380_REG_LED_P_EN_CTRL		0xa008
> +#define RTL8380_REG_LED_SW_P_EN_CTRL(led)	(0xa010 + 4 * (led))
> +#define RTL8380_REG_LED_SW_CTRL(port)		(0xa01c + 4 * (port))
> +
> +#define RTL8380_PORT_LED_COUNT				3
> +#define RTL8380_GROUP_SETTING_WIDTH			5
> +#define RTL8380_GROUP_SETTING_OFFSET(_grp, _idx)	\
> +	(RTL8380_GROUP_SETTING_WIDTH * ((_idx) + RTL8380_PORT_LED_COUNT * (_grp)))
> +#define RTL8380_GROUP_SETTING(_grp, _idx)	{				\
> +		.reg = RTL8380_REG_LED_MODE_CTRL,				\
> +		.lsb = RTL8380_GROUP_SETTING_OFFSET(_grp, _idx),		\
> +		.msb = RTL8380_GROUP_SETTING_OFFSET(_grp, (_idx) + 1) - 1,	\
> +	}
> +
> +enum rtl83xx_port_trigger {
> +	RTL83XX_TRIG_LINK_ACT = 0,
> +	RTL83XX_TRIG_LINK = 1,
> +	RTL83XX_TRIG_ACT = 2,
> +	RTL83XX_TRIG_ACT_RX = 3,
> +	RTL83XX_TRIG_ACT_TX = 4,
> +	RTL83XX_TRIG_LINK_1G = 7,
> +	RTL83XX_TRIG_LINK_100M = 8,
> +	RTL83XX_TRIG_LINK_10M = 9,
> +	RTL83XX_TRIG_LINK_ACT_1G = 10,
> +	RTL83XX_TRIG_LINK_ACT_100M = 11,
> +	RTL83XX_TRIG_LINK_ACT_10M = 12,
> +	RTL83XX_TRIG_LINK_ACT_1G_100M = 13,
> +	RTL83XX_TRIG_LINK_ACT_1G_10M = 14,
> +	RTL83XX_TRIG_LINK_ACT_100M_10M = 15,
> +	RTL83XX_TRIG_DISABLED = 31,
> +};
as you sometimes do right align, this enum is a perfect candidate I'd 
think to do so as well?
> +
> +static const struct led_port_modes rtl8380_port_led_modes = {
> +	.off = 0,
> +	.on = 5,
> +	.blink  = {{  32, 1},
I know it's just stupid style, but why not put the first entry on the 
next line with one indentation level? I think that make it looks a 
little more natural
> +		   {  64, 2},
> +		   { 128, 3},

I think this enum here deserves a comment as to why this order is so 
funky. Also to prevent someone passing along later and trying to 'fix' it :)

> +		   { 256, 6},
> +		   { 512, 4},
> +		   {1024, 7}}
> +};
> +
> +static const struct led_port_modes rtl8390_port_led_modes = {
> +	.off = 0,
> +	.on = 7,
> +	.blink = {{  32, 1},
> +		  {  64, 2},
> +		  { 128, 3},
> +		  { 256, 4},
> +		  { 512, 5},
> +		  {1024, 6}}
> +};
> +
> +static int rtl8380_port_trigger_xlate(u32 port_led_trigger)
> +{
> +	switch (port_led_trigger) {
> +	case PTRG_NONE:
> +		return RTL83XX_TRIG_DISABLED;
> +	case PTRG_ACT_RX:
> +		return RTL83XX_TRIG_ACT_RX;
> +	case PTRG_ACT_TX:
> +		return RTL83XX_TRIG_ACT_TX;
> +	case PTRG_ACT:
> +		return RTL83XX_TRIG_ACT;
> +	case PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK;
> +	case PTRG_LINK_10:
> +		return RTL83XX_TRIG_LINK_10M;
> +	case PTRG_LINK_100:
> +		return RTL83XX_TRIG_LINK_100M;
> +	case PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK_1G;
> +	case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK_ACT;
> +	case PTRG_ACT | PTRG_LINK_10:
> +		return RTL83XX_TRIG_LINK_ACT_10M;
> +	case PTRG_ACT | PTRG_LINK_100:
> +		return RTL83XX_TRIG_LINK_ACT_100M;
> +	case PTRG_ACT | PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK_ACT_1G;
> +	case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100:
> +		return RTL83XX_TRIG_LINK_ACT_100M_10M;
> +	case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK_ACT_1G_10M;
> +	case PTRG_ACT | PTRG_LINK_100 | PTRG_LINK_1000:
> +		return RTL83XX_TRIG_LINK_ACT_1G_100M;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +/*
> + * Find the group the LED with this trigger setting can be assigned to, or

afaik you can use '@trigger' which then relates to the argument. I had 
to read this line 3 times to realize that 'this' related to the trigger'

Also, what are you 'finding', i'm not seeing a loop or anything, where 
are we finding?

'Map switch port @led to map to a trigger @trigger'? idk, but I'm not 
sure what is happening here yet.

btw, you forgot to add documentation for your arguments ;)

> + * initialise a new empty group.
> + *
> + * Return group number on success, or < 0 on failure.
> + * */
> +static struct led_port_group *rtl8380_port_led_map_group(struct switch_port_led *led, u32 trigger)
> +{
> +	int rtl_trigger = rtl8380_port_trigger_xlate(trigger);
> +	struct switch_port_led_ctrl *ctrl = led->ctrl;
> +	struct led_port_group *group;
> +	u32 current_trigger;
> +	int err;
> +
> +	if (rtl_trigger < 0)
> +	       return ERR_PTR(rtl_trigger);
> +
> +	/*
> +	 * Static groups:
does this deserve some explanation? why are there 2 groups? how are they 
grouped? why are the grouped the way they are grouped? that could also 
help you define a name for the magic value 23 potentially?
> +	 *   - group 0: ports 0-23
> +	 *   - group 1: ports 24-27
> +	 */
> +	if (led->port > 23)
> +		group = switch_port_led_get_group(led, 1);
> +	else
> +		group = switch_port_led_get_group(led, 0);
> +
> +	err = regmap_field_read(group->setting, &current_trigger);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	if (current_trigger != rtl_trigger && !bitmap_empty(group->ports, group->size)) {
> +		dev_warn(ctrl->dev, "cannot add (%d,%d) to group: 0x%02x != 0x%02x\n",
> +			led->port, led->index, current_trigger, rtl_trigger);
> +		return ERR_PTR(-ENOSPC);
> +	}
> +
> +	return group;
> +}
> +
> +static int rtl8380_port_led_set_port_enabled(struct switch_port_led *led, bool enabled)
> +{
> +	int reg = RTL8380_REG_LED_P_EN_CTRL;
> +	int val = enabled ? BIT(led->port) : 0;
> +
> +	return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
> +}
> +
> +static int rtl8380_port_led_set_hw_managed(struct switch_port_led *led, bool hw_managed)
> +{
> +	int reg = RTL8380_REG_LED_SW_P_EN_CTRL(led->index);
> +	int val = hw_managed ? 0 : BIT(led->port);
> +
> +	return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
> +}
> +
> +static int rtl8380_port_led_write_mode(struct switch_port_led *led, unsigned int mode)
> +{
> +	int reg = RTL8380_REG_LED_SW_CTRL(led->port);
> +	int offset = 3 * led->index;
I think this 3 (which is reused later also) could have a define 
(REALTEK_something_LED_PER_something) or something (and maybe even an 
accompanying _MASK later I find oyu have `

port_led_count

` so you probably could use that?
> +	u32 mask = GENMASK(2, 0) << offset;
> +	u32 value = mode << offset;
> +
> +	return regmap_update_bits(led->ctrl->map, reg, mask, value);
> +}
> +
> +static int rtl8380_port_led_read_mode(struct switch_port_led *led)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(led->ctrl->map, RTL8380_REG_LED_SW_CTRL(led->port), &val);
> +	if (ret)
> +		return ret;
> +
> +	return (val >> (3 * led->index)) & GENMASK(2, 0);
> +}
> +
> +static int rtl8380_port_led_init(struct switch_port_led_ctrl *ctrl, unsigned int led_count)
> +{
> +	u32 led_count_mask = GENMASK(led_count - 1, 0);
> +	u32 led_mask = GENMASK(5, 0);
> +
> +	return regmap_update_bits(ctrl->map, RTL8380_REG_LED_GLB_CTRL, led_mask,
> +			(led_count_mask << 3) | led_count_mask);
> +}
> +
> +static void switch_port_led_brightness_set(struct led_classdev *led_cdev,
> +	enum led_brightness brightness)
> +{
> +	struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led, led);
> +	struct switch_port_led_ctrl *ctrl = pled->ctrl;
> +
> +	ctrl->cfg->write_mode(pled, brightness ? ctrl->cfg->modes->on : ctrl->cfg->modes->off);
> +}
> +
> +static enum led_brightness switch_port_led_brightness_get(struct led_classdev *led_cdev)
> +{
> +	struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led, led);
> +	struct switch_port_led_ctrl *ctrl = pled->ctrl;
> +
> +	return ctrl->cfg->read_mode(pled) != ctrl->cfg->modes->off;
> +}
> +
> +static int switch_port_led_blink_set(struct led_classdev *led_cdev,
> +	unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led, led);
> +	struct switch_port_led_ctrl *ctrl = pled->ctrl;
> +
> +	const struct led_port_blink_mode *blink = &ctrl->cfg->modes->blink[0];
> +	unsigned long interval_ms = *delay_on + *delay_off;
> +	unsigned int i = 0;
> +
> +	if (interval_ms && *delay_on == 0)
> +		return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->off);
> +
> +	if (interval_ms && *delay_off == 0)
> +		return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->on);
> +
> +	if (!interval_ms)
You are explicit on the delay, why not on the interval?

+	if (interval_ms == 0)

> +		interval_ms = 500;
> +
> +	/*
> +	 * Since the modes always double in interval, the geometric mean of
> +	 * intervals [i] and [i + 1] is equal to sqrt(2) * interval[i]
> +	 */
> +	while (i < (REALTEK_PORT_LED_BLINK_COUNT - 1) &&
> +		interval_ms > (2 * 141 * blink[i].interval) / 100)
> +		i++;
> +
> +	*delay_on = blink[i].interval;
> +	*delay_off = blink[i].interval;
> +
> +	return ctrl->cfg->write_mode(pled, blink[i].mode);
> +}
> +
> +static struct led_hw_trigger_type switch_port_rtl_hw_trigger_type;
> +
> +static ssize_t rtl_hw_trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct switch_port_led *pled = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%x\n", pled->trigger_flags);
> +}
> +
> +static ssize_t rtl_hw_trigger_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	struct switch_port_led *pled = dev_get_drvdata(dev);
> +	struct led_port_group *group, *new_group;
from a namespace pov; why are you not putting these variables inside 
your if statement and scope them there? however ...
> +	unsigned int member_count;
> +	int trigger;
> +	int nchars;
> +	int value;
> +	int err;
> +
> +	if (sscanf(buf, "%x%n", &value, &nchars) != 1 || nchars + 1 < count)
> +		return -EINVAL;
> +
> +	mutex_lock(&pled->ctrl->lock);
> +
> +	/*
> +	 * If the private trigger is already active:
> +	 *   - modify the current group if we are the only member,
> +	 *   - or join a new group if one is available
> +	 */
> +	if (pled->current_group) {

you are doing everything within this if; why not instead? fail early 
fail often

+	if (!pled->current_group) {
+		dev_dbg(dev, "no led not part of any group\n");
+		err = -Esomething;
+		goto out;
+	};

> +		group = pled->current_group;
> +
> +		trigger = pled->ctrl->cfg->trigger_xlate(value);
> +		if (trigger < 0) {
> +			err = trigger;
> +			goto err_out;
> +		}
> +
> +		member_count = bitmap_weight(group->ports, group->size);
> +
> +		if (member_count == 1) {
> +			err = regmap_field_write(group->setting, trigger);
> +			if (err)
> +				goto err_out;
> +
> +			goto out;
> +		}
> +
> +		new_group = pled->ctrl->cfg->map_group(pled, value);
> +		if (IS_ERR(new_group)) {
> +			err = PTR_ERR(new_group);
> +			goto err_out;
> +		}
> +
> +		if (member_count == 0) {
> +			err = regmap_field_write(new_group->setting, trigger);
> +			if (err)
> +				goto err_out;
> +		}
> +	
> +		bitmap_clear(group->ports, pled->port, 1);
> +		bitmap_set(new_group->ports, pled->port, 1);
> +		pled->current_group = new_group;
> +	}
> +
> +out:
> +	pled->trigger_flags = value;
> +
> +err_out:
> +	mutex_unlock(&pled->ctrl->lock);
> +
> +	if (err)
> +		return err;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(rtl_hw_trigger);
> +
> +static struct attribute *rtl_hw_trigger_attrs[] = {
> +	&dev_attr_rtl_hw_trigger.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(rtl_hw_trigger);
Is there any possibility this can be done in a generic way at all? I 
know this doesn't exist, and it's a very good first step ... but these 
are netdev_switch_triggers maybe? Or would that not work because our 
triggers are TOO RTL specific? Could we make them less specific (in the 
future)?
> +
> +static int switch_port_led_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led, led);
> +	struct led_port_group *group;
> +	int rtl_trigger;
> +	int err = 0;
> +
> +	mutex_lock(&pled->ctrl->lock);
> +
> +	rtl_trigger = pled->ctrl->cfg->trigger_xlate(pled->trigger_flags);
> +	if (rtl_trigger < 0) {
> +		err = rtl_trigger;
> +		goto out;
> +	}
> +
> +	group = pled->ctrl->cfg->map_group(pled, pled->trigger_flags);
> +	if (IS_ERR(group)) {
> +		err = PTR_ERR(group);
> +		goto out;
> +	}
> +
> +	if (bitmap_empty(group->ports, group->size)) {
> +		err = regmap_field_write(group->setting, rtl_trigger);
> +		if (err)
> +			goto out;
> +	}
> +
> +	bitmap_set(group->ports, pled->port, 1);
would it be useful to somehow codify the length? MACRO, sizeof, 
pled->port_size something?
> +	pled->current_group = group;
> +
> +	err = pled->ctrl->cfg->set_hw_managed(pled, true);

if this where some generic function that would need to come from the 
ops-struct, I'd get it; but why not call the function directly? Otherwise:

+	if (pled->ctrl->cfg->set_hw_managed)
+		err = pled->ctrl->cfg->set_hw_managed(pled, true);

> +
> +out:
> +	mutex_unlock(&pled->ctrl->lock);
personally, I like to check for nullness on these things, but I don't 
know what is most commonely done here ... or if mutex_unlock does its 
own nice checks.
> +
> +	return err;
> +}
> +
> +static void switch_port_led_trigger_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led, led);
> +	struct led_port_group *group;
> +
> +	mutex_lock(&pled->ctrl->lock);
> +
> +	pled->ctrl->cfg->set_hw_managed(pled, false);
as above
> +
> +	group = pled->current_group;
what's the purpose of this assignment? you make a copy, then set it to 
null; then clear the bitmap. Can you not just first clear the bitmap, 
then null the assignment? Or is there some hidden race-condition you are 
protecting against in this way? If so, better leave a comment to 
indicate this potential race-condition if we can't protect this via 
code; though I thought that's what the mutex was for to begin with ... 
maybe add an 'if (pled->current_group == NULL) return check before we 
even try to get the mutex?
> +	pled->current_group = NULL;
> +	bitmap_clear(group->ports, pled->port, 1);
> +
> +	mutex_unlock(&pled->ctrl->lock);
> +}
> +
> +static struct led_trigger switch_port_rtl_hw_trigger = {
> +	.name = "realtek-switchport",
> +	.activate = switch_port_led_trigger_activate,
> +	.deactivate = switch_port_led_trigger_deactivate,
> +	.trigger_type = &switch_port_rtl_hw_trigger_type,
> +};
> +
> +static void realtek_port_led_read_address(struct device_node *np,
> +	int *port_index, int *led_index)
> +{
> +	const __be32 *addr;
> +
> +	addr = of_get_address(np, 0, NULL, NULL);
> +	if (addr) {
also here, i'm a bigger fan of failing on !addr (or returning with a 
dbg/warning) and avoid needless nesting
> +		*port_index = of_read_number(addr, 1);
> +		*led_index = of_read_number(addr + 1, 1);
> +	}
> +}
> +
> +static struct switch_port_led *switch_port_led_probe_single(
> +	struct switch_port_led_ctrl *ctrl, struct device_node *np)
> +{
> +	struct led_init_data init_data = {};
> +	struct switch_port_led *pled;
> +	unsigned int port_index;
> +	unsigned int led_index;
> +	int err;
> +
> +	realtek_port_led_read_address(np, &port_index, &led_index);
> +
> +	if (port_index >= ctrl->cfg->port_count || led_index >= ctrl->cfg->port_led_count)
> +		return ERR_PTR(-ENODEV);
> +
> +	pled = devm_kzalloc(ctrl->dev, sizeof(*pled), GFP_KERNEL);
> +	if (!pled)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	pled->ctrl = ctrl;
> +	pled->port = port_index;
> +	pled->index = led_index;
> +
> +	pled->led.max_brightness = 1;

+	pled->led.max_brightness = LED_ON;

> +	pled->led.brightness_set = switch_port_led_brightness_set;
> +	pled->led.brightness_get = switch_port_led_brightness_get;
> +	pled->led.blink_set = switch_port_led_blink_set;
> +	pled->led.trigger_type = &switch_port_rtl_hw_trigger_type;
> +
> +	ctrl->cfg->set_hw_managed(pled, false);
> +	ctrl->cfg->set_port_enabled(pled, true);
> +
> +	err = devm_led_classdev_register_ext(ctrl->dev, &pled->led, &init_data);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	err = devm_device_add_groups(pled->led.dev, rtl_hw_trigger_groups);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	dev_set_drvdata(pled->led.dev, pled);
> +
> +	return pled;
> +}
> +
> +static const struct switch_port_led_config rtl8380_port_led_config = {
> +	.port_count = 28,
> +	.port_led_count = 3,
> +	.group_count = 2,
> +	.modes = &rtl8380_port_led_modes,
> +	.port_led_init = rtl8380_port_led_init,
> +	.set_port_enabled = rtl8380_port_led_set_port_enabled,
> +	.set_hw_managed = rtl8380_port_led_set_hw_managed,
> +	.write_mode = rtl8380_port_led_write_mode,
> +	.read_mode = rtl8380_port_led_read_mode,
> +	.trigger_xlate = rtl8380_port_trigger_xlate,
> +	.map_group = rtl8380_port_led_map_group,
> +	.group_settings = {
> +		RTL8380_GROUP_SETTING(0, 0),
> +		RTL8380_GROUP_SETTING(0, 1),
> +		RTL8380_GROUP_SETTING(0, 2),
> +		RTL8380_GROUP_SETTING(1, 0),
> +		RTL8380_GROUP_SETTING(1, 1),
> +		RTL8380_GROUP_SETTING(1, 2),
> +	},
> +};
> +
> +static int realtek_port_led_probe(struct platform_device *pdev)
> +{
> +	struct switch_port_led_ctrl *ctrl;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np, *child;
> +	struct reg_field group_setting;
> +	unsigned int member_map_count;
> +	struct led_port_group *group;
> +	struct switch_port_led *pled;
> +	u32 leds_per_port = 0;
> +	int child_count;
> +	int err, i;
> +
> +	np = dev->of_node;
can we always trust the np or do we have to check for it? (input sanitation)
> +
> +	dev_info(dev, "probing port leds\n");
dev_dbg? if we have sensible data to put here (after we found stuff 
probably) a dev_info certainly is useful of course (or if the probing 
itself takes long, known when we started/finish is nice of course)
> +
> +//	if (!pdev->mfd_cell)
> +//		return dev_err_probe(dev, -ENODEV, "must be instantiated as MFD child\n");
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
curious, as `dev_err_probe` is new to me (it's been a while, but I like 
it) any reason not to dev_err_probe here for consistency?
> +
> +	mutex_init(&ctrl->lock);
> +
> +	ctrl->dev = dev;
> +	ctrl->cfg = of_device_get_match_data(dev);
> +	if (!ctrl->cfg)
> +		return dev_err_probe(dev, -ENODEV, "failed to find matching data\n");
> +
> +	ctrl->map = device_node_to_regmap(of_get_parent(np));
> +	if (IS_ERR_OR_NULL(ctrl->map))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->map), "failed to find parent regmap\n");
> +
> +	member_map_count = ctrl->cfg->port_led_count * ctrl->cfg->group_count;
port_member_led_count_map maybe to be slightly more descriptive?
> +	ctrl->groups = devm_kcalloc(dev, member_map_count,
> +		sizeof(*ctrl->groups), GFP_KERNEL);
> +	if (!ctrl->groups)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < member_map_count; i++) {
> +		group = &ctrl->groups[i];
> +		group_setting = ctrl->cfg->group_settings[i];
> +
> +		group->size = ctrl->cfg->port_count;
> +		group->setting = devm_regmap_field_alloc(dev, ctrl->map, group_setting);
> +		if (!group->setting)
> +			return -ENOMEM;
> +
> +		group->ports = devm_bitmap_zalloc(dev, ctrl->cfg->port_count, GFP_KERNEL);
> +		if (!group->ports)
> +			return -ENOMEM;
> +	}
> +
> +	err = devm_led_trigger_register(dev, &switch_port_rtl_hw_trigger);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to register private trigger");
> +
> +	child_count = of_get_child_count(np);
> +	dev_info(dev, "%d child nodes\n", child_count);
though this could be useful info in a post probe dev_info msg

+	dev_dbg(dev, "%d child nodes\n", child_count);

> +
> +	if (!child_count)
> +		return 0;
> +
> +	for_each_available_child_of_node(np, child) {
> +		if (of_n_addr_cells(child) != 2 || of_n_size_cells(child) != 0) {
> +			dev_err(dev, "#address-cells (%d) is not 2 or #size-cells (%d) is not 0\n",
> +				(u32) of_n_addr_cells(child), (u32) of_n_size_cells(child));
I often do this too; developer lazyness :) but for debugging reasons, 
it's MUCH nicer to have 2 individual statements here, as it reads far 
more pleasant. (Though you did already put the values in there;  In 
terms of language, is `not exactly` is what I'd use though.
> +			of_node_put(child);
> +			return -EINVAL;
> +		}
> +
> +		if (!of_node_name_prefix(child, "led")) {
> +			dev_dbg(dev, "skipping unsupported node %s\n", of_node_full_name(child));

quoting variables helps in identifying 'weird content' (empty strings, 
spaces etc) so doing it for consistency is usually good.

btw, I suppose the led framework doesn't have a nice getter? like the 
gpiod_optional etc getters? the prefixes/names are part of the gpio 
framework there (you can still supply a prefix optionally).

+			dev_dbg(dev, "skipping unsupported node '%s'\n", of_node_full_name(child));

> +			continue;
> +		}
> +
> +		pled = switch_port_led_probe_single(ctrl, child);
> +		if (IS_ERR(pled)) {
> +			dev_warn(dev, "failed to register led: %ld\n", PTR_ERR(pled));
> +			continue;
> +		}
> +
> +		if (pled->index + 1 > leds_per_port)
> +			leds_per_port = pled->index + 1;
> +
> +		if (leds_per_port > ctrl->cfg->port_led_count)
shouldn't this then be a `port_led_count_max`?
> +			return dev_err_probe(dev, -EINVAL,
> +				"too many LEDs per port: %d > %d\n",
> +				leds_per_port, ctrl->cfg->port_led_count);
> +	}
> +

it's capture the return value; so you can do something like

dev_info(dev, "probed X ports Y leds something pretty);
return ret; (we don't have a dev_info_probe do we :p)

> +	return ctrl->cfg->port_led_init(ctrl, leds_per_port);
> +}
> +
> +static const struct of_device_id of_switch_port_led_match[] = {
> +	{
> +		.compatible = "realtek,rtl8380-port-led",
the same on 8381 and 8382?
> +		.data = &rtl8380_port_led_config
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_switch_port_led_match);
> +
> +static struct platform_driver realtek_switch_port_led_driver = {
> +	.probe = realtek_port_led_probe,
> +	.driver = {
> +		.name = "realtek-switch-port-led",
> +		.of_match_table = of_switch_port_led_match
> +	}
> +};
> +module_platform_driver(realtek_switch_port_led_driver);
> +
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_DESCRIPTION("Realtek SoC switch port LED driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch
> new file mode 100644
> index 000000000000..e33737a4f0da
> --- /dev/null
> +++ b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-maple.patch
> @@ -0,0 +1,51 @@
> +From 0efa8d564f27519d20fed39f5d650e50cbc34a0b Mon Sep 17 00:00:00 2001
> +From: Sander Vanheule <sander@svanheule.net>
> +Date: Mon, 11 Jul 2022 21:37:17 +0200
> +Subject: [PATCH 02/14] pinctrl: add pinctrl for Realtek maple
> +
> +Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
> +RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
> +RTL838x has been tested, but functionality should be near identical.
> +
> +Since control bits for different muxes are scattered throughout the
> +switch core's register space, pin control features are provided by a
> +table of regmap fields. This should be flexible enough to allow
> +extension to other SoC generations providing similar features.
> +
> +Signed-off-by: Sander Vanheule <sander@svanheule.net>
> +---
> + drivers/pinctrl/Kconfig                  |  11 +
> + drivers/pinctrl/Makefile                 |   1 +
> + drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 +++++++++++++++++++++++
> + 3 files changed, 382 insertions(+)
> + create mode 100644 drivers/pinctrl/pinctrl-rtl-switchcore.c
> +
> +--- a/drivers/pinctrl/Kconfig
> ++++ b/drivers/pinctrl/Kconfig
> +@@ -215,6 +215,16 @@ config PINCTRL_ROCKCHIP
> + 	select MFD_SYSCON
> + 	select OF_GPIO
> +
> ++config PINCTRL_RTL_SWITCHCORE
> ++	tristate "Realtek switch core pinctrl driver"
> ++	depends on MFD_REALTEK_SWITCHCORE || COMPILE_TEST
> ++	select PINMUX
> ++	select GENERIC_PINMUX_FUNCTIONS
> ++	select MFD_SYSCON
> ++	help
> ++	  Driver for pin control fields in RTL83xx and RTL93xx managed ethernet
> ++	  switch SoCs.
> ++
> + config PINCTRL_SINGLE
> + 	tristate "One-register-per-pin type device tree based pinctrl driver"
> + 	depends on OF
> +--- a/drivers/pinctrl/Makefile
> ++++ b/drivers/pinctrl/Makefile
> +@@ -30,6 +30,7 @@ obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-
> + obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
> + obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
> + obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
> ++obj-$(CONFIG_PINCTRL_RTL_SWITCHCORE) += pinctrl-rtl-switchcore.o
> + obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
> + obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
> + obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
> diff --git a/target/linux/realtek/rtl838x/config-5.10 b/target/linux/realtek/rtl838x/config-5.10
> index 065948532057..5c69832be41d 100644
> --- a/target/linux/realtek/rtl838x/config-5.10
> +++ b/target/linux/realtek/rtl838x/config-5.10
> @@ -99,6 +99,7 @@ CONFIG_IRQ_MIPS_CPU=y
>   CONFIG_IRQ_WORK=y
>   CONFIG_JFFS2_ZLIB=y
>   CONFIG_LEDS_GPIO=y
> +CONFIG_LEDS_RTL_SWITCH_PORT=y
>   CONFIG_LEGACY_PTYS=y
>   CONFIG_LEGACY_PTY_COUNT=256
>   CONFIG_LIBFDT=y
>

On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
> On 16-07-2022 21:09, Sander Vanheule wrote:
> > Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
> > RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
> > RTL838x has been tested, but functionality should be near identical.
> > 
> > Since control bits for different muxes are scattered throughout the
> > switch core's register space, pin control features are provided by a
> > table of regmap fields. This should be flexible enough to allow
> > extension to other SoC generations providing similar features.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> >   .../drivers/leds/leds-rtl-switch-port.c       | 668 ++++++++++++++++++
> >   ...inctrl-add-pinctrl-for-Realtek-maple.patch |  51 ++
> >   target/linux/realtek/rtl838x/config-5.10      |   1 +
> >   3 files changed, 720 insertions(+)
> >   create mode 100644 target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-
> > port.c
> >   create mode 100644 target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-
> > Realtek-maple.patch
> > 
> > diff --git a/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
> > b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
> > new file mode 100644
> > index 000000000000..76dfede7ba15
> > --- /dev/null
> > +++ b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
> > @@ -0,0 +1,668 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> Probably want bitops.h for BIT()
> 
> +#include <linux/bitops.h>
> 

Will add.

> > +#include <linux/leds.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +/*
> > + * Realtek switch port LED
> > + *
> > + * The switch ASIC can control up to three LEDs per phy, based on a number of
> > + * matching conditions. Alternatively, each individual LED output can also be
> > + * configured for manual control.
> > + */
> 
> Is this an internal define? `REALTEK_PORT_LED_TRIGGER_NONE` (etc) maybe 
> better? (or LEDTRIG if it really must be shorter)
> 
> It wasn't until much later that I figured out that this was the 
> abbreviation for port trigger *doh*

I got tired of typing so much by prefixing everything with REALTEK_... This is in fact an
internal definition, used only for the private trigger.

The translation between DT trigger flags and the actual HW trigger settings may look like
an unnecessary complication here, but I've done this with broader compatibility in mind.
On RTL93xx, the HW trigger also uses (different) bit flags [1], instead of an enumeration
of triggers [2]. Other hardware, from other vendors, would also use a different aproach,
but there is currently no framework yet to offload netdev triggers.

[1] https://svanheule.net/realtek/longan/register/led_set0_0_ctrl
[2] https://svanheule.net/realtek/maple/register/led_mode_ctrl

> 
> > +#define PTRG_NONE              0
> > +#define PTRG_ACT_RX            BIT(0)
> > +#define PTRG_ACT_TX            BIT(1)
> > +#define PTRG_ACT               PTRG_ACT_RX | PTRG_ACT_TX
> > +#define PTRG_LINK_10           BIT(2)
> > +#define PTRG_LINK_100          BIT(3)
> > +#define PTRG_LINK_1000         BIT(4)

These flags should actually go into a header somewhere, but this was still development
code.

> > +
> > +struct led_port_blink_mode {
> > +       u16 interval; /* Toggle interval in ms */
> > +       u8 mode; /* ASIC mode bits */
> > +};
> > +
> > +#define REALTEK_PORT_LED_BLINK_COUNT   6
> > +struct led_port_modes {
> realtek_led_port_modes?
> > +       u8 off;
> > +       u8 on;
> > +       struct led_port_blink_mode blink[REALTEK_PORT_LED_BLINK_COUNT];
> 
> ah here's the other list. So would it make sense to re-use this same 
> structure for the sys-led, or is that over-optimizing things ...

Like I noted on the other patch, it could make sense, yes.

> 
> Why though are on/off and the blink entries separated?  Isn't 'off' a 
> interval of 'inviite (max-int)' and on a blink rate of 0? While you may 
> have to do some logic later, it keeps the interface more consistent? 
> idk; just some food for thought.

Because Realtek wouldn't be the same company if everything made sense. The value for "off"
is actually 5. 6 and 7 are just more blink intervals [3]. I guess they decided to add more
rates, but didn't want to break backwards compatibilty somewhere.

[3] https://svanheule.net/realtek/maple/register/led_sw_p_ctrl

> 
> > +};
> > +
> > +struct led_port_group {
> > +       struct regmap_field *setting;
> > +       unsigned int size;
> > +       /* bitmap to keep track of associated ports */
> Can you explain a bit what's supposed to be here? right now its just an 
> it pointer, but appearantly it will point at some bitmap? can we not 
> make a struct representing this bitmap and then use that instead?

This is how bitmaps are defined in the kernel. cpumask structs do wrap their bitmap in a
struct, but I see little point in adding an extra layer here.

> > +       unsigned long *ports;
> > +};
> > +
> > +struct switch_port_led_config;
> 
> can we do this forward declaration the other way around? You forward 
> declare it here because need the TYPE for the cfg pointer.
> 
> Later however, you need it to indicate the argument of a function 
> pointer. I think the type declaration ways heavier and makes it thus 
> 'nicer' to do it the other way around. though that does indeed mean you 
> have to do 2 forard declarations for your two structs, but would still 
> be cleaner imo

Forward declaring the ctrl struct would also be an option. I'll play with it a bit, see
what makes most sense (e.g. smallest HW to biggest HW unit control struct).

> > +
> > +struct switch_port_led_ctrl {
> > +       struct device *dev;
> > +       struct regmap *map;
> > +       const struct switch_port_led_config *cfg;
> > +       struct mutex lock;
> > +       struct led_port_group *groups;
> > +};
> > +
> > +struct switch_port_led {
> > +       struct led_classdev led;
> > +       struct switch_port_led_ctrl *ctrl;
> > +       unsigned int port;
> > +       unsigned int index;
> > +       u32 trigger_flags;
> > +       struct led_port_group *current_group;
> > +};
> > +
> > +struct switch_port_led_config {
> > +       unsigned int port_count;
> > +       unsigned int port_led_count;
> > +       unsigned int group_count;
> > +       const struct led_port_modes *modes;
> > +       /* Set the number of LEDs per port */
> > +       int (*port_led_init)(struct switch_port_led_ctrl *ctrl,
> > +               unsigned int led_count);
> what is the significance for both these comments (above and below)? the 
> comment states to set the leds per port; but the function is called 
> led_init? Below the function is called set enabled, so that's pretty 
> obvious that you set the port enabled switch port led?

Left-over from development and incomplete understanding of how things work.
port_led_init() is called to finish the peripheral initialisation. set_port_enabled() is
called to enable the LEDs on a single port. While this concep applies cleanly to RTL838x,
it's a bit harder to force RTL839x into this scheme. I'll look at improving the naming of
these ops, insofar as I haven't done that yet.

> > +       /* Enable or disable all LEDs for a certain port */
> > +       int (*set_port_enabled)(struct switch_port_led *led, bool enabled);
> > +       int (*set_hw_managed)(struct switch_port_led *led, bool hw_managed);
> > +       int (*write_mode)(struct switch_port_led *led, unsigned int mode);
> > +       int (*read_mode)(struct switch_port_led *led);
> > +       int (*trigger_xlate)(u32 trigger);
> > +       struct led_port_group *(*map_group)(struct switch_port_led *led, u32 trigger);
> > +       struct reg_field group_settings[];
> > +};
> > +
> > +static struct led_port_group *switch_port_led_get_group(
> > +       struct switch_port_led *pled, unsigned int group)
> > +{
> > +       return &pled->ctrl->groups[group * pled->ctrl->cfg->port_led_count + pled-
> > >index];
> also in this patch; don't fear parenthesis :) they are our friend ;)

I'm a physicist. Maybe programming operator precedence wasn't deeply ingrained into my
brain at school, mathematical operators certainly were ;)

> > +}
> > +
> > +/*
> > + * SoC specific implementation for RTL8380 series (Maple)
> Maybe 'x' the 0 here?
> 
> + * SoC specific implementation for RTL838x series (Maple)
> 

Sure, why not.

> > + */
> > +#define RTL8380_REG_LED_MODE_SEL               0x1004
> do we have any idea what the SDK names these registers? Usually they 
> match the (secret) datasheet ...

Normally I just copied the names from the SDK [4]. Be happy I haven't implemented the
DMY_REG5 register yet. :)

[4] https://svanheule.net/realtek/maple/feature/led

> > +#define RTL8380_REG_LED_GLB_CTRL               0xa000
> > +#define RTL8380_REG_LED_MODE_CTRL              0xa004
> > +#define RTL8380_REG_LED_P_EN_CTRL              0xa008
> > +#define RTL8380_REG_LED_SW_P_EN_CTRL(led)      (0xa010 + 4 * (led))
> > +#define RTL8380_REG_LED_SW_CTRL(port)          (0xa01c + 4 * (port))
> > +
> > +#define RTL8380_PORT_LED_COUNT                         3
> > +#define RTL8380_GROUP_SETTING_WIDTH                    5
> > +#define RTL8380_GROUP_SETTING_OFFSET(_grp, _idx)       \
> > +       (RTL8380_GROUP_SETTING_WIDTH * ((_idx) + RTL8380_PORT_LED_COUNT * (_grp)))
> > +#define RTL8380_GROUP_SETTING(_grp, _idx)      {                               \
> > +               .reg = RTL8380_REG_LED_MODE_CTRL,                               \
> > +               .lsb = RTL8380_GROUP_SETTING_OFFSET(_grp, _idx),                \
> > +               .msb = RTL8380_GROUP_SETTING_OFFSET(_grp, (_idx) + 1) - 1,      \
> > +       }
> > +
> > +enum rtl83xx_port_trigger {
> > +       RTL83XX_TRIG_LINK_ACT = 0,
> > +       RTL83XX_TRIG_LINK = 1,
> > +       RTL83XX_TRIG_ACT = 2,
> > +       RTL83XX_TRIG_ACT_RX = 3,
> > +       RTL83XX_TRIG_ACT_TX = 4,
> > +       RTL83XX_TRIG_LINK_1G = 7,
> > +       RTL83XX_TRIG_LINK_100M = 8,
> > +       RTL83XX_TRIG_LINK_10M = 9,
> > +       RTL83XX_TRIG_LINK_ACT_1G = 10,
> > +       RTL83XX_TRIG_LINK_ACT_100M = 11,
> > +       RTL83XX_TRIG_LINK_ACT_10M = 12,
> > +       RTL83XX_TRIG_LINK_ACT_1G_100M = 13,
> > +       RTL83XX_TRIG_LINK_ACT_1G_10M = 14,
> > +       RTL83XX_TRIG_LINK_ACT_100M_10M = 15,
> > +       RTL83XX_TRIG_DISABLED = 31,
> > +};
> as you sometimes do right align, this enum is a perfect candidate I'd 
> think to do so as well?

Would make it look more like a table, yes. Any idea if it is conventional to align the
assignments in kernel code?

> > +
> > +static const struct led_port_modes rtl8380_port_led_modes = {
> > +       .off = 0,
> > +       .on = 5,
> > +       .blink  = {{  32, 1},
> I know it's just stupid style, but why not put the first entry on the 
> next line with one indentation level? I think that make it looks a 
> little more natural

But saving previous lines! Could do that, yes.

> > +                  {  64, 2},
> > +                  { 128, 3},
> 
> I think this enum here deserves a comment as to why this order is so 
> funky. Also to prevent someone passing along later and trying to 'fix' it :)

Something along the lines of the following?

/*
 * Yes, this table is correct and was probably amended during development.
 * Don't try to fix it.
 */

> 
> > +                  { 256, 6},
> > +                  { 512, 4},
> > +                  {1024, 7}}
> > +};
> > +
> > +static const struct led_port_modes rtl8390_port_led_modes = {
> > +       .off = 0,
> > +       .on = 7,
> > +       .blink = {{  32, 1},
> > +                 {  64, 2},
> > +                 { 128, 3},
> > +                 { 256, 4},
> > +                 { 512, 5},
> > +                 {1024, 6}}

See, this one makes more sense :)

> > +};
> > +
> > +static int rtl8380_port_trigger_xlate(u32 port_led_trigger)
> > +{
> > +       switch (port_led_trigger) {
> > +       case PTRG_NONE:
> > +               return RTL83XX_TRIG_DISABLED;
> > +       case PTRG_ACT_RX:
> > +               return RTL83XX_TRIG_ACT_RX;
> > +       case PTRG_ACT_TX:
> > +               return RTL83XX_TRIG_ACT_TX;
> > +       case PTRG_ACT:
> > +               return RTL83XX_TRIG_ACT;
> > +       case PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK;
> > +       case PTRG_LINK_10:
> > +               return RTL83XX_TRIG_LINK_10M;
> > +       case PTRG_LINK_100:
> > +               return RTL83XX_TRIG_LINK_100M;
> > +       case PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK_1G;
> > +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK_ACT;
> > +       case PTRG_ACT | PTRG_LINK_10:
> > +               return RTL83XX_TRIG_LINK_ACT_10M;
> > +       case PTRG_ACT | PTRG_LINK_100:
> > +               return RTL83XX_TRIG_LINK_ACT_100M;
> > +       case PTRG_ACT | PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK_ACT_1G;
> > +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100:
> > +               return RTL83XX_TRIG_LINK_ACT_100M_10M;
> > +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK_ACT_1G_10M;
> > +       case PTRG_ACT | PTRG_LINK_100 | PTRG_LINK_1000:
> > +               return RTL83XX_TRIG_LINK_ACT_1G_100M;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> > +/*
> > + * Find the group the LED with this trigger setting can be assigned to, or
> 
> afaik you can use '@trigger' which then relates to the argument. I had 
> to read this line 3 times to realize that 'this' related to the trigger'
> 
> Also, what are you 'finding', i'm not seeing a loop or anything, where 
> are we finding?
> 
> 'Map switch port @led to map to a trigger @trigger'? idk, but I'm not 
> sure what is happening here yet.
> 
> btw, you forgot to add documentation for your arguments ;)

Ok, I'll clean up the comments.

The reason this looks weird, is because the led_map_group() functions are entirely natural
for RTL839x and later, where LEDs can be assigned to different "LED sets" [5]. I need
something that works for all generations, so I'm forcing the RTL838x implementation into
these two static, somewhat artificial groups ("sets").

[5] https://svanheule.net/realtek/cypress/feature/led

> 
> > + * initialise a new empty group.
> > + *
> > + * Return group number on success, or < 0 on failure.
> > + * */
> > +static struct led_port_group *rtl8380_port_led_map_group(struct switch_port_led *led,
> > u32 trigger)
> > +{
> > +       int rtl_trigger = rtl8380_port_trigger_xlate(trigger);
> > +       struct switch_port_led_ctrl *ctrl = led->ctrl;
> > +       struct led_port_group *group;
> > +       u32 current_trigger;
> > +       int err;
> > +
> > +       if (rtl_trigger < 0)
> > +              return ERR_PTR(rtl_trigger);
> > +
> > +       /*
> > +        * Static groups:
> does this deserve some explanation? why are there 2 groups? how are they 
> grouped? why are the grouped the way they are grouped? that could also 
> help you define a name for the magic value 23 potentially?

Ports 0-24 can be provided by three 8-port phy-s, each using a pair of QSGMII connections.
Ports 24-27 can come from a 4-port phy using a single QSGMII connection. No idea why
Realtek made this division though, since they also allow combo ports on ports 20-23...

> > +        *   - group 0: ports 0-23
> > +        *   - group 1: ports 24-27
> > +        */
> > +       if (led->port > 23)
> > +               group = switch_port_led_get_group(led, 1);
> > +       else
> > +               group = switch_port_led_get_group(led, 0);
> > +
> > +       err = regmap_field_read(group->setting, &current_trigger);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       if (current_trigger != rtl_trigger && !bitmap_empty(group->ports, group-
> > >size)) {
> > +               dev_warn(ctrl->dev, "cannot add (%d,%d) to group: 0x%02x != 0x%02x\n",
> > +                       led->port, led->index, current_trigger, rtl_trigger);
> > +               return ERR_PTR(-ENOSPC);
> > +       }
> > +
> > +       return group;
> > +}
> > +
> > +static int rtl8380_port_led_set_port_enabled(struct switch_port_led *led, bool
> > enabled)
> > +{
> > +       int reg = RTL8380_REG_LED_P_EN_CTRL;
> > +       int val = enabled ? BIT(led->port) : 0;
> > +
> > +       return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
> > +}
> > +
> > +static int rtl8380_port_led_set_hw_managed(struct switch_port_led *led, bool
> > hw_managed)
> > +{
> > +       int reg = RTL8380_REG_LED_SW_P_EN_CTRL(led->index);
> > +       int val = hw_managed ? 0 : BIT(led->port);
> > +
> > +       return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
> > +}
> > +
> > +static int rtl8380_port_led_write_mode(struct switch_port_led *led, unsigned int
> > mode)
> > +{
> > +       int reg = RTL8380_REG_LED_SW_CTRL(led->port);
> > +       int offset = 3 * led->index;
> I think this 3 (which is reused later also) could have a define 
> (REALTEK_something_LED_PER_something) or something (and maybe even an 
> accompanying _MASK later I find oyu have `
> 
> port_led_count
> 
> ` so you probably could use that?

True, a #define would make sense.

> > +       u32 mask = GENMASK(2, 0) << offset;
> > +       u32 value = mode << offset;
> > +
> > +       return regmap_update_bits(led->ctrl->map, reg, mask, value);
> > +}
> > +
> > +static int rtl8380_port_led_read_mode(struct switch_port_led *led)
> > +{
> > +       u32 val;
> > +       int ret;
> > +
> > +       ret = regmap_read(led->ctrl->map, RTL8380_REG_LED_SW_CTRL(led->port), &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return (val >> (3 * led->index)) & GENMASK(2, 0);
> > +}
> > +
> > +static int rtl8380_port_led_init(struct switch_port_led_ctrl *ctrl, unsigned int
> > led_count)
> > +{
> > +       u32 led_count_mask = GENMASK(led_count - 1, 0);
> > +       u32 led_mask = GENMASK(5, 0);
> > +
> > +       return regmap_update_bits(ctrl->map, RTL8380_REG_LED_GLB_CTRL, led_mask,
> > +                       (led_count_mask << 3) | led_count_mask);
> > +}
> > +
> > +static void switch_port_led_brightness_set(struct led_classdev *led_cdev,
> > +       enum led_brightness brightness)
> > +{
> > +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
> > led);
> > +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
> > +
> > +       ctrl->cfg->write_mode(pled, brightness ? ctrl->cfg->modes->on : ctrl->cfg-
> > >modes->off);
> > +}
> > +
> > +static enum led_brightness switch_port_led_brightness_get(struct led_classdev
> > *led_cdev)
> > +{
> > +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
> > led);
> > +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
> > +
> > +       return ctrl->cfg->read_mode(pled) != ctrl->cfg->modes->off;
> > +}
> > +
> > +static int switch_port_led_blink_set(struct led_classdev *led_cdev,
> > +       unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
> > led);
> > +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
> > +
> > +       const struct led_port_blink_mode *blink = &ctrl->cfg->modes->blink[0];
> > +       unsigned long interval_ms = *delay_on + *delay_off;
> > +       unsigned int i = 0;
> > +
> > +       if (interval_ms && *delay_on == 0)
> > +               return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->off);
> > +
> > +       if (interval_ms && *delay_off == 0)
> > +               return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->on);
> > +
> > +       if (!interval_ms)
> You are explicit on the delay, why not on the interval?
> 
> +       if (interval_ms == 0)
> 

Sure, will change.

> > +               interval_ms = 500;
> > +
> > +       /*
> > +        * Since the modes always double in interval, the geometric mean of
> > +        * intervals [i] and [i + 1] is equal to sqrt(2) * interval[i]
> > +        */
> > +       while (i < (REALTEK_PORT_LED_BLINK_COUNT - 1) &&
> > +               interval_ms > (2 * 141 * blink[i].interval) / 100)
> > +               i++;
> > +
> > +       *delay_on = blink[i].interval;
> > +       *delay_off = blink[i].interval;
> > +
> > +       return ctrl->cfg->write_mode(pled, blink[i].mode);
> > +}
> > +
> > +static struct led_hw_trigger_type switch_port_rtl_hw_trigger_type;
> > +
> > +static ssize_t rtl_hw_trigger_show(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > +{
> > +       struct switch_port_led *pled = dev_get_drvdata(dev);
> > +
> > +       return sprintf(buf, "%x\n", pled->trigger_flags);
> > +}
> > +
> > +static ssize_t rtl_hw_trigger_store(struct device *dev, struct device_attribute
> > *attr,
> > +               const char *buf, size_t count)
> > +{
> > +       struct switch_port_led *pled = dev_get_drvdata(dev);
> > +       struct led_port_group *group, *new_group;
> from a namespace pov; why are you not putting these variables inside 
> your if statement and scope them there? however ...
> > +       unsigned int member_count;
> > +       int trigger;
> > +       int nchars;
> > +       int value;
> > +       int err;
> > +
> > +       if (sscanf(buf, "%x%n", &value, &nchars) != 1 || nchars + 1 < count)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&pled->ctrl->lock);
> > +
> > +       /*
> > +        * If the private trigger is already active:
> > +        *   - modify the current group if we are the only member,
> > +        *   - or join a new group if one is available
> > +        */
> > +       if (pled->current_group) {
> 
> you are doing everything within this if; why not instead? fail early 
> fail often
> 
> +       if (!pled->current_group) {
> +               dev_dbg(dev, "no led not part of any group\n");
> +               err = -Esomething;
> +               goto out;
> +       };

pled->current_group is only set when the private trigger is activated. It is not an error
to set the trigger mask when another trigger is selected, and this is intentional. The
reason I kept the bulk of the code inside the if(){} is because I found it more indicative
of the optional nature of this code.

If the trigger flags could only be set after you've enabled the offloading trigger, the
driver would be unable to enable any but the first LED, since we wouldn't have any groups
left to reassign the LEDs to (remember the static groups).

Again, this probably makes more sense on RTL839x, and I have to refactor this part of the
code a bit. I'm currently duplicating the join group/leave group logic in two places,
which makes it tricky to modify it later.


> 
> > +               group = pled->current_group;
> > +
> > +               trigger = pled->ctrl->cfg->trigger_xlate(value);
> > +               if (trigger < 0) {
> > +                       err = trigger;
> > +                       goto err_out;
> > +               }
> > +
> > +               member_count = bitmap_weight(group->ports, group->size);
> > +
> > +               if (member_count == 1) {
> > +                       err = regmap_field_write(group->setting, trigger);
> > +                       if (err)
> > +                               goto err_out;
> > +
> > +                       goto out;
> > +               }
> > +
> > +               new_group = pled->ctrl->cfg->map_group(pled, value);
> > +               if (IS_ERR(new_group)) {
> > +                       err = PTR_ERR(new_group);
> > +                       goto err_out;
> > +               }
> > +
> > +               if (member_count == 0) {
> > +                       err = regmap_field_write(new_group->setting, trigger);
> > +                       if (err)
> > +                               goto err_out;
> > +               }
> > +       
> > +               bitmap_clear(group->ports, pled->port, 1);
> > +               bitmap_set(new_group->ports, pled->port, 1);
> > +               pled->current_group = new_group;
> > +       }
> > +
> > +out:
> > +       pled->trigger_flags = value;
> > +
> > +err_out:
> > +       mutex_unlock(&pled->ctrl->lock);
> > +
> > +       if (err)
> > +               return err;
> > +
> > +       return count;
> > +}
> > +static DEVICE_ATTR_RW(rtl_hw_trigger);
> > +
> > +static struct attribute *rtl_hw_trigger_attrs[] = {
> > +       &dev_attr_rtl_hw_trigger.attr,
> > +       NULL,
> > +};
> > +ATTRIBUTE_GROUPS(rtl_hw_trigger);
> Is there any possibility this can be done in a generic way at all? I 
> know this doesn't exist, and it's a very good first step ... but these 
> are netdev_switch_triggers maybe? Or would that not work because our 
> triggers are TOO RTL specific? Could we make them less specific (in the 
> future)?

This private trigger is sort of an exercise in netdev trigger offloading. There was a 
discussion earlier about a generic netdev offloading interface on some qca8k patches, but
that didn't materialise into something useful yet AFAIK.

> > +
> > +static int switch_port_led_trigger_activate(struct led_classdev *led_cdev)
> > +{
> > +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
> > led);
> > +       struct led_port_group *group;
> > +       int rtl_trigger;
> > +       int err = 0;
> > +
> > +       mutex_lock(&pled->ctrl->lock);
> > +
> > +       rtl_trigger = pled->ctrl->cfg->trigger_xlate(pled->trigger_flags);
> > +       if (rtl_trigger < 0) {
> > +               err = rtl_trigger;
> > +               goto out;
> > +       }
> > +
> > +       group = pled->ctrl->cfg->map_group(pled, pled->trigger_flags);
> > +       if (IS_ERR(group)) {
> > +               err = PTR_ERR(group);
> > +               goto out;
> > +       }
> > +
> > +       if (bitmap_empty(group->ports, group->size)) {
> > +               err = regmap_field_write(group->setting, rtl_trigger);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +       bitmap_set(group->ports, pled->port, 1);
> would it be useful to somehow codify the length? MACRO, sizeof, 
> pled->port_size something?

A macro could help, but I don't think I understand how the other would work? This just
indicates that 1 extra port was added to the selected group, so there is no sizeof() we
can use.

> > +       pled->current_group = group;
> > +
> > +       err = pled->ctrl->cfg->set_hw_managed(pled, true);
> 
> if this where some generic function that would need to come from the 
> ops-struct, I'd get it; but why not call the function directly? Otherwise:
> 
> +       if (pled->ctrl->cfg->set_hw_managed)
> +               err = pled->ctrl->cfg->set_hw_managed(pled, true);

The set_hw_managed() op is not optional. In fact, I've reversed the logic as implemented
in silicon, since that defaults to HW controlled and has to be set to SW controlled.

> 
> > +
> > +out:
> > +       mutex_unlock(&pled->ctrl->lock);
> personally, I like to check for nullness on these things, but I don't 
> know what is most commonely done here ... or if mutex_unlock does its 
> own nice checks.

If lock is NULL, that's driver error, not a runtime error. Should never happen after
succesful device probing.

> > +
> > +       return err;
> > +}
> > +
> > +static void switch_port_led_trigger_deactivate(struct led_classdev *led_cdev)
> > +{
> > +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
> > led);
> > +       struct led_port_group *group;
> > +
> > +       mutex_lock(&pled->ctrl->lock);
> > +
> > +       pled->ctrl->cfg->set_hw_managed(pled, false);
> as above
> > +
> > +       group = pled->current_group;
> what's the purpose of this assignment? you make a copy, then set it to 
> null; then clear the bitmap. Can you not just first clear the bitmap, 
> then null the assignment? Or is there some hidden race-condition you are 
> protecting against in this way? If so, better leave a comment to 
> indicate this potential race-condition if we can't protect this via 
> code; though I thought that's what the mutex was for to begin with ... 
> maybe add an 'if (pled->current_group == NULL) return check before we 
> even try to get the mutex?

I added the mutex after trying to be smart by writing the code this way. But with the
mutex, I can probably just clear the bit first and then clear the pointer. If the trigger
was succesfully activated, ->current_group cannot be NULL.

> > +       pled->current_group = NULL;
> > +       bitmap_clear(group->ports, pled->port, 1);
> > +
> > +       mutex_unlock(&pled->ctrl->lock);
> > +}
> > +
> > +static struct led_trigger switch_port_rtl_hw_trigger = {
> > +       .name = "realtek-switchport",
> > +       .activate = switch_port_led_trigger_activate,
> > +       .deactivate = switch_port_led_trigger_deactivate,
> > +       .trigger_type = &switch_port_rtl_hw_trigger_type,
> > +};
> > +
> > +static void realtek_port_led_read_address(struct device_node *np,
> > +       int *port_index, int *led_index)
> > +{
> > +       const __be32 *addr;
> > +
> > +       addr = of_get_address(np, 0, NULL, NULL);
> > +       if (addr) {
> also here, i'm a bigger fan of failing on !addr (or returning with a 
> dbg/warning) and avoid needless nesting

Should probably fail when we can't find the address. Otherwise this would default to
[something], and that just doens't make any sense.

> > +               *port_index = of_read_number(addr, 1);
> > +               *led_index = of_read_number(addr + 1, 1);
> > +       }
> > +}
> > +
> > +static struct switch_port_led *switch_port_led_probe_single(
> > +       struct switch_port_led_ctrl *ctrl, struct device_node *np)
> > +{
> > +       struct led_init_data init_data = {};
> > +       struct switch_port_led *pled;
> > +       unsigned int port_index;
> > +       unsigned int led_index;
> > +       int err;
> > +
> > +       realtek_port_led_read_address(np, &port_index, &led_index);
> > +
> > +       if (port_index >= ctrl->cfg->port_count || led_index >= ctrl->cfg-
> > >port_led_count)
> > +               return ERR_PTR(-ENODEV);
> > +
> > +       pled = devm_kzalloc(ctrl->dev, sizeof(*pled), GFP_KERNEL);
> > +       if (!pled)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       init_data.fwnode = of_fwnode_handle(np);
> > +
> > +       pled->ctrl = ctrl;
> > +       pled->port = port_index;
> > +       pled->index = led_index;
> > +
> > +       pled->led.max_brightness = 1;
> 
> +       pled->led.max_brightness = LED_ON;

Ah, but LED_ON is deprecated! ;)

> 
> > +       pled->led.brightness_set = switch_port_led_brightness_set;
> > +       pled->led.brightness_get = switch_port_led_brightness_get;
> > +       pled->led.blink_set = switch_port_led_blink_set;
> > +       pled->led.trigger_type = &switch_port_rtl_hw_trigger_type;
> > +
> > +       ctrl->cfg->set_hw_managed(pled, false);
> > +       ctrl->cfg->set_port_enabled(pled, true);
> > +
> > +       err = devm_led_classdev_register_ext(ctrl->dev, &pled->led, &init_data);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       err = devm_device_add_groups(pled->led.dev, rtl_hw_trigger_groups);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       dev_set_drvdata(pled->led.dev, pled);
> > +
> > +       return pled;
> > +}
> > +
> > +static const struct switch_port_led_config rtl8380_port_led_config = {
> > +       .port_count = 28,
> > +       .port_led_count = 3,
> > +       .group_count = 2,
> > +       .modes = &rtl8380_port_led_modes,
> > +       .port_led_init = rtl8380_port_led_init,
> > +       .set_port_enabled = rtl8380_port_led_set_port_enabled,
> > +       .set_hw_managed = rtl8380_port_led_set_hw_managed,
> > +       .write_mode = rtl8380_port_led_write_mode,
> > +       .read_mode = rtl8380_port_led_read_mode,
> > +       .trigger_xlate = rtl8380_port_trigger_xlate,
> > +       .map_group = rtl8380_port_led_map_group,
> > +       .group_settings = {
> > +               RTL8380_GROUP_SETTING(0, 0),
> > +               RTL8380_GROUP_SETTING(0, 1),
> > +               RTL8380_GROUP_SETTING(0, 2),
> > +               RTL8380_GROUP_SETTING(1, 0),
> > +               RTL8380_GROUP_SETTING(1, 1),
> > +               RTL8380_GROUP_SETTING(1, 2),
> > +       },
> > +};
> > +
> > +static int realtek_port_led_probe(struct platform_device *pdev)
> > +{
> > +       struct switch_port_led_ctrl *ctrl;
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np, *child;
> > +       struct reg_field group_setting;
> > +       unsigned int member_map_count;
> > +       struct led_port_group *group;
> > +       struct switch_port_led *pled;
> > +       u32 leds_per_port = 0;
> > +       int child_count;
> > +       int err, i;
> > +
> > +       np = dev->of_node;
> can we always trust the np or do we have to check for it? (input sanitation)
> > +
> > +       dev_info(dev, "probing port leds\n");
> dev_dbg? if we have sensible data to put here (after we found stuff 
> probably) a dev_info certainly is useful of course (or if the probing 
> itself takes long, known when we started/finish is nice of course)

Development print. Will remove.

> > +
> > +//     if (!pdev->mfd_cell)
> > +//             return dev_err_probe(dev, -ENODEV, "must be instantiated as MFD
> > child\n");
> > +
> > +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> > +       if (!ctrl)
> > +               return -ENOMEM;
> curious, as `dev_err_probe` is new to me (it's been a while, but I like 
> it) any reason not to dev_err_probe here for consistency?

Apparently the alloc() functions are verbose by themselves, and don't need any other
output (or so I've seen reviewers say).

> > +
> > +       mutex_init(&ctrl->lock);
> > +
> > +       ctrl->dev = dev;
> > +       ctrl->cfg = of_device_get_match_data(dev);
> > +       if (!ctrl->cfg)
> > +               return dev_err_probe(dev, -ENODEV, "failed to find matching data\n");
> > +
> > +       ctrl->map = device_node_to_regmap(of_get_parent(np));
> > +       if (IS_ERR_OR_NULL(ctrl->map))
> > +               return dev_err_probe(dev, PTR_ERR(ctrl->map), "failed to find parent
> > regmap\n");
> > +
> > +       member_map_count = ctrl->cfg->port_led_count * ctrl->cfg->group_count;
> port_member_led_count_map maybe to be slightly more descriptive?

Maybe I've changed this already. If not, I'll give it some thought.

> > +       ctrl->groups = devm_kcalloc(dev, member_map_count,
> > +               sizeof(*ctrl->groups), GFP_KERNEL);
> > +       if (!ctrl->groups)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < member_map_count; i++) {
> > +               group = &ctrl->groups[i];
> > +               group_setting = ctrl->cfg->group_settings[i];
> > +
> > +               group->size = ctrl->cfg->port_count;
> > +               group->setting = devm_regmap_field_alloc(dev, ctrl->map,
> > group_setting);
> > +               if (!group->setting)
> > +                       return -ENOMEM;
> > +
> > +               group->ports = devm_bitmap_zalloc(dev, ctrl->cfg->port_count,
> > GFP_KERNEL);
> > +               if (!group->ports)
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       err = devm_led_trigger_register(dev, &switch_port_rtl_hw_trigger);
> > +       if (err)
> > +               return dev_err_probe(dev, err, "failed to register private trigger");
> > +
> > +       child_count = of_get_child_count(np);
> > +       dev_info(dev, "%d child nodes\n", child_count);
> though this could be useful info in a post probe dev_info msg
> 
> +       dev_dbg(dev, "%d child nodes\n", child_count);
> 

Definitely dev_dbg() at most, or just nothing at all in the final version.

> > +
> > +       if (!child_count)
> > +               return 0;
> > +
> > +       for_each_available_child_of_node(np, child) {
> > +               if (of_n_addr_cells(child) != 2 || of_n_size_cells(child) != 0) {
> > +                       dev_err(dev, "#address-cells (%d) is not 2 or #size-cells (%d)
> > is not 0\n",
> > +                               (u32) of_n_addr_cells(child), (u32)
> > of_n_size_cells(child));
> I often do this too; developer lazyness :) but for debugging reasons, 
> it's MUCH nicer to have 2 individual statements here, as it reads far 
> more pleasant. (Though you did already put the values in there;  In 
> terms of language, is `not exactly` is what I'd use though.
> > +                       of_node_put(child);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               if (!of_node_name_prefix(child, "led")) {
> > +                       dev_dbg(dev, "skipping unsupported node %s\n",
> > of_node_full_name(child));
> 
> quoting variables helps in identifying 'weird content' (empty strings, 
> spaces etc) so doing it for consistency is usually good.
> 
> btw, I suppose the led framework doesn't have a nice getter? like the 
> gpiod_optional etc getters? the prefixes/names are part of the gpio 
> framework there (you can still supply a prefix optionally).

They do require names to start with "led" in the binding, so maybe they do. I'll
investigate.

> 
> +                       dev_dbg(dev, "skipping unsupported node '%s'\n",
> of_node_full_name(child));
> 
> > +                       continue;
> > +               }
> > +
> > +               pled = switch_port_led_probe_single(ctrl, child);
> > +               if (IS_ERR(pled)) {
> > +                       dev_warn(dev, "failed to register led: %ld\n", PTR_ERR(pled));
> > +                       continue;
> > +               }
> > +
> > +               if (pled->index + 1 > leds_per_port)
> > +                       leds_per_port = pled->index + 1;
> > +
> > +               if (leds_per_port > ctrl->cfg->port_led_count)
> shouldn't this then be a `port_led_count_max`?

Yes, would make sense to name it like that.

> > +                       return dev_err_probe(dev, -EINVAL,
> > +                               "too many LEDs per port: %d > %d\n",
> > +                               leds_per_port, ctrl->cfg->port_led_count);
> > +       }
> > +
> 
> it's capture the return value; so you can do something like
> 
> dev_info(dev, "probed X ports Y leds something pretty);
> return ret; (we don't have a dev_info_probe do we :p)

I've used dev_err_probe with a 0 return value, but that produces "error: SUCCESS" levels
of confusing output.

> 
> > +       return ctrl->cfg->port_led_init(ctrl, leds_per_port);
> > +}
> > +
> > +static const struct of_device_id of_switch_port_led_match[] = {
> > +       {
> > +               .compatible = "realtek,rtl8380-port-led",
> the same on 8381 and 8382?

Yes.

Thanks for giving this such a thorough look!

Best,
Sander

> > +               .data = &rtl8380_port_led_config
> > +       },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, of_switch_port_led_match);
> > +
> > +static struct platform_driver realtek_switch_port_led_driver = {
> > +       .probe = realtek_port_led_probe,
> > +       .driver = {
> > +               .name = "realtek-switch-port-led",
> > +               .of_match_table = of_switch_port_led_match
> > +       }
> > +};
> > +module_platform_driver(realtek_switch_port_led_driver);
> > +
> > +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> > +MODULE_DESCRIPTION("Realtek SoC switch port LED driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
> > maple.patch b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
> > maple.patch
> > new file mode 100644
> > index 000000000000..e33737a4f0da
> > --- /dev/null
> > +++ b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
> > maple.patch
> > @@ -0,0 +1,51 @@
> > +From 0efa8d564f27519d20fed39f5d650e50cbc34a0b Mon Sep 17 00:00:00 2001
> > +From: Sander Vanheule <sander@svanheule.net>
> > +Date: Mon, 11 Jul 2022 21:37:17 +0200
> > +Subject: [PATCH 02/14] pinctrl: add pinctrl for Realtek maple
> > +
> > +Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
> > +RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
> > +RTL838x has been tested, but functionality should be near identical.
> > +
> > +Since control bits for different muxes are scattered throughout the
> > +switch core's register space, pin control features are provided by a
> > +table of regmap fields. This should be flexible enough to allow
> > +extension to other SoC generations providing similar features.
> > +
> > +Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > +---
> > + drivers/pinctrl/Kconfig                  |  11 +
> > + drivers/pinctrl/Makefile                 |   1 +
> > + drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 +++++++++++++++++++++++
> > + 3 files changed, 382 insertions(+)
> > + create mode 100644 drivers/pinctrl/pinctrl-rtl-switchcore.c
> > +
> > +--- a/drivers/pinctrl/Kconfig
> > ++++ b/drivers/pinctrl/Kconfig
> > +@@ -215,6 +215,16 @@ config PINCTRL_ROCKCHIP
> > +       select MFD_SYSCON
> > +       select OF_GPIO
> > +
> > ++config PINCTRL_RTL_SWITCHCORE
> > ++      tristate "Realtek switch core pinctrl driver"
> > ++      depends on MFD_REALTEK_SWITCHCORE || COMPILE_TEST
> > ++      select PINMUX
> > ++      select GENERIC_PINMUX_FUNCTIONS
> > ++      select MFD_SYSCON
> > ++      help
> > ++        Driver for pin control fields in RTL83xx and RTL93xx managed ethernet
> > ++        switch SoCs.
> > ++
> > + config PINCTRL_SINGLE
> > +       tristate "One-register-per-pin type device tree based pinctrl driver"
> > +       depends on OF
> > +--- a/drivers/pinctrl/Makefile
> > ++++ b/drivers/pinctrl/Makefile
> > +@@ -30,6 +30,7 @@ obj-$(CONFIG_PINCTRL_PALMAS) += pinctrl-
> > + obj-$(CONFIG_PINCTRL_PIC32)   += pinctrl-pic32.o
> > + obj-$(CONFIG_PINCTRL_PISTACHIO)       += pinctrl-pistachio.o
> > + obj-$(CONFIG_PINCTRL_ROCKCHIP)        += pinctrl-rockchip.o
> > ++obj-$(CONFIG_PINCTRL_RTL_SWITCHCORE) += pinctrl-rtl-switchcore.o
> > + obj-$(CONFIG_PINCTRL_SINGLE)  += pinctrl-single.o
> > + obj-$(CONFIG_PINCTRL_SIRF)    += sirf/
> > + obj-$(CONFIG_PINCTRL_SX150X)  += pinctrl-sx150x.o
> > diff --git a/target/linux/realtek/rtl838x/config-5.10
> > b/target/linux/realtek/rtl838x/config-5.10
> > index 065948532057..5c69832be41d 100644
> > --- a/target/linux/realtek/rtl838x/config-5.10
> > +++ b/target/linux/realtek/rtl838x/config-5.10
> > @@ -99,6 +99,7 @@ CONFIG_IRQ_MIPS_CPU=y
> >   CONFIG_IRQ_WORK=y
> >   CONFIG_JFFS2_ZLIB=y
> >   CONFIG_LEDS_GPIO=y
> > +CONFIG_LEDS_RTL_SWITCH_PORT=y
> >   CONFIG_LEGACY_PTYS=y
> >   CONFIG_LEGACY_PTY_COUNT=256
> >   CONFIG_LIBFDT=y
> > 
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On 29-07-2022 23:34, Sander Vanheule wrote:
> On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
>> On 16-07-2022 21:09, Sander Vanheule wrote:
>>> Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
>>> RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
>>> RTL838x has been tested, but functionality should be near identical.
>>>
>>> Since control bits for different muxes are scattered throughout the
>>> switch core's register space, pin control features are provided by a
>>> table of regmap fields. This should be flexible enough to allow
>>> extension to other SoC generations providing similar features.
>>>
>>> Signed-off-by: Sander Vanheule <sander@svanheule.net>
>>> ---
>>>    .../drivers/leds/leds-rtl-switch-port.c       | 668 ++++++++++++++++++
>>>    ...inctrl-add-pinctrl-for-Realtek-maple.patch |  51 ++
>>>    target/linux/realtek/rtl838x/config-5.10      |   1 +
>>>    3 files changed, 720 insertions(+)
>>>    create mode 100644 target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-
>>> port.c
>>>    create mode 100644 target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-
>>> Realtek-maple.patch
>>>
>>> diff --git a/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
>>> b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
>>> new file mode 100644
>>> index 000000000000..76dfede7ba15
>>> --- /dev/null
>>> +++ b/target/linux/realtek/files-5.10/drivers/leds/leds-rtl-switch-port.c
>>> @@ -0,0 +1,668 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>> Probably want bitops.h for BIT()
>>
>> +#include <linux/bitops.h>
>>
> Will add.
>
>>> +#include <linux/leds.h>
>>> +#include <linux/mfd/core.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +/*
>>> + * Realtek switch port LED
>>> + *
>>> + * The switch ASIC can control up to three LEDs per phy, based on a number of
>>> + * matching conditions. Alternatively, each individual LED output can also be
>>> + * configured for manual control.
>>> + */
>> Is this an internal define? `REALTEK_PORT_LED_TRIGGER_NONE` (etc) maybe
>> better? (or LEDTRIG if it really must be shorter)
>>
>> It wasn't until much later that I figured out that this was the
>> abbreviation for port trigger *doh*
> I got tired of typing so much by prefixing everything with REALTEK_... This is in fact an
> internal definition, used only for the private trigger.
>
> The translation between DT trigger flags and the actual HW trigger settings may look like
> an unnecessary complication here, but I've done this with broader compatibility in mind.
> On RTL93xx, the HW trigger also uses (different) bit flags [1], instead of an enumeration
> of triggers [2]. Other hardware, from other vendors, would also use a different aproach,
> but there is currently no framework yet to offload netdev triggers.
>
> [1] https://svanheule.net/realtek/longan/register/led_set0_0_ctrl
> [2] https://svanheule.net/realtek/maple/register/led_mode_ctrl
>
>>> +#define PTRG_NONE              0
>>> +#define PTRG_ACT_RX            BIT(0)
>>> +#define PTRG_ACT_TX            BIT(1)
>>> +#define PTRG_ACT               PTRG_ACT_RX | PTRG_ACT_TX
>>> +#define PTRG_LINK_10           BIT(2)
>>> +#define PTRG_LINK_100          BIT(3)
>>> +#define PTRG_LINK_1000         BIT(4)
> These flags should actually go into a header somewhere, but this was still development
> code.
>
>>> +
>>> +struct led_port_blink_mode {
>>> +       u16 interval; /* Toggle interval in ms */
>>> +       u8 mode; /* ASIC mode bits */
>>> +};
>>> +
>>> +#define REALTEK_PORT_LED_BLINK_COUNT   6
>>> +struct led_port_modes {
>> realtek_led_port_modes?
>>> +       u8 off;
>>> +       u8 on;
>>> +       struct led_port_blink_mode blink[REALTEK_PORT_LED_BLINK_COUNT];
>> ah here's the other list. So would it make sense to re-use this same
>> structure for the sys-led, or is that over-optimizing things ...
> Like I noted on the other patch, it could make sense, yes.
>
>> Why though are on/off and the blink entries separated?  Isn't 'off' a
>> interval of 'inviite (max-int)' and on a blink rate of 0? While you may
>> have to do some logic later, it keeps the interface more consistent?
>> idk; just some food for thought.
> Because Realtek wouldn't be the same company if everything made sense. The value for "off"
> is actually 5. 6 and 7 are just more blink intervals [3]. I guess they decided to add more
> rates, but didn't want to break backwards compatibilty somewhere.
>
> [3] https://svanheule.net/realtek/maple/register/led_sw_p_ctrl
>
>>> +};
>>> +
>>> +struct led_port_group {
>>> +       struct regmap_field *setting;
>>> +       unsigned int size;
>>> +       /* bitmap to keep track of associated ports */
>> Can you explain a bit what's supposed to be here? right now its just an
>> it pointer, but appearantly it will point at some bitmap? can we not
>> make a struct representing this bitmap and then use that instead?
> This is how bitmaps are defined in the kernel. cpumask structs do wrap their bitmap in a
> struct, but I see little point in adding an extra layer here.
>
>>> +       unsigned long *ports;
>>> +};
>>> +
>>> +struct switch_port_led_config;
>> can we do this forward declaration the other way around? You forward
>> declare it here because need the TYPE for the cfg pointer.
>>
>> Later however, you need it to indicate the argument of a function
>> pointer. I think the type declaration ways heavier and makes it thus
>> 'nicer' to do it the other way around. though that does indeed mean you
>> have to do 2 forard declarations for your two structs, but would still
>> be cleaner imo
> Forward declaring the ctrl struct would also be an option. I'll play with it a bit, see
> what makes most sense (e.g. smallest HW to biggest HW unit control struct).
>
>>> +
>>> +struct switch_port_led_ctrl {
>>> +       struct device *dev;
>>> +       struct regmap *map;
>>> +       const struct switch_port_led_config *cfg;
>>> +       struct mutex lock;
>>> +       struct led_port_group *groups;
>>> +};
>>> +
>>> +struct switch_port_led {
>>> +       struct led_classdev led;
>>> +       struct switch_port_led_ctrl *ctrl;
>>> +       unsigned int port;
>>> +       unsigned int index;
>>> +       u32 trigger_flags;
>>> +       struct led_port_group *current_group;
>>> +};
>>> +
>>> +struct switch_port_led_config {
>>> +       unsigned int port_count;
>>> +       unsigned int port_led_count;
>>> +       unsigned int group_count;
>>> +       const struct led_port_modes *modes;
>>> +       /* Set the number of LEDs per port */
>>> +       int (*port_led_init)(struct switch_port_led_ctrl *ctrl,
>>> +               unsigned int led_count);
>> what is the significance for both these comments (above and below)? the
>> comment states to set the leds per port; but the function is called
>> led_init? Below the function is called set enabled, so that's pretty
>> obvious that you set the port enabled switch port led?
> Left-over from development and incomplete understanding of how things work.
> port_led_init() is called to finish the peripheral initialisation. set_port_enabled() is
> called to enable the LEDs on a single port. While this concep applies cleanly to RTL838x,
> it's a bit harder to force RTL839x into this scheme. I'll look at improving the naming of
> these ops, insofar as I haven't done that yet.
>
>>> +       /* Enable or disable all LEDs for a certain port */
>>> +       int (*set_port_enabled)(struct switch_port_led *led, bool enabled);
>>> +       int (*set_hw_managed)(struct switch_port_led *led, bool hw_managed);
>>> +       int (*write_mode)(struct switch_port_led *led, unsigned int mode);
>>> +       int (*read_mode)(struct switch_port_led *led);
>>> +       int (*trigger_xlate)(u32 trigger);
>>> +       struct led_port_group *(*map_group)(struct switch_port_led *led, u32 trigger);
>>> +       struct reg_field group_settings[];
>>> +};
>>> +
>>> +static struct led_port_group *switch_port_led_get_group(
>>> +       struct switch_port_led *pled, unsigned int group)
>>> +{
>>> +       return &pled->ctrl->groups[group * pled->ctrl->cfg->port_led_count + pled-
>>>> index];
>> also in this patch; don't fear parenthesis :) they are our friend ;)
> I'm a physicist. Maybe programming operator precedence wasn't deeply ingrained into my
> brain at school, mathematical operators certainly were ;)

I actually started with technical applied physics, but switched to 
CompSci after 2 years. Back then (25years ago), operator precedence got 
hammered in hard "Do not use unnecessary parenthesis, this is bad" 
without telling you why however. Maybe lack of readability when using 
having 20 opening parenthesis in a row? idk. What is even more 
interesting, was that in highschool, in the netherlands, the operator 
precedence has even slightly changed, where these meme's where along the 
lines "what is the answer to this equation" and depending on the 
generation you got different results ;)


>
>>> +}
>>> +
>>> +/*
>>> + * SoC specific implementation for RTL8380 series (Maple)
>> Maybe 'x' the 0 here?
>>
>> + * SoC specific implementation for RTL838x series (Maple)
>>
> Sure, why not.
>
>>> + */
>>> +#define RTL8380_REG_LED_MODE_SEL               0x1004
>> do we have any idea what the SDK names these registers? Usually they
>> match the (secret) datasheet ...
> Normally I just copied the names from the SDK [4]. Be happy I haven't implemented the
> DMY_REG5 register yet. :)
>
> [4] https://svanheule.net/realtek/maple/feature/led
>
>>> +#define RTL8380_REG_LED_GLB_CTRL               0xa000
>>> +#define RTL8380_REG_LED_MODE_CTRL              0xa004
>>> +#define RTL8380_REG_LED_P_EN_CTRL              0xa008
>>> +#define RTL8380_REG_LED_SW_P_EN_CTRL(led)      (0xa010 + 4 * (led))
>>> +#define RTL8380_REG_LED_SW_CTRL(port)          (0xa01c + 4 * (port))
>>> +
>>> +#define RTL8380_PORT_LED_COUNT                         3
>>> +#define RTL8380_GROUP_SETTING_WIDTH                    5
>>> +#define RTL8380_GROUP_SETTING_OFFSET(_grp, _idx)       \
>>> +       (RTL8380_GROUP_SETTING_WIDTH * ((_idx) + RTL8380_PORT_LED_COUNT * (_grp)))
>>> +#define RTL8380_GROUP_SETTING(_grp, _idx)      {                               \
>>> +               .reg = RTL8380_REG_LED_MODE_CTRL,                               \
>>> +               .lsb = RTL8380_GROUP_SETTING_OFFSET(_grp, _idx),                \
>>> +               .msb = RTL8380_GROUP_SETTING_OFFSET(_grp, (_idx) + 1) - 1,      \
>>> +       }
>>> +
>>> +enum rtl83xx_port_trigger {
>>> +       RTL83XX_TRIG_LINK_ACT = 0,
>>> +       RTL83XX_TRIG_LINK = 1,
>>> +       RTL83XX_TRIG_ACT = 2,
>>> +       RTL83XX_TRIG_ACT_RX = 3,
>>> +       RTL83XX_TRIG_ACT_TX = 4,
>>> +       RTL83XX_TRIG_LINK_1G = 7,
>>> +       RTL83XX_TRIG_LINK_100M = 8,
>>> +       RTL83XX_TRIG_LINK_10M = 9,
>>> +       RTL83XX_TRIG_LINK_ACT_1G = 10,
>>> +       RTL83XX_TRIG_LINK_ACT_100M = 11,
>>> +       RTL83XX_TRIG_LINK_ACT_10M = 12,
>>> +       RTL83XX_TRIG_LINK_ACT_1G_100M = 13,
>>> +       RTL83XX_TRIG_LINK_ACT_1G_10M = 14,
>>> +       RTL83XX_TRIG_LINK_ACT_100M_10M = 15,
>>> +       RTL83XX_TRIG_DISABLED = 31,
>>> +};
>> as you sometimes do right align, this enum is a perfect candidate I'd
>> think to do so as well?
> Would make it look more like a table, yes. Any idea if it is conventional to align the
> assignments in kernel code?
I see it both; some that don't like to align, some that do it 
excessively, personal preference seems to reign, but making it read 
nice/easy should always be the goal imo.
>
>>> +
>>> +static const struct led_port_modes rtl8380_port_led_modes = {
>>> +       .off = 0,
>>> +       .on = 5,
>>> +       .blink  = {{  32, 1},
>> I know it's just stupid style, but why not put the first entry on the
>> next line with one indentation level? I think that make it looks a
>> little more natural
> But saving previous lines! Could do that, yes.
>
>>> +                  {  64, 2},
>>> +                  { 128, 3},
>> I think this enum here deserves a comment as to why this order is so
>> funky. Also to prevent someone passing along later and trying to 'fix' it :)
> Something along the lines of the following?
>
> /*
>   * Yes, this table is correct and was probably amended during development.
>   * Don't try to fix it.
>   */
Hehe, sure yeah, but anything works really, just so that the reader is 
aware of some funkyness
>
>>> +                  { 256, 6},
>>> +                  { 512, 4},
>>> +                  {1024, 7}}
>>> +};
>>> +
>>> +static const struct led_port_modes rtl8390_port_led_modes = {
>>> +       .off = 0,
>>> +       .on = 7,
>>> +       .blink = {{  32, 1},
>>> +                 {  64, 2},
>>> +                 { 128, 3},
>>> +                 { 256, 4},
>>> +                 { 512, 5},
>>> +                 {1024, 6}}
> See, this one makes more sense :)
>
>>> +};
>>> +
>>> +static int rtl8380_port_trigger_xlate(u32 port_led_trigger)
>>> +{
>>> +       switch (port_led_trigger) {
>>> +       case PTRG_NONE:
>>> +               return RTL83XX_TRIG_DISABLED;
>>> +       case PTRG_ACT_RX:
>>> +               return RTL83XX_TRIG_ACT_RX;
>>> +       case PTRG_ACT_TX:
>>> +               return RTL83XX_TRIG_ACT_TX;
>>> +       case PTRG_ACT:
>>> +               return RTL83XX_TRIG_ACT;
>>> +       case PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK;
>>> +       case PTRG_LINK_10:
>>> +               return RTL83XX_TRIG_LINK_10M;
>>> +       case PTRG_LINK_100:
>>> +               return RTL83XX_TRIG_LINK_100M;
>>> +       case PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK_1G;
>>> +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100 | PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK_ACT;
>>> +       case PTRG_ACT | PTRG_LINK_10:
>>> +               return RTL83XX_TRIG_LINK_ACT_10M;
>>> +       case PTRG_ACT | PTRG_LINK_100:
>>> +               return RTL83XX_TRIG_LINK_ACT_100M;
>>> +       case PTRG_ACT | PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK_ACT_1G;
>>> +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_100:
>>> +               return RTL83XX_TRIG_LINK_ACT_100M_10M;
>>> +       case PTRG_ACT | PTRG_LINK_10 | PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK_ACT_1G_10M;
>>> +       case PTRG_ACT | PTRG_LINK_100 | PTRG_LINK_1000:
>>> +               return RTL83XX_TRIG_LINK_ACT_1G_100M;
>>> +       default:
>>> +               return -EINVAL;
>>> +       }
>>> +}
>>> +
>>> +/*
>>> + * Find the group the LED with this trigger setting can be assigned to, or
>> afaik you can use '@trigger' which then relates to the argument. I had
>> to read this line 3 times to realize that 'this' related to the trigger'
>>
>> Also, what are you 'finding', i'm not seeing a loop or anything, where
>> are we finding?
>>
>> 'Map switch port @led to map to a trigger @trigger'? idk, but I'm not
>> sure what is happening here yet.
>>
>> btw, you forgot to add documentation for your arguments ;)
> Ok, I'll clean up the comments.
>
> The reason this looks weird, is because the led_map_group() functions are entirely natural
> for RTL839x and later, where LEDs can be assigned to different "LED sets" [5]. I need
> something that works for all generations, so I'm forcing the RTL838x implementation into
> these two static, somewhat artificial groups ("sets").
>
> [5] https://svanheule.net/realtek/cypress/feature/led
yeah, I"ve since learned a bit more on the led behavior, and it's clear 
that it is 'evolved logic, glued uppon existing (FPGA) code they had.
>
>>> + * initialise a new empty group.
>>> + *
>>> + * Return group number on success, or < 0 on failure.
>>> + * */
>>> +static struct led_port_group *rtl8380_port_led_map_group(struct switch_port_led *led,
>>> u32 trigger)
>>> +{
>>> +       int rtl_trigger = rtl8380_port_trigger_xlate(trigger);
>>> +       struct switch_port_led_ctrl *ctrl = led->ctrl;
>>> +       struct led_port_group *group;
>>> +       u32 current_trigger;
>>> +       int err;
>>> +
>>> +       if (rtl_trigger < 0)
>>> +              return ERR_PTR(rtl_trigger);
>>> +
>>> +       /*
>>> +        * Static groups:
>> does this deserve some explanation? why are there 2 groups? how are they
>> grouped? why are the grouped the way they are grouped? that could also
>> help you define a name for the magic value 23 potentially?
> Ports 0-24 can be provided by three 8-port phy-s, each using a pair of QSGMII connections.
> Ports 24-27 can come from a 4-port phy using a single QSGMII connection. No idea why
> Realtek made this division though, since they also allow combo ports on ports 20-23...
Yeah, studying the datasheet I found that too, which is made more weird 
if you look at the 838[0123] as they basically are the same chip, just 
with different MII configurations. Evolutions/glued history I suppose ...
>
>>> +        *   - group 0: ports 0-23
>>> +        *   - group 1: ports 24-27
>>> +        */
>>> +       if (led->port > 23)
>>> +               group = switch_port_led_get_group(led, 1);
>>> +       else
>>> +               group = switch_port_led_get_group(led, 0);
>>> +
>>> +       err = regmap_field_read(group->setting, &current_trigger);
>>> +       if (err)
>>> +               return ERR_PTR(err);
>>> +
>>> +       if (current_trigger != rtl_trigger && !bitmap_empty(group->ports, group-
>>>> size)) {
>>> +               dev_warn(ctrl->dev, "cannot add (%d,%d) to group: 0x%02x != 0x%02x\n",
>>> +                       led->port, led->index, current_trigger, rtl_trigger);
>>> +               return ERR_PTR(-ENOSPC);
>>> +       }
>>> +
>>> +       return group;
>>> +}
>>> +
>>> +static int rtl8380_port_led_set_port_enabled(struct switch_port_led *led, bool
>>> enabled)
>>> +{
>>> +       int reg = RTL8380_REG_LED_P_EN_CTRL;
>>> +       int val = enabled ? BIT(led->port) : 0;
>>> +
>>> +       return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
>>> +}
>>> +
>>> +static int rtl8380_port_led_set_hw_managed(struct switch_port_led *led, bool
>>> hw_managed)
>>> +{
>>> +       int reg = RTL8380_REG_LED_SW_P_EN_CTRL(led->index);
>>> +       int val = hw_managed ? 0 : BIT(led->port);
>>> +
>>> +       return regmap_update_bits(led->ctrl->map, reg, BIT(led->port), val);
>>> +}
>>> +
>>> +static int rtl8380_port_led_write_mode(struct switch_port_led *led, unsigned int
>>> mode)
>>> +{
>>> +       int reg = RTL8380_REG_LED_SW_CTRL(led->port);
>>> +       int offset = 3 * led->index;
>> I think this 3 (which is reused later also) could have a define
>> (REALTEK_something_LED_PER_something) or something (and maybe even an
>> accompanying _MASK later I find oyu have `
>>
>> port_led_count
>>
>> ` so you probably could use that?
> True, a #define would make sense.
i means, that I think you actually already have `led->port_led_count` in 
this struct I believe I saw; which makes sense if you want to have 
different counts for different leds later. keeping it in your 
compatible-data struct makes sense, but lacking that, a define is good 
enough too :)
>
>>> +       u32 mask = GENMASK(2, 0) << offset;
>>> +       u32 value = mode << offset;
>>> +
>>> +       return regmap_update_bits(led->ctrl->map, reg, mask, value);
>>> +}
>>> +
>>> +static int rtl8380_port_led_read_mode(struct switch_port_led *led)
>>> +{
>>> +       u32 val;
>>> +       int ret;
>>> +
>>> +       ret = regmap_read(led->ctrl->map, RTL8380_REG_LED_SW_CTRL(led->port), &val);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       return (val >> (3 * led->index)) & GENMASK(2, 0);
>>> +}
>>> +
>>> +static int rtl8380_port_led_init(struct switch_port_led_ctrl *ctrl, unsigned int
>>> led_count)
>>> +{
>>> +       u32 led_count_mask = GENMASK(led_count - 1, 0);
>>> +       u32 led_mask = GENMASK(5, 0);
>>> +
>>> +       return regmap_update_bits(ctrl->map, RTL8380_REG_LED_GLB_CTRL, led_mask,
>>> +                       (led_count_mask << 3) | led_count_mask);
>>> +}
>>> +
>>> +static void switch_port_led_brightness_set(struct led_classdev *led_cdev,
>>> +       enum led_brightness brightness)
>>> +{
>>> +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
>>> led);
>>> +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
>>> +
>>> +       ctrl->cfg->write_mode(pled, brightness ? ctrl->cfg->modes->on : ctrl->cfg-
>>>> modes->off);
>>> +}
>>> +
>>> +static enum led_brightness switch_port_led_brightness_get(struct led_classdev
>>> *led_cdev)
>>> +{
>>> +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
>>> led);
>>> +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
>>> +
>>> +       return ctrl->cfg->read_mode(pled) != ctrl->cfg->modes->off;
>>> +}
>>> +
>>> +static int switch_port_led_blink_set(struct led_classdev *led_cdev,
>>> +       unsigned long *delay_on, unsigned long *delay_off)
>>> +{
>>> +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
>>> led);
>>> +       struct switch_port_led_ctrl *ctrl = pled->ctrl;
>>> +
>>> +       const struct led_port_blink_mode *blink = &ctrl->cfg->modes->blink[0];
>>> +       unsigned long interval_ms = *delay_on + *delay_off;
>>> +       unsigned int i = 0;
>>> +
>>> +       if (interval_ms && *delay_on == 0)
>>> +               return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->off);
>>> +
>>> +       if (interval_ms && *delay_off == 0)
>>> +               return ctrl->cfg->write_mode(pled, ctrl->cfg->modes->on);
>>> +
>>> +       if (!interval_ms)
>> You are explicit on the delay, why not on the interval?
>>
>> +       if (interval_ms == 0)
>>
> Sure, will change.
>
>>> +               interval_ms = 500;
>>> +
>>> +       /*
>>> +        * Since the modes always double in interval, the geometric mean of
>>> +        * intervals [i] and [i + 1] is equal to sqrt(2) * interval[i]
>>> +        */
>>> +       while (i < (REALTEK_PORT_LED_BLINK_COUNT - 1) &&
>>> +               interval_ms > (2 * 141 * blink[i].interval) / 100)
>>> +               i++;
>>> +
>>> +       *delay_on = blink[i].interval;
>>> +       *delay_off = blink[i].interval;
>>> +
>>> +       return ctrl->cfg->write_mode(pled, blink[i].mode);
>>> +}
>>> +
>>> +static struct led_hw_trigger_type switch_port_rtl_hw_trigger_type;
>>> +
>>> +static ssize_t rtl_hw_trigger_show(struct device *dev, struct device_attribute *attr,
>>> char *buf)
>>> +{
>>> +       struct switch_port_led *pled = dev_get_drvdata(dev);
>>> +
>>> +       return sprintf(buf, "%x\n", pled->trigger_flags);
>>> +}
>>> +
>>> +static ssize_t rtl_hw_trigger_store(struct device *dev, struct device_attribute
>>> *attr,
>>> +               const char *buf, size_t count)
>>> +{
>>> +       struct switch_port_led *pled = dev_get_drvdata(dev);
>>> +       struct led_port_group *group, *new_group;
>> from a namespace pov; why are you not putting these variables inside
>> your if statement and scope them there? however ...
>>> +       unsigned int member_count;
>>> +       int trigger;
>>> +       int nchars;
>>> +       int value;
>>> +       int err;
>>> +
>>> +       if (sscanf(buf, "%x%n", &value, &nchars) != 1 || nchars + 1 < count)
>>> +               return -EINVAL;
>>> +
>>> +       mutex_lock(&pled->ctrl->lock);
>>> +
>>> +       /*
>>> +        * If the private trigger is already active:
>>> +        *   - modify the current group if we are the only member,
>>> +        *   - or join a new group if one is available
>>> +        */
>>> +       if (pled->current_group) {
>> you are doing everything within this if; why not instead? fail early
>> fail often
>>
>> +       if (!pled->current_group) {
>> +               dev_dbg(dev, "no led not part of any group\n");
>> +               err = -Esomething;
>> +               goto out;
>> +       };
> pled->current_group is only set when the private trigger is activated. It is not an error
> to set the trigger mask when another trigger is selected, and this is intentional. The
> reason I kept the bulk of the code inside the if(){} is because I found it more indicative
> of the optional nature of this code.
That makes sense indeed, might depend on the context of the function, if 
the functions main raison d'être is this optional behavior, within the 
context, it's not optional :p but I get what you mean, and both 
solutions work just as well. Btw, it doesn't have to be an error case to 
do the above; but indeed, if you use the goto to skip code, that's not 
nice ;)
>
> If the trigger flags could only be set after you've enabled the offloading trigger, the
> driver would be unable to enable any but the first LED, since we wouldn't have any groups
> left to reassign the LEDs to (remember the static groups).
>
> Again, this probably makes more sense on RTL839x, and I have to refactor this part of the
> code a bit. I'm currently duplicating the join group/leave group logic in two places,
> which makes it tricky to modify it later.
since this is RFC and 'learning' I totally get why the things they are 
what they are :)
>
>>> +               group = pled->current_group;
>>> +
>>> +               trigger = pled->ctrl->cfg->trigger_xlate(value);
>>> +               if (trigger < 0) {
>>> +                       err = trigger;
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               member_count = bitmap_weight(group->ports, group->size);
>>> +
>>> +               if (member_count == 1) {
>>> +                       err = regmap_field_write(group->setting, trigger);
>>> +                       if (err)
>>> +                               goto err_out;
>>> +
>>> +                       goto out;
>>> +               }
>>> +
>>> +               new_group = pled->ctrl->cfg->map_group(pled, value);
>>> +               if (IS_ERR(new_group)) {
>>> +                       err = PTR_ERR(new_group);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               if (member_count == 0) {
>>> +                       err = regmap_field_write(new_group->setting, trigger);
>>> +                       if (err)
>>> +                               goto err_out;
>>> +               }
>>> +
>>> +               bitmap_clear(group->ports, pled->port, 1);
>>> +               bitmap_set(new_group->ports, pled->port, 1);
>>> +               pled->current_group = new_group;
>>> +       }
>>> +
>>> +out:
>>> +       pled->trigger_flags = value;
>>> +
>>> +err_out:
>>> +       mutex_unlock(&pled->ctrl->lock);
>>> +
>>> +       if (err)
>>> +               return err;
>>> +
>>> +       return count;
>>> +}
>>> +static DEVICE_ATTR_RW(rtl_hw_trigger);
>>> +
>>> +static struct attribute *rtl_hw_trigger_attrs[] = {
>>> +       &dev_attr_rtl_hw_trigger.attr,
>>> +       NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(rtl_hw_trigger);
>> Is there any possibility this can be done in a generic way at all? I
>> know this doesn't exist, and it's a very good first step ... but these
>> are netdev_switch_triggers maybe? Or would that not work because our
>> triggers are TOO RTL specific? Could we make them less specific (in the
>> future)?
> This private trigger is sort of an exercise in netdev trigger offloading. There was a
> discussion earlier about a generic netdev offloading interface on some qca8k patches, but
> that didn't materialise into something useful yet AFAIK.
Do you have a link I can read up on? I'm very curious. I think with DSA 
becoming more prominent, this will need to happen at some point ...
>>> +
>>> +static int switch_port_led_trigger_activate(struct led_classdev *led_cdev)
>>> +{
>>> +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
>>> led);
>>> +       struct led_port_group *group;
>>> +       int rtl_trigger;
>>> +       int err = 0;
>>> +
>>> +       mutex_lock(&pled->ctrl->lock);
>>> +
>>> +       rtl_trigger = pled->ctrl->cfg->trigger_xlate(pled->trigger_flags);
>>> +       if (rtl_trigger < 0) {
>>> +               err = rtl_trigger;
>>> +               goto out;
>>> +       }
>>> +
>>> +       group = pled->ctrl->cfg->map_group(pled, pled->trigger_flags);
>>> +       if (IS_ERR(group)) {
>>> +               err = PTR_ERR(group);
>>> +               goto out;
>>> +       }
>>> +
>>> +       if (bitmap_empty(group->ports, group->size)) {
>>> +               err = regmap_field_write(group->setting, rtl_trigger);
>>> +               if (err)
>>> +                       goto out;
>>> +       }
>>> +
>>> +       bitmap_set(group->ports, pled->port, 1);
>> would it be useful to somehow codify the length? MACRO, sizeof,
>> pled->port_size something?
> A macro could help, but I don't think I understand how the other would work? This just
> indicates that 1 extra port was added to the selected group, so there is no sizeof() we
> can use.

pled->port_size = 1;

as part of your compatible datastruct I was thinking.

>
>>> +       pled->current_group = group;
>>> +
>>> +       err = pled->ctrl->cfg->set_hw_managed(pled, true);
>> if this where some generic function that would need to come from the
>> ops-struct, I'd get it; but why not call the function directly? Otherwise:
>>
>> +       if (pled->ctrl->cfg->set_hw_managed)
>> +               err = pled->ctrl->cfg->set_hw_managed(pled, true);
> The set_hw_managed() op is not optional. In fact, I've reversed the logic as implemented
> in silicon, since that defaults to HW controlled and has to be set to SW controlled.

In that case, the following at least ensures you cannot do `err = 
pled->ctrl->cfg->NULL` :p (unless of course you can absolutely guarantee 
this can not ever happen.

+       if (!pled->ctrl->cfg->set_hw_managed) {
+          dev_err(dev, "No hardware management function set, this is a developer error!\n"
+          return -EINVAL;
+       }

>
>>> +
>>> +out:
>>> +       mutex_unlock(&pled->ctrl->lock);
>> personally, I like to check for nullness on these things, but I don't
>> know what is most commonely done here ... or if mutex_unlock does its
>> own nice checks.
> If lock is NULL, that's driver error, not a runtime error. Should never happen after
> succesful device probing.
that's what I thought; but couldn't remember for sure anymore.
>
>>> +
>>> +       return err;
>>> +}
>>> +
>>> +static void switch_port_led_trigger_deactivate(struct led_classdev *led_cdev)
>>> +{
>>> +       struct switch_port_led *pled = container_of(led_cdev, struct switch_port_led,
>>> led);
>>> +       struct led_port_group *group;
>>> +
>>> +       mutex_lock(&pled->ctrl->lock);
>>> +
>>> +       pled->ctrl->cfg->set_hw_managed(pled, false);
>> as above
>>> +
>>> +       group = pled->current_group;
>> what's the purpose of this assignment? you make a copy, then set it to
>> null; then clear the bitmap. Can you not just first clear the bitmap,
>> then null the assignment? Or is there some hidden race-condition you are
>> protecting against in this way? If so, better leave a comment to
>> indicate this potential race-condition if we can't protect this via
>> code; though I thought that's what the mutex was for to begin with ...
>> maybe add an 'if (pled->current_group == NULL) return check before we
>> even try to get the mutex?
> I added the mutex after trying to be smart by writing the code this way. But with the
> mutex, I can probably just clear the bit first and then clear the pointer. If the trigger
> was succesfully activated, ->current_group cannot be NULL.
>
>>> +       pled->current_group = NULL;
>>> +       bitmap_clear(group->ports, pled->port, 1);
>>> +
>>> +       mutex_unlock(&pled->ctrl->lock);
>>> +}
>>> +
>>> +static struct led_trigger switch_port_rtl_hw_trigger = {
>>> +       .name = "realtek-switchport",
>>> +       .activate = switch_port_led_trigger_activate,
>>> +       .deactivate = switch_port_led_trigger_deactivate,
>>> +       .trigger_type = &switch_port_rtl_hw_trigger_type,
>>> +};
>>> +
>>> +static void realtek_port_led_read_address(struct device_node *np,
>>> +       int *port_index, int *led_index)
>>> +{
>>> +       const __be32 *addr;
>>> +
>>> +       addr = of_get_address(np, 0, NULL, NULL);
>>> +       if (addr) {
>> also here, i'm a bigger fan of failing on !addr (or returning with a
>> dbg/warning) and avoid needless nesting
> Should probably fail when we can't find the address. Otherwise this would default to
> [something], and that just doens't make any sense.
>
>>> +               *port_index = of_read_number(addr, 1);
>>> +               *led_index = of_read_number(addr + 1, 1);
>>> +       }
>>> +}
>>> +
>>> +static struct switch_port_led *switch_port_led_probe_single(
>>> +       struct switch_port_led_ctrl *ctrl, struct device_node *np)
>>> +{
>>> +       struct led_init_data init_data = {};
>>> +       struct switch_port_led *pled;
>>> +       unsigned int port_index;
>>> +       unsigned int led_index;
>>> +       int err;
>>> +
>>> +       realtek_port_led_read_address(np, &port_index, &led_index);
>>> +
>>> +       if (port_index >= ctrl->cfg->port_count || led_index >= ctrl->cfg-
>>>> port_led_count)
>>> +               return ERR_PTR(-ENODEV);
>>> +
>>> +       pled = devm_kzalloc(ctrl->dev, sizeof(*pled), GFP_KERNEL);
>>> +       if (!pled)
>>> +               return ERR_PTR(-ENOMEM);
>>> +
>>> +       init_data.fwnode = of_fwnode_handle(np);
>>> +
>>> +       pled->ctrl = ctrl;
>>> +       pled->port = port_index;
>>> +       pled->index = led_index;
>>> +
>>> +       pled->led.max_brightness = 1;
>> +       pled->led.max_brightness = LED_ON;
> Ah, but LED_ON is deprecated! ;)
:p
>
>>> +       pled->led.brightness_set = switch_port_led_brightness_set;
>>> +       pled->led.brightness_get = switch_port_led_brightness_get;
>>> +       pled->led.blink_set = switch_port_led_blink_set;
>>> +       pled->led.trigger_type = &switch_port_rtl_hw_trigger_type;
>>> +
>>> +       ctrl->cfg->set_hw_managed(pled, false);
>>> +       ctrl->cfg->set_port_enabled(pled, true);
>>> +
>>> +       err = devm_led_classdev_register_ext(ctrl->dev, &pled->led, &init_data);
>>> +       if (err)
>>> +               return ERR_PTR(err);
>>> +
>>> +       err = devm_device_add_groups(pled->led.dev, rtl_hw_trigger_groups);
>>> +       if (err)
>>> +               return ERR_PTR(err);
>>> +
>>> +       dev_set_drvdata(pled->led.dev, pled);
>>> +
>>> +       return pled;
>>> +}
>>> +
>>> +static const struct switch_port_led_config rtl8380_port_led_config = {
>>> +       .port_count = 28,
>>> +       .port_led_count = 3,
>>> +       .group_count = 2,
>>> +       .modes = &rtl8380_port_led_modes,
>>> +       .port_led_init = rtl8380_port_led_init,
>>> +       .set_port_enabled = rtl8380_port_led_set_port_enabled,
>>> +       .set_hw_managed = rtl8380_port_led_set_hw_managed,
>>> +       .write_mode = rtl8380_port_led_write_mode,
>>> +       .read_mode = rtl8380_port_led_read_mode,
>>> +       .trigger_xlate = rtl8380_port_trigger_xlate,
>>> +       .map_group = rtl8380_port_led_map_group,
>>> +       .group_settings = {
>>> +               RTL8380_GROUP_SETTING(0, 0),
>>> +               RTL8380_GROUP_SETTING(0, 1),
>>> +               RTL8380_GROUP_SETTING(0, 2),
>>> +               RTL8380_GROUP_SETTING(1, 0),
>>> +               RTL8380_GROUP_SETTING(1, 1),
>>> +               RTL8380_GROUP_SETTING(1, 2),
>>> +       },
>>> +};
>>> +
>>> +static int realtek_port_led_probe(struct platform_device *pdev)
>>> +{
>>> +       struct switch_port_led_ctrl *ctrl;
>>> +       struct device *dev = &pdev->dev;
>>> +       struct device_node *np, *child;
>>> +       struct reg_field group_setting;
>>> +       unsigned int member_map_count;
>>> +       struct led_port_group *group;
>>> +       struct switch_port_led *pled;
>>> +       u32 leds_per_port = 0;
>>> +       int child_count;
>>> +       int err, i;
>>> +
>>> +       np = dev->of_node;
>> can we always trust the np or do we have to check for it? (input sanitation)
>>> +
>>> +       dev_info(dev, "probing port leds\n");
>> dev_dbg? if we have sensible data to put here (after we found stuff
>> probably) a dev_info certainly is useful of course (or if the probing
>> itself takes long, known when we started/finish is nice of course)
> Development print. Will remove.
>
>>> +
>>> +//     if (!pdev->mfd_cell)
>>> +//             return dev_err_probe(dev, -ENODEV, "must be instantiated as MFD
>>> child\n");
>>> +
>>> +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
>>> +       if (!ctrl)
>>> +               return -ENOMEM;
>> curious, as `dev_err_probe` is new to me (it's been a while, but I like
>> it) any reason not to dev_err_probe here for consistency?
> Apparently the alloc() functions are verbose by themselves, and don't need any other
> output (or so I've seen reviewers say).
>
>>> +
>>> +       mutex_init(&ctrl->lock);
>>> +
>>> +       ctrl->dev = dev;
>>> +       ctrl->cfg = of_device_get_match_data(dev);
>>> +       if (!ctrl->cfg)
>>> +               return dev_err_probe(dev, -ENODEV, "failed to find matching data\n");
>>> +
>>> +       ctrl->map = device_node_to_regmap(of_get_parent(np));
>>> +       if (IS_ERR_OR_NULL(ctrl->map))
>>> +               return dev_err_probe(dev, PTR_ERR(ctrl->map), "failed to find parent
>>> regmap\n");
>>> +
>>> +       member_map_count = ctrl->cfg->port_led_count * ctrl->cfg->group_count;
>> port_member_led_count_map maybe to be slightly more descriptive?
> Maybe I've changed this already. If not, I'll give it some thought.
>
>>> +       ctrl->groups = devm_kcalloc(dev, member_map_count,
>>> +               sizeof(*ctrl->groups), GFP_KERNEL);
>>> +       if (!ctrl->groups)
>>> +               return -ENOMEM;
>>> +
>>> +       for (i = 0; i < member_map_count; i++) {
>>> +               group = &ctrl->groups[i];
>>> +               group_setting = ctrl->cfg->group_settings[i];
>>> +
>>> +               group->size = ctrl->cfg->port_count;
>>> +               group->setting = devm_regmap_field_alloc(dev, ctrl->map,
>>> group_setting);
>>> +               if (!group->setting)
>>> +                       return -ENOMEM;
>>> +
>>> +               group->ports = devm_bitmap_zalloc(dev, ctrl->cfg->port_count,
>>> GFP_KERNEL);
>>> +               if (!group->ports)
>>> +                       return -ENOMEM;
>>> +       }
>>> +
>>> +       err = devm_led_trigger_register(dev, &switch_port_rtl_hw_trigger);
>>> +       if (err)
>>> +               return dev_err_probe(dev, err, "failed to register private trigger");
>>> +
>>> +       child_count = of_get_child_count(np);
>>> +       dev_info(dev, "%d child nodes\n", child_count);
>> though this could be useful info in a post probe dev_info msg
>>
>> +       dev_dbg(dev, "%d child nodes\n", child_count);
>>
> Definitely dev_dbg() at most, or just nothing at all in the final version.
>
>>> +
>>> +       if (!child_count)
>>> +               return 0;
>>> +
>>> +       for_each_available_child_of_node(np, child) {
>>> +               if (of_n_addr_cells(child) != 2 || of_n_size_cells(child) != 0) {
>>> +                       dev_err(dev, "#address-cells (%d) is not 2 or #size-cells (%d)
>>> is not 0\n",
>>> +                               (u32) of_n_addr_cells(child), (u32)
>>> of_n_size_cells(child));
>> I often do this too; developer lazyness :) but for debugging reasons,
>> it's MUCH nicer to have 2 individual statements here, as it reads far
>> more pleasant. (Though you did already put the values in there;  In
>> terms of language, is `not exactly` is what I'd use though.
>>> +                       of_node_put(child);
>>> +                       return -EINVAL;
>>> +               }
>>> +
>>> +               if (!of_node_name_prefix(child, "led")) {
>>> +                       dev_dbg(dev, "skipping unsupported node %s\n",
>>> of_node_full_name(child));
>> quoting variables helps in identifying 'weird content' (empty strings,
>> spaces etc) so doing it for consistency is usually good.
>>
>> btw, I suppose the led framework doesn't have a nice getter? like the
>> gpiod_optional etc getters? the prefixes/names are part of the gpio
>> framework there (you can still supply a prefix optionally).
> They do require names to start with "led" in the binding, so maybe they do. I'll
> investigate.
>
>> +                       dev_dbg(dev, "skipping unsupported node '%s'\n",
>> of_node_full_name(child));
>>
>>> +                       continue;
>>> +               }
>>> +
>>> +               pled = switch_port_led_probe_single(ctrl, child);
>>> +               if (IS_ERR(pled)) {
>>> +                       dev_warn(dev, "failed to register led: %ld\n", PTR_ERR(pled));
>>> +                       continue;
>>> +               }
>>> +
>>> +               if (pled->index + 1 > leds_per_port)
>>> +                       leds_per_port = pled->index + 1;
>>> +
>>> +               if (leds_per_port > ctrl->cfg->port_led_count)
>> shouldn't this then be a `port_led_count_max`?
> Yes, would make sense to name it like that.
>
>>> +                       return dev_err_probe(dev, -EINVAL,
>>> +                               "too many LEDs per port: %d > %d\n",
>>> +                               leds_per_port, ctrl->cfg->port_led_count);
>>> +       }
>>> +
>> it's capture the return value; so you can do something like
>>
>> dev_info(dev, "probed X ports Y leds something pretty);
>> return ret; (we don't have a dev_info_probe do we :p)
> I've used dev_err_probe with a 0 return value, but that produces "error: SUCCESS" levels
> of confusing output.

ugh, yeah don't do that :)

so maybe having dev_info_probe is not a bad thing to have :p

>
>>> +       return ctrl->cfg->port_led_init(ctrl, leds_per_port);
>>> +}
>>> +
>>> +static const struct of_device_id of_switch_port_led_match[] = {
>>> +       {
>>> +               .compatible = "realtek,rtl8380-port-led",
>> the same on 8381 and 8382?
> Yes.
>
> Thanks for giving this such a thorough look!
>
> Best,
> Sander
>
>>> +               .data = &rtl8380_port_led_config
>>> +       },
>>> +       { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_switch_port_led_match);
>>> +
>>> +static struct platform_driver realtek_switch_port_led_driver = {
>>> +       .probe = realtek_port_led_probe,
>>> +       .driver = {
>>> +               .name = "realtek-switch-port-led",
>>> +               .of_match_table = of_switch_port_led_match
>>> +       }
>>> +};
>>> +module_platform_driver(realtek_switch_port_led_driver);
>>> +
>>> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
>>> +MODULE_DESCRIPTION("Realtek SoC switch port LED driver");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
>>> maple.patch b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
>>> maple.patch
>>> new file mode 100644
>>> index 000000000000..e33737a4f0da
>>> --- /dev/null
>>> +++ b/target/linux/realtek/patches-5.10/201-pinctrl-add-pinctrl-for-Realtek-
>>> maple.patch
>>> @@ -0,0 +1,51 @@
>>> +From 0efa8d564f27519d20fed39f5d650e50cbc34a0b Mon Sep 17 00:00:00 2001
>>> +From: Sander Vanheule <sander@svanheule.net>
>>> +Date: Mon, 11 Jul 2022 21:37:17 +0200
>>> +Subject: [PATCH 02/14] pinctrl: add pinctrl for Realtek maple
>>> +
>>> +Adds a pin control driver for RTL838x managed gigabit switch SoCs, and
>>> +RTL833x managed fast ethernet switch SoCs, both codenamed "Maple". Only
>>> +RTL838x has been tested, but functionality should be near identical.
>>> +
>>> +Since control bits for different muxes are scattered throughout the
>>> +switch core's register space, pin control features are provided by a
>>> +table of regmap fields. This should be flexible enough to allow
>>> +extension to other SoC generations providing similar features.
>>> +
>>> +Signed-off-by: Sander Vanheule <sander@svanheule.net>
>>> +---
>>> + drivers/pinctrl/Kconfig                  |  11 +
>>> + drivers/pinctrl/Makefile                 |   1 +
>>> + drivers/pinctrl/pinctrl-rtl-switchcore.c | 370 +++++++++++++++++++++++
>>> + 3 files changed, 382 insertions(+)
>>> + create mode 100644 drivers/pinctrl/pinctrl-rtl-switchcore.c
>>> +
>>> +--- a/drivers/pinctrl/Kconfig
>>> ++++ b/drivers/pinctrl/Kconfig
>>> +@@ -215,6 +215,16 @@ config PINCTRL_ROCKCHIP
>>> +       select MFD_SYSCON
>>> +       select OF_GPIO
>>> +
>>> ++config PINCTRL_RTL_SWITCHCORE
>>> ++      tristate "Realtek switch core pinctrl driver"
>>> ++      depends on MFD_REALTEK_SWITCHCORE || COMPILE_TEST
>>> ++      select PINMUX
>>> ++      select GENERIC_PINMUX_FUNCTIONS
>>> ++      select MFD_SYSCON
>>> ++      help
>>> ++        Driver for pin control fields in RTL83xx and RTL93xx managed ethernet
>>> ++        switch SoCs.
>>> ++
>>> + config PINCTRL_SINGLE
>>> +       tristate "One-register-per-pin type device tree based pinctrl driver"
>>> +       depends on OF
>>> +--- a/drivers/pinctrl/Makefile
>>> ++++ b/drivers/pinctrl/Makefile
>>> +@@ -30,6 +30,7 @@ obj-$(CONFIG_PINCTRL_PALMAS) += pinctrl-
>>> + obj-$(CONFIG_PINCTRL_PIC32)   += pinctrl-pic32.o
>>> + obj-$(CONFIG_PINCTRL_PISTACHIO)       += pinctrl-pistachio.o
>>> + obj-$(CONFIG_PINCTRL_ROCKCHIP)        += pinctrl-rockchip.o
>>> ++obj-$(CONFIG_PINCTRL_RTL_SWITCHCORE) += pinctrl-rtl-switchcore.o
>>> + obj-$(CONFIG_PINCTRL_SINGLE)  += pinctrl-single.o
>>> + obj-$(CONFIG_PINCTRL_SIRF)    += sirf/
>>> + obj-$(CONFIG_PINCTRL_SX150X)  += pinctrl-sx150x.o
>>> diff --git a/target/linux/realtek/rtl838x/config-5.10
>>> b/target/linux/realtek/rtl838x/config-5.10
>>> index 065948532057..5c69832be41d 100644
>>> --- a/target/linux/realtek/rtl838x/config-5.10
>>> +++ b/target/linux/realtek/rtl838x/config-5.10
>>> @@ -99,6 +99,7 @@ CONFIG_IRQ_MIPS_CPU=y
>>>    CONFIG_IRQ_WORK=y
>>>    CONFIG_JFFS2_ZLIB=y
>>>    CONFIG_LEDS_GPIO=y
>>> +CONFIG_LEDS_RTL_SWITCH_PORT=y
>>>    CONFIG_LEGACY_PTYS=y
>>>    CONFIG_LEGACY_PTY_COUNT=256
>>>    CONFIG_LIBFDT=y
>>>
>>
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel@lists.openwrt.org
>> https://lists.openwrt.org/mailman/listinfo/openwrt-devel