mbox series

[0/9] Introduce Photonicat power management MCU driver

Message ID 20240906093630.2428329-1-bigfoot@classfun.cn
Headers show
Series Introduce Photonicat power management MCU driver | expand

Message

Junhao Xie Sept. 6, 2024, 9:36 a.m. UTC
Initial support for the power management MCU in the Ariaboard Photonicat
This patch series depends on Add support for Ariaboard Photonicat RK3568 [1]

Currently implemented features:
  Implement serial communication protocol with MCU [2].
  Support watchdog in MCU.
  Shutdown by power button and system notifies MCU to power-off.
  Read charger and battery supply voltage and simply calculate capacity.
  Read board temperature sensor.
  Set status of the network status LED.
  Read and set the MCU real-time clock date time.

[1] https://lore.kernel.org/linux-arm-kernel/20240906045706.1004813-1-bigfoot@classfun.cn/
[2] https://photonicat.com/wiki/PMU_Protocol

Junhao Xie (9):
  mfd: Add driver for Photonicat power management MCU
  power: reset: add Photonicat PMU poweroff driver
  watchdog: Add Photonicat PMU watchdog driver
  power: supply: photonicat-supply: Add Photonicat PMU battery and
    charger
  rtc: Add Photonicat PMU real-time clock
  hwmon: Add support for Photonicat PMU board temperature sensor
  leds: add Photonicat PMU LED driver
  dt-bindings: Add documentation for Photonicat PMU
  arm64: dts: rockchip: add Photonicat PMU support for Ariaboard
    Photonicat

 .../hwmon/ariaboard,photonicat-pmu-hwmon.yaml |  40 ++
 .../leds/ariaboard,photonicat-pmu-leds.yaml   |  41 ++
 .../mfd/ariaboard,photonicat-pmu.yaml         | 107 ++++
 .../ariaboard,photonicat-pmu-poweroff.yaml    |  34 ++
 .../ariaboard,photonicat-pmu-supply.yaml      |  55 ++
 .../rtc/ariaboard,photonicat-pmu-rtc.yaml     |  37 ++
 .../ariaboard,photonicat-pmu-watchdog.yaml    |  37 ++
 .../boot/dts/rockchip/rk3568-photonicat.dts   |  43 ++
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/photonicat-hwmon.c              | 129 +++++
 drivers/leds/Kconfig                          |  11 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-photonicat.c                |  75 +++
 drivers/mfd/Kconfig                           |  13 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/photonicat-pmu.c                  | 501 ++++++++++++++++++
 drivers/power/reset/Kconfig                   |  12 +
 drivers/power/reset/Makefile                  |   1 +
 drivers/power/reset/photonicat-poweroff.c     |  95 ++++
 drivers/power/supply/Kconfig                  |  12 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/photonicat-supply.c      | 250 +++++++++
 drivers/rtc/Kconfig                           |  12 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-photonicat.c                  | 190 +++++++
 drivers/watchdog/Kconfig                      |  12 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/photonicat-wdt.c             | 124 +++++
 include/linux/mfd/photonicat-pmu.h            |  86 +++
 30 files changed, 1933 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
 create mode 100644 Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
 create mode 100644 Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml
 create mode 100644 Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
 create mode 100644 drivers/hwmon/photonicat-hwmon.c
 create mode 100644 drivers/leds/leds-photonicat.c
 create mode 100644 drivers/mfd/photonicat-pmu.c
 create mode 100644 drivers/power/reset/photonicat-poweroff.c
 create mode 100644 drivers/power/supply/photonicat-supply.c
 create mode 100644 drivers/rtc/rtc-photonicat.c
 create mode 100644 drivers/watchdog/photonicat-wdt.c
 create mode 100644 include/linux/mfd/photonicat-pmu.h

Comments

