Hi Daniel, On 22/06/2020 21:46, Daniel Lezcano wrote: > On 18/06/2020 15:38, Neil Armstrong wrote: >> The new Khadas VIM2 and VIM3 boards controls the cooling fan via the >> on-board microcontroller. >> >> This implements the FAN control as thermal devices and as cell of the Khadas >> MCU MFD driver. >> >> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> >> Reviewed-by: Amit Kucheria <amit.kuche...@linaro.org> >> --- >> Hi Lee, >> >> Could you apply this patch via the MFD tree since it depends on >> the linux/mfd/khadas-mcu.h header ? >> >> This patch is unchanged from the v3 serie. >> >> Thanks, >> Neil >> >> drivers/thermal/Kconfig | 11 ++ >> drivers/thermal/Makefile | 1 + >> drivers/thermal/khadas_mcu_fan.c | 174 +++++++++++++++++++++++++++++++ >> 3 files changed, 186 insertions(+) >> create mode 100644 drivers/thermal/khadas_mcu_fan.c >> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >> index 3eb2348e5242..0125561488c9 100644 >> --- a/drivers/thermal/Kconfig >> +++ b/drivers/thermal/Kconfig >> @@ -500,4 +500,15 @@ config SPRD_THERMAL >> help >> Support for the Spreadtrum thermal sensor driver in the Linux thermal >> framework. >> + >> +config KHADAS_MCU_FAN_THERMAL >> + tristate "Khadas MCU controller FAN cooling support" >> + depends on OF || COMPILE_TEST >> + depends on MFD_KHADAS_MCU >> + select MFD_CORE >> + select REGMAP >> + help >> + If you say yes here you get support for the FAN controlled >> + by the Microcontroller found on the Khadas VIM boards. >> + >> endif >> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile >> index 0c8b84a09b9a..4b6aabaa7e31 100644 >> --- a/drivers/thermal/Makefile >> +++ b/drivers/thermal/Makefile >> @@ -61,3 +61,4 @@ obj-$(CONFIG_ZX2967_THERMAL) += zx2967_thermal.o >> obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o >> obj-$(CONFIG_AMLOGIC_THERMAL) += amlogic_thermal.o >> obj-$(CONFIG_SPRD_THERMAL) += sprd_thermal.o >> +obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL) += khadas_mcu_fan.o >> diff --git a/drivers/thermal/khadas_mcu_fan.c >> b/drivers/thermal/khadas_mcu_fan.c >> new file mode 100644 >> index 000000000000..6995b443cad4 >> --- /dev/null >> +++ b/drivers/thermal/khadas_mcu_fan.c >> @@ -0,0 +1,174 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * Khadas MCU Controlled FAN driver >> + * >> + * Copyright (C) 2020 BayLibre SAS >> + * Author(s): Neil Armstrong <narmstr...@baylibre.com> >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/mfd/khadas-mcu.h> >> +#include <linux/regmap.h> >> +#include <linux/sysfs.h> >> +#include <linux/thermal.h> >> + >> +#define MAX_LEVEL 3 >> + >> +struct khadas_mcu_fan_ctx { >> + struct khadas_mcu *mcu; >> + unsigned int level; >> + struct thermal_cooling_device *cdev; >> +}; >> + >> +static int khadas_mcu_fan_set_level(struct khadas_mcu_fan_ctx *ctx, >> + unsigned int level) >> +{ >> + int ret; >> + >> + ret = regmap_write(ctx->mcu->regmap, KHADAS_MCU_CMD_FAN_STATUS_CTRL_REG, >> + level); >> + if (ret) >> + return ret; >> + >> + ctx->level = level; >> + >> + return 0; >> +} >> + >> +static int khadas_mcu_fan_get_max_state(struct thermal_cooling_device *cdev, >> + unsigned long *state) >> +{ >> + struct khadas_mcu_fan_ctx *ctx = cdev->devdata; >> + >> + if (!ctx) >> + return -EINVAL; > > It is pointless to check 'ctx' is NULL, that was done at probe time. > >> + *state = MAX_LEVEL; >> + >> + return 0; >> +} >> + >> +static int khadas_mcu_fan_get_cur_state(struct thermal_cooling_device *cdev, >> + unsigned long *state) >> +{ >> + struct khadas_mcu_fan_ctx *ctx = cdev->devdata; >> + >> + if (!ctx) >> + return -EINVAL; > > Ditto > >> + *state = ctx->level; >> + >> + return 0; >> +} >> + >> +static int >> +khadas_mcu_fan_set_cur_state(struct thermal_cooling_device *cdev, >> + unsigned long state) >> +{ >> + struct khadas_mcu_fan_ctx *ctx = cdev->devdata; >> + >> + if (!ctx || (state > MAX_LEVEL)) >> + return -EINVAL; > > Ditto
Will remove these. > >> + if (state == ctx->level) >> + return 0; >> + >> + return khadas_mcu_fan_set_level(ctx, state); >> +} >> + >> +static const struct thermal_cooling_device_ops khadas_mcu_fan_cooling_ops = >> { >> + .get_max_state = khadas_mcu_fan_get_max_state, >> + .get_cur_state = khadas_mcu_fan_get_cur_state, >> + .set_cur_state = khadas_mcu_fan_set_cur_state, >> +}; >> + >> +static int khadas_mcu_fan_probe(struct platform_device *pdev) >> +{ >> + struct khadas_mcu *mcu = dev_get_drvdata(pdev->dev.parent); >> + struct thermal_cooling_device *cdev; >> + struct device *dev = &pdev->dev; >> + struct khadas_mcu_fan_ctx *ctx; >> + int ret; >> + >> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return -ENOMEM; >> + ctx->mcu = mcu; >> + platform_set_drvdata(pdev, ctx); >> + >> + cdev = devm_thermal_of_cooling_device_register(dev->parent, >> + dev->parent->of_node, "khadas-mcu-fan", ctx, >> + &khadas_mcu_fan_cooling_ops); >> + if (IS_ERR(cdev)) { >> + ret = PTR_ERR(cdev); >> + dev_err(dev, >> + "Failed to register khadas-mcu-fan as cooling >> device: %d\n", >> + ret); >> + return ret; >> + } >> + ctx->cdev = cdev; >> + thermal_cdev_update(cdev); >> + >> + return 0; >> +} >> + >> +static int khadas_mcu_fan_disable(struct device *dev) >> +{ >> + struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev); >> + unsigned int level_save = ctx->level; >> + int ret; >> + >> + ret = khadas_mcu_fan_set_level(ctx, 0); >> + if (ret) >> + return ret; >> + >> + ctx->level = level_save; > > Nitpicking : move the save section to suspend. OK, but moving this makes khadas_mcu_fan_disable() useless. > >> + return 0; >> +} >> + >> +static void khadas_mcu_fan_shutdown(struct platform_device *pdev) >> +{ >> + khadas_mcu_fan_disable(&pdev->dev); >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int khadas_mcu_fan_suspend(struct device *dev) >> +{ >> + return khadas_mcu_fan_disable(dev); >> +} >> + >> +static int khadas_mcu_fan_resume(struct device *dev) >> +{ >> + struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev); >> + >> + return khadas_mcu_fan_set_level(ctx, ctx->level); > > Out of curiosity, did you check the fan is not continuously spinning > after a resume when the suspend happened during a mitigation phase? No, but I took the logic from the hmwmon pwm-fan driver. Not sure this is critical here. Neil > >> +} >> +#endif >> + >> +static SIMPLE_DEV_PM_OPS(khadas_mcu_fan_pm, khadas_mcu_fan_suspend, >> + khadas_mcu_fan_resume); >> + >> +static const struct platform_device_id khadas_mcu_fan_id_table[] = { >> + { .name = "khadas-mcu-fan-ctrl", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(platform, khadas_mcu_fan_id_table); >> + >> +static struct platform_driver khadas_mcu_fan_driver = { >> + .probe = khadas_mcu_fan_probe, >> + .shutdown = khadas_mcu_fan_shutdown, >> + .driver = { >> + .name = "khadas-mcu-fan-ctrl", >> + .pm = &khadas_mcu_fan_pm, >> + }, >> + .id_table = khadas_mcu_fan_id_table, >> +}; >> + >> +module_platform_driver(khadas_mcu_fan_driver); >> + >> +MODULE_AUTHOR("Neil Armstrong <narmstr...@baylibre.com>"); >> +MODULE_DESCRIPTION("Khadas MCU FAN driver"); >> +MODULE_LICENSE("GPL"); >> > >