[RFC PATCH 3/7] realtek: add pinctrl for Realtek maple
Sander Vanheule
sander at svanheule.net
Fri Jul 29 14:34:10 PDT 2022
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 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>
>
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, ¤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?
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 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
> >
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list