Junhao Xie Sept. 6, 2024, 10:40 a.m. UTC | #1
On 2024/9/6 17:43, Krzysztof Kozlowski wrote:
> On 06/09/2024 11:36, Junhao Xie wrote:
>> Add a driver for Photonicat power management MCU, which
[...]>> +void *pcat_data_get_data(struct pcat_data *data)
>> +{
>> +	if (!data)
>> +		return NULL;
>> +	return data->data;
>> +}
>> +EXPORT_SYMBOL_GPL(pcat_data_get_data);
> 
> You need kerneldoc... or just drop it. Looks a bit useless as an
> export... Is it because you want to hide from your own driver pcat_data?
> What for? It's your driver...

Now struct pcat_data is in mfd-photonicat.c,
I will move it to photonicat-pmu.h and remove pcat_data_get_data.

> 
[...]
>> +void pcat_pmu_unregister_notify(struct pcat_pmu *pmu, struct notifier_block *nb)
> 
> You need kerneldoc.

I will add missing kernel documentation for all exported functions.

> 
[...]
>> +
>> +static const struct of_device_id pcat_pmu_dt_ids[] = {
>> +	{ .compatible = "ariaboard,photonicat-pmu", },
> 
> Undocumented compatible.
> 
> Remember about correct order of patches. ABI documentation is before users.
> 

I will adjust order of patches.

> 
[...]
>> +
>> +MODULE_ALIAS("platform:photonicat-pmu");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
> 
> And it is not even correct. This is not a platform driver!
> 

Yes, I will remove MODULE_ALIAS line

> 
> Best regards,
> Krzysztof
> 

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao
Guenter Roeck Sept. 6, 2024, 11:52 a.m. UTC | #2
On 9/6/24 02:36, Junhao Xie wrote:
> This driver provides access to Photonicat PMU watchdog functionality.
> 
> Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
> ---
>   drivers/watchdog/Kconfig          |  12 +++
>   drivers/watchdog/Makefile         |   1 +
>   drivers/watchdog/photonicat-wdt.c | 124 ++++++++++++++++++++++++++++++
>   3 files changed, 137 insertions(+)
>   create mode 100644 drivers/watchdog/photonicat-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bae1d97cce89..4094216a1c09 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -300,6 +300,18 @@ config MENZ069_WATCHDOG
>   	  This driver can also be built as a module. If so the module
>   	  will be called menz069_wdt.
>   
> +config PHOTONICAT_PMU_WDT
> +	tristate "Photonicat PMU Watchdog"
> +	depends on MFD_PHOTONICAT_PMU
> +	select WATCHDOG_CORE
> +	help
> +	  This driver provides access to Photonicat PMU watchdog functionality.
> +
> +	  Say Y here to include support for the Photonicat PMU Watchdog.
> +
> +	  This driver can also be built as a module. If so the module
> +	  will be called photonicat-wdt.
> +
>   config WDAT_WDT
>   	tristate "ACPI Watchdog Action Table (WDAT)"
>   	depends on ACPI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b51030f035a6..14375af84039 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -234,6 +234,7 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o
>   obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
>   obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
>   obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
> +obj-$(CONFIG_PHOTONICAT_PMU_WDT) += photonicat-wdt.o
>   obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>   obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>   obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> diff --git a/drivers/watchdog/photonicat-wdt.c b/drivers/watchdog/photonicat-wdt.c
> new file mode 100644
> index 000000000000..1e758fcfb49f
> --- /dev/null
> +++ b/drivers/watchdog/photonicat-wdt.c
> @@ -0,0 +1,124 @@
> +// 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/watchdog.h>
> +
> +struct pcat_watchdog {
> +	struct device *dev;

I don't see what this is used for.

> +	struct pcat_pmu *pmu;
> +	struct watchdog_device wdd;
> +	u8 timeout;
> +	bool started;
> +};
> +
> +static const struct watchdog_info pcat_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "Photonicat PMU Watchdog",
> +};
> +
> +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
> +{
> +	int ret;
> +	u8 time = 0;

Unnecessary initialization.

> +	u8 times[3] = { 60, 60, 0 };
> +
> +	time = MIN(255, MAX(0, timeout));
> +
> +	ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET,
> +				  times, sizeof(times));

