On 2024/10/2 23:35, Lee Jones wrote:
> On Fri, 06 Sep 2024, Junhao Xie wrote:
>
>> Photonicat has a network status LED that can be controlled by system.
>> The LED status can be set through command 0x19.
[...]
>> +config LEDS_PHOTONICAT_PMU
>> + tristate "LED Support for Photonicat PMU"
>> + depends on LEDS_CLASS
>> + depends on MFD_PHOTONICAT_PMU
>> + help
>> + Photonicat has a network status LED that can be controlled by system,
>
> "the system"
>
>> + this option enables support for LEDs connected to the Photonicat PMU.
[...]
>> +++ b/drivers/leds/leds-photonicat.c
>> @@ -0,0 +1,75 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 Junhao Xie <bigfoot@xxxxxxxxxxx>
>> + */
>> +
>> +#include <linux/mfd/photonicat-pmu.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/leds.h>
>
> Alphabetical.
>
>> +struct pcat_leds {
>> + struct device *dev;
>
> Where is this used?
I used it to print logs, but now it doesn't, I will remove it.
>
>> + struct pcat_pmu *pmu;
>
> Why do you need to store this?
>
> Can't you get this at the call-site by:
>
> dev_get_drvdata(cdev->dev->parent)
Yes, I will change it.
>> + struct led_classdev cdev;
>> +};
[...]
>> +static int pcat_leds_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>
> Small sized variables at the bottom please.
>
>> + struct device *dev = &pdev->dev;
>> + struct pcat_leds *leds;
>> + const char *label;
>> +
>> + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
>> + if (!leds)
>> + return -ENOMEM;
>> +
>> + leds->dev = dev;
>
> Where is this used?
>
>> + leds->pmu = dev_get_drvdata(dev->parent);
>> + platform_set_drvdata(pdev, leds);
>
> Where do you platform_get_drvdata()
>
>> + ret = of_property_read_string(dev->of_node, "label", &label);
[...]
>> +static const struct of_device_id pcat_leds_dt_ids[] = {
>> + { .compatible = "ariaboard,photonicat-pmu-leds", },
>
> How many LEDs are there?
Photonicat has three LEDs:
- system operation status indicator
- charging status indicator
- network status indicator
and currently only one LED (network status indicator) can be controlled.
>> + { /* sentinel */ }
>> +};
[...]
>> --
>> 2.46.0
Thanks for your review, I will fix all problems in next version!
Best regards,
Junhao
