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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6e748aa6-de2c-4a90-b07f-7a9d3e441d19@classfun.cn>
Date: Fri, 8 Nov 2024 11:48:50 +0800
From: Junhao Xie <bigfoot@...ssfun.cn>
To: Lee Jones <lee@...nel.org>
Cc: devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-rtc@...r.kernel.org,
 linux-watchdog@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Pavel Machek <pavel@....cz>,
 Sebastian Reichel <sre@...nel.org>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Wim Van Sebroeck <wim@...ux-watchdog.org>, Heiko Stuebner <heiko@...ech.de>,
 Chukun Pan <amadeus@....edu.cn>, Junhao Xie <bigfoot@...ssfun.cn>
Subject: Re: [PATCH 7/9] leds: add Photonicat PMU LED driver

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@...ssfun.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