Where does this actually send the timeout to the chip ?

> +	if (!ret)
> +		data->started = timeout != 0;
> +
> +	return ret;
> +}
> +
> +static int pcat_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> +	return pcat_wdt_setup(data, data->timeout);
> +}
> +
> +static int pcat_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> +	return pcat_wdt_setup(data, 0);
> +}
> +
> +static int pcat_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> +	return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0);
> +}
> +
> +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
> +{
> +	int ret = 0;
> +	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
> +
> +	data->timeout = val;

This needs to store 'timeout' in wdev. Storing it locally is unnecessary.

> +	if (data->started)
> +		ret = pcat_wdt_setup(data, data->timeout);

This is misleading because it would permit setting the timeout to
0 when the watchdog isn't running, and then when the watchdog is started
it would not really start it. The code should not use a local "started"
variable but call watchdog_active(). It should also not accept "0"
as a valid timeout.

> +
> +	return ret;
> +}
> +
> +static const struct watchdog_ops pcat_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcat_wdt_start,
> +	.stop = pcat_wdt_stop,
> +	.ping = pcat_wdt_ping,
> +	.set_timeout = pcat_wdt_set_timeout,
> +};
> +
> +static int pcat_watchdog_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pcat_watchdog *watchdog;
> +
> +	watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL);
> +	if (!watchdog)
> +		return -ENOMEM;
> +
> +	watchdog->dev = dev;
> +	watchdog->pmu = dev_get_drvdata(dev->parent);
> +	watchdog->wdd.info = &pcat_wdt_info;
> +	watchdog->wdd.ops = &pcat_wdt_ops;
> +	watchdog->wdd.timeout = 60;
> +	watchdog->wdd.max_timeout = U8_MAX;
> +	watchdog->wdd.min_timeout = 0;

This effectively lets the user ... kind of ... stop the watchdog
by setting the timeout to 0. This is not acceptable.

> +	watchdog->wdd.parent = dev;
> +
> +	watchdog_stop_on_reboot(&watchdog->wdd);
> +	watchdog_set_drvdata(&watchdog->wdd, watchdog);
> +	platform_set_drvdata(pdev, watchdog);
> +
No watchdog_init_timeout() ?

