[v2,7/7] leds: Add support for RTL8231 LED scan matrix

Message ID 752444cff2a7ec5da38dba368c64a5ed7dd87279.1621279162.git.sander@svanheule.net
State New
Headers show
Series
  • [v2,1/7] regmap: Add MDIO bus support
Related show

Commit Message

Sander Vanheule May 17, 2021, 7:28 p.m.
Both single and bi-color scanning modes are supported. The driver will
verify that the addresses are valid for the current mode, before
registering the LEDs. LEDs can be turned on, off, or toggled at one of
six predefined rates from 40ms to 1280ms.

Implements a platform device for use as child device with RTL8231 MFD,
and uses the parent regmap to access the required registers.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/leds/Kconfig        |  10 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-rtl8231.c | 293 ++++++++++++++++++++++++++++++++++++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/leds/leds-rtl8231.c

Comments

Andy Shevchenko May 17, 2021, 10 p.m. | #1
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Both single and bi-color scanning modes are supported. The driver will
> verify that the addresses are valid for the current mode, before
> registering the LEDs. LEDs can be turned on, off, or toggled at one of
> six predefined rates from 40ms to 1280ms.
>
> Implements a platform device for use as child device with RTL8231 MFD,

as a child

> and uses the parent regmap to access the required registers.

...

> +         When built as a module, this module will be named rtl8231_leds.

Again, what it's written here is not what is in Makefile.

> +obj-$(CONFIG_LEDS_RTL8231)             += leds-rtl8231.o

...

> +/**
> + * struct led_toggle_rate - description of an LED blinking mode
> + * @interval:  LED toggle rate in ms
> + * @mode:      Register field value used to active this mode

activate

> + *
> + * For LED hardware accelerated blinking, with equal on and off delay.
> + * Both delays are given by @interval, so the interval at which the LED blinks
> + * (i.e. turn on and off once) is double this value.
> + */

...

> +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
> +{
> +       unsigned int mode;

> +       unsigned int i = 0;

This...

> +       if (regmap_field_read(pled->reg_field, &mode))
> +               return 0;
> +
> +       while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode)
> +               i++;

...and this will be better as a for-loop.

> +       if (i < pled->modes->num_toggle_rates)
> +               return pled->modes->toggle_rates[i].interval;

> +       else

Redundant.

> +               return 0;
> +}

...

> +       unsigned int i = 0;

As per above.

...

> +               interval = 500;

interval_ms

> +               /*
> +                * If the current mode is blinking, choose the delay that (likely) changed.
> +                * Otherwise, choose the interval that would have the same total delay.
> +                */
> +               interval = rtl8231_led_current_interval(pled);

> +

Redundant blank line.

> +               if (interval > 0 && interval == *delay_off)
> +                       interval = *delay_on;
> +               else if (interval > 0 && interval == *delay_on)
> +                       interval = *delay_off;
> +               else
> +                       interval = (*delay_on + *delay_off) / 2;
> +       }

...

> +       u32 addr[2];
> +       int err;
> +

> +       if (!fwnode_property_count_u32(fwnode, "reg"))

err = fwnode_property_count_u32(...);
if (err < 0)
  return err;
if (err == 0)
  return -ENODEV;

> +               return -ENODEV;
> +
> +       err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr));

If count returns 1? What's the point of counting if you always want two?

> +       if (err)
> +               return err;
> +
> +       *addr_port = addr[0];
> +       *addr_led = addr[1];
> +
> +       return 0;
> +}

...

> +       pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL);
> +       if (IS_ERR(pled))

Wrong.

> +               return PTR_ERR(pled);

...

> +       err = rtl8231_led_read_address(fwnode, &port_index, &led_index);
> +

Redundant blank line.

> +       if (err) {
> +               dev_err(dev, "LED address invalid\n");
> +               return err;

> +       } else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) {

Redundant 'else'

> +               dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index);
> +               return -ENODEV;
> +       }

...

> +       map = dev_get_regmap(dev->parent, NULL);
> +       if (IS_ERR_OR_NULL(map)) {

Split it into two conditionals.

> +               dev_err(dev, "failed to retrieve regmap\n");
> +               if (!map)
> +                       return -ENODEV;
> +               else
> +                       return PTR_ERR(map);
> +       }

...

