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

[Lee Jones]

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.
> 
> Signed-off-by: Junhao Xie <bigfoot@xxxxxxxxxxx>
> ---
>  drivers/leds/Kconfig           | 11 +++++
>  drivers/leds/Makefile          |  1 +
>  drivers/leds/leds-photonicat.c | 75 ++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/leds/leds-photonicat.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 8d9d8da376e4..539adb5944e6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -381,6 +381,17 @@ config LEDS_PCA9532_GPIO
>  	  To use a pin as gpio pca9532_type in pca9532_platform data needs to
>  	  set to PCA9532_TYPE_GPIO.
>  
> +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.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called leds-photonicat.
> +
>  config LEDS_GPIO
>  	tristate "LED Support for GPIO connected LEDs"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 18afbb5a23ee..dcd5312aee12 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
>  obj-$(CONFIG_LEDS_PCA955X)		+= leds-pca955x.o
>  obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
>  obj-$(CONFIG_LEDS_PCA995X)		+= leds-pca995x.o
> +obj-$(CONFIG_LEDS_PHOTONICAT_PMU)	+= leds-photonicat.o
>  obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>  obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
> diff --git a/drivers/leds/leds-photonicat.c b/drivers/leds/leds-photonicat.c
> new file mode 100644
> index 000000000000..3aa5ce525b83
> --- /dev/null
> +++ 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?

> +	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)

> +	struct led_classdev cdev;
> +};
> +
> +static int pcat_led_status_set(struct led_classdev *cdev,
> +			       enum led_brightness brightness)
> +{
> +	struct pcat_leds *leds = container_of(cdev, struct pcat_leds, cdev);
> +	struct pcat_data_cmd_led_setup setup = { 0, 0, 0 };
> +
> +	if (brightness)
> +		setup.on_time = 100;
> +	else
> +		setup.down_time = 100;
> +	return pcat_pmu_write_data(leds->pmu, PCAT_CMD_NET_STATUS_LED_SETUP,
> +				   &setup, sizeof(setup));
> +}
> +
> +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);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "No label property\n");
> +
> +	leds->cdev.name = label;
> +	leds->cdev.max_brightness = 1;
> +	leds->cdev.brightness_set_blocking = pcat_led_status_set;
> +
> +	return devm_led_classdev_register(dev, &leds->cdev);
> +}
> +
> +static const struct of_device_id pcat_leds_dt_ids[] = {
> +	{ .compatible = "ariaboard,photonicat-pmu-leds", },

How many LEDs are there?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pcat_leds_dt_ids);
> +
> +static struct platform_driver pcat_leds_driver = {
> +	.driver = {
> +		.name = "photonicat-leds",
> +		.of_match_table = pcat_leds_dt_ids,
> +	},
> +	.probe = pcat_leds_probe,
> +};
> +module_platform_driver(pcat_leds_driver);
> +
> +MODULE_AUTHOR("Junhao Xie <bigfoot@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Photonicat PMU Status LEDs");
> +MODULE_LICENSE("GPL");
> -- 
> 2.46.0
> 

-- 
0)
Lee Jones [李琼斯]