> +	return devm_watchdog_register_device(dev, &watchdog->wdd);
> +}
> +
> +static const struct of_device_id pcat_watchdog_dt_ids[] = {
> +	{ .compatible = "ariaboard,photonicat-pmu-watchdog", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids);
> +
> +static struct platform_driver pcat_watchdog_driver = {
> +	.driver = {
> +		.name = "photonicat-watchdog",
> +		.of_match_table = pcat_watchdog_dt_ids,
> +	},
> +	.probe = pcat_watchdog_probe,
> +};
> +module_platform_driver(pcat_watchdog_driver);
> +
> +MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
> +MODULE_DESCRIPTION("Photonicat PMU watchdog");
> +MODULE_LICENSE("GPL");
Junhao Xie Sept. 6, 2024, 1:41 p.m. UTC | #3
On 2024/9/6 19:52, Guenter Roeck wrote:
> On 9/6/24 02:36, Junhao Xie wrote:
>> This driver provides access to Photonicat PMU watchdog functionality.
>>
[...]
>> +
>> +struct pcat_watchdog {
>> +    struct device *dev;
> 
> I don't see what this is used for.

I used to use this for logging, but now they are gone, I will delete it.

> 
[...]
>> +
>> +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
>> +{
>> +    int ret;
>> +    u8 time = 0;
> 
> Unnecessary initialization.
> 
>> +    u8 times[3] = { 60, 60, 0 };
>> +
>> +    time = MIN(255, MAX(0, timeout));
>> +
>> +    ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET,
>> +                  times, sizeof(times));
> 
> Where does this actually send the timeout to the chip ?
> 

I forgot to fill in timeout into times[2] during refactoring process, I will fix it.

>> +    if (!ret)
[...]>> +
>> +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
>> +{
>> +    int ret = 0;
>> +    struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
>> +
>> +    data->timeout = val;
> 
> This needs to store 'timeout' in wdev. Storing it locally is unnecessary.
> 
>> +    if (data->started)
>> +        ret = pcat_wdt_setup(data, data->timeout);
> 
> This is misleading because it would permit setting the timeout to
> 0 when the watchdog isn't running, and then when the watchdog is started
> it would not really start it. The code should not use a local "started"
> variable but call watchdog_active(). It should also not accept "0"
> as a valid timeout.
> 

I will fix the pcat_wdt_set_timeout.

>> +
[...]
>> +
>> +    watchdog->dev = dev;
>> +    watchdog->pmu = dev_get_drvdata(dev->parent);
>> +    watchdog->wdd.info = &pcat_wdt_info;
>> +    watchdog->wdd.ops = &pcat_wdt_ops;
>> +    watchdog->wdd.timeout = 60;
>> +    watchdog->wdd.max_timeout = U8_MAX;
>> +    watchdog->wdd.min_timeout = 0;
> 
> This effectively lets the user ... kind of ... stop the watchdog
> by setting the timeout to 0. This is not acceptable.
> 
>> +    watchdog->wdd.parent = dev;
>> +
>> +    watchdog_stop_on_reboot(&watchdog->wdd);
>> +    watchdog_set_drvdata(&watchdog->wdd, watchdog);
>> +    platform_set_drvdata(pdev, watchdog);
>> +
> No watchdog_init_timeout() ?

Thanks for your correction, I will fix it.

> 
>> +    return devm_watchdog_register_device(dev, &watchdog->wdd);
[...]
>> +MODULE_LICENSE("GPL");
> 

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao
Junhao Xie Sept. 6, 2024, 1:56 p.m. UTC | #4
On 2024/9/6 17:53, Krzysztof Kozlowski wrote:
> On 06/09/2024 11:36, Junhao Xie wrote:
>> This commit adds support for Photonicat power management MCU on
>> Ariaboard Photonicat.
>>
[...]
>> +
>> +		pcat_pmu_battery: supply-battery {
> 
> Drop unused labels. Everywhere. You are not making the code more readable.

I will remove them.

> 
>> +			compatible = "ariaboard,photonicat-pmu-supply";
>> +			label = "battery";
>> +			monitored-battery = <&battery>;
>> +			power-supplies = <&pcat_pmu_charger>;
> 
> Why do you reference internal design of the device as DTS? You cannot
> have here other power supply, can you?

I mistakenly thought power_supply_am_i_supplied() required power-supplies
property, it actually does not, I will remove it.

> 
> Best regards,
> Krzysztof
> 

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao
Chukun Pan Sept. 8, 2024, 9:30 a.m. UTC | #5
Hi Junhao,

> Initial support for the power management MCU in the Ariaboard Photonicat
> This patch series depends on Add support for Ariaboard Photonicat RK3568 [1]

The official website says it's "Renesas RA2E1 cortex M23 ultra-low power MCU"
Perhaps renaming the 'Photonicat MCU' to 'Renesas RA2E1 MCU' would be better?
Junhao Xie Sept. 11, 2024, 6:23 a.m. UTC | #6
On 2024/9/8 17:30, Chukun Pan wrote:
>> Initial support for the power management MCU in the Ariaboard Photonicat
>> This patch series depends on Add support for Ariaboard Photonicat RK3568 [1]
> 
> The official website says it's "Renesas RA2E1 cortex M23 ultra-low power MCU"
> Perhaps renaming the 'Photonicat MCU' to 'Renesas RA2E1 MCU' would be better?

Renesas RA2E1 is a MCU product line, and Ariaboard wrote firmware for this MCU.
Maybe "Renesas RA2E1 MCU in Photonicat" would be better?


Best regards,
Junhao