On Wed, Jan 26, 2022 at 11:05 PM <sven@xxxxxxxxxxxxxxxx> wrote:
>
> From: Sven Schwermer <sven.schwermer@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>
> By allowing to group multiple monochrome PWM LEDs into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.
...
> + help
> + This option enables support for PWM driven monochrome LEDs that are
> + grouped into multicolor LEDs.
What would be the module name if compiled as a module?
...
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
mod_devicetable.h
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
property.h
> +#include <linux/pwm.h>
...
> + int i;
> + unsigned long long duty;
> + int ret = 0;
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> + struct pwm_mc_led *priv = container_of(mc_cdev, struct pwm_mc_led, mc_cdev);
Can we order in reversed xmas tree order?
struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
struct pwm_mc_led *priv = container_of(mc_cdev, struct
pwm_mc_led, mc_cdev);
unsigned long long duty;
int ret = 0;
int i;
Same for other functions.
...
> + dev_err(&pdev->dev, "expected multi-led node\n");
> + ret = -ENODEV;
> + goto out;
return dev_err_probe(dev, -ENODEV, ...);
...
> + /* count the nodes inside the multi-led node */
> + fwnode_for_each_child_node(mcnode, fwnode)
> + ++count;
Postincrement shall work the same way.
...
> + ret = PTR_ERR(pwmled->pwm);
> + dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
> + fwnode_handle_put(fwnode);
> + goto destroy_mutex;
fwnode_handle_put();
return dev_err_probe(...);
...
> + dev_err(&pdev->dev, "cannot read color: %d\n", ret);
> + fwnode_handle_put(fwnode);
> + goto destroy_mutex;
fwnode_handle_put();
return dev_err_probe();
> + }
...
> + ++priv->mc_cdev.num_colors;
Postincrement shall work the same way.
> + }
...
> + dev_err(&pdev->dev,
> + "failed to register multicolor PWM led for %s: %d\n",
> + cdev->name, ret);
> + goto destroy_mutex;
return dev_err_probe();
...
> + dev_err(&pdev->dev, "failed to set led PWM value for %s: %d",
> + cdev->name, ret);
> + goto destroy_mutex;
return dev_err_probe();
...
> +destroy_mutex:
> + mutex_destroy(&priv->lock);
Wrong ordering here and in ->remove().
Don't mix devm_* with non-devm_* calls.
> +release_mcnode:
> + fwnode_handle_put(mcnode);
> +out:
> + return ret;
Return directly.
> +}
> +
> +static int led_pwm_mc_remove(struct platform_device *pdev)
> +{
> + struct pwm_mc_led *priv = platform_get_drvdata(pdev);
> +
> + mutex_destroy(&priv->lock);
> + return 0;
> +}
...
> +static const struct of_device_id of_pwm_leds_mc_match[] = {
> + { .compatible = "pwm-leds-multicolor", },
> + {},
No comma needed for terminator entry.
> +};
...
> +static struct platform_driver led_pwm_mc_driver = {
> + .probe = led_pwm_mc_probe,
> + .remove = led_pwm_mc_remove,
> + .driver = {
> + .name = "leds_pwm_multicolor",
> + .of_match_table = of_pwm_leds_mc_match,
> + },
> +};
> +
Redundant blank line.
> +module_platform_driver(led_pwm_mc_driver);
--
With Best Regards,
Andy Shevchenko
