Re: [PATCH v3 2/2] leds: Add PWM multicolor driver

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jan 26, 2022 at 11:05 PM <sven@svenschwermer.de> wrote:
>
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>
> 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