| Message ID | 20240614-cros_ec-led-review-v1-1-946f2549fac2@weissschuh.net |
|---|---|
| State | New |
| Headers | show |
| Series | leds: cros_ec: Implement review comments from Lee | expand |
Definitely not seen a commit message like that before > Implement review comments from Lee as requested in [0] for > "leds: Add ChromeOS EC driver". > > Changes: > * Inline DRV_NAME string constant. > * Use dev_err() instead of dev_warn() to report errors. > * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). > * Make loop variable "int" where they belong. > * Move "Unknown LED" comment to where it belongs. > * Register trigger during _probe(). > * Use module_platform_driver() and drop all the custom boilerplate. If you're fixing many things, then I would expect to receive many patches. One patch for functional change please. If you can reasonably group fixes of similar elk, then please do. However one patch that does a bunch of different things is a no-go from me, sorry. > [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9 > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/leds/leds-cros_ec.c | 50 +++++++++++++-------------------------------- > 1 file changed, 14 insertions(+), 36 deletions(-) > > diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c > index 7bb21a587713..275522b81ea5 100644 > --- a/drivers/leds/leds-cros_ec.c > +++ b/drivers/leds/leds-cros_ec.c > @@ -14,8 +14,6 @@ > #include <linux/platform_data/cros_ec_commands.h> > #include <linux/platform_data/cros_ec_proto.h> > > -#define DRV_NAME "cros-ec-led" > - > static const char * const cros_ec_led_functions[] = { > [EC_LED_ID_BATTERY_LED] = LED_FUNCTION_CHARGING, > [EC_LED_ID_POWER_LED] = LED_FUNCTION_POWER, > @@ -152,7 +150,7 @@ static int cros_ec_led_count_subleds(struct device *dev, > > if (common_range != range) { > /* The multicolor LED API expects a uniform max_brightness */ > - dev_warn(dev, "Inconsistent LED brightness values\n"); > + dev_err(dev, "Inconsistent LED brightness values\n"); > return -EINVAL; > } > } > @@ -176,22 +174,21 @@ static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cde > return led_get_color_name(color); > } > > -static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec, > +static int cros_ec_led_probe_one(struct device *dev, struct cros_ec_device *cros_ec, > enum ec_led_id id) > { > union cros_ec_led_cmd_data arg = {}; > struct cros_ec_led_priv *priv; > struct led_classdev *led_cdev; > struct mc_subled *subleds; > - int ret, num_subleds; > - size_t i, subled; > + int i, ret, num_subleds; > + size_t subled; > > arg.req.led_id = id; > arg.req.flags = EC_LED_FLAGS_QUERY; > ret = cros_ec_led_send_cmd(cros_ec, &arg); > - /* Unknown LED, skip */ > if (ret == -EINVAL) > - return 0; > + return 0; /* Unknown LED, skip */ > if (ret == -EOPNOTSUPP) > return -ENODEV; > if (ret < 0) > @@ -247,11 +244,14 @@ static int cros_ec_led_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); > struct cros_ec_device *cros_ec = ec_dev->ec_dev; > - int ret = 0; > - size_t i; > + int i, ret = 0; > + > + ret = devm_led_trigger_register(dev, &cros_ec_led_trigger); > + if (ret) > + return ret; > > for (i = 0; i < EC_LED_ID_COUNT; i++) { > - ret = cros_ec_led_probe_led(dev, cros_ec, i); > + ret = cros_ec_led_probe_one(dev, cros_ec, i); > if (ret) > break; > } > @@ -260,38 +260,16 @@ static int cros_ec_led_probe(struct platform_device *pdev) > } > > static const struct platform_device_id cros_ec_led_id[] = { > - { DRV_NAME, 0 }, > + { "cros-ec-led", 0 }, > {} > }; > > static struct platform_driver cros_ec_led_driver = { > - .driver.name = DRV_NAME, > + .driver.name = "cros-ec-led", > .probe = cros_ec_led_probe, > .id_table = cros_ec_led_id, > }; > - > -static int __init cros_ec_led_init(void) > -{ > - int ret; > - > - ret = led_trigger_register(&cros_ec_led_trigger); > - if (ret) > - return ret; > - > - ret = platform_driver_register(&cros_ec_led_driver); > - if (ret) > - led_trigger_unregister(&cros_ec_led_trigger); > - > - return ret; > -}; > -module_init(cros_ec_led_init); > - > -static void __exit cros_ec_led_exit(void) > -{ > - platform_driver_unregister(&cros_ec_led_driver); > - led_trigger_unregister(&cros_ec_led_trigger); > -}; > -module_exit(cros_ec_led_exit); > +module_platform_driver(cros_ec_led_driver); > > MODULE_DEVICE_TABLE(platform, cros_ec_led_id); > MODULE_DESCRIPTION("ChromeOS EC LED Driver"); > > --- > base-commit: b6774dd948171c478c7aa19318b1d7ae9cf6d7a4 > change-id: 20240614-cros_ec-led-review-ec93abc65933 > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net> >
Hi Lee, On 2024-06-20 10:31:14+0000, Lee Jones wrote: > Definitely not seen a commit message like that before I assumed that this patch would be squashed into the original commit. My question in which form you wanted the changes should have included "incremental series". > > Implement review comments from Lee as requested in [0] for > > "leds: Add ChromeOS EC driver". > > > > Changes: > > * Inline DRV_NAME string constant. > > * Use dev_err() instead of dev_warn() to report errors. > > * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). > > * Make loop variable "int" where they belong. > > * Move "Unknown LED" comment to where it belongs. > > * Register trigger during _probe(). > > * Use module_platform_driver() and drop all the custom boilerplate. > > If you're fixing many things, then I would expect to receive many > patches. One patch for functional change please. If you can reasonably > group fixes of similar elk, then please do. However one patch that does > a bunch of different things is a no-go from me, sorry. Absolutely, if these changes are to end up as actual commits then they need to look different. I'll resend them as proper series. > > [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9 > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > drivers/leds/leds-cros_ec.c | 50 +++++++++++++-------------------------------- > > 1 file changed, 14 insertions(+), 36 deletions(-) <snip> Sorry for the confusion, Thomas
On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > Hi Lee, > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > Definitely not seen a commit message like that before > > I assumed that this patch would be squashed into the original commit. > > My question in which form you wanted the changes should have included > "incremental series". Incremental means on top. A lot of maintainers don't support history re-writes, but I've reserved that right since forever, so I can squash it if you like. > > > Implement review comments from Lee as requested in [0] for > > > "leds: Add ChromeOS EC driver". > > > > > > Changes: > > > * Inline DRV_NAME string constant. > > > * Use dev_err() instead of dev_warn() to report errors. > > > * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). > > > * Make loop variable "int" where they belong. > > > * Move "Unknown LED" comment to where it belongs. > > > * Register trigger during _probe(). > > > * Use module_platform_driver() and drop all the custom boilerplate. > > > > If you're fixing many things, then I would expect to receive many > > patches. One patch for functional change please. If you can reasonably > > group fixes of similar elk, then please do. However one patch that does > > a bunch of different things is a no-go from me, sorry. > > Absolutely, if these changes are to end up as actual commits then they > need to look different. > I'll resend them as proper series. > > > > [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9 > > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > > --- > > > drivers/leds/leds-cros_ec.c | 50 +++++++++++++-------------------------------- > > > 1 file changed, 14 insertions(+), 36 deletions(-) > > <snip> > > Sorry for the confusion, > Thomas
On 2024-06-20 13:27:41+0000, Lee Jones wrote: > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > Hi Lee, > > > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > > Definitely not seen a commit message like that before > > > > I assumed that this patch would be squashed into the original commit. > > > > My question in which form you wanted the changes should have included > > "incremental series". > > Incremental means on top. > > A lot of maintainers don't support history re-writes, but I've reserved > that right since forever, so I can squash it if you like. If it is already visible somewhere and a squash would inconvenience anybody I'll resend it. But if not I'd be happy about a squash. (I couldn't and still can't find the public tree where driver is in) > > > > Implement review comments from Lee as requested in [0] for > > > > "leds: Add ChromeOS EC driver". > > > > > > > > Changes: > > > > * Inline DRV_NAME string constant. > > > > * Use dev_err() instead of dev_warn() to report errors. > > > > * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). > > > > * Make loop variable "int" where they belong. > > > > * Move "Unknown LED" comment to where it belongs. > > > > * Register trigger during _probe(). > > > > * Use module_platform_driver() and drop all the custom boilerplate. > > > > > > If you're fixing many things, then I would expect to receive many > > > patches. One patch for functional change please. If you can reasonably > > > group fixes of similar elk, then please do. However one patch that does > > > a bunch of different things is a no-go from me, sorry. > > > > Absolutely, if these changes are to end up as actual commits then they > > need to look different. > > I'll resend them as proper series. > > > > > > [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9 > > > > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > > > --- > > > > drivers/leds/leds-cros_ec.c | 50 +++++++++++++-------------------------------- > > > > 1 file changed, 14 insertions(+), 36 deletions(-) > > > > <snip> > > > > Sorry for the confusion, > > Thomas > > -- > Lee Jones [李琼斯]
On Fri, 14 Jun 2024 12:04:29 +0200, Thomas Weißschuh wrote: > Implement review comments from Lee as requested in [0] for > "leds: Add ChromeOS EC driver". > > Changes: > * Inline DRV_NAME string constant. > * Use dev_err() instead of dev_warn() to report errors. > * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). > * Make loop variable "int" where they belong. > * Move "Unknown LED" comment to where it belongs. > * Register trigger during _probe(). > * Use module_platform_driver() and drop all the custom boilerplate. > > [...] Applied, thanks! [1/1] leds: cros_ec: Implement review comments from Lee commit: 1b95b0039874092a24f9ee58c8bedf68485221a7 -- Lee Jones [李琼斯]
On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > On 2024-06-20 13:27:41+0000, Lee Jones wrote: > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > Hi Lee, > > > > > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > > > Definitely not seen a commit message like that before > > > > > > I assumed that this patch would be squashed into the original commit. > > > > > > My question in which form you wanted the changes should have included > > > "incremental series". > > > > Incremental means on top. > > > > A lot of maintainers don't support history re-writes, but I've reserved > > that right since forever, so I can squash it if you like. > > If it is already visible somewhere and a squash would inconvenience > anybody I'll resend it. > But if not I'd be happy about a squash. > > (I couldn't and still can't find the public tree where driver is in) Odd, neither can I! Okay applied the whole set again, squashed the patch and submitted for testing.
On 2024-06-20 18:15:11+0000, Lee Jones wrote: > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > On 2024-06-20 13:27:41+0000, Lee Jones wrote: > > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > > > Hi Lee, > > > > > > > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > > > > Definitely not seen a commit message like that before > > > > > > > > I assumed that this patch would be squashed into the original commit. > > > > > > > > My question in which form you wanted the changes should have included > > > > "incremental series". > > > > > > Incremental means on top. > > > > > > A lot of maintainers don't support history re-writes, but I've reserved > > > that right since forever, so I can squash it if you like. > > > > If it is already visible somewhere and a squash would inconvenience > > anybody I'll resend it. > > But if not I'd be happy about a squash. > > > > (I couldn't and still can't find the public tree where driver is in) > > Odd, neither can I! Okay applied the whole set again, squashed the > patch and submitted for testing. Thanks! FYI: The Ack you asked for in the cros_kbd_led_backlight series [0], which should go through the LED tree (and has a MFD component), was given by Tzung-Bi in [1]. (In case it fell through the cracks. If not, please disregard) Thomas [0] https://lore.kernel.org/lkml/20240526-cros_ec-kbd-led-framework-v3-0-ee577415a521@weissschuh.net/ [1] https://lore.kernel.org/lkml/ZmutW6vh7pD1HLf5@google.com/
On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > On 2024-06-20 18:15:11+0000, Lee Jones wrote: > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > On 2024-06-20 13:27:41+0000, Lee Jones wrote: > > > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > > > > > Hi Lee, > > > > > > > > > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > > > > > Definitely not seen a commit message like that before > > > > > > > > > > I assumed that this patch would be squashed into the original commit. > > > > > > > > > > My question in which form you wanted the changes should have included > > > > > "incremental series". > > > > > > > > Incremental means on top. > > > > > > > > A lot of maintainers don't support history re-writes, but I've reserved > > > > that right since forever, so I can squash it if you like. > > > > > > If it is already visible somewhere and a squash would inconvenience > > > anybody I'll resend it. > > > But if not I'd be happy about a squash. > > > > > > (I couldn't and still can't find the public tree where driver is in) > > > > Odd, neither can I! Okay applied the whole set again, squashed the > > patch and submitted for testing. > > Thanks! > > FYI: > > The Ack you asked for in the cros_kbd_led_backlight series [0], > which should go through the LED tree (and has a MFD component), > was given by Tzung-Bi in [1]. > > (In case it fell through the cracks. If not, please disregard) Now I'm really confused. This patch not for that set though, right? You're talking about: mfd: cros_ec: Register keyboard backlight subdevice platform/chrome: cros_kbd_led_backlight: allow binding through MFD <-- this one leds: class: Add flag to avoid automatic renaming of LED devices leds: class: Warn about name collisions earlier But this fix-up patch belongs in: mfd: cros_ec: Register LED subdevice leds: Add ChromeOS EC driver <-- this one leds: core: Unexport led_colors[] array leds: multicolor: Use led_get_color_name() function leds: core: Introduce led_get_color_name() function Right? > [0] https://lore.kernel.org/lkml/20240526-cros_ec-kbd-led-framework-v3-0-ee577415a521@weissschuh.net/ > [1] https://lore.kernel.org/lkml/ZmutW6vh7pD1HLf5@google.com/
On 2024-06-21 11:40:38+0000, Lee Jones wrote: > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > On 2024-06-20 18:15:11+0000, Lee Jones wrote: > > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > > > On 2024-06-20 13:27:41+0000, Lee Jones wrote: > > > > > On Thu, 20 Jun 2024, Thomas Weißschuh wrote: > > > > > > > > > > > Hi Lee, > > > > > > > > > > > > On 2024-06-20 10:31:14+0000, Lee Jones wrote: > > > > > > > Definitely not seen a commit message like that before > > > > > > > > > > > > I assumed that this patch would be squashed into the original commit. > > > > > > > > > > > > My question in which form you wanted the changes should have included > > > > > > "incremental series". > > > > > > > > > > Incremental means on top. > > > > > > > > > > A lot of maintainers don't support history re-writes, but I've reserved > > > > > that right since forever, so I can squash it if you like. > > > > > > > > If it is already visible somewhere and a squash would inconvenience > > > > anybody I'll resend it. > > > > But if not I'd be happy about a squash. > > > > > > > > (I couldn't and still can't find the public tree where driver is in) > > > > > > Odd, neither can I! Okay applied the whole set again, squashed the > > > patch and submitted for testing. > > > > Thanks! > > > > FYI: > > > > The Ack you asked for in the cros_kbd_led_backlight series [0], > > which should go through the LED tree (and has a MFD component), > > was given by Tzung-Bi in [1]. > > > > (In case it fell through the cracks. If not, please disregard) > > Now I'm really confused. > > This patch not for that set though, right? > > You're talking about: > > mfd: cros_ec: Register keyboard backlight subdevice > platform/chrome: cros_kbd_led_backlight: allow binding through MFD <-- this one > leds: class: Add flag to avoid automatic renaming of LED devices > leds: class: Warn about name collisions earlier > > But this fix-up patch belongs in: > > mfd: cros_ec: Register LED subdevice > leds: Add ChromeOS EC driver <-- this one > leds: core: Unexport led_colors[] array > leds: multicolor: Use led_get_color_name() function > leds: core: Introduce led_get_color_name() function > > Right? Yes, all correct. Which is why I referred to it explicitly as the "cros_kbd_led_backlight series". It was meant as a heads-up. Mentioned here as I was writing with you anyways. > > [0] https://lore.kernel.org/lkml/20240526-cros_ec-kbd-led-framework-v3-0-ee577415a521@weissschuh.net/ > > [1] https://lore.kernel.org/lkml/ZmutW6vh7pD1HLf5@google.com/
diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c index 7bb21a587713..275522b81ea5 100644 --- a/drivers/leds/leds-cros_ec.c +++ b/drivers/leds/leds-cros_ec.c @@ -14,8 +14,6 @@ #include <linux/platform_data/cros_ec_commands.h> #include <linux/platform_data/cros_ec_proto.h> -#define DRV_NAME "cros-ec-led" - static const char * const cros_ec_led_functions[] = { [EC_LED_ID_BATTERY_LED] = LED_FUNCTION_CHARGING, [EC_LED_ID_POWER_LED] = LED_FUNCTION_POWER, @@ -152,7 +150,7 @@ static int cros_ec_led_count_subleds(struct device *dev, if (common_range != range) { /* The multicolor LED API expects a uniform max_brightness */ - dev_warn(dev, "Inconsistent LED brightness values\n"); + dev_err(dev, "Inconsistent LED brightness values\n"); return -EINVAL; } } @@ -176,22 +174,21 @@ static const char *cros_ec_led_get_color_name(struct led_classdev_mc *led_mc_cde return led_get_color_name(color); } -static int cros_ec_led_probe_led(struct device *dev, struct cros_ec_device *cros_ec, +static int cros_ec_led_probe_one(struct device *dev, struct cros_ec_device *cros_ec, enum ec_led_id id) { union cros_ec_led_cmd_data arg = {}; struct cros_ec_led_priv *priv; struct led_classdev *led_cdev; struct mc_subled *subleds; - int ret, num_subleds; - size_t i, subled; + int i, ret, num_subleds; + size_t subled; arg.req.led_id = id; arg.req.flags = EC_LED_FLAGS_QUERY; ret = cros_ec_led_send_cmd(cros_ec, &arg); - /* Unknown LED, skip */ if (ret == -EINVAL) - return 0; + return 0; /* Unknown LED, skip */ if (ret == -EOPNOTSUPP) return -ENODEV; if (ret < 0) @@ -247,11 +244,14 @@ static int cros_ec_led_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); struct cros_ec_device *cros_ec = ec_dev->ec_dev; - int ret = 0; - size_t i; + int i, ret = 0; + + ret = devm_led_trigger_register(dev, &cros_ec_led_trigger); + if (ret) + return ret; for (i = 0; i < EC_LED_ID_COUNT; i++) { - ret = cros_ec_led_probe_led(dev, cros_ec, i); + ret = cros_ec_led_probe_one(dev, cros_ec, i); if (ret) break; } @@ -260,38 +260,16 @@ static int cros_ec_led_probe(struct platform_device *pdev) } static const struct platform_device_id cros_ec_led_id[] = { - { DRV_NAME, 0 }, + { "cros-ec-led", 0 }, {} }; static struct platform_driver cros_ec_led_driver = { - .driver.name = DRV_NAME, + .driver.name = "cros-ec-led", .probe = cros_ec_led_probe, .id_table = cros_ec_led_id, }; - -static int __init cros_ec_led_init(void) -{ - int ret; - - ret = led_trigger_register(&cros_ec_led_trigger); - if (ret) - return ret; - - ret = platform_driver_register(&cros_ec_led_driver); - if (ret) - led_trigger_unregister(&cros_ec_led_trigger); - - return ret; -}; -module_init(cros_ec_led_init); - -static void __exit cros_ec_led_exit(void) -{ - platform_driver_unregister(&cros_ec_led_driver); - led_trigger_unregister(&cros_ec_led_trigger); -}; -module_exit(cros_ec_led_exit); +module_platform_driver(cros_ec_led_driver); MODULE_DEVICE_TABLE(platform, cros_ec_led_id); MODULE_DESCRIPTION("ChromeOS EC LED Driver");
Implement review comments from Lee as requested in [0] for "leds: Add ChromeOS EC driver". Changes: * Inline DRV_NAME string constant. * Use dev_err() instead of dev_warn() to report errors. * Rename cros_ec_led_probe_led() to cros_ec_led_probe_one(). * Make loop variable "int" where they belong. * Move "Unknown LED" comment to where it belongs. * Register trigger during _probe(). * Use module_platform_driver() and drop all the custom boilerplate. [0] https://lore.kernel.org/lkml/20240614093445.GF3029315@google.com/T/#m8750abdef6a968ace765645189170814196c9ce9 Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/leds/leds-cros_ec.c | 50 +++++++++++++-------------------------------- 1 file changed, 14 insertions(+), 36 deletions(-) --- base-commit: b6774dd948171c478c7aa19318b1d7ae9cf6d7a4 change-id: 20240614-cros_ec-led-review-ec93abc65933 Best regards,