> +       if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) {

It seems that device_property_match_string() and accompanying
functions have wrong description of returned codes, i.e. it returns
the index of the matched string. It's possible that some APIs are
broken (but I believe that the former is the case).

That said, I think the proper comparison should be >= 0.

> +               port_counts = rtl8231_led_port_counts_single;
> +               regmap_update_bits(map, RTL8231_REG_FUNC0,
> +                       RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE);
> +       } else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) {

Ditto.

> +               port_counts = rtl8231_led_port_counts_bicolor;
> +               regmap_update_bits(map, RTL8231_REG_FUNC0,
> +                       RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR);
> +       } else {
> +               dev_err(dev, "scan mode missing or invalid\n");
> +               return -EINVAL;
> +       }
Sander Vanheule May 23, 2021, 9:53 p.m. | #2
Hi Andy,

Also here I've tried to address your remarks for v3, some extra details below.

On Tue, 2021-05-18 at 01:00 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:
> > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
> > +{
> > +       unsigned int mode;
> 
> > +       unsigned int i = 0;
> 
> This...
> 
> > +       if (regmap_field_read(pled->reg_field, &mode))
> > +               return 0;
> > +
> > +       while (i < pled->modes->num_toggle_rates && mode != pled->modes-
> > >toggle_rates[i].mode)
> > +               i++;
> 
> ...and this will be better as a for-loop.
> 
> > +       if (i < pled->modes->num_toggle_rates)
> > +               return pled->modes->toggle_rates[i].interval;
> 
> > +       else
> 
> Redundant.
> 
> > +               return 0;
> > +}

Shrunk down to "for (...) if (...) return ...;" in v3.


> 
> > +               interval = 500;
> 
> interval_ms

Good suggestion, thanks. Don't need those comments in the code then.


> 
> > +       u32 addr[2];
> > +       int err;
> > +
> 
> > +       if (!fwnode_property_count_u32(fwnode, "reg"))
> 
> err = fwnode_property_count_u32(...);
> if (err < 0)
>   return err;
> if (err == 0)
>   return -ENODEV;
> 
> > +               return -ENODEV;
> > +
> > +       err = fwnode_property_read_u32_array(fwnode, "reg", addr,
> > ARRAY_SIZE(addr));
> 
> If count returns 1? What's the point of counting if you always want two?

If count returns 1, or more than 2, that's an error. But this check was missing
in v2, so I added it in v3.


