[RFC PATCH 3/7] realtek: add pinctrl for Realtek maple
Olliver Schinagl
oliver at schinagl.nl
Fri Jul 29 05:58:31 PDT 2022
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 at 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, ¤t_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 at 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 at 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 at 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
>
More information about the openwrt-devel
mailing list