Hi,
this series is also available at
https://git.kernel.org/pub/scm/linux/kernel/git/kabel/linux.git/log/?h=leds-cleanup-for-pavel
this is a cleanup of some LED subsystem drivers. The main reason behind
this is that I wanted to avoid code repetition by moving the parsing
of `linux,default-trigger` DT property from specific drivers to LED
core. Before this series 32 drivers parse this property (31 in
drivers/leds and one in drivers/input/keyboard/cap11xx.c).
After applying this series only 10 drivers are parsing this property.
The reason is that in discussion [1] Rob Herring says that
`linux,default-trigger` DT property is deprecated in favor of the
`function` DT property. This makes sense in a way since DT should not
be Linux specific.
After all drivers are converted we can maybe start work on slow
deprecation of this property. I do realize that we can't take it away,
but we can at least convert device trees in Linux repository to stop
using it in favor of `function` (and for default-on trigger in favor
of the `default-state` DT property), and print a deprecation warning
to the user when this `linux,default-trigger` property is present.
I wanted to prepare the way for slow deprecation of the DT property,
but it turns out that it is more difficult.
The first thing I wanted to do was to move the parsing of the
`linux,default-trigger` property to LED core. Currently many drivers
do this themselves. But it can't be moved that simply.
The first patch in this series adds the parsing of this DT property
into led_classdev_register_ext. If fwnode is given in init_data, the
property is read. This patch also removes the parsing of this property
from drivers where led_classdev_register_ext is already called. These
are:
an30259a, aw2013, cr0014114, el15203000, gpio, lm3532, lm3692x,
lp8860, lt3593, tlc591xx and turris-omnia.
Patches 2 to 6 do a simple conversion of some drivers to use
led_classdev_register_ext. These drivers are:
bcm6328, bcm6358, lm3697, max77650, mt6323 and pm8058.
In patches 7 to 10 I did a bigger refactor: either they first parsed
all LED nodes and only after that started registering them, or they
used too deep nesting or were weird in some other ways:
is31fl32xx, is31fl319x, lm36274 and ns2.
There is still a long way to go: some drivers still use the old
platform_data framework (which has a different structure for every
driver) instead of device properties via fwnode_* functions or OF).
Some of these can be changed to use device tree only, since they
already support it and the platform_data isn't used by anything in
the kernel (for example tca6507 can work with platform_data but
there is no board definition using it, all usage is via DT).
Some will be harder, because the platform_data code is still used
(pca9532 is used in arch/arm/mach-iop32x/n2100.c). Even this can
be done by converting the drivers to use fwnode_* API and converting
the mach code to use swnodes. I shall look into this later.
This series is compile tested on top of Pavel's tree. Since I
obviously don't have the various hardware that this code touches,
I am unable to test it. I therefore add maintainers and authors of
these drivers to Cc.
Marek
[1] https://lore.kernel.org/linux-leds/[email protected]/T/#m3b6c154f49d0467a707c0f9a552ec87bcbd89df2
Cc: Álvaro Fernández Rojas <[email protected]>
Cc: Bartosz Golaszewski <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Dan Murphy <[email protected]>
Cc: David Rivshin <[email protected]>
Cc: H. Nikolaus Schaller <[email protected]>
Cc: Jaedon Shin <[email protected]>
Cc: John Crispin <[email protected]>
Cc: Kevin Cernekee <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Ryder Lee <[email protected]>
Cc: Sean Wang <[email protected]>
Cc: Simon Guinot <[email protected]>
Cc: Simon Guinot <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Vincent Donnefort <[email protected]>
Marek Behún (10):
leds: parse linux,default-trigger DT property in LED core
leds: bcm6328, bcm6358: use struct led_init_data when registering
leds: lm3697: use struct led_init_data when registering
leds: max77650: use struct led_init_data when registering
leds: mt6323: use struct led_init_data when registering
leds: pm8058: use struct led_init_data when registering
leds: is31fl32xx: use struct led_init_data when registering
leds: is31fl319x: use struct led_init_data when registering
leds: lm36274: use struct led_init_data when registering
leds: ns2: refactor and use struct led_init_data
drivers/leds/Kconfig | 2 +-
drivers/leds/led-class.c | 5 +
drivers/leds/leds-an30259a.c | 3 -
drivers/leds/leds-aw2013.c | 3 -
drivers/leds/leds-bcm6328.c | 10 +-
drivers/leds/leds-bcm6358.c | 10 +-
drivers/leds/leds-cr0014114.c | 3 -
drivers/leds/leds-el15203000.c | 3 -
drivers/leds/leds-gpio.c | 3 -
drivers/leds/leds-is31fl319x.c | 204 ++++++++---------
drivers/leds/leds-is31fl32xx.c | 95 +++-----
drivers/leds/leds-lm3532.c | 3 -
drivers/leds/leds-lm36274.c | 100 +++++----
drivers/leds/leds-lm3692x.c | 3 -
drivers/leds/leds-lm3697.c | 18 +-
drivers/leds/leds-lp8860.c | 4 -
drivers/leds/leds-lt3593.c | 3 -
drivers/leds/leds-max77650.c | 24 +-
drivers/leds/leds-mt6323.c | 13 +-
drivers/leds/leds-ns2.c | 361 ++++++++++---------------------
drivers/leds/leds-pm8058.c | 38 ++--
drivers/leds/leds-tlc591xx.c | 2 -
drivers/leds/leds-turris-omnia.c | 2 -
23 files changed, 337 insertions(+), 575 deletions(-)
--
2.26.2
By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.
Signed-off-by: Marek Behún <[email protected]>
Cc: Álvaro Fernández Rojas <[email protected]>
Cc: Kevin Cernekee <[email protected]>
Cc: Jaedon Shin <[email protected]>
---
drivers/leds/leds-bcm6328.c | 10 ++++------
drivers/leds/leds-bcm6358.c | 10 ++++------
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
index bad7efb751120..1aa47f3086060 100644
--- a/drivers/leds/leds-bcm6328.c
+++ b/drivers/leds/leds-bcm6328.c
@@ -328,6 +328,9 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
void __iomem *mem, spinlock_t *lock,
unsigned long *blink_leds, unsigned long *blink_delay)
{
+ struct led_init_data init_data = {
+ .fwnode = of_fwnode_handle(nc),
+ };
struct bcm6328_led *led;
const char *state;
int rc;
@@ -345,11 +348,6 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
if (of_property_read_bool(nc, "active-low"))
led->active_low = true;
- led->cdev.name = of_get_property(nc, "label", NULL) ? : nc->name;
- led->cdev.default_trigger = of_get_property(nc,
- "linux,default-trigger",
- NULL);
-
if (!of_property_read_string(nc, "default-state", &state)) {
if (!strcmp(state, "on")) {
led->cdev.brightness = LED_FULL;
@@ -383,7 +381,7 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
led->cdev.brightness_set = bcm6328_led_set;
led->cdev.blink_set = bcm6328_blink_set;
- rc = led_classdev_register(dev, &led->cdev);
+ rc = led_classdev_register_ext(dev, &led->cdev, &init_data);
if (rc < 0)
return rc;
diff --git a/drivers/leds/leds-bcm6358.c b/drivers/leds/leds-bcm6358.c
index 94fefd456ba07..2be38211f5383 100644
--- a/drivers/leds/leds-bcm6358.c
+++ b/drivers/leds/leds-bcm6358.c
@@ -94,6 +94,9 @@ static void bcm6358_led_set(struct led_classdev *led_cdev,
static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
void __iomem *mem, spinlock_t *lock)
{
+ struct led_init_data init_data = {
+ .fwnode = of_fwnode_handle(nc),
+ };
struct bcm6358_led *led;
const char *state;
int rc;
@@ -109,11 +112,6 @@ static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
if (of_property_read_bool(nc, "active-low"))
led->active_low = true;
- led->cdev.name = of_get_property(nc, "label", NULL) ? : nc->name;
- led->cdev.default_trigger = of_get_property(nc,
- "linux,default-trigger",
- NULL);
-
if (!of_property_read_string(nc, "default-state", &state)) {
if (!strcmp(state, "on")) {
led->cdev.brightness = LED_FULL;
@@ -137,7 +135,7 @@ static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
led->cdev.brightness_set = bcm6358_led_set;
- rc = led_classdev_register(dev, &led->cdev);
+ rc = led_classdev_register_ext(dev, &led->cdev, &init_data);
if (rc < 0)
return rc;
--
2.26.2
By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.
Signed-off-by: Marek Behún <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Bjorn Andersson <[email protected]>
---
drivers/leds/leds-pm8058.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
index 7869ccdf70ce6..f6190e4af60fe 100644
--- a/drivers/leds/leds-pm8058.c
+++ b/drivers/leds/leds-pm8058.c
@@ -87,36 +87,37 @@ static enum led_brightness pm8058_led_get(struct led_classdev *cled)
static int pm8058_led_probe(struct platform_device *pdev)
{
+ struct led_init_data init_data = {};
+ struct device *dev = &pdev->dev;
+ enum led_brightness maxbright;
+ struct device_node *np;
struct pm8058_led *led;
- struct device_node *np = pdev->dev.of_node;
- int ret;
struct regmap *map;
const char *state;
- enum led_brightness maxbright;
+ int ret;
- led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+ led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
if (!led)
return -ENOMEM;
- led->ledtype = (u32)(unsigned long)of_device_get_match_data(&pdev->dev);
+ led->ledtype = (u32)(unsigned long)device_get_match_data(dev);
- map = dev_get_regmap(pdev->dev.parent, NULL);
+ map = dev_get_regmap(dev->parent, NULL);
if (!map) {
- dev_err(&pdev->dev, "Parent regmap unavailable.\n");
+ dev_err(dev, "Parent regmap unavailable.\n");
return -ENXIO;
}
led->map = map;
+ np = dev_of_node(dev);
+
ret = of_property_read_u32(np, "reg", &led->reg);
if (ret) {
- dev_err(&pdev->dev, "no register offset specified\n");
+ dev_err(dev, "no register offset specified\n");
return -EINVAL;
}
/* Use label else node name */
- led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
- led->cdev.default_trigger =
- of_get_property(np, "linux,default-trigger", NULL);
led->cdev.brightness_set = pm8058_led_set;
led->cdev.brightness_get = pm8058_led_get;
if (led->ledtype == PM8058_LED_TYPE_COMMON)
@@ -142,14 +143,13 @@ static int pm8058_led_probe(struct platform_device *pdev)
led->ledtype == PM8058_LED_TYPE_FLASH)
led->cdev.flags = LED_CORE_SUSPENDRESUME;
- ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
- if (ret) {
- dev_err(&pdev->dev, "unable to register led \"%s\"\n",
- led->cdev.name);
- return ret;
- }
+ init_data.fwnode = of_fwnode_handle(np);
+
+ ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+ if (ret)
+ dev_err(dev, "Failed to register LED for node %pOF\n", np);
- return 0;
+ return ret;
}
static const struct of_device_id pm8058_leds_id_table[] = {
@@ -173,7 +173,7 @@ static struct platform_driver pm8058_led_driver = {
.probe = pm8058_led_probe,
.driver = {
.name = "pm8058-leds",
- .of_match_table = pm8058_leds_id_table,
+ .of_match_table = of_match_ptr(pm8058_leds_id_table),
},
};
module_platform_driver(pm8058_led_driver);
--
2.26.2
By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.
Also, move forward from platform data to device tree only:
since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
platform data structure is absorbed into the driver, because nothing
else in the source tree used it. Since nobody complained and all usage
of this driver is via device tree, refactor the code to work with
device tree only. As Linus Walleij wrote, the device tree should be the
way forward anyway.
Also build this driver if COMPILE_TEST is enabled.
Signed-off-by: Marek Behún <[email protected]>
Cc: Simon Guinot <[email protected]>
Cc: Simon Guinot <[email protected]>
Cc: Vincent Donnefort <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Linus Walleij <[email protected]>
---
drivers/leds/Kconfig | 2 +-
drivers/leds/leds-ns2.c | 361 ++++++++++++----------------------------
2 files changed, 112 insertions(+), 251 deletions(-)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4f6464a169d57..58c33636afdbf 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -644,7 +644,7 @@ config LEDS_MC13783
config LEDS_NS2
tristate "LED support for Network Space v2 GPIO LEDs"
depends on LEDS_CLASS
- depends on MACH_KIRKWOOD || MACH_ARMADA_370
+ depends on MACH_KIRKWOOD || MACH_ARMADA_370 || COMPILE_TEST
default y
help
This option enables support for the dual-GPIO LEDs found on the
diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index bd806e7c8017b..6a5d326c5bddc 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -30,20 +30,6 @@ struct ns2_led_modval {
int slow_level;
};
-struct ns2_led {
- const char *name;
- const char *default_trigger;
- struct gpio_desc *cmd;
- struct gpio_desc *slow;
- int num_modes;
- struct ns2_led_modval *modval;
-};
-
-struct ns2_led_platform_data {
- int num_leds;
- struct ns2_led *leds;
-};
-
/*
* The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
* modes are available: off, on and SATA activity blinking. The LED modes are
@@ -51,7 +37,7 @@ struct ns2_led_platform_data {
* for the command/slow GPIOs corresponds to a LED mode.
*/
-struct ns2_led_data {
+struct ns2_led {
struct led_classdev cdev;
struct gpio_desc *cmd;
struct gpio_desc *slow;
@@ -62,95 +48,81 @@ struct ns2_led_data {
struct ns2_led_modval *modval;
};
-static int ns2_led_get_mode(struct ns2_led_data *led_dat,
- enum ns2_led_modes *mode)
+static int ns2_led_get_mode(struct ns2_led *led, enum ns2_led_modes *mode)
{
- int i;
- int ret = -EINVAL;
- int cmd_level;
- int slow_level;
-
- cmd_level = gpiod_get_value_cansleep(led_dat->cmd);
- slow_level = gpiod_get_value_cansleep(led_dat->slow);
-
- for (i = 0; i < led_dat->num_modes; i++) {
- if (cmd_level == led_dat->modval[i].cmd_level &&
- slow_level == led_dat->modval[i].slow_level) {
- *mode = led_dat->modval[i].mode;
- ret = 0;
- break;
+ int i, cmd_level, slow_level;
+
+ cmd_level = gpiod_get_value_cansleep(led->cmd);
+ slow_level = gpiod_get_value_cansleep(led->slow);
+
+ for (i = 0; i < led->num_modes; i++) {
+ if (cmd_level == led->modval[i].cmd_level &&
+ slow_level == led->modval[i].slow_level) {
+ *mode = led->modval[i].mode;
+ return 0;
}
}
- return ret;
+ return -EINVAL;
}
-static void ns2_led_set_mode(struct ns2_led_data *led_dat,
- enum ns2_led_modes mode)
+static void ns2_led_set_mode(struct ns2_led *led, enum ns2_led_modes mode)
{
int i;
- bool found = false;
unsigned long flags;
- for (i = 0; i < led_dat->num_modes; i++)
- if (mode == led_dat->modval[i].mode) {
- found = true;
+ for (i = 0; i < led->num_modes; i++)
+ if (mode == led->modval[i].mode)
break;
- }
- if (!found)
+ if (i == led->num_modes)
return;
- write_lock_irqsave(&led_dat->rw_lock, flags);
+ write_lock_irqsave(&led->rw_lock, flags);
- if (!led_dat->can_sleep) {
- gpiod_set_value(led_dat->cmd,
- led_dat->modval[i].cmd_level);
- gpiod_set_value(led_dat->slow,
- led_dat->modval[i].slow_level);
+ if (!led->can_sleep) {
+ gpiod_set_value(led->cmd, led->modval[i].cmd_level);
+ gpiod_set_value(led->slow, led->modval[i].slow_level);
goto exit_unlock;
}
- gpiod_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
- gpiod_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
+ gpiod_set_value_cansleep(led->cmd, led->modval[i].cmd_level);
+ gpiod_set_value_cansleep(led->slow, led->modval[i].slow_level);
exit_unlock:
- write_unlock_irqrestore(&led_dat->rw_lock, flags);
+ write_unlock_irqrestore(&led->rw_lock, flags);
}
static void ns2_led_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
- struct ns2_led_data *led_dat =
- container_of(led_cdev, struct ns2_led_data, cdev);
+ struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
enum ns2_led_modes mode;
if (value == LED_OFF)
mode = NS_V2_LED_OFF;
- else if (led_dat->sata)
+ else if (led->sata)
mode = NS_V2_LED_SATA;
else
mode = NS_V2_LED_ON;
- ns2_led_set_mode(led_dat, mode);
+ ns2_led_set_mode(led, mode);
}
static int ns2_led_set_blocking(struct led_classdev *led_cdev,
- enum led_brightness value)
+ enum led_brightness value)
{
ns2_led_set(led_cdev, value);
return 0;
}
-static ssize_t ns2_led_sata_store(struct device *dev,
- struct device_attribute *attr,
- const char *buff, size_t count)
+static ssize_t sata_store(struct device *dev, struct device_attribute *attr,
+ const char *buff, size_t count)
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
- struct ns2_led_data *led_dat =
- container_of(led_cdev, struct ns2_led_data, cdev);
- int ret;
+ struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
unsigned long enable;
+ int ret;
ret = kstrtoul(buff, 10, &enable);
if (ret < 0)
@@ -158,34 +130,33 @@ static ssize_t ns2_led_sata_store(struct device *dev,
enable = !!enable;
- if (led_dat->sata == enable)
+ if (led->sata == enable)
goto exit;
- led_dat->sata = enable;
+ led->sata = enable;
if (!led_get_brightness(led_cdev))
goto exit;
if (enable)
- ns2_led_set_mode(led_dat, NS_V2_LED_SATA);
+ ns2_led_set_mode(led, NS_V2_LED_SATA);
else
- ns2_led_set_mode(led_dat, NS_V2_LED_ON);
+ ns2_led_set_mode(led, NS_V2_LED_ON);
exit:
return count;
}
-static ssize_t ns2_led_sata_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t sata_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
- struct ns2_led_data *led_dat =
- container_of(led_cdev, struct ns2_led_data, cdev);
+ struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
- return sprintf(buf, "%d\n", led_dat->sata);
+ return sprintf(buf, "%d\n", led->sata);
}
-static DEVICE_ATTR(sata, 0644, ns2_led_sata_show, ns2_led_sata_store);
+static DEVICE_ATTR_RW(sata);
static struct attribute *ns2_led_attrs[] = {
&dev_attr_sata.attr,
@@ -193,147 +164,101 @@ static struct attribute *ns2_led_attrs[] = {
};
ATTRIBUTE_GROUPS(ns2_led);
-static int
-create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
- const struct ns2_led *template)
+static int ns2_led_register(struct device *dev, struct ns2_led *led,
+ struct device_node *np)
{
- int ret;
+ struct led_init_data init_data = {};
+ struct ns2_led_modval *modval;
enum ns2_led_modes mode;
+ int ret, nmodes, i;
+
+ led->cmd = devm_gpiod_get_from_of_node(dev, np, "cmd-gpio", 0,
+ GPIOD_ASIS, np->name);
+ if (IS_ERR(led->cmd))
+ return PTR_ERR(led->cmd);
+
+ led->slow = devm_gpiod_get_from_of_node(dev, np, "slow-gpio", 0,
+ GPIOD_ASIS, np->name);
+ if (IS_ERR(led->slow))
+ return PTR_ERR(led->slow);
+
+ ret = of_property_count_u32_elems(np, "modes-map");
+ if (ret <= 0 || ret % 3) {
+ dev_err(dev, "Missing or malformed modes-map in node %pOF\n",
+ np);
+ return ret < 0 ? ret : -EINVAL;
+ }
+
+ nmodes = ret / 3;
+ modval = devm_kcalloc(dev, nmodes, sizeof(*modval), GFP_KERNEL);
+ if (!modval)
+ return -ENOMEM;
+
+ for (i = 0; i < nmodes; ++i) {
+ u32 val;
+
+ of_property_read_u32_index(np, "modes-map", 3 * i, &val);
+ modval[i].mode = val;
+ of_property_read_u32_index(np, "modes-map", 3 * i + 1, &val);
+ modval[i].cmd_level = val;
+ of_property_read_u32_index(np, "modes-map", 3 * i + 2, &val);
+ modval[i].slow_level = val;
+ }
+
+ led->num_modes = nmodes;
+ led->modval = modval;
- rwlock_init(&led_dat->rw_lock);
-
- led_dat->cdev.name = template->name;
- led_dat->cdev.default_trigger = template->default_trigger;
- led_dat->cdev.blink_set = NULL;
- led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
- led_dat->cdev.groups = ns2_led_groups;
- led_dat->cmd = template->cmd;
- led_dat->slow = template->slow;
- led_dat->can_sleep = gpiod_cansleep(led_dat->cmd) |
- gpiod_cansleep(led_dat->slow);
- if (led_dat->can_sleep)
- led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
+ rwlock_init(&led->rw_lock);
+
+ led->cdev.flags |= LED_CORE_SUSPENDRESUME;
+ led->cdev.groups = ns2_led_groups;
+ led->can_sleep = gpiod_cansleep(led->cmd) || gpiod_cansleep(led->slow);
+ if (led->can_sleep)
+ led->cdev.brightness_set_blocking = ns2_led_set_blocking;
else
- led_dat->cdev.brightness_set = ns2_led_set;
- led_dat->modval = template->modval;
- led_dat->num_modes = template->num_modes;
+ led->cdev.brightness_set = ns2_led_set;
- ret = ns2_led_get_mode(led_dat, &mode);
+ ret = ns2_led_get_mode(led, &mode);
if (ret < 0)
return ret;
/* Set LED initial state. */
- led_dat->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
- led_dat->cdev.brightness =
- (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
+ led->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
+ led->cdev.brightness = (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
- ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
- if (ret < 0)
- return ret;
+ init_data.fwnode = of_fwnode_handle(np);
- return 0;
-}
+ ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+ if (ret < 0)
+ dev_err(dev, "Failed to register LED for node %pOF\n", np);
-static void delete_ns2_led(struct ns2_led_data *led_dat)
-{
- led_classdev_unregister(&led_dat->cdev);
+ return ret;
}
-#ifdef CONFIG_OF_GPIO
-/*
- * Translate OpenFirmware node properties into platform_data.
- */
-static int
-ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
+static int ns2_led_probe(struct platform_device *pdev)
{
- struct device_node *np = dev->of_node;
+ struct device *dev = &pdev->dev;
struct device_node *child;
- struct ns2_led *led, *leds;
- int ret, num_leds = 0;
+ struct ns2_led *leds;
+ int count, ret;
- num_leds = of_get_child_count(np);
- if (!num_leds)
+ count = device_get_child_node_count(dev);
+ if (count < 1) {
+ dev_err(dev, "Device has no child nodes\n");
return -ENODEV;
+ }
- leds = devm_kcalloc(dev, num_leds, sizeof(struct ns2_led),
- GFP_KERNEL);
+ leds = devm_kzalloc(dev, array_size(count, sizeof(*leds)), GFP_KERNEL);
if (!leds)
return -ENOMEM;
- led = leds;
- for_each_child_of_node(np, child) {
- const char *string;
- int i, num_modes;
- struct ns2_led_modval *modval;
- struct gpio_desc *gd;
-
- ret = of_property_read_string(child, "label", &string);
- led->name = (ret == 0) ? string : child->name;
-
- gd = gpiod_get_from_of_node(child, "cmd-gpio", 0,
- GPIOD_ASIS, led->name);
- if (IS_ERR(gd)) {
- ret = PTR_ERR(gd);
- goto err_node_put;
- }
- led->cmd = gd;
- gd = gpiod_get_from_of_node(child, "slow-gpio", 0,
- GPIOD_ASIS, led->name);
- if (IS_ERR(gd)) {
- ret = PTR_ERR(gd);
- goto err_node_put;
- }
- led->slow = gd;
-
- ret = of_property_read_string(child, "linux,default-trigger",
- &string);
- if (ret == 0)
- led->default_trigger = string;
-
- ret = of_property_count_u32_elems(child, "modes-map");
- if (ret < 0 || ret % 3) {
- dev_err(dev,
- "Missing or malformed modes-map property\n");
- ret = -EINVAL;
- goto err_node_put;
- }
-
- num_modes = ret / 3;
- modval = devm_kcalloc(dev,
- num_modes,
- sizeof(struct ns2_led_modval),
- GFP_KERNEL);
- if (!modval) {
- ret = -ENOMEM;
- goto err_node_put;
- }
-
- for (i = 0; i < num_modes; i++) {
- of_property_read_u32_index(child,
- "modes-map", 3 * i,
- (u32 *) &modval[i].mode);
- of_property_read_u32_index(child,
- "modes-map", 3 * i + 1,
- (u32 *) &modval[i].cmd_level);
- of_property_read_u32_index(child,
- "modes-map", 3 * i + 2,
- (u32 *) &modval[i].slow_level);
- }
-
- led->num_modes = num_modes;
- led->modval = modval;
-
- led++;
+ for_each_available_child_of_node(dev_of_node(dev), child) {
+ ret = ns2_led_register(dev, leds++, child);
+ if (ret)
+ return ret;
}
- pdata->leds = leds;
- pdata->num_leds = num_leds;
-
return 0;
-
-err_node_put:
- of_node_put(child);
- return ret;
}
static const struct of_device_id of_ns2_leds_match[] = {
@@ -341,73 +266,9 @@ static const struct of_device_id of_ns2_leds_match[] = {
{},
};
MODULE_DEVICE_TABLE(of, of_ns2_leds_match);
-#endif /* CONFIG_OF_GPIO */
-
-struct ns2_led_priv {
- int num_leds;
- struct ns2_led_data leds_data[];
-};
-
-static int ns2_led_probe(struct platform_device *pdev)
-{
- struct ns2_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
- struct ns2_led_priv *priv;
- int i;
- int ret;
-
-#ifdef CONFIG_OF_GPIO
- if (!pdata) {
- pdata = devm_kzalloc(&pdev->dev,
- sizeof(struct ns2_led_platform_data),
- GFP_KERNEL);
- if (!pdata)
- return -ENOMEM;
-
- ret = ns2_leds_get_of_pdata(&pdev->dev, pdata);
- if (ret)
- return ret;
- }
-#else
- if (!pdata)
- return -EINVAL;
-#endif /* CONFIG_OF_GPIO */
-
- priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds_data, pdata->num_leds), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
- priv->num_leds = pdata->num_leds;
-
- for (i = 0; i < priv->num_leds; i++) {
- ret = create_ns2_led(pdev, &priv->leds_data[i],
- &pdata->leds[i]);
- if (ret < 0) {
- for (i = i - 1; i >= 0; i--)
- delete_ns2_led(&priv->leds_data[i]);
- return ret;
- }
- }
-
- platform_set_drvdata(pdev, priv);
-
- return 0;
-}
-
-static int ns2_led_remove(struct platform_device *pdev)
-{
- int i;
- struct ns2_led_priv *priv;
-
- priv = platform_get_drvdata(pdev);
-
- for (i = 0; i < priv->num_leds; i++)
- delete_ns2_led(&priv->leds_data[i]);
-
- return 0;
-}
static struct platform_driver ns2_led_driver = {
.probe = ns2_led_probe,
- .remove = ns2_led_remove,
.driver = {
.name = "leds-ns2",
.of_match_table = of_match_ptr(of_ns2_leds_match),
--
2.26.2
On Wed 16 Sep 18:16 CDT 2020, Marek Beh?n wrote:
> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
>
> Signed-off-by: Marek Beh?n <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> ---
> drivers/leds/leds-pm8058.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
> index 7869ccdf70ce6..f6190e4af60fe 100644
> --- a/drivers/leds/leds-pm8058.c
> +++ b/drivers/leds/leds-pm8058.c
> @@ -87,36 +87,37 @@ static enum led_brightness pm8058_led_get(struct led_classdev *cled)
>
> static int pm8058_led_probe(struct platform_device *pdev)
> {
> + struct led_init_data init_data = {};
> + struct device *dev = &pdev->dev;
> + enum led_brightness maxbright;
> + struct device_node *np;
> struct pm8058_led *led;
> - struct device_node *np = pdev->dev.of_node;
> - int ret;
> struct regmap *map;
> const char *state;
> - enum led_brightness maxbright;
> + int ret;
>
> - led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
The pdev->dev -> dev and of_node changes are reasonable, but shouldn't
be part of this patch. It simply makes it hard to reason about he actual
change.
Please respin this with only the introduction of led_init_data.
Thanks,
Bjorn
> if (!led)
> return -ENOMEM;
>
> - led->ledtype = (u32)(unsigned long)of_device_get_match_data(&pdev->dev);
> + led->ledtype = (u32)(unsigned long)device_get_match_data(dev);
>
> - map = dev_get_regmap(pdev->dev.parent, NULL);
> + map = dev_get_regmap(dev->parent, NULL);
> if (!map) {
> - dev_err(&pdev->dev, "Parent regmap unavailable.\n");
> + dev_err(dev, "Parent regmap unavailable.\n");
> return -ENXIO;
> }
> led->map = map;
>
> + np = dev_of_node(dev);
> +
> ret = of_property_read_u32(np, "reg", &led->reg);
> if (ret) {
> - dev_err(&pdev->dev, "no register offset specified\n");
> + dev_err(dev, "no register offset specified\n");
> return -EINVAL;
> }
>
> /* Use label else node name */
> - led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
> - led->cdev.default_trigger =
> - of_get_property(np, "linux,default-trigger", NULL);
> led->cdev.brightness_set = pm8058_led_set;
> led->cdev.brightness_get = pm8058_led_get;
> if (led->ledtype == PM8058_LED_TYPE_COMMON)
> @@ -142,14 +143,13 @@ static int pm8058_led_probe(struct platform_device *pdev)
> led->ledtype == PM8058_LED_TYPE_FLASH)
> led->cdev.flags = LED_CORE_SUSPENDRESUME;
>
> - ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
> - if (ret) {
> - dev_err(&pdev->dev, "unable to register led \"%s\"\n",
> - led->cdev.name);
> - return ret;
> - }
> + init_data.fwnode = of_fwnode_handle(np);
> +
> + ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> + if (ret)
> + dev_err(dev, "Failed to register LED for node %pOF\n", np);
>
> - return 0;
> + return ret;
> }
>
> static const struct of_device_id pm8058_leds_id_table[] = {
> @@ -173,7 +173,7 @@ static struct platform_driver pm8058_led_driver = {
> .probe = pm8058_led_probe,
> .driver = {
> .name = "pm8058-leds",
> - .of_match_table = pm8058_leds_id_table,
> + .of_match_table = of_match_ptr(pm8058_leds_id_table),
> },
> };
> module_platform_driver(pm8058_led_driver);
> --
> 2.26.2
>
On Wed, 16 Sep 2020 19:46:25 -0500
Bjorn Andersson <[email protected]> wrote:
> The pdev->dev -> dev and of_node changes are reasonable, but shouldn't
> be part of this patch. It simply makes it hard to reason about he actual
> change.
>
> Please respin this with only the introduction of led_init_data.
>
> Thanks,
> Bjorn
>
Am working on this :)
On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
Hi Marek,
> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
>
> Also, move forward from platform data to device tree only:
> since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> platform data structure is absorbed into the driver, because nothing
> else in the source tree used it. Since nobody complained and all usage
Well, I probably should have...
I am using this driver on the Seagate Superbee NAS devices. This devices
are based on a x86 SoC. Since I have been unable to get from the ODM the
LED information written in the ACPI tables, then platform data are used
to pass the LED description to the driver.
The support of this boards is not available mainline yet but it is still
on my todo list. So that's why I am complaining right now :) If it is
not too much trouble I'd like to keep platform data support in this
driver.
Thanks in advance.
Simon
> of this driver is via device tree, refactor the code to work with
> device tree only. As Linus Walleij wrote, the device tree should be the
> way forward anyway.
>
> Also build this driver if COMPILE_TEST is enabled.
>
> Signed-off-by: Marek Behún <[email protected]>
> Cc: Simon Guinot <[email protected]>
> Cc: Simon Guinot <[email protected]>
> Cc: Vincent Donnefort <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Cc: Linus Walleij <[email protected]>
> ---
> drivers/leds/Kconfig | 2 +-
> drivers/leds/leds-ns2.c | 361 ++++++++++++----------------------------
> 2 files changed, 112 insertions(+), 251 deletions(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4f6464a169d57..58c33636afdbf 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -644,7 +644,7 @@ config LEDS_MC13783
> config LEDS_NS2
> tristate "LED support for Network Space v2 GPIO LEDs"
> depends on LEDS_CLASS
> - depends on MACH_KIRKWOOD || MACH_ARMADA_370
> + depends on MACH_KIRKWOOD || MACH_ARMADA_370 || COMPILE_TEST
> default y
> help
> This option enables support for the dual-GPIO LEDs found on the
> diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
> index bd806e7c8017b..6a5d326c5bddc 100644
> --- a/drivers/leds/leds-ns2.c
> +++ b/drivers/leds/leds-ns2.c
> @@ -30,20 +30,6 @@ struct ns2_led_modval {
> int slow_level;
> };
>
> -struct ns2_led {
> - const char *name;
> - const char *default_trigger;
> - struct gpio_desc *cmd;
> - struct gpio_desc *slow;
> - int num_modes;
> - struct ns2_led_modval *modval;
> -};
> -
> -struct ns2_led_platform_data {
> - int num_leds;
> - struct ns2_led *leds;
> -};
> -
> /*
> * The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
> * modes are available: off, on and SATA activity blinking. The LED modes are
> @@ -51,7 +37,7 @@ struct ns2_led_platform_data {
> * for the command/slow GPIOs corresponds to a LED mode.
> */
>
> -struct ns2_led_data {
> +struct ns2_led {
> struct led_classdev cdev;
> struct gpio_desc *cmd;
> struct gpio_desc *slow;
> @@ -62,95 +48,81 @@ struct ns2_led_data {
> struct ns2_led_modval *modval;
> };
>
> -static int ns2_led_get_mode(struct ns2_led_data *led_dat,
> - enum ns2_led_modes *mode)
> +static int ns2_led_get_mode(struct ns2_led *led, enum ns2_led_modes *mode)
> {
> - int i;
> - int ret = -EINVAL;
> - int cmd_level;
> - int slow_level;
> -
> - cmd_level = gpiod_get_value_cansleep(led_dat->cmd);
> - slow_level = gpiod_get_value_cansleep(led_dat->slow);
> -
> - for (i = 0; i < led_dat->num_modes; i++) {
> - if (cmd_level == led_dat->modval[i].cmd_level &&
> - slow_level == led_dat->modval[i].slow_level) {
> - *mode = led_dat->modval[i].mode;
> - ret = 0;
> - break;
> + int i, cmd_level, slow_level;
> +
> + cmd_level = gpiod_get_value_cansleep(led->cmd);
> + slow_level = gpiod_get_value_cansleep(led->slow);
> +
> + for (i = 0; i < led->num_modes; i++) {
> + if (cmd_level == led->modval[i].cmd_level &&
> + slow_level == led->modval[i].slow_level) {
> + *mode = led->modval[i].mode;
> + return 0;
> }
> }
>
> - return ret;
> + return -EINVAL;
> }
>
> -static void ns2_led_set_mode(struct ns2_led_data *led_dat,
> - enum ns2_led_modes mode)
> +static void ns2_led_set_mode(struct ns2_led *led, enum ns2_led_modes mode)
> {
> int i;
> - bool found = false;
> unsigned long flags;
>
> - for (i = 0; i < led_dat->num_modes; i++)
> - if (mode == led_dat->modval[i].mode) {
> - found = true;
> + for (i = 0; i < led->num_modes; i++)
> + if (mode == led->modval[i].mode)
> break;
> - }
>
> - if (!found)
> + if (i == led->num_modes)
> return;
>
> - write_lock_irqsave(&led_dat->rw_lock, flags);
> + write_lock_irqsave(&led->rw_lock, flags);
>
> - if (!led_dat->can_sleep) {
> - gpiod_set_value(led_dat->cmd,
> - led_dat->modval[i].cmd_level);
> - gpiod_set_value(led_dat->slow,
> - led_dat->modval[i].slow_level);
> + if (!led->can_sleep) {
> + gpiod_set_value(led->cmd, led->modval[i].cmd_level);
> + gpiod_set_value(led->slow, led->modval[i].slow_level);
> goto exit_unlock;
> }
>
> - gpiod_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
> - gpiod_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
> + gpiod_set_value_cansleep(led->cmd, led->modval[i].cmd_level);
> + gpiod_set_value_cansleep(led->slow, led->modval[i].slow_level);
>
> exit_unlock:
> - write_unlock_irqrestore(&led_dat->rw_lock, flags);
> + write_unlock_irqrestore(&led->rw_lock, flags);
> }
>
> static void ns2_led_set(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> - struct ns2_led_data *led_dat =
> - container_of(led_cdev, struct ns2_led_data, cdev);
> + struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
> enum ns2_led_modes mode;
>
> if (value == LED_OFF)
> mode = NS_V2_LED_OFF;
> - else if (led_dat->sata)
> + else if (led->sata)
> mode = NS_V2_LED_SATA;
> else
> mode = NS_V2_LED_ON;
>
> - ns2_led_set_mode(led_dat, mode);
> + ns2_led_set_mode(led, mode);
> }
>
> static int ns2_led_set_blocking(struct led_classdev *led_cdev,
> - enum led_brightness value)
> + enum led_brightness value)
> {
> ns2_led_set(led_cdev, value);
> return 0;
> }
>
> -static ssize_t ns2_led_sata_store(struct device *dev,
> - struct device_attribute *attr,
> - const char *buff, size_t count)
> +static ssize_t sata_store(struct device *dev, struct device_attribute *attr,
> + const char *buff, size_t count)
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> - struct ns2_led_data *led_dat =
> - container_of(led_cdev, struct ns2_led_data, cdev);
> - int ret;
> + struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
> unsigned long enable;
> + int ret;
>
> ret = kstrtoul(buff, 10, &enable);
> if (ret < 0)
> @@ -158,34 +130,33 @@ static ssize_t ns2_led_sata_store(struct device *dev,
>
> enable = !!enable;
>
> - if (led_dat->sata == enable)
> + if (led->sata == enable)
> goto exit;
>
> - led_dat->sata = enable;
> + led->sata = enable;
>
> if (!led_get_brightness(led_cdev))
> goto exit;
>
> if (enable)
> - ns2_led_set_mode(led_dat, NS_V2_LED_SATA);
> + ns2_led_set_mode(led, NS_V2_LED_SATA);
> else
> - ns2_led_set_mode(led_dat, NS_V2_LED_ON);
> + ns2_led_set_mode(led, NS_V2_LED_ON);
>
> exit:
> return count;
> }
>
> -static ssize_t ns2_led_sata_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t sata_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> - struct ns2_led_data *led_dat =
> - container_of(led_cdev, struct ns2_led_data, cdev);
> + struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
>
> - return sprintf(buf, "%d\n", led_dat->sata);
> + return sprintf(buf, "%d\n", led->sata);
> }
>
> -static DEVICE_ATTR(sata, 0644, ns2_led_sata_show, ns2_led_sata_store);
> +static DEVICE_ATTR_RW(sata);
>
> static struct attribute *ns2_led_attrs[] = {
> &dev_attr_sata.attr,
> @@ -193,147 +164,101 @@ static struct attribute *ns2_led_attrs[] = {
> };
> ATTRIBUTE_GROUPS(ns2_led);
>
> -static int
> -create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
> - const struct ns2_led *template)
> +static int ns2_led_register(struct device *dev, struct ns2_led *led,
> + struct device_node *np)
> {
> - int ret;
> + struct led_init_data init_data = {};
> + struct ns2_led_modval *modval;
> enum ns2_led_modes mode;
> + int ret, nmodes, i;
> +
> + led->cmd = devm_gpiod_get_from_of_node(dev, np, "cmd-gpio", 0,
> + GPIOD_ASIS, np->name);
> + if (IS_ERR(led->cmd))
> + return PTR_ERR(led->cmd);
> +
> + led->slow = devm_gpiod_get_from_of_node(dev, np, "slow-gpio", 0,
> + GPIOD_ASIS, np->name);
> + if (IS_ERR(led->slow))
> + return PTR_ERR(led->slow);
> +
> + ret = of_property_count_u32_elems(np, "modes-map");
> + if (ret <= 0 || ret % 3) {
> + dev_err(dev, "Missing or malformed modes-map in node %pOF\n",
> + np);
> + return ret < 0 ? ret : -EINVAL;
> + }
> +
> + nmodes = ret / 3;
> + modval = devm_kcalloc(dev, nmodes, sizeof(*modval), GFP_KERNEL);
> + if (!modval)
> + return -ENOMEM;
> +
> + for (i = 0; i < nmodes; ++i) {
> + u32 val;
> +
> + of_property_read_u32_index(np, "modes-map", 3 * i, &val);
> + modval[i].mode = val;
> + of_property_read_u32_index(np, "modes-map", 3 * i + 1, &val);
> + modval[i].cmd_level = val;
> + of_property_read_u32_index(np, "modes-map", 3 * i + 2, &val);
> + modval[i].slow_level = val;
> + }
> +
> + led->num_modes = nmodes;
> + led->modval = modval;
>
> - rwlock_init(&led_dat->rw_lock);
> -
> - led_dat->cdev.name = template->name;
> - led_dat->cdev.default_trigger = template->default_trigger;
> - led_dat->cdev.blink_set = NULL;
> - led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
> - led_dat->cdev.groups = ns2_led_groups;
> - led_dat->cmd = template->cmd;
> - led_dat->slow = template->slow;
> - led_dat->can_sleep = gpiod_cansleep(led_dat->cmd) |
> - gpiod_cansleep(led_dat->slow);
> - if (led_dat->can_sleep)
> - led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
> + rwlock_init(&led->rw_lock);
> +
> + led->cdev.flags |= LED_CORE_SUSPENDRESUME;
> + led->cdev.groups = ns2_led_groups;
> + led->can_sleep = gpiod_cansleep(led->cmd) || gpiod_cansleep(led->slow);
> + if (led->can_sleep)
> + led->cdev.brightness_set_blocking = ns2_led_set_blocking;
> else
> - led_dat->cdev.brightness_set = ns2_led_set;
> - led_dat->modval = template->modval;
> - led_dat->num_modes = template->num_modes;
> + led->cdev.brightness_set = ns2_led_set;
>
> - ret = ns2_led_get_mode(led_dat, &mode);
> + ret = ns2_led_get_mode(led, &mode);
> if (ret < 0)
> return ret;
>
> /* Set LED initial state. */
> - led_dat->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
> - led_dat->cdev.brightness =
> - (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
> + led->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
> + led->cdev.brightness = (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
>
> - ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
> - if (ret < 0)
> - return ret;
> + init_data.fwnode = of_fwnode_handle(np);
>
> - return 0;
> -}
> + ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> + if (ret < 0)
> + dev_err(dev, "Failed to register LED for node %pOF\n", np);
>
> -static void delete_ns2_led(struct ns2_led_data *led_dat)
> -{
> - led_classdev_unregister(&led_dat->cdev);
> + return ret;
> }
>
> -#ifdef CONFIG_OF_GPIO
> -/*
> - * Translate OpenFirmware node properties into platform_data.
> - */
> -static int
> -ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
> +static int ns2_led_probe(struct platform_device *pdev)
> {
> - struct device_node *np = dev->of_node;
> + struct device *dev = &pdev->dev;
> struct device_node *child;
> - struct ns2_led *led, *leds;
> - int ret, num_leds = 0;
> + struct ns2_led *leds;
> + int count, ret;
>
> - num_leds = of_get_child_count(np);
> - if (!num_leds)
> + count = device_get_child_node_count(dev);
> + if (count < 1) {
> + dev_err(dev, "Device has no child nodes\n");
> return -ENODEV;
> + }
>
> - leds = devm_kcalloc(dev, num_leds, sizeof(struct ns2_led),
> - GFP_KERNEL);
> + leds = devm_kzalloc(dev, array_size(count, sizeof(*leds)), GFP_KERNEL);
> if (!leds)
> return -ENOMEM;
>
> - led = leds;
> - for_each_child_of_node(np, child) {
> - const char *string;
> - int i, num_modes;
> - struct ns2_led_modval *modval;
> - struct gpio_desc *gd;
> -
> - ret = of_property_read_string(child, "label", &string);
> - led->name = (ret == 0) ? string : child->name;
> -
> - gd = gpiod_get_from_of_node(child, "cmd-gpio", 0,
> - GPIOD_ASIS, led->name);
> - if (IS_ERR(gd)) {
> - ret = PTR_ERR(gd);
> - goto err_node_put;
> - }
> - led->cmd = gd;
> - gd = gpiod_get_from_of_node(child, "slow-gpio", 0,
> - GPIOD_ASIS, led->name);
> - if (IS_ERR(gd)) {
> - ret = PTR_ERR(gd);
> - goto err_node_put;
> - }
> - led->slow = gd;
> -
> - ret = of_property_read_string(child, "linux,default-trigger",
> - &string);
> - if (ret == 0)
> - led->default_trigger = string;
> -
> - ret = of_property_count_u32_elems(child, "modes-map");
> - if (ret < 0 || ret % 3) {
> - dev_err(dev,
> - "Missing or malformed modes-map property\n");
> - ret = -EINVAL;
> - goto err_node_put;
> - }
> -
> - num_modes = ret / 3;
> - modval = devm_kcalloc(dev,
> - num_modes,
> - sizeof(struct ns2_led_modval),
> - GFP_KERNEL);
> - if (!modval) {
> - ret = -ENOMEM;
> - goto err_node_put;
> - }
> -
> - for (i = 0; i < num_modes; i++) {
> - of_property_read_u32_index(child,
> - "modes-map", 3 * i,
> - (u32 *) &modval[i].mode);
> - of_property_read_u32_index(child,
> - "modes-map", 3 * i + 1,
> - (u32 *) &modval[i].cmd_level);
> - of_property_read_u32_index(child,
> - "modes-map", 3 * i + 2,
> - (u32 *) &modval[i].slow_level);
> - }
> -
> - led->num_modes = num_modes;
> - led->modval = modval;
> -
> - led++;
> + for_each_available_child_of_node(dev_of_node(dev), child) {
> + ret = ns2_led_register(dev, leds++, child);
> + if (ret)
> + return ret;
> }
>
> - pdata->leds = leds;
> - pdata->num_leds = num_leds;
> -
> return 0;
> -
> -err_node_put:
> - of_node_put(child);
> - return ret;
> }
>
> static const struct of_device_id of_ns2_leds_match[] = {
> @@ -341,73 +266,9 @@ static const struct of_device_id of_ns2_leds_match[] = {
> {},
> };
> MODULE_DEVICE_TABLE(of, of_ns2_leds_match);
> -#endif /* CONFIG_OF_GPIO */
> -
> -struct ns2_led_priv {
> - int num_leds;
> - struct ns2_led_data leds_data[];
> -};
> -
> -static int ns2_led_probe(struct platform_device *pdev)
> -{
> - struct ns2_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> - struct ns2_led_priv *priv;
> - int i;
> - int ret;
> -
> -#ifdef CONFIG_OF_GPIO
> - if (!pdata) {
> - pdata = devm_kzalloc(&pdev->dev,
> - sizeof(struct ns2_led_platform_data),
> - GFP_KERNEL);
> - if (!pdata)
> - return -ENOMEM;
> -
> - ret = ns2_leds_get_of_pdata(&pdev->dev, pdata);
> - if (ret)
> - return ret;
> - }
> -#else
> - if (!pdata)
> - return -EINVAL;
> -#endif /* CONFIG_OF_GPIO */
> -
> - priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds_data, pdata->num_leds), GFP_KERNEL);
> - if (!priv)
> - return -ENOMEM;
> - priv->num_leds = pdata->num_leds;
> -
> - for (i = 0; i < priv->num_leds; i++) {
> - ret = create_ns2_led(pdev, &priv->leds_data[i],
> - &pdata->leds[i]);
> - if (ret < 0) {
> - for (i = i - 1; i >= 0; i--)
> - delete_ns2_led(&priv->leds_data[i]);
> - return ret;
> - }
> - }
> -
> - platform_set_drvdata(pdev, priv);
> -
> - return 0;
> -}
> -
> -static int ns2_led_remove(struct platform_device *pdev)
> -{
> - int i;
> - struct ns2_led_priv *priv;
> -
> - priv = platform_get_drvdata(pdev);
> -
> - for (i = 0; i < priv->num_leds; i++)
> - delete_ns2_led(&priv->leds_data[i]);
> -
> - return 0;
> -}
>
> static struct platform_driver ns2_led_driver = {
> .probe = ns2_led_probe,
> - .remove = ns2_led_remove,
> .driver = {
> .name = "leds-ns2",
> .of_match_table = of_match_ptr(of_ns2_leds_match),
> --
> 2.26.2
On Fri, 18 Sep 2020 15:02:06 +0200
Simon Guinot <[email protected]> wrote:
> On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
>
> Hi Marek,
>
> > By using struct led_init_data when registering we do not need to parse
> > `label` DT property nor `linux,default-trigger` property.
> >
> > Also, move forward from platform data to device tree only:
> > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > platform data structure is absorbed into the driver, because nothing
> > else in the source tree used it. Since nobody complained and all usage
>
> Well, I probably should have...
>
> I am using this driver on the Seagate Superbee NAS devices. This devices
> are based on a x86 SoC. Since I have been unable to get from the ODM the
> LED information written in the ACPI tables, then platform data are used
> to pass the LED description to the driver.
>
> The support of this boards is not available mainline yet but it is still
> on my todo list. So that's why I am complaining right now :) If it is
> not too much trouble I'd like to keep platform data support in this
> driver.
>
> Thanks in advance.
>
> Simon
>
Simon, what if we refactored the driver to use fwnode API instead of OF
API? Then if it is impossible for you to write DTS for that device,
instead of platform data you could implement your device via swnode
fwnodes. :)
static const struct property_entry entries[] = {
PROPERTY_ENTRY_STRING("compatible", "lacie,ns2-leds"),
...
};
Look at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/chromeos_laptop.c?h=v5.9-rc5
search for PROPERTY_ENTRY.
I am willing to work on this with you, but I would really like to rid
the LED drivers of platform data.
Marek
On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> On Fri, 18 Sep 2020 15:02:06 +0200
> Simon Guinot <[email protected]> wrote:
>
> > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> >
> > Hi Marek,
> >
> > > By using struct led_init_data when registering we do not need to parse
> > > `label` DT property nor `linux,default-trigger` property.
> > >
> > > Also, move forward from platform data to device tree only:
> > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > platform data structure is absorbed into the driver, because nothing
> > > else in the source tree used it. Since nobody complained and all usage
> >
> > Well, I probably should have...
> >
> > I am using this driver on the Seagate Superbee NAS devices. This devices
> > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > LED information written in the ACPI tables, then platform data are used
> > to pass the LED description to the driver.
> >
> > The support of this boards is not available mainline yet but it is still
> > on my todo list. So that's why I am complaining right now :) If it is
> > not too much trouble I'd like to keep platform data support in this
> > driver.
> >
> > Thanks in advance.
> >
> > Simon
> >
>
> Simon, what if we refactored the driver to use fwnode API instead of OF
> API? Then if it is impossible for you to write DTS for that device,
> instead of platform data you could implement your device via swnode
> fwnodes. :)
Yes. That would be perfect.
Simon
On Mon, 21 Sep 2020 14:53:43 +0200
Simon Guinot <[email protected]> wrote:
> On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > On Fri, 18 Sep 2020 15:02:06 +0200
> > Simon Guinot <[email protected]> wrote:
> >
> > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > >
> > > Hi Marek,
> > >
> > > > By using struct led_init_data when registering we do not need to parse
> > > > `label` DT property nor `linux,default-trigger` property.
> > > >
> > > > Also, move forward from platform data to device tree only:
> > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > platform data structure is absorbed into the driver, because nothing
> > > > else in the source tree used it. Since nobody complained and all usage
> > >
> > > Well, I probably should have...
> > >
> > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > LED information written in the ACPI tables, then platform data are used
> > > to pass the LED description to the driver.
> > >
> > > The support of this boards is not available mainline yet but it is still
> > > on my todo list. So that's why I am complaining right now :) If it is
> > > not too much trouble I'd like to keep platform data support in this
> > > driver.
> > >
> > > Thanks in advance.
> > >
> > > Simon
> > >
> >
> > Simon, what if we refactored the driver to use fwnode API instead of OF
> > API? Then if it is impossible for you to write DTS for that device,
> > instead of platform data you could implement your device via swnode
> > fwnodes. :)
>
> Yes. That would be perfect.
>
> Simon
BTW if you have access to device schematics I could try to write DTS,
with schematics and the current board source file it should not be that
hard. But I can't test it, since I don't have the board.
Marek
On Mon, Sep 21, 2020 at 03:02:08PM +0200, Marek Behun wrote:
> On Mon, 21 Sep 2020 14:53:43 +0200
> Simon Guinot <[email protected]> wrote:
>
> > On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > > On Fri, 18 Sep 2020 15:02:06 +0200
> > > Simon Guinot <[email protected]> wrote:
> > >
> > > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > >
> > > > Hi Marek,
> > > >
> > > > > By using struct led_init_data when registering we do not need to parse
> > > > > `label` DT property nor `linux,default-trigger` property.
> > > > >
> > > > > Also, move forward from platform data to device tree only:
> > > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > > platform data structure is absorbed into the driver, because nothing
> > > > > else in the source tree used it. Since nobody complained and all usage
> > > >
> > > > Well, I probably should have...
> > > >
> > > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > > LED information written in the ACPI tables, then platform data are used
> > > > to pass the LED description to the driver.
> > > >
> > > > The support of this boards is not available mainline yet but it is still
> > > > on my todo list. So that's why I am complaining right now :) If it is
> > > > not too much trouble I'd like to keep platform data support in this
> > > > driver.
> > > >
> > > > Thanks in advance.
> > > >
> > > > Simon
> > > >
> > >
> > > Simon, what if we refactored the driver to use fwnode API instead of OF
> > > API? Then if it is impossible for you to write DTS for that device,
> > > instead of platform data you could implement your device via swnode
> > > fwnodes. :)
> >
> > Yes. That would be perfect.
> >
> > Simon
>
> BTW if you have access to device schematics I could try to write DTS,
> with schematics and the current board source file it should not be that
> hard. But I can't test it, since I don't have the board.
Don't worry, I'll do the writing and the testing of the fwnode in the
x86 board files. This boards are not mainlined yet. So it is my problem.
And actually if you don't have the time I can do the writing of the
fwnode support in the driver as well. And you can just let the driver
with the OF support. That's fine.
But if you are willing to add fwnode support to the driver yourself,
then you are more than welcome to do it. On my side, I can help with
the testing. I can check that the ARM boards ant their DTB are still
supported by the driver. And I can also check the support of the x86
boards with the addition of the fwnode properties.
Simon
On Mon, 21 Sep 2020 16:03:22 +0200
Simon Guinot <[email protected]> wrote:
> On Mon, Sep 21, 2020 at 03:02:08PM +0200, Marek Behun wrote:
> > On Mon, 21 Sep 2020 14:53:43 +0200
> > Simon Guinot <[email protected]> wrote:
> >
> > > On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > > > On Fri, 18 Sep 2020 15:02:06 +0200
> > > > Simon Guinot <[email protected]> wrote:
> > > >
> > > > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > > >
> > > > > Hi Marek,
> > > > >
> > > > > > By using struct led_init_data when registering we do not need to parse
> > > > > > `label` DT property nor `linux,default-trigger` property.
> > > > > >
> > > > > > Also, move forward from platform data to device tree only:
> > > > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > > > platform data structure is absorbed into the driver, because nothing
> > > > > > else in the source tree used it. Since nobody complained and all usage
> > > > >
> > > > > Well, I probably should have...
> > > > >
> > > > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > > > LED information written in the ACPI tables, then platform data are used
> > > > > to pass the LED description to the driver.
> > > > >
> > > > > The support of this boards is not available mainline yet but it is still
> > > > > on my todo list. So that's why I am complaining right now :) If it is
> > > > > not too much trouble I'd like to keep platform data support in this
> > > > > driver.
> > > > >
> > > > > Thanks in advance.
> > > > >
> > > > > Simon
> > > > >
> > > >
> > > > Simon, what if we refactored the driver to use fwnode API instead of OF
> > > > API? Then if it is impossible for you to write DTS for that device,
> > > > instead of platform data you could implement your device via swnode
> > > > fwnodes. :)
> > >
> > > Yes. That would be perfect.
> > >
> > > Simon
> >
> > BTW if you have access to device schematics I could try to write DTS,
> > with schematics and the current board source file it should not be that
> > hard. But I can't test it, since I don't have the board.
>
> Don't worry, I'll do the writing and the testing of the fwnode in the
> x86 board files. This boards are not mainlined yet. So it is my problem.
>
> And actually if you don't have the time I can do the writing of the
> fwnode support in the driver as well. And you can just let the driver
> with the OF support. That's fine.
>
> But if you are willing to add fwnode support to the driver yourself,
> then you are more than welcome to do it. On my side, I can help with
> the testing. I can check that the ARM boards ant their DTB are still
> supported by the driver. And I can also check the support of the x86
> boards with the addition of the fwnode properties.
>
> Simon
Very well, I shall convert the driver to fwnode API and send the patch.
Marek