On Wed, Jun 15, 2022 at 5:49 PM Jean-Jacques Hiblot
<jjhiblot@xxxxxxxxxxxxxxx> wrote:
>
> By allowing to group multiple monochrome LED into multicolor LEDs,
> all involved LEDs can be controlled in-sync. This enables using effects
> using triggers, etc.
...
> +#include <linux/err.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
Missed math.h
...
> +static int iterate_subleds(struct device *dev, struct led_mcg_priv *priv,
> + struct fwnode_handle *mcnode)
Use namespace even for static functions (think about tracing, for example).
led_mcg_iterave_subleds
> +{
> + struct mc_subled *subled = priv->mc_cdev.subled_info;
> + struct fwnode_handle *fwnode;
> + int ret;
> +
> + /* iterate over the nodes inside the multi-led node */
> + fwnode_for_each_child_node(mcnode, fwnode) {
> + u32 color;
> + struct led_classdev *led_cdev;
> +
> + led_cdev = devm_fwnode_led_get(dev, fwnode, 0);
> + if (IS_ERR(led_cdev)) {
> + ret = PTR_ERR(led_cdev);
> + dev_err(dev, "unable to request LED: %d\n", ret);
ret = dev_err_probe(...);
> + goto release_fwnode;
> + }
> + priv->monochromatics[priv->mc_cdev.num_colors] = led_cdev;
> +
> + ret = fwnode_property_read_u32(fwnode, "color", &color);
> + if (ret) {
> + dev_err(dev, "cannot read color: %d\n", ret);
> + goto release_fwnode;
> + }
> + subled[priv->mc_cdev.num_colors].color_index = color;
> +
> + /* Make the sysfs of the monochromatic LED read-only */
> + led_cdev->flags |= LED_SYSFS_DISABLE;
> +
> + priv->mc_cdev.num_colors++;
> + }
> +
> + return 0;
> +
> +release_fwnode:
> + fwnode_handle_put(fwnode);
> + return ret;
> +}
...
> + /* count the nodes inside the multi-led node */
> + fwnode_for_each_child_node(mcnode, fwnode)
> + count++;
Don't we have a _count API? Hmm... Indeed, we have it only for a
device and not for fwnode...
...
> + priv = devm_kzalloc(&pdev->dev,
> + struct_size(priv, monochromatics, count),
> + GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto release_mcnode;
This is the wrong order. You shouldn't mix non-devm_ APIs with devm_
like this. devm_ calls always should be first. You have two options
(at least?): 1) drop devm_ and switch to plain error handling and
->remove(); 2) make devm_ wrappers for the certain calls.
> + }
...
> + if (ret) {
> + dev_err(&pdev->dev,
> + "failed to register multicolor led for %s: %d\n",
> + cdev->name, ret);
Taking into account above,
return dev_err_probe(...);
> + goto release_mcnode;
> + }
--
With Best Regards,
Andy Shevchenko
