[RFC PATCH 2/7] realtek: add switch core MFD driver
Olliver Schinagl
oliver at schinagl.nl
Fri Jul 29 05:58:28 PDT 2022
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
> +#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.
> +
> +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 ...
> +#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))))
> +
> +#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] :)
+ char model_char[4] = { 0x00 };
is probably easier ;)
> + 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
> + 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!
> +
> + 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 ...
> +
> +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'.
btw, be careful with your constness, const is good; but you probably
want a const * const here no? (i always get confused :p)
> +{
> + 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 ;)
+ 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.
> +}
> +
> +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 ...
> +}
> +
> +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.
> + 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??
> + { /* 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;
> + 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?
> +
> + 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 ;)
> + 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?
> + 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
>
More information about the openwrt-devel
mailing list