> 
> > +       if (!device_property_match_string(dev, "realtek,led-scan-mode",
> > "single-color")) {
> 
> It seems that device_property_match_string() and accompanying
> functions have wrong description of returned codes, i.e. it returns
> the index of the matched string. It's possible that some APIs are
> broken (but I believe that the former is the case).
> 
> That said, I think the proper comparison should be >= 0.

Thanks, fixed.


Best,
Sander

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 49d99cb084db..e5ff6150800c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -593,6 +593,16 @@  config LEDS_REGULATOR
 	help
 	  This option enables support for regulator driven LEDs.
 
+config LEDS_RTL8231
+	tristate "RTL8231 LED matrix support"
+	depends on LEDS_CLASS
+	depends on MFD_RTL8231
+	default MFD_RTL8231
+	help
+	  This options enables support for using the LED scanning matrix output
+	  of the RTL8231 GPIO and LED expander chip.
+	  When built as a module, this module will be named rtl8231_leds.
+
 config LEDS_BD2802
 	tristate "LED driver for BD2802 RGB LED"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7e604d3028c8..ce0f44a87dee 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -80,6 +80,7 @@  obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
 obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
+obj-$(CONFIG_LEDS_RTL8231)		+= leds-rtl8231.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_SGM3140)		+= leds-sgm3140.o
diff --git a/drivers/leds/leds-rtl8231.c b/drivers/leds/leds-rtl8231.c
new file mode 100644
index 000000000000..7712bbaaf7c1
--- /dev/null
+++ b/drivers/leds/leds-rtl8231.c
@@ -0,0 +1,293 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/rtl8231.h>
+
+/**
+ * struct led_toggle_rate - description of an LED blinking mode
+ * @interval:	LED toggle rate in ms
+ * @mode:	Register field value used to active this mode
+ *
+ * For LED hardware accelerated blinking, with equal on and off delay.
+ * Both delays are given by @interval, so the interval at which the LED blinks
+ * (i.e. turn on and off once) is double this value.
+ */
+struct led_toggle_rate {
+	u16 interval;
+	u8 mode;
+};
+
+/**
+ * struct led_modes - description of all LED modes
+ * @toggle_rates:	Array of led_toggle_rate values, sorted by ascending interval
+ * @num_toggle_rates:	Number of elements in @led_toggle_rate
+ * @off:		Register field value to turn LED off
+ * @on:			Register field value to turn LED on
+ */
+struct led_modes {
+	const struct led_toggle_rate *toggle_rates;
+	unsigned int num_toggle_rates;
+	u8 off;
+	u8 on;
+};
+
+struct rtl8231_led {
+	struct led_classdev led;
+	const struct led_modes *modes;
+	struct regmap_field *reg_field;
+};
+#define to_rtl8231_led(_cdev) container_of(_cdev, struct rtl8231_led, led)
+
+#define RTL8231_NUM_LEDS	3
+#define RTL8231_LED_PER_REG	5
+#define RTL8231_BITS_PER_LED	3
+
+static const unsigned int rtl8231_led_port_counts_single[RTL8231_NUM_LEDS] = {32, 32, 24};
+static const unsigned int rtl8231_led_port_counts_bicolor[RTL8231_NUM_LEDS] = {24, 24, 24};
+
+static const unsigned int rtl8231_led_base[RTL8231_NUM_LEDS] = {
+	RTL8231_REG_LED0_BASE,
+	RTL8231_REG_LED1_BASE,
+	RTL8231_REG_LED2_BASE,
+};
+
+static const struct led_toggle_rate rtl8231_toggle_rates[] = {
+	{  40, 1},
+	{  80, 2},
+	{ 160, 3},
+	{ 320, 4},
+	{ 640, 5},
+	{1280, 6},
+};
+
+static const struct led_modes rtl8231_led_modes = {
+	.off = 0,
+	.on = 7,
+	.num_toggle_rates = ARRAY_SIZE(rtl8231_toggle_rates),
+	.toggle_rates = rtl8231_toggle_rates,
+};
+
+static void rtl8231_led_brightness_set(struct led_classdev *led_cdev,
+	enum led_brightness brightness)
+{
+	struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
+
+	if (brightness)
+		regmap_field_write(pled->reg_field, pled->modes->on);
+	else
+		regmap_field_write(pled->reg_field, pled->modes->off);
+}
+
+static enum led_brightness rtl8231_led_brightness_get(struct led_classdev *led_cdev)
+{
+	struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
+	u32 current_mode = pled->modes->off;
+
+	regmap_field_read(pled->reg_field, &current_mode);
+
+	if (current_mode == pled->modes->off)
+		return LED_OFF;
+	else
+		return LED_ON;
+}
+
+static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
+{
+	unsigned int mode;
+	unsigned int i = 0;
+
+	if (regmap_field_read(pled->reg_field, &mode))
+		return 0;
+
+	while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode)
+		i++;
+
+	if (i < pled->modes->num_toggle_rates)
+		return pled->modes->toggle_rates[i].interval;
+	else
+		return 0;
+}
+
+static int rtl8231_led_blink_set(struct led_classdev *led_cdev, unsigned long *delay_on,
+	unsigned long *delay_off)
+{
+	struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
+	const struct led_modes *modes = pled->modes;
+	unsigned int interval;
+	unsigned int i = 0;
+	int err;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* Choose 500ms as default interval */
+		interval = 500;
+	} else {
+		/*
+		 * If the current mode is blinking, choose the delay that (likely) changed.
+		 * Otherwise, choose the interval that would have the same total delay.
+		 */
+		interval = rtl8231_led_current_interval(pled);
+
+		if (interval > 0 && interval == *delay_off)
+			interval = *delay_on;
+		else if (interval > 0 && interval == *delay_on)
+			interval = *delay_off;
+		else
+			interval = (*delay_on + *delay_off) / 2;
+	}
+
+	/* Find clamped toggle interval */
+	while (i < (modes->num_toggle_rates - 1) && interval > modes->toggle_rates[i].interval)
+		i++;
+
+	interval = modes->toggle_rates[i].interval;
+
+	err = regmap_field_write(pled->reg_field, modes->toggle_rates[i].mode);
+	if (err)
+		return err;
+
+	*delay_on = interval;
+	*delay_off = interval;
+
+	return 0;
+}
+
+static int rtl8231_led_read_address(struct fwnode_handle *fwnode, unsigned int *addr_port,
+	unsigned int *addr_led)
+{
+	u32 addr[2];
+	int err;
+
+	if (!fwnode_property_count_u32(fwnode, "reg"))
+		return -ENODEV;
+
+	err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr));
+	if (err)
+		return err;
+
+	*addr_port = addr[0];
+	*addr_led = addr[1];
+
+	return 0;
+}
+
+static struct reg_field rtl8231_led_get_field(unsigned int port_index, unsigned int led_index)
+{
+	unsigned int offset, shift;
+	struct reg_field field;
+
+	offset = port_index / RTL8231_LED_PER_REG;
+	shift = (port_index % RTL8231_LED_PER_REG) * RTL8231_BITS_PER_LED;
+
+	field.reg = rtl8231_led_base[led_index] + offset;
+	field.lsb = shift;
+	field.msb = shift + RTL8231_BITS_PER_LED - 1;
+
+	return field;
+}
+
+static int rtl8231_led_probe_single(struct device *dev, struct regmap *map,
+	const unsigned int *port_counts, struct fwnode_handle *fwnode)
+{
+	struct led_init_data init_data = {};
+	struct rtl8231_led *pled;
+	unsigned int port_index;
+	unsigned int led_index;
+	struct reg_field field;
+	int err;
+
+	pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL);
+	if (IS_ERR(pled))
+		return PTR_ERR(pled);
+
+	err = rtl8231_led_read_address(fwnode, &port_index, &led_index);
+
+	if (err) {
+		dev_err(dev, "LED address invalid\n");
+		return err;
+	} else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) {
+		dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index);
+		return -ENODEV;
+	}
+
+	field = rtl8231_led_get_field(port_index, led_index);
+	pled->reg_field = devm_regmap_field_alloc(dev, map, field);
+	if (IS_ERR(pled->reg_field))
+		return PTR_ERR(pled->reg_field);
+
+	pled->modes = &rtl8231_led_modes;
+
+	pled->led.max_brightness = 1;
+	pled->led.brightness_get = rtl8231_led_brightness_get;
+	pled->led.brightness_set = rtl8231_led_brightness_set;
+	pled->led.blink_set = rtl8231_led_blink_set;
+
+	init_data.fwnode = fwnode;
+
+	return devm_led_classdev_register_ext(dev, &pled->led, &init_data);
+}
+
+static int rtl8231_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const unsigned int *port_counts;
+	struct fwnode_handle *child;
+	struct regmap *map;
+	int err;
+
+	map = dev_get_regmap(dev->parent, NULL);
+	if (IS_ERR_OR_NULL(map)) {
+		dev_err(dev, "failed to retrieve regmap\n");
+		if (!map)
+			return -ENODEV;
+		else
+			return PTR_ERR(map);
+	}
+
+	if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) {
+		port_counts = rtl8231_led_port_counts_single;
+		regmap_update_bits(map, RTL8231_REG_FUNC0,
+			RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE);
+	} else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) {
+		port_counts = rtl8231_led_port_counts_bicolor;
+		regmap_update_bits(map, RTL8231_REG_FUNC0,
+			RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR);
+	} else {
+		dev_err(dev, "scan mode missing or invalid\n");
+		return -EINVAL;
+	}
+
+	fwnode_for_each_available_child_node(dev->fwnode, child) {
+		err = rtl8231_led_probe_single(dev, map, port_counts, child);
+		if (err)
+			dev_warn(dev, "failed to register LED %pfwP\n", child);
+		continue;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_rtl8231_led_match[] = {
+	{ .compatible = "realtek,rtl8231-leds" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_rtl8231_led_match);
+
+static struct platform_driver rtl8231_led_driver = {
+	.driver = {
+		.name = "rtl8231-leds",
+		.of_match_table = of_rtl8231_led_match,
+	},
+	.probe = rtl8231_led_probe,
+};
+module_platform_driver(rtl8231_led_driver);
+
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_DESCRIPTION("Realtek RTL8231 LED support");
+MODULE_LICENSE("GPL v2");