| Message ID | 20240903101930.16251-9-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Turris Omnia LED driver changes | expand |
On Tue, Sep 3, 2024 at 1:20 PM Marek Behún <kabel@kernel.org> wrote: > > Use dev_err_probe() instead of dev_err() + separate return where > appropriate. ... > + if (ret) > + return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np); Side note: Not sure how np is being used besides the messaging. If it's only for the messaging, I would rather see %pfw and fwnode based approach. ... > count = of_get_available_child_count(np); > - if (!count) { > - dev_err(dev, "LEDs are not defined in device tree!\n"); > - return -ENODEV; > - } else if (count > OMNIA_BOARD_LEDS) { > - dev_err(dev, "Too many LEDs defined in device tree!\n"); > - return -EINVAL; > - } > + if (!count) Dunno if 'if (count == 0)' would look better from the readability perspective. > + return dev_err_probe(dev, -ENODEV, "LEDs are not defined in device tree!\n"); > + else if (count > OMNIA_BOARD_LEDS) Even in the original code the 'else' is redundant. > + return dev_err_probe(dev, -EINVAL, "Too many LEDs defined in device tree!\n");
On Tue, Sep 03, 2024 at 01:45:49PM +0300, Andy Shevchenko wrote: > On Tue, Sep 3, 2024 at 1:20 PM Marek Behún <kabel@kernel.org> wrote: > > > > Use dev_err_probe() instead of dev_err() + separate return where > > appropriate. > > ... > > > + if (ret) > > + return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np); > > Side note: Not sure how np is being used besides the messaging. If > it's only for the messaging, I would rather see %pfw and fwnode based > approach. This board has a number of Ethernet switches described in DT. Nobody takes ACPI seriously for any sort of complex networking. So using fwnode is probably pointless, this board will probably never be used with anything other than DT. Andrew
On Tue, Sep 03, 2024 at 05:56:29PM +0200, Andrew Lunn wrote: > On Tue, Sep 03, 2024 at 01:45:49PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 3, 2024 at 1:20 PM Marek Behún <kabel@kernel.org> wrote: ... > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np); > > > > Side note: Not sure how np is being used besides the messaging. If > > it's only for the messaging, I would rather see %pfw and fwnode based > > approach. > > This board has a number of Ethernet switches described in DT. Nobody > takes ACPI seriously for any sort of complex networking. So using > fwnode is probably pointless, this board will probably never be used > with anything other than DT. Okay, fair enough!
On Tue, Sep 03, 2024 at 05:56:29PM +0200, Andrew Lunn wrote: > On Tue, Sep 03, 2024 at 01:45:49PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 3, 2024 at 1:20 PM Marek Behún <kabel@kernel.org> wrote: > > > > > > Use dev_err_probe() instead of dev_err() + separate return where > > > appropriate. > > > > ... > > > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np); > > > > Side note: Not sure how np is being used besides the messaging. If > > it's only for the messaging, I would rather see %pfw and fwnode based > > approach. > > This board has a number of Ethernet switches described in DT. Nobody > takes ACPI seriously for any sort of complex networking. So using > fwnode is probably pointless, this board will probably never be used > with anything other than DT. Although this is true, my opinion is that converting drivers to fwnode API would still have some theoretical merit. For example using the device_property_*() functions, where possible, seems nicer semantically. I think I tried it once, or at least asked Pavel if I should, and he turned me down, so I abandoned the effort. But the patch would be a very simple one. Marek
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c index 8d3bddc90fe0..857dba811d5e 100644 --- a/drivers/leds/leds-turris-omnia.c +++ b/drivers/leds/leds-turris-omnia.c @@ -238,33 +238,23 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, /* put the LED into software mode */ ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(led->reg) | OMNIA_CMD_LED_MODE_USER); - if (ret) { - dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np, - ret); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np); /* disable the LED */ ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_STATE, OMNIA_CMD_LED_STATE_LED(led->reg)); - if (ret) { - dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "Cannot set LED %pOF brightness\n", np); /* Set initial color and cache it */ ret = omnia_led_send_color_cmd(client, led); - if (ret < 0) { - dev_err(dev, "Cannot set LED %pOF initial color: %i\n", np, - ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "Cannot set LED %pOF initial color\n", np); ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data); - if (ret < 0) { - dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "Cannot register LED %pOF\n", np); return 1; } @@ -406,13 +396,10 @@ static int omnia_leds_probe(struct i2c_client *client) int ret, count; count = of_get_available_child_count(np); - if (!count) { - dev_err(dev, "LEDs are not defined in device tree!\n"); - return -ENODEV; - } else if (count > OMNIA_BOARD_LEDS) { - dev_err(dev, "Too many LEDs defined in device tree!\n"); - return -EINVAL; - } + if (!count) + return dev_err_probe(dev, -ENODEV, "LEDs are not defined in device tree!\n"); + else if (count > OMNIA_BOARD_LEDS) + return dev_err_probe(dev, -EINVAL, "Too many LEDs defined in device tree!\n"); leds = devm_kzalloc(dev, struct_size(leds, leds, count), GFP_KERNEL); if (!leds) @@ -422,11 +409,8 @@ static int omnia_leds_probe(struct i2c_client *client) i2c_set_clientdata(client, leds); ret = omnia_mcu_get_features(client); - if (ret < 0) { - dev_err(dev, "Cannot determine MCU supported features: %d\n", - ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "Cannot determine MCU supported features\n"); leds->has_gamma_correction = ret & OMNIA_FEAT_LED_GAMMA_CORRECTION; @@ -441,10 +425,8 @@ static int omnia_leds_probe(struct i2c_client *client) mutex_init(&leds->lock); ret = devm_led_trigger_register(dev, &omnia_hw_trigger); - if (ret < 0) { - dev_err(dev, "Cannot register private LED trigger: %d\n", ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "Cannot register private LED trigger\n"); led = &leds->leds[0]; for_each_available_child_of_node_scoped(np, child) {
Use dev_err_probe() instead of dev_err() + separate return where appropriate. Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/leds/leds-turris-omnia.c | 50 ++++++++++---------------------- 1 file changed, 16 insertions(+), 34 deletions(-)