Re: [PATCH 7/9] leds: add Photonicat PMU LED driver

[Junhao Xie <bigfoot@classfun.cn>]

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@classfun.cn>
>> + */
>> +
>> +#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