[RFC PATCH 2/7] realtek: add switch core MFD driver
Sander Vanheule
sander at svanheule.net
Tue Aug 9 12:56:02 PDT 2022
Hi,
On Tue, 2022-08-09 at 17:57 +0200, Olliver Schinagl wrote:
> Hey Sander,
>
> On 29-07-2022 22:19, Sander Vanheule wrote:
> > On Fri, 2022-07-29 at 14:58 +0200, Olliver Schinagl wrote:
> > > Hey Sander,
> > >
> > > On 16-07-2022 21:09, Sander Vanheule wrote:
> > > > Realtek managed switch SoCs such as the RTL8380 consist of a MIPS CPU
> > > > with a number of basic peripherals, and an ethernet switch peripheral.
> > > > Besides performing ethernet related tasks, this switch core also
> > > > provides SoC management features. These switch core features are badly
> > > > separated, and cannot be divided into distinct IO ranges.
> > > >
> > > > This MFD core driver is intended to manage the switch core regmap, and
> > > > to expose some limited features that don't warrant their own driver.
> > > > These include SoC identification and system LED management.
> > > >
> > > > Signed-off-by: Sander Vanheule <sander at svanheule.net>
> > > > ---
> > > > .../drivers/mfd/realtek-switchcore.c | 244
> > > > ++++++++++++++++++
> > > > ...0-mfd-add-Realtek-switch-core-driver.patch | 50 ++++
> > > > target/linux/realtek/rtl838x/config-5.10 | 2 +
> > > > 3 files changed, 296 insertions(+)
> > > > create mode 100644 target/linux/realtek/files-
> > > > 5.10/drivers/mfd/realtek-switchcore.c
> > > > create mode 100644 target/linux/realtek/patches-5.10/200-mfd-add-
> > > > Realtek-switch-
> > > > core-driver.patch
> > > >
> > > > diff --git a/target/linux/realtek/files-5.10/drivers/mfd/realtek-
> > > > switchcore.c
> > > > b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
> > > > new file mode 100644
> > > > index 000000000000..5b3f6eee6fe2
> > > > --- /dev/null
> > > > +++ b/target/linux/realtek/files-5.10/drivers/mfd/realtek-switchcore.c
> > > > @@ -0,0 +1,244 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +
> > > > +#include <linux/bitfield.h>
> > > Probably want bitops.h here as well, as BIT() and GENMASK() come from
> > > there
> > Will add.
> >
> > > > +#include <linux/leds.h>
> > > > +#include <linux/mfd/core.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +struct realtek_switchcore_ctrl;
> > > I take it a switchcore is the generic name, and an rtl8380 etc just
> > > implements one? Makes sense if so.
> > Switchcore is the name of the ASIC peripheral that handles the networking,
> > and is really
> > the system controller as well, managing pin muxes and such. RTL838x (Maple)
> > SoC have one
> > register layout, RTL839x (Cypress) has another set. In one generation
> > (RTL83xx, RTL93xx)
> > the provided features are usually similar, but every SoC type really
> > requires its own
> > implementation.
> >
> > > > +
> > > > +struct realtek_switchcore_data {
> > > > + struct reg_field sys_led_field;
> > > > + const struct mfd_cell *mfd_devices;
> > > > + unsigned int mfd_device_count;
> > > > + void (*probe_model_name)(const struct realtek_switchcore_ctrl
> > > > *ctrl);
> > > > +};
> > > > +
> > > > +struct realtek_switchcore_ctrl {
> > > > + struct device *dev;
> > > > + struct regmap *map;
> > > > + const struct realtek_switchcore_data *data;
> > > > + struct led_classdev sys_led;
> > > > + struct regmap_field *sys_led_field;
> > > > + bool active_low;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Model name probe
> > > > + *
> > > > + * Reads the family-specific MODEL_NAME_INFO register
> > > > + * to identify the SoC model and revision
> > > > + */
> > > any reasons to put the defines close to the function rather then before
> > > the code as is commonly done? I'm not saying it's a bad idea; it just
> > > not what is done normally ...
> > I put the RTL838x defines here, because I was planning to put the
> > information for one SoC
> > type together. The register definitions could go into a separate MFD header
> > though, but I
> > kept them here for now to more easily maintain an overview.
> >
> > > > +#define RTL8380_REG_MODEL_NAME_INFO 0x00d4
> > > > +#define RTL8380_REG_CHIP_INFO 0x00d8
> > > > +#define MODEL_NAME_CHAR_XLATE(val) ((val) ? 'A' + (val) - 1
> > > > : '\0')
> > > > +#define MODEL_NAME_CHAR(reg, msb,
> > > > lsb) (MODEL_NAME_CHAR_XLATE(FIELD_GET(GENMASK((msb), (lsb)),
> > > > (val))))
> > > > +
> > These macro's are certainly not SoC-specific, and (following conventions)
> > should indeed be
> > higher up in the file.
> >
> > > > +#define RTL8380_REG_INT_RW_CTRL 0x0058
> > > _personally_ i prefer alignment with spaces here so that any changes in
> > > tab with (for indentation) is not affecting the alignment here.
> > > > +
> > > > +static void rtl8380_probe_model_name(const struct
> > > > realtek_switchcore_ctrl *ctrl)
> > > > +{
> > > > + char model_char[4] = {0, 0, 0, 0};
> > > While I love being explicit, what if you have [20] :)
> > Then I shouldn't be putting this on the stack either, when this becomes too
> > big. But for
> > all current SoC series, 3 is the maximum number of characters that can be
> > defined in the
> > model name (plus NULL termination)
> I was being a smart-ass; but you answerd yourself below, that {} is
> probably better as your initializer.
> >
> > > + char model_char[4] = { 0x00 };
> > >
> > > is probably easier ;)
> > memset() would also work, but I should be able to use {} if we just want to
> > empty-
> > initialise a const-size array.
> sure, but the { 0x00 }, initializer auto-repeats itself for all entries.
> >
> >
> > > > + char chip_rev;
> > > > + u32 model_id;
> > > > + u32 val = 0;
> > > > +
> > > > + regmap_read(ctrl->map, RTL8380_REG_MODEL_NAME_INFO, &val);
> > > > + model_id = FIELD_GET(GENMASK(31, 16), val);
> > > > + model_char[0] = MODEL_NAME_CHAR(val, 15, 11);
> > > > + model_char[1] = MODEL_NAME_CHAR(val, 10, 6);
> > > > + model_char[2] = MODEL_NAME_CHAR(val, 5, 1);
> > > > +
> > > > + /* CHIP_INFO register must be unlocked by writing 0xa to the top
> > > > bits */
> > > > + regmap_write(ctrl->map, RTL8380_REG_CHIP_INFO,
> > > > FIELD_PREP(GENMASK(31, 28), 0xa));
> > > Personally, I prefer self-explanatory code; so having a function
> > > 'rtl_switchcore_unlock() makes it readable as code, and allows for re-use
> > This only unlocks the register with model info. If this is the same for all
> > (some) models,
> > it could be split out into a small function.
> what I also kind a meant, if it's a small function, it becomes
> self-describing because of its function name ;)
Sure, makes sense to add a small (inline) function for register unlocking.
> >
> > > > + regmap_read(ctrl->map, RTL8380_REG_CHIP_INFO, &val);
> > > > + chip_rev = MODEL_NAME_CHAR(val, 20, 16) ?: '0';
> > > I'm new to this syntax, so MODEL_NAME_CHAR produces a value, and if so
> > > it gets used, otherwise '0', right? so it's the same as:
> > >
> > > + chip_rev = MODEL_NAME_CHAR(val, 20, 16) ? MODEL_NAME_CHAR(val, 20,
> > > 16)
> > > : '0';
> > >
> > > This surely is from a newer version of the C standard is it not? I would
> > > expect that you
> > > assign either '0' or 'nothing'? Learned something new today!
> > Yes, that's what this means. AFAIK it's a GCC extension, but it is used in
> > other places in
> > the kernel too.
> >
> > > > +
> > > > + dev_info(ctrl->dev, "found RTL%04x%s rev. %c\n", model_id,
> > > > model_char, chip_rev);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Realtek hardware system LED
> > > > + *
> > > > + * The switch SoC supports one hardware managed direct LED output
> > > > + * to manage a system LED, with two supported blinking rates.
> > > > + */
> > > > +enum {
> > > > + SWITCHCORE_SYS_LED_OFF = 0,
> > > > + SWITCHCORE_SYS_LED_BLINK_64MS,
> > > > + SWITCHCORE_SYS_LED_BLINK_1024MS,
> > > > + SWITCHCORE_SYS_LED_ON
> > > > +};
> > > I recall someone else in your patch series/this patch, you have a struct
> > > with off/on and an struct afaik with the various blink rates, is there
> > > any relation? Does this need to be different? Or is it that the system
> > > led is completely different/behaves differently from the networking
> > > leds? (simpler/cheaper on the die for sure) but interface wise, does it
> > > need to be different ...
> > Yes, this is a common pattern, and is actually also used on the RTL8231 [1].
> > Maybe a
> > driver/leds/realtek/ directory would be warranted, with realtek-leds-
> > core.{h,c} to provide
> > some common code.
> >
> > The sys-led peripheral is so limited however, that I didn't think it
> > worthwhile to write a
> > separate driver for it. But then again, a small driver is easier to review
> > and now I made
> > the MFD core driver more complicated. :)
> >
> > [1]
> > https://lore.kernel.org/all/bf17f2e45ea8f8beb8db556c635db38fa7112d33.1623532208.git.sander@svanheule.net/
> >
> > > > +
> > > > +static void switchcore_sys_led_set_rate(const struct
> > > > realtek_switchcore_ctrl *ctrl,
> > > > + unsigned int mode)
> > > Linus nowdays prefers to have slightly longer but cleaner lines :)
> > >
> > > and i think checkpatch will tell you to align to 'const'.
> > I always use this style, and checkpatch doesn't complain about it. Reviewers
> > do though ;)
> > Sometimes aligning with the opening parenthesis would push the code really
> > far to the
> > right, so I prefer to just have it consistently indented with one extra tab.
> >
> > > btw, be careful with your constness, const is good; but you probably
> > > want a const * const here no? (i always get confused :p)
> > The value pointed to by the pointer is const, so we can't touch that, but we
> > could change
> > our local copy of the pointer and have it point somewhere else, yes. I could
> > also make
> > 'mode' const, but then a lot code would have to be updated.
> >
> > > > +{
> > > > + regmap_field_write(ctrl->sys_led_field, mode);
> > > > +}
> > > > +
> > > > +static void switchcore_sys_led_brightness_set(struct led_classdev
> > > > *led_cdev,
> > > > + enum led_brightness brightness)
> > > > +{
> > > > + struct realtek_switchcore_ctrl *ctrl =
> > > > + container_of(led_cdev, struct realtek_switchcore_ctrl,
> > > > sys_led);
> > > > +
> > > > + if ((!ctrl->active_low && brightness == LED_OFF) ||
> > > I know very smart people are really good in knowing all operator
> > > precedence by heart, but for all readers, having extra parenthesis is
> > > much clearer and far more readable. (I never understood why in school
> > > they where teaching this anyway, thou shall memorize operator
> > > precedence, because being explicit and using parenthesis is bad) there's
> > > lot of cases in this series; so I won't comment on all of them ;)
> > Might get complaints from reviewers for using too many parentheses then...
> > When I read
> > this, I see "[boolean] A is false and [non-boolean] B equals C". I
> > personally don't find
> > this to be too confusing. Perhaps ordering it as "B == C && !A" would be
> > clearer, as it
> > aligns with operator precedence.
>
> The general direction that I've seen upstream, is that the whole 'use as
> little parenthesis as possible, know your operator precedence) has kind
> of gone. Being more precies is preferred.
>
>
> Also, be very careful, 'what you see/read' is not what someone else
> see's/reads; and that's there the crux of the matter is. Adding the
> parenthesis doesn't leave _anything_ up to interpretation.
>
>
> While I agree with useless parenthesis (hard to find, but I'm sure
> people find a way to create those), anybody that says 'too many
> parenthesis, know your operator precedence' shouldn't be allowed to
> review :p Any bug that was caused by lack of parenthesis, was a
> unnecessary one driven by this odd desire to reduce parenthesis for no
> good reason.
Too many parentheses can make things quite heavy IMHO, but I'll go over the
files to see where I can improve things.
> >
> > > + if ((!ctrl->active_low && (brightness == LED_OFF)) ||
> > >
> > > > + (ctrl->active_low && brightness != LED_OFF))
> > > > + switchcore_sys_led_set_rate(ctrl,
> > > > SWITCHCORE_SYS_LED_OFF);
> > > > + else
> > > > + switchcore_sys_led_set_rate(ctrl,
> > > > SWITCHCORE_SYS_LED_ON);
> > > > +}
> > > > +
> > > > +static enum led_brightness switchcore_sys_led_brightness_get(
> > > > + struct led_classdev *led_cdev)
> > > > +{
> > > > + struct realtek_switchcore_ctrl *ctrl =
> > > > + container_of(led_cdev, struct realtek_switchcore_ctrl,
> > > > sys_led);
> > > > + u32 val;
> > > > +
> > > > + regmap_field_read(ctrl->sys_led_field, &val);
> > > > +
> > > > + if ((!ctrl->active_low && val == SWITCHCORE_SYS_LED_OFF) ||
> > > > + (ctrl->active_low && val == SWITCHCORE_SYS_LED_ON))
> > > > + return LED_OFF;
> > > > + else
> > > > + return LED_ON;
> > > I know i state below to use this :p but according to 'led.h' I see that
> > > this is obsolete/useless (not sure what the replacement is though).
> > > LED_FULL is maybe the replacement? I don't know to be honest.
> > You're right, LED_{ON,OFF} is deprecated and I should just return 0 or 1,
> > consistent with
> > max_brightness being 1.
> I'm not sure if that is correct however; I'd have to digg in some other
> examples, but they speak of 'dynamic max brightness' which more aligns
> with 0 - 255. I've seen more 0/255 then 0/1, but I have no idea what
> macro's for that.
0-255 implies that 123 is also valid, with a unique brightness, but it isn't.
These LED outputs are binary on/off, which maps to a brightness range of 0-1.
> > > > +}
> > > > +
> > > > +static int switchcore_sys_led_blink_set(struct led_classdev *led_cdev,
> > > > + unsigned long *delay_on, unsigned long *delay_off)
> > > > +{
> > > > + struct realtek_switchcore_ctrl *ctrl =
> > > > + container_of(led_cdev, struct realtek_switchcore_ctrl,
> > > > sys_led);
> > > > + u32 blink_interval = *delay_on + *delay_off;
> > > > +
> > > > + /* Split range at geometric mean of 64 and 1024 */
> > > > + if (blink_interval == 0 || blink_interval > 2 * 256) {
> > > > + *delay_on = 1024;
> > > > + *delay_off = 1024;
> > > > + switchcore_sys_led_set_rate(ctrl,
> > > > SWITCHCORE_SYS_LED_BLINK_1024MS);
> > > > + } else {
> > > > + *delay_on = 64;
> > > > + *delay_off = 64;
> > > > + switchcore_sys_led_set_rate(ctrl,
> > > > SWITCHCORE_SYS_LED_BLINK_64MS);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int switchcore_sys_led_probe(struct realtek_switchcore_ctrl
> > > > *ctrl,
> > > > + struct device_node *np)
> > > > +{
> > > > + struct led_classdev *sys_led = &ctrl->sys_led;
> > > > + struct led_init_data init_data = {};
> > > > +
> > > > + init_data.fwnode = of_fwnode_handle(np);
> > > > +
> > > > + ctrl->sys_led_field =
> > > > + devm_regmap_field_alloc(ctrl->dev, ctrl->map, ctrl-
> > > > >data-
> > > > > sys_led_field);
> > > btw, here you can also see why these alignment hacks are tricky, because
> > > it now looks like it's a indentation, just like the if below :) so if
> > > you insist on splitting it on multiple lines, it is more common to do
> > > the split on the function args after the comma
> > > > + if (IS_ERR(ctrl->sys_led_field))
> > > > + return PTR_ERR(ctrl->sys_led_field);
> > > > +
> > > > + ctrl->active_low = of_property_read_bool(np, "active-low");
> > > > +
> > > > + sys_led->max_brightness = 1;
> > > we don't have define or fixed value for this we can reference instead?
> > > LED_FULL or something?
> > > > + sys_led->brightness_set = switchcore_sys_led_brightness_set;
> > > > + sys_led->brightness_get = switchcore_sys_led_brightness_get;
> > > > + sys_led->blink_set = switchcore_sys_led_blink_set;
> > > interesting, we don't do an 'ops-struct' here?
> > > > +
> > > > + return devm_led_classdev_register_ext(ctrl->dev, sys_led,
> > > > &init_data);
> > > is it worth while to (also, based on compatible?) use
> > > `devm_led_classdev_multicolor_register_ext`? I know this is tricky, the
> > > led drive supports both, and when connecting an RGB led, you really are
> > > looking into driving a multi-color led? On the one hand, single mode
> > > ledclass is easier in many ways, but from userspace, might be easier to
> > > have an RGB led as a multi-color one. Just something to mull over ...
> > No, multicolor LEDs really require a different type of driver, controlling
> > all color
> > components at the same time. There is some recent work [2] to allow
> > combining individual
> > (GPIO) LEDs into a single multicolor LED from the device tree.
> >
> > [2]
> > https://lore.kernel.org/all/20220615154918.521687-1-jjhiblot@traphandler.com/
> >
> > > > +}
> > > > +
> > > > +static const struct mfd_cell rtl8380_mfd_devices[] = {
> > > > + OF_MFD_CELL("realtek-switchcore-port-leds",
> > > > + NULL, NULL, 0, 0, "realtek,rtl8380-port-led"),
> > > > + OF_MFD_CELL("realtek-switchcore-aux-mdio",
> > > > + NULL, NULL, 0, 0, "realtek,rtl8380-aux-mdio"),
> > > can we not call it "realtek,rtl8380-aux-mdio", I'll probably write the
> > > same comment in the pinctrl; but it's an external pin, it's a pad/pin,
> > > so prefixing with aux adds no value here does it? it's our mdio interface.
> > It's called aux-mdio because this is the auxiliary/secondary MDIO master,
> > used to talk to
> > RTL8231 GPIO expanders; although it could in theory talk to any MDIO device.
> > The main MDIO
> > master (for ethernet phy-s) is in a different place and already handled by
> > the current
> > ethernet drivers.
> I understand, but from a hardware pov; this is really your only MDIO,
> exactly because it's the only external/AuX one. So you could argue, the
> 'other' one being 'INT-MDIO' to indicate its internalness. Saying 'the
> external AUX MDIO interface' is like thus a bit weird linguistically.
> (aux coming from auxiliary, meaning external/outside afaik)
The RTL838x series has (what I call) auxilairy MDIO on pins 110 and 111 (muxed
with GPIO3 and GPIO2); used to communicate with RTL8231 chips. The switches with
more than 8 ports all have external phy-s, which does require external presence
of an MDIO bus. The bus used for this purpose is on pins 120 and 121, and is the
one I would refer to as the "main MDIO" bus. The two MDIO busses have separate
control registers and device address spaces.
> >
> > > > + OF_MFD_CELL("realtek-switchcore-pinctrl",
> > > > + NULL, NULL, 0, 0, "realtek,rtl8380-pinctrl"),
> > > > +};
> > > > +
> > > > +static const struct realtek_switchcore_data rtl8380_switchcore_data = {
> > > > + .sys_led_field = REG_FIELD(0xa000, 16, 17),
> > > > + .mfd_devices = rtl8380_mfd_devices,
> > > > + .mfd_device_count = ARRAY_SIZE(rtl8380_mfd_devices),
> > > > + .probe_model_name = rtl8380_probe_model_name,
> > > > +};
> > > > +
> > > > +static const struct of_device_id of_realtek_switchcore_match[] = {
> > > > + {
> > > > + .compatible = "realtek,rtl8380-switchcore",
> > > > + .data = &rtl8380_switchcore_data,
> > > > + },
> > > does this driver also support rtl8381 and rtl8382??
> > It does, but I didn't want to use the family name (maple) and have already
> > used "rtl8380"
> > as a family-placeholder in other drivers. Same goes for "rtl8390", which
> > does not collide
> > with the name of an actual SoC.
>
> I wanted to maybe say, make sure you also add your compatibles for the
> 81 and 82 if you already support these. See the other discussion about
> the SPI compatible.
>
> Our realtek compatible's are a bit messy atm as it is.
>
> Calling your datastructure rtl838x_switchcore_data is of course fine,
> this is internal data, and using it on all 3 compatibles is a-ok.
Taking the end goal of this driver into account, which includes ethernet
support, this makes sense. The RTL8380/81/82 support different amounts of port,
which may warrant a different compatible. Perhaps a two-part compatible makes
sense here, since the register layouts are otherwise identical:
compatible = "realtek,rtl8381-switchcore", "realtek,maple-switchcore";
For the RTL8381M specifically, this is even more important because the ID
register will tell you it's an RTL8380M. The SDK reads an extra undocumented in
some debug register to find out which version it's currently running on.
The fallback compatible in turn, would allow for new versions of some generation
to show up, without having to immediately update the driver during development.
> >
> > > > + { /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, of_realtek_switchcore_match);
> > > > +
> > > > +static int realtek_switchcore_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > + struct device_node *np = dev->of_node;
> > > > + struct device_node *np_sys_led;
> > > > + const struct of_device_id *match;
> > > > + struct realtek_switchcore_ctrl *ctrl;
> > > > + int err;
> > > > +
> > > > + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> > > > + if (!ctrl)
> > > > + return -ENOMEM;
> > > > +
> > > > + match = of_match_device(of_realtek_switchcore_match, dev);
> > > > + if (match)
> > > > + ctrl->data = (struct realtek_switchcore_data *) match-
> > > > >data;
> > > > + else
> > > > + return dev_err_probe(dev, -EINVAL, "no device match\n");
> > > What's wrong with doing the standard 'fail early, fail often' approach?
> > > It also removes one level of indentation. E.g.
> > >
> > > + if (!match)
> > > + return dev_err_probe(dev, -EINVAL, "no device match found
> > > in
> > > devicetree\n");
> > > +
> > > + ctrl->data = (struct realtek_switchcore_data *) match->data;
> > Must've missed this one, normally I do try to use the structure you're
> > proposing. That
> > being said, this could use of_device_get_match_data() instead. If that
> > fails, then the
> > developer did something wrong because a compatible that triggers this driver
> > should always
> > provide match data.
> even better!
> >
> > > > + ctrl->dev = dev;
> > > > +
> > > > + if (!np)
> > > > + return dev_err_probe(dev, -ENODEV, "no DT node
> > > > found\n");
> > > I'd put this check first; since its your 'cheapest' (no processing
> > > needed) so fail earlier here, instead of doing 'expensive' operations
> > > first, just to fail on something simple here (yes, its only the probe
> > > that doesn't happen often, I know :p) Also, it makes logically a bit
> > > more sense, you fail on a missing DT node here, but appearantly we did
> > > find the compatible?
> > Sure, could be moved up.
> >
> > > > +
> > > > + ctrl->map = device_node_to_regmap(np);
> > > > + if (!ctrl->map)
> > > > + return dev_err_probe(dev, -ENXIO, "failed to get
> > > > regmap\n");
> > > > +
> > > > + if (ctrl->data->probe_model_name)
> > > > + ctrl->data->probe_model_name(ctrl);
> > > > +
> > > > + /* Parse optional led-sys child */
> > > Is this due to the MFD nature of this driver? wouldn't it be nicer to
> > > have a specific function for all the led-mfd-ness to group it nicely
> > > together? makes also for self documenting code by function name ;)
> > I did consider grouping this in a separate function at some point. Guess I
> > didn't get
> > around to actually doing it.
> >
> > > > + np_sys_led = of_get_child_by_name(np, "led-sys");
> > > > + if (IS_ERR(np_sys_led))
> > > > + return PTR_ERR(np_sys_led);
> > > > +
> > > > + if (np_sys_led && of_device_is_available(np_sys_led)) {
> > > > + err = switchcore_sys_led_probe(ctrl, np_sys_led);
> > > > + if (err)
> > > > + dev_warn(dev, "probing of system led failed
> > > > %d\n", err);
> > > > + }
> > > > +
> > > > + /* Find sub-devices */
> > > hmm, this comment doesn't match the code, does it? It actually adds all
> > > subdevices?
> > It does match the code in my opinion, but isn't actually that useful because
> > the code
> > really says it al. data->mfd_devices also should never be empty, so maybe
> > the leading "if"
> > isn't actually that useful.
> well the comment says 'find sub-devices' but your not 'finding'
> anything, you are not searching. You just add any device you have. or at
> least, that is what the code is saying. If you where finding/searching
> i'd expect some loop or something?
That loop is (somewhere) inside mfd_add_devices(). I suppose that function's
name should serve as sufficient documentation, so the comment can be removed.
> >
> > Best,
> > Sander
> >
> > > > + if (ctrl->data->mfd_devices)
> > > > + mfd_add_devices(dev, 0, ctrl->data->mfd_devices,
> > > > + ctrl->data->mfd_device_count, NULL, 0, NULL);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver realtek_switchcore_driver = {
> > > > + .probe = realtek_switchcore_probe,
> > > > + .driver = {
> > > > + .name = "realtek-switchcore",
> > > > + .of_match_table = of_realtek_switchcore_match
> > > > + }
> > > > +};
> > > > +module_platform_driver(realtek_switchcore_driver);
> > > > +
> > > > +MODULE_AUTHOR("Sander Vanheule <sander at svanheule.net>");
> > > > +MODULE_DESCRIPTION("Realtek SoC switch core driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/target/linux/realtek/patches-5.10/200-mfd-add-Realtek-
> > > > switch-core-
> > > > driver.patch b/target/linux/realtek/patches-5.10/200-mfd-add-Realtek-
> > > > switch-core-
> > > > driver.patch
> > > > new file mode 100644
> > > > index 000000000000..e0f4678f28f3
> > > > --- /dev/null
> > > > +++ b/target/linux/realtek/patches-5.10/200-mfd-add-Realtek-switch-core-
> > > > driver.patch
> > > > @@ -0,0 +1,50 @@
> > > > +From 2b4df5433baaa10eeb23a58012329d4ec47216ba Mon Sep 17 00:00:00 2001
> > > > +From: Sander Vanheule <sander at svanheule.net>
> > > > +Date: Mon, 11 Jul 2022 09:24:18 +0200
> > > > +Subject: [PATCH 01/14] mfd: add Realtek switch core driver
> > > > +
> > > > +Realtek managed switch SoCs such as the RTL8380 consist of a MIPS CPU
> > > > +with a number of basic peripherals, and an ethernet switch peripheral.
> > > > +Besides performing ethernet related tasks, this switch core also
> > > > +provides SoC management features. These switch core features are badly
> > > > +separated, and cannot be divided into distinct IO ranges.
> > > > +
> > > > +This MFD core driver is intended to manage the switch core regmap, and
> > > > +to expose some limited features that don't warrant their own driver.
> > > > +These include SoC identification and system LED management.
> > > > +
> > > > +Signed-off-by: Sander Vanheule <sander at svanheule.net>
> > > > +---
> > > > + drivers/mfd/Kconfig | 11 ++
> > > > + drivers/mfd/Makefile | 2 +
> > > > + drivers/mfd/realtek-switchcore.c | 242 +++++++++++++++++++++++++++++++
> > > > + 3 files changed, 255 insertions(+)
> > > > + create mode 100644 drivers/mfd/realtek-switchcore.c
> > > > +
> > > > +--- a/drivers/mfd/Kconfig
> > > > ++++ b/drivers/mfd/Kconfig
> > > > +@@ -1000,6 +1000,16 @@ config MFD_RETU
> > > > + Retu and Tahvo are a multi-function devices found on Nokia
> > > > + Internet Tablets (770, N800 and N810).
> > > > +
> > > > ++config MFD_REALTEK_SWITCHCORE
> > > > ++ tristate "Realtek switch core driver"
> > > > ++ select MFD_CORE
> > > > ++ select REGMAP_MMIO
> > > > ++ help
> > > > ++ Say yes here if you want to support the switch core found in
> > > > RTL838x,
> > > > ++ RTL839x, RTL930x, and RTL931x SoCs. The switch core provides
> > > > ethernet
> > > > ++ functionality, and management features for the SoC like pin
> > > > control.
> > > > ++ The mfd cell drivers have to be selected separately.
> > > > ++
> > > > + config MFD_PCF50633
> > > > + tristate "NXP PCF50633"
> > > > + depends on I2C
> > > > +--- a/drivers/mfd/Makefile
> > > > ++++ b/drivers/mfd/Makefile
> > > > +@@ -267,3 +267,5 @@ obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-
> > > > + obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> > > > + obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o
> > > > + obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o
> > > > ++
> > > > ++obj-$(CONFIG_MFD_REALTEK_SWITCHCORE) += realtek-switchcore.o
> > > > diff --git a/target/linux/realtek/rtl838x/config-5.10
> > > > b/target/linux/realtek/rtl838x/config-5.10
> > > > index c117fc489a81..065948532057 100644
> > > > --- a/target/linux/realtek/rtl838x/config-5.10
> > > > +++ b/target/linux/realtek/rtl838x/config-5.10
> > > > @@ -109,6 +109,8 @@ CONFIG_MDIO_DEVICE=y
> > > > CONFIG_MDIO_DEVRES=y
> > > > CONFIG_MDIO_I2C=y
> > > > CONFIG_MEMFD_CREATE=y
> > > > +CONFIG_MFD_CORE=y
> > > > +CONFIG_MFD_REALTEK_SWITCHCORE=y
> > > > CONFIG_MFD_SYSCON=y
> > > > CONFIG_MIGRATION=y
> > > > CONFIG_MIPS=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