| Message ID | 20230103013145.9570-3-hal.feng@starfivetech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Temperature sensor support for StarFive JH7110 RISC-V SoC | expand |
On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: > From: Emil Renner Berthing <kernel@esmil.dk> > > Register definitions and conversion constants based on sfctemp driver by > Samin in the StarFive 5.10 kernel. > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Co-developed-by: Samin Guo <samin.guo@starfivetech.com> > Signed-off-by: Samin Guo <samin.guo@starfivetech.com> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com> This is obviously version 4 of the previous patch series, with various enhancements. Please do not "sell" that as v1 of a new patch series. Submit it as next version, and provide a change log. > --- > Documentation/hwmon/index.rst | 1 + > Documentation/hwmon/sfctemp.rst | 33 +++ > MAINTAINERS | 8 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/sfctemp.c | 350 ++++++++++++++++++++++++++++++++ > 6 files changed, 403 insertions(+) > create mode 100644 Documentation/hwmon/sfctemp.rst > create mode 100644 drivers/hwmon/sfctemp.c > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index c1d11cf13eef..f7ede608b6e3 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers > sch5627 > sch5636 > scpi-hwmon > + sfctemp > sht15 > sht21 > sht3x > diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst > new file mode 100644 > index 000000000000..9fbd5bb1f356 > --- /dev/null > +++ b/Documentation/hwmon/sfctemp.rst > @@ -0,0 +1,33 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +Kernel driver sfctemp > +===================== > + > +Supported chips: > + - StarFive JH7100 > + - StarFive JH7110 > + > +Authors: > + - Emil Renner Berthing <kernel@esmil.dk> > + > +Description > +----------- > + > +This driver adds support for reading the built-in temperature sensor on the > +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd. > + > +``sysfs`` interface > +------------------- > + > +The temperature sensor can be enabled, disabled and queried via the standard > +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of > +``X``: > + > +================ ==== ============================================= > +Name Perm Description > +================ ==== ============================================= > +temp1_enable RW Enable or disable temperature sensor. > + Automatically enabled by the driver, > + but may be disabled to save power. > +temp1_input RO Temperature reading in milli-degrees Celsius. > +================ ==== ============================================= > diff --git a/MAINTAINERS b/MAINTAINERS > index 85e8f83161d7..ab3cd5827b26 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18661,6 +18661,14 @@ L: netdev@vger.kernel.org > S: Supported > F: drivers/net/ethernet/sfc/ > > +SFCTEMP HWMON DRIVER > +M: Emil Renner Berthing <kernel@esmil.dk> > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml > +F: Documentation/hwmon/sfctemp.rst > +F: drivers/hwmon/sfctemp.c > + > SFF/SFP/SFP+ MODULE SUPPORT > M: Russell King <linux@armlinux.org.uk> > L: netdev@vger.kernel.org > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 7ac3daaf59ce..c6bbfcca3a14 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1910,6 +1910,16 @@ config SENSORS_STTS751 > This driver can also be built as a module. If so, the module > will be called stts751. > > +config SENSORS_SFCTEMP > + tristate "Starfive JH71x0 temperature sensor" > + depends on SOC_STARFIVE || COMPILE_TEST > + help > + If you say yes here you get support for temperature sensor > + on the Starfive JH71x0 SoCs. > + > + This driver can also be built as a module. If so, the module > + will be called sfctemp. > + > config SENSORS_SMM665 > tristate "Summit Microelectronics SMM665" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 11d076cad8a2..5a4a02c5535c 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI) += sbrmi.o > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o > obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o > +obj-$(CONFIG_SENSORS_SFCTEMP) += sfctemp.o > obj-$(CONFIG_SENSORS_SL28CPLD) += sl28cpld-hwmon.o > obj-$(CONFIG_SENSORS_SHT15) += sht15.o > obj-$(CONFIG_SENSORS_SHT21) += sht21.o > diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c > new file mode 100644 > index 000000000000..e56716ad9587 > --- /dev/null > +++ b/drivers/hwmon/sfctemp.c > @@ -0,0 +1,350 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> > + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> > + */ > +#include <linux/clk.h> > +#include <linux/completion.h> > +#include <linux/delay.h> > +#include <linux/hwmon.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > + > +/* > + * TempSensor reset. The RSTN can be de-asserted once the analog core has > + * powered up. Trst(min 100ns) > + * 0:reset 1:de-assert > + */ > +#define SFCTEMP_RSTN BIT(0) Missing include of linux/bits.h > + > +/* > + * TempSensor analog core power down. The analog core will be powered up > + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the > + * analog core is powered up. > + * 0:power up 1:power down > + */ > +#define SFCTEMP_PD BIT(1) > + > +/* > + * TempSensor start conversion enable. > + * 0:disable 1:enable > + */ > +#define SFCTEMP_RUN BIT(2) > + > +/* > + * TempSensor conversion value output. > + * Temp(C)=DOUT*Y/4094 - K > + */ > +#define SFCTEMP_DOUT_POS 16 > +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) > + > +/* DOUT to Celcius conversion constants */ > +#define SFCTEMP_Y1000 237500L > +#define SFCTEMP_Z 4094L > +#define SFCTEMP_K1000 81100L > + > +struct sfctemp { > + /* serialize access to hardware register and enabled below */ > + struct mutex lock; > + struct completion conversion_done; > + void __iomem *regs; > + struct clk *clk_sense; > + struct clk *clk_bus; > + struct reset_control *rst_sense; > + struct reset_control *rst_bus; > + bool enabled; > +}; > + > +static irqreturn_t sfctemp_isr(int irq, void *data) > +{ > + struct sfctemp *sfctemp = data; > + > + complete(&sfctemp->conversion_done); > + return IRQ_HANDLED; > +} > + > +static void sfctemp_power_up(struct sfctemp *sfctemp) > +{ > + /* make sure we're powered down first */ > + writel(SFCTEMP_PD, sfctemp->regs); > + udelay(1); > + > + writel(0, sfctemp->regs); > + /* wait t_pu(50us) + t_rst(100ns) */ > + usleep_range(60, 200); > + > + /* de-assert reset */ > + writel(SFCTEMP_RSTN, sfctemp->regs); > + udelay(1); /* wait t_su(500ps) */ > +} > + > +static void sfctemp_power_down(struct sfctemp *sfctemp) > +{ > + writel(SFCTEMP_PD, sfctemp->regs); > +} > + > +static void sfctemp_run_single(struct sfctemp *sfctemp) > +{ > + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); > + udelay(1); > + writel(SFCTEMP_RSTN, sfctemp->regs); The datasheet (or, rather, programming manual) does not appear to be public, so I have to guess here. The code suggests that running a single conversion may be a choice, not a requirement. If it is indeed a choice, the reasoning needs to be explained since it adds a lot of complexity and dependencies to the driver (for example, interrupt support is only mandatory or even needed due to this choice). It also adds a significant delay to temperature read operations, which may have practical impact on thermal control software. If the chip only supports single temperature readings, that needs to be explained as well (and why SFCTEMP_RUN has to be reset in that case). > +} > + > +static int sfctemp_enable(struct sfctemp *sfctemp) > +{ > + int ret = 0; > + > + mutex_lock(&sfctemp->lock); > + if (sfctemp->enabled) > + goto done; > + > + ret = clk_prepare_enable(sfctemp->clk_bus); > + if (ret) > + goto err; > + ret = reset_control_deassert(sfctemp->rst_bus); > + if (ret) > + goto err_disable_bus; > + > + ret = clk_prepare_enable(sfctemp->clk_sense); > + if (ret) > + goto err_assert_bus; > + ret = reset_control_deassert(sfctemp->rst_sense); > + if (ret) > + goto err_disable_sense; > + > + sfctemp_power_up(sfctemp); > + sfctemp->enabled = true; > +done: > + mutex_unlock(&sfctemp->lock); > + return ret; > + > +err_disable_sense: > + clk_disable_unprepare(sfctemp->clk_sense); > +err_assert_bus: > + reset_control_assert(sfctemp->rst_bus); > +err_disable_bus: > + clk_disable_unprepare(sfctemp->clk_bus); > +err: > + mutex_unlock(&sfctemp->lock); > + return ret; > +} > + > +static int sfctemp_disable(struct sfctemp *sfctemp) > +{ > + mutex_lock(&sfctemp->lock); > + if (!sfctemp->enabled) > + goto done; > + > + sfctemp_power_down(sfctemp); > + reset_control_assert(sfctemp->rst_sense); > + clk_disable_unprepare(sfctemp->clk_sense); > + reset_control_assert(sfctemp->rst_bus); > + clk_disable_unprepare(sfctemp->clk_bus); > + sfctemp->enabled = false; > +done: > + mutex_unlock(&sfctemp->lock); > + return 0; > +} > + > +static void sfctemp_disable_action(void *data) > +{ > + sfctemp_disable(data); > +} > + > +static int sfctemp_convert(struct sfctemp *sfctemp, long *val) > +{ > + int ret; > + > + mutex_lock(&sfctemp->lock); > + if (!sfctemp->enabled) { > + ret = -ENODATA; > + goto out; > + } > + > + sfctemp_run_single(sfctemp); > + > + ret = wait_for_completion_interruptible_timeout(&sfctemp->conversion_done, > + msecs_to_jiffies(10)); > + if (ret <= 0) { > + if (ret == 0) > + ret = -ETIMEDOUT; > + goto out; > + } > + > + /* calculate temperature in milli Celcius */ > + *val = (long)((readl(sfctemp->regs) & SFCTEMP_DOUT_MSK) >> SFCTEMP_DOUT_POS) > + * SFCTEMP_Y1000 / SFCTEMP_Z - SFCTEMP_K1000; > + > + ret = 0; > +out: > + mutex_unlock(&sfctemp->lock); > + return ret; > +} > + > +static umode_t sfctemp_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_temp: > + switch (attr) { > + case hwmon_temp_enable: > + return 0644; > + case hwmon_temp_input: > + return 0444; > + } > + return 0; > + default: > + return 0; > + } > +} > + > +static int sfctemp_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct sfctemp *sfctemp = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_temp: > + switch (attr) { > + case hwmon_temp_enable: > + *val = sfctemp->enabled; > + return 0; > + case hwmon_temp_input: > + return sfctemp_convert(sfctemp, val); > + } > + return -EINVAL; > + default: > + return -EINVAL; > + } > +} > + > +static int sfctemp_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + struct sfctemp *sfctemp = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_temp: > + switch (attr) { > + case hwmon_temp_enable: > + if (val == 0) > + return sfctemp_disable(sfctemp); > + if (val == 1) > + return sfctemp_enable(sfctemp); > + break; > + } > + return -EINVAL; > + default: > + return -EINVAL; > + } > +} > + > +static const struct hwmon_channel_info *sfctemp_info[] = { > + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), > + HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT), > + NULL > +}; > + > +static const struct hwmon_ops sfctemp_hwmon_ops = { > + .is_visible = sfctemp_is_visible, > + .read = sfctemp_read, > + .write = sfctemp_write, > +}; > + > +static const struct hwmon_chip_info sfctemp_chip_info = { > + .ops = &sfctemp_hwmon_ops, > + .info = sfctemp_info, > +}; > + > +static int sfctemp_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device *hwmon_dev; > + struct sfctemp *sfctemp; > + int ret; > + > + sfctemp = devm_kzalloc(dev, sizeof(*sfctemp), GFP_KERNEL); > + if (!sfctemp) > + return -ENOMEM; > + > + dev_set_drvdata(dev, sfctemp); > + mutex_init(&sfctemp->lock); > + init_completion(&sfctemp->conversion_done); > + > + sfctemp->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(sfctemp->regs)) > + return PTR_ERR(sfctemp->regs); > + > + sfctemp->clk_sense = devm_clk_get(dev, "sense"); > + if (IS_ERR(sfctemp->clk_sense)) > + return dev_err_probe(dev, PTR_ERR(sfctemp->clk_sense), > + "error getting sense clock\n"); > + > + sfctemp->clk_bus = devm_clk_get(dev, "bus"); > + if (IS_ERR(sfctemp->clk_bus)) > + return dev_err_probe(dev, PTR_ERR(sfctemp->clk_bus), > + "error getting bus clock\n"); > + > + sfctemp->rst_sense = devm_reset_control_get_exclusive(dev, "sense"); > + if (IS_ERR(sfctemp->rst_sense)) > + return dev_err_probe(dev, PTR_ERR(sfctemp->rst_sense), > + "error getting sense reset\n"); > + > + sfctemp->rst_bus = devm_reset_control_get_exclusive(dev, "bus"); > + if (IS_ERR(sfctemp->rst_bus)) > + return dev_err_probe(dev, PTR_ERR(sfctemp->rst_bus), > + "error getting busreset\n"); > + > + ret = reset_control_assert(sfctemp->rst_sense); > + if (ret) > + return dev_err_probe(dev, ret, "error asserting sense reset\n"); > + > + ret = reset_control_assert(sfctemp->rst_bus); > + if (ret) > + return dev_err_probe(dev, ret, "error asserting bus reset\n"); > + > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + return ret; > + > + ret = devm_request_irq(dev, ret, sfctemp_isr, 0, pdev->name, sfctemp); > + if (ret) > + return dev_err_probe(dev, ret, "error requesting irq\n"); > + > + ret = devm_add_action(dev, sfctemp_disable_action, sfctemp); > + if (ret) > + return ret; > + > + ret = sfctemp_enable(sfctemp); > + if (ret) > + return dev_err_probe(dev, ret, "error enabling temperature sensor: %d\n", ret); > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, pdev->name, sfctemp, > + &sfctemp_chip_info, NULL); > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct of_device_id sfctemp_of_match[] = { > + { .compatible = "starfive,jh7100-temp" }, > + { .compatible = "starfive,jh7110-temp" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, sfctemp_of_match); > + > +static struct platform_driver sfctemp_driver = { > + .probe = sfctemp_probe, > + .driver = { > + .name = "sfctemp", > + .of_match_table = sfctemp_of_match, > + }, > +}; > +module_platform_driver(sfctemp_driver); > + > +MODULE_AUTHOR("Emil Renner Berthing"); > +MODULE_DESCRIPTION("StarFive JH71x0 temperature sensor driver"); > +MODULE_LICENSE("GPL");
On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote: > On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: >> From: Emil Renner Berthing <kernel@esmil.dk> >> >> Register definitions and conversion constants based on sfctemp driver by >> Samin in the StarFive 5.10 kernel. >> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> Co-developed-by: Samin Guo <samin.guo@starfivetech.com> >> Signed-off-by: Samin Guo <samin.guo@starfivetech.com> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com> > > This is obviously version 4 of the previous patch series, > with various enhancements. Please do not "sell" that as > v1 of a new patch series. Submit it as next version, > and provide a change log. Sorry to reply too late. I will resend this series as v4 tomorrow. > >> --- >> Documentation/hwmon/index.rst | 1 + >> Documentation/hwmon/sfctemp.rst | 33 +++ >> MAINTAINERS | 8 + >> drivers/hwmon/Kconfig | 10 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/sfctemp.c | 350 ++++++++++++++++++++++++++++++++ >> 6 files changed, 403 insertions(+) >> create mode 100644 Documentation/hwmon/sfctemp.rst >> create mode 100644 drivers/hwmon/sfctemp.c >> >> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >> index c1d11cf13eef..f7ede608b6e3 100644 >> --- a/Documentation/hwmon/index.rst >> +++ b/Documentation/hwmon/index.rst >> @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers >> sch5627 >> sch5636 >> scpi-hwmon >> + sfctemp >> sht15 >> sht21 >> sht3x >> diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst >> new file mode 100644 >> index 000000000000..9fbd5bb1f356 >> --- /dev/null >> +++ b/Documentation/hwmon/sfctemp.rst >> @@ -0,0 +1,33 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> + >> +Kernel driver sfctemp >> +===================== >> + >> +Supported chips: >> + - StarFive JH7100 >> + - StarFive JH7110 >> + >> +Authors: >> + - Emil Renner Berthing <kernel@esmil.dk> >> + >> +Description >> +----------- >> + >> +This driver adds support for reading the built-in temperature sensor on the >> +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd. >> + >> +``sysfs`` interface >> +------------------- >> + >> +The temperature sensor can be enabled, disabled and queried via the standard >> +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of >> +``X``: >> + >> +================ ==== ============================================= >> +Name Perm Description >> +================ ==== ============================================= >> +temp1_enable RW Enable or disable temperature sensor. >> + Automatically enabled by the driver, >> + but may be disabled to save power. >> +temp1_input RO Temperature reading in milli-degrees Celsius. >> +================ ==== ============================================= >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 85e8f83161d7..ab3cd5827b26 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -18661,6 +18661,14 @@ L: netdev@vger.kernel.org >> S: Supported >> F: drivers/net/ethernet/sfc/ >> >> +SFCTEMP HWMON DRIVER >> +M: Emil Renner Berthing <kernel@esmil.dk> >> +L: linux-hwmon@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml >> +F: Documentation/hwmon/sfctemp.rst >> +F: drivers/hwmon/sfctemp.c >> + >> SFF/SFP/SFP+ MODULE SUPPORT >> M: Russell King <linux@armlinux.org.uk> >> L: netdev@vger.kernel.org >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 7ac3daaf59ce..c6bbfcca3a14 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1910,6 +1910,16 @@ config SENSORS_STTS751 >> This driver can also be built as a module. If so, the module >> will be called stts751. >> >> +config SENSORS_SFCTEMP >> + tristate "Starfive JH71x0 temperature sensor" >> + depends on SOC_STARFIVE || COMPILE_TEST >> + help >> + If you say yes here you get support for temperature sensor >> + on the Starfive JH71x0 SoCs. >> + >> + This driver can also be built as a module. If so, the module >> + will be called sfctemp. >> + >> config SENSORS_SMM665 >> tristate "Summit Microelectronics SMM665" >> depends on I2C >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 11d076cad8a2..5a4a02c5535c 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI) += sbrmi.o >> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o >> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o >> obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o >> +obj-$(CONFIG_SENSORS_SFCTEMP) += sfctemp.o >> obj-$(CONFIG_SENSORS_SL28CPLD) += sl28cpld-hwmon.o >> obj-$(CONFIG_SENSORS_SHT15) += sht15.o >> obj-$(CONFIG_SENSORS_SHT21) += sht21.o >> diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c >> new file mode 100644 >> index 000000000000..e56716ad9587 >> --- /dev/null >> +++ b/drivers/hwmon/sfctemp.c >> @@ -0,0 +1,350 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> >> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >> + */ >> +#include <linux/clk.h> >> +#include <linux/completion.h> >> +#include <linux/delay.h> >> +#include <linux/hwmon.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset.h> >> + >> +/* >> + * TempSensor reset. The RSTN can be de-asserted once the analog core has >> + * powered up. Trst(min 100ns) >> + * 0:reset 1:de-assert >> + */ >> +#define SFCTEMP_RSTN BIT(0) > > Missing include of linux/bits.h Will add it. Thanks. > >> + >> +/* >> + * TempSensor analog core power down. The analog core will be powered up >> + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the >> + * analog core is powered up. >> + * 0:power up 1:power down >> + */ >> +#define SFCTEMP_PD BIT(1) >> + >> +/* >> + * TempSensor start conversion enable. >> + * 0:disable 1:enable >> + */ >> +#define SFCTEMP_RUN BIT(2) >> + >> +/* >> + * TempSensor conversion value output. >> + * Temp(C)=DOUT*Y/4094 - K >> + */ >> +#define SFCTEMP_DOUT_POS 16 >> +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) >> + >> +/* DOUT to Celcius conversion constants */ >> +#define SFCTEMP_Y1000 237500L >> +#define SFCTEMP_Z 4094L >> +#define SFCTEMP_K1000 81100L >> + >> +struct sfctemp { >> + /* serialize access to hardware register and enabled below */ >> + struct mutex lock; >> + struct completion conversion_done; >> + void __iomem *regs; >> + struct clk *clk_sense; >> + struct clk *clk_bus; >> + struct reset_control *rst_sense; >> + struct reset_control *rst_bus; >> + bool enabled; >> +}; >> + >> +static irqreturn_t sfctemp_isr(int irq, void *data) >> +{ >> + struct sfctemp *sfctemp = data; >> + >> + complete(&sfctemp->conversion_done); >> + return IRQ_HANDLED; >> +} >> + >> +static void sfctemp_power_up(struct sfctemp *sfctemp) >> +{ >> + /* make sure we're powered down first */ >> + writel(SFCTEMP_PD, sfctemp->regs); >> + udelay(1); >> + >> + writel(0, sfctemp->regs); >> + /* wait t_pu(50us) + t_rst(100ns) */ >> + usleep_range(60, 200); >> + >> + /* de-assert reset */ >> + writel(SFCTEMP_RSTN, sfctemp->regs); >> + udelay(1); /* wait t_su(500ps) */ >> +} >> + >> +static void sfctemp_power_down(struct sfctemp *sfctemp) >> +{ >> + writel(SFCTEMP_PD, sfctemp->regs); >> +} >> + >> +static void sfctemp_run_single(struct sfctemp *sfctemp) >> +{ >> + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); >> + udelay(1); >> + writel(SFCTEMP_RSTN, sfctemp->regs); > > The datasheet (or, rather, programming manual) does not appear > to be public, so I have to guess here. > > The code suggests that running a single conversion may be a choice, > not a requirement. If it is indeed a choice, the reasoning needs to be > explained since it adds a lot of complexity and dependencies to the > driver (for example, interrupt support is only mandatory or even needed > due to this choice). It also adds a significant delay to temperature > read operations, which may have practical impact on thermal control > software. > > If the chip only supports single temperature readings, that needs to be > explained as well (and why SFCTEMP_RUN has to be reset in that case). The chip supports continuous conversion. When you set SFCTEMP_RUN, the temperature raw data will be generated all the time. However, it will also generate interrupts all the time when the conversion is finished, because of the hardware limitation. So in this driver, we just support the single conversion. Thank you for your feedback. Best regards, Hal
On 2/6/23 09:12, Hal Feng wrote: > On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote: >> On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: >>> From: Emil Renner Berthing <kernel@esmil.dk> >>> >>> Register definitions and conversion constants based on sfctemp driver by >>> Samin in the StarFive 5.10 kernel. >>> >>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >>> Co-developed-by: Samin Guo <samin.guo@starfivetech.com> >>> Signed-off-by: Samin Guo <samin.guo@starfivetech.com> >>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com> >> >> This is obviously version 4 of the previous patch series, >> with various enhancements. Please do not "sell" that as >> v1 of a new patch series. Submit it as next version, >> and provide a change log. > > Sorry to reply too late. I will resend this series as v4 tomorrow. > >> >>> --- >>> Documentation/hwmon/index.rst | 1 + >>> Documentation/hwmon/sfctemp.rst | 33 +++ >>> MAINTAINERS | 8 + >>> drivers/hwmon/Kconfig | 10 + >>> drivers/hwmon/Makefile | 1 + >>> drivers/hwmon/sfctemp.c | 350 ++++++++++++++++++++++++++++++++ >>> 6 files changed, 403 insertions(+) >>> create mode 100644 Documentation/hwmon/sfctemp.rst >>> create mode 100644 drivers/hwmon/sfctemp.c >>> >>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>> index c1d11cf13eef..f7ede608b6e3 100644 >>> --- a/Documentation/hwmon/index.rst >>> +++ b/Documentation/hwmon/index.rst >>> @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers >>> sch5627 >>> sch5636 >>> scpi-hwmon >>> + sfctemp >>> sht15 >>> sht21 >>> sht3x >>> diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst >>> new file mode 100644 >>> index 000000000000..9fbd5bb1f356 >>> --- /dev/null >>> +++ b/Documentation/hwmon/sfctemp.rst >>> @@ -0,0 +1,33 @@ >>> +.. SPDX-License-Identifier: GPL-2.0 >>> + >>> +Kernel driver sfctemp >>> +===================== >>> + >>> +Supported chips: >>> + - StarFive JH7100 >>> + - StarFive JH7110 >>> + >>> +Authors: >>> + - Emil Renner Berthing <kernel@esmil.dk> >>> + >>> +Description >>> +----------- >>> + >>> +This driver adds support for reading the built-in temperature sensor on the >>> +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd. >>> + >>> +``sysfs`` interface >>> +------------------- >>> + >>> +The temperature sensor can be enabled, disabled and queried via the standard >>> +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of >>> +``X``: >>> + >>> +================ ==== ============================================= >>> +Name Perm Description >>> +================ ==== ============================================= >>> +temp1_enable RW Enable or disable temperature sensor. >>> + Automatically enabled by the driver, >>> + but may be disabled to save power. >>> +temp1_input RO Temperature reading in milli-degrees Celsius. >>> +================ ==== ============================================= >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 85e8f83161d7..ab3cd5827b26 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -18661,6 +18661,14 @@ L: netdev@vger.kernel.org >>> S: Supported >>> F: drivers/net/ethernet/sfc/ >>> >>> +SFCTEMP HWMON DRIVER >>> +M: Emil Renner Berthing <kernel@esmil.dk> >>> +L: linux-hwmon@vger.kernel.org >>> +S: Maintained >>> +F: Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml >>> +F: Documentation/hwmon/sfctemp.rst >>> +F: drivers/hwmon/sfctemp.c >>> + >>> SFF/SFP/SFP+ MODULE SUPPORT >>> M: Russell King <linux@armlinux.org.uk> >>> L: netdev@vger.kernel.org >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>> index 7ac3daaf59ce..c6bbfcca3a14 100644 >>> --- a/drivers/hwmon/Kconfig >>> +++ b/drivers/hwmon/Kconfig >>> @@ -1910,6 +1910,16 @@ config SENSORS_STTS751 >>> This driver can also be built as a module. If so, the module >>> will be called stts751. >>> >>> +config SENSORS_SFCTEMP >>> + tristate "Starfive JH71x0 temperature sensor" >>> + depends on SOC_STARFIVE || COMPILE_TEST >>> + help >>> + If you say yes here you get support for temperature sensor >>> + on the Starfive JH71x0 SoCs. >>> + >>> + This driver can also be built as a module. If so, the module >>> + will be called sfctemp. >>> + >>> config SENSORS_SMM665 >>> tristate "Summit Microelectronics SMM665" >>> depends on I2C >>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >>> index 11d076cad8a2..5a4a02c5535c 100644 >>> --- a/drivers/hwmon/Makefile >>> +++ b/drivers/hwmon/Makefile >>> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI) += sbrmi.o >>> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o >>> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o >>> obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o >>> +obj-$(CONFIG_SENSORS_SFCTEMP) += sfctemp.o >>> obj-$(CONFIG_SENSORS_SL28CPLD) += sl28cpld-hwmon.o >>> obj-$(CONFIG_SENSORS_SHT15) += sht15.o >>> obj-$(CONFIG_SENSORS_SHT21) += sht21.o >>> diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c >>> new file mode 100644 >>> index 000000000000..e56716ad9587 >>> --- /dev/null >>> +++ b/drivers/hwmon/sfctemp.c >>> @@ -0,0 +1,350 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> >>> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >>> + */ >>> +#include <linux/clk.h> >>> +#include <linux/completion.h> >>> +#include <linux/delay.h> >>> +#include <linux/hwmon.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/io.h> >>> +#include <linux/module.h> >>> +#include <linux/mutex.h> >>> +#include <linux/of.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/reset.h> >>> + >>> +/* >>> + * TempSensor reset. The RSTN can be de-asserted once the analog core has >>> + * powered up. Trst(min 100ns) >>> + * 0:reset 1:de-assert >>> + */ >>> +#define SFCTEMP_RSTN BIT(0) >> >> Missing include of linux/bits.h > > Will add it. Thanks. > >> >>> + >>> +/* >>> + * TempSensor analog core power down. The analog core will be powered up >>> + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the >>> + * analog core is powered up. >>> + * 0:power up 1:power down >>> + */ >>> +#define SFCTEMP_PD BIT(1) >>> + >>> +/* >>> + * TempSensor start conversion enable. >>> + * 0:disable 1:enable >>> + */ >>> +#define SFCTEMP_RUN BIT(2) >>> + >>> +/* >>> + * TempSensor conversion value output. >>> + * Temp(C)=DOUT*Y/4094 - K >>> + */ >>> +#define SFCTEMP_DOUT_POS 16 >>> +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) >>> + >>> +/* DOUT to Celcius conversion constants */ >>> +#define SFCTEMP_Y1000 237500L >>> +#define SFCTEMP_Z 4094L >>> +#define SFCTEMP_K1000 81100L >>> + >>> +struct sfctemp { >>> + /* serialize access to hardware register and enabled below */ >>> + struct mutex lock; >>> + struct completion conversion_done; >>> + void __iomem *regs; >>> + struct clk *clk_sense; >>> + struct clk *clk_bus; >>> + struct reset_control *rst_sense; >>> + struct reset_control *rst_bus; >>> + bool enabled; >>> +}; >>> + >>> +static irqreturn_t sfctemp_isr(int irq, void *data) >>> +{ >>> + struct sfctemp *sfctemp = data; >>> + >>> + complete(&sfctemp->conversion_done); >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static void sfctemp_power_up(struct sfctemp *sfctemp) >>> +{ >>> + /* make sure we're powered down first */ >>> + writel(SFCTEMP_PD, sfctemp->regs); >>> + udelay(1); >>> + >>> + writel(0, sfctemp->regs); >>> + /* wait t_pu(50us) + t_rst(100ns) */ >>> + usleep_range(60, 200); >>> + >>> + /* de-assert reset */ >>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>> + udelay(1); /* wait t_su(500ps) */ >>> +} >>> + >>> +static void sfctemp_power_down(struct sfctemp *sfctemp) >>> +{ >>> + writel(SFCTEMP_PD, sfctemp->regs); >>> +} >>> + >>> +static void sfctemp_run_single(struct sfctemp *sfctemp) >>> +{ >>> + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); >>> + udelay(1); >>> + writel(SFCTEMP_RSTN, sfctemp->regs); >> >> The datasheet (or, rather, programming manual) does not appear >> to be public, so I have to guess here. >> >> The code suggests that running a single conversion may be a choice, >> not a requirement. If it is indeed a choice, the reasoning needs to be >> explained since it adds a lot of complexity and dependencies to the >> driver (for example, interrupt support is only mandatory or even needed >> due to this choice). It also adds a significant delay to temperature >> read operations, which may have practical impact on thermal control >> software. >> >> If the chip only supports single temperature readings, that needs to be >> explained as well (and why SFCTEMP_RUN has to be reset in that case). > > The chip supports continuous conversion. When you set SFCTEMP_RUN, the > temperature raw data will be generated all the time. However, it will > also generate interrupts all the time when the conversion is finished, > because of the hardware limitation. So in this driver, we just support > the single conversion. > Sorry, I don't follow the logic. The interrupt is, for all practical purposes, useless because there are no limits and exceeding any such limits is therefore not supported. The only reason to have and enable to interrupt is because continuous mode is disabled. The code could be simplified a lot if interrupt support would be dropped and continuous mode would be enabled. Guenter > Thank you for your feedback. > > Best regards, > Hal
On Mon, 6 Feb 2023 11:21:38 -0800, Guenter Roeck wrote: > On 2/6/23 09:12, Hal Feng wrote: >> On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote: >>> On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: [...] >>>> diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c >>>> new file mode 100644 >>>> index 000000000000..e56716ad9587 >>>> --- /dev/null >>>> +++ b/drivers/hwmon/sfctemp.c >>>> @@ -0,0 +1,350 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> >>>> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >>>> + */ >>>> +#include <linux/clk.h> >>>> +#include <linux/completion.h> >>>> +#include <linux/delay.h> >>>> +#include <linux/hwmon.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/io.h> >>>> +#include <linux/module.h> >>>> +#include <linux/mutex.h> >>>> +#include <linux/of.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/reset.h> >>>> + >>>> +/* >>>> + * TempSensor reset. The RSTN can be de-asserted once the analog core has >>>> + * powered up. Trst(min 100ns) >>>> + * 0:reset 1:de-assert >>>> + */ >>>> +#define SFCTEMP_RSTN BIT(0) >>> >>> Missing include of linux/bits.h >> >> Will add it. Thanks. >> >>> >>>> + >>>> +/* >>>> + * TempSensor analog core power down. The analog core will be powered up >>>> + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the >>>> + * analog core is powered up. >>>> + * 0:power up 1:power down >>>> + */ >>>> +#define SFCTEMP_PD BIT(1) >>>> + >>>> +/* >>>> + * TempSensor start conversion enable. >>>> + * 0:disable 1:enable >>>> + */ >>>> +#define SFCTEMP_RUN BIT(2) >>>> + >>>> +/* >>>> + * TempSensor conversion value output. >>>> + * Temp(C)=DOUT*Y/4094 - K >>>> + */ >>>> +#define SFCTEMP_DOUT_POS 16 >>>> +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) >>>> + >>>> +/* DOUT to Celcius conversion constants */ >>>> +#define SFCTEMP_Y1000 237500L >>>> +#define SFCTEMP_Z 4094L >>>> +#define SFCTEMP_K1000 81100L >>>> + >>>> +struct sfctemp { >>>> + /* serialize access to hardware register and enabled below */ >>>> + struct mutex lock; >>>> + struct completion conversion_done; >>>> + void __iomem *regs; >>>> + struct clk *clk_sense; >>>> + struct clk *clk_bus; >>>> + struct reset_control *rst_sense; >>>> + struct reset_control *rst_bus; >>>> + bool enabled; >>>> +}; >>>> + >>>> +static irqreturn_t sfctemp_isr(int irq, void *data) >>>> +{ >>>> + struct sfctemp *sfctemp = data; >>>> + >>>> + complete(&sfctemp->conversion_done); >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static void sfctemp_power_up(struct sfctemp *sfctemp) >>>> +{ >>>> + /* make sure we're powered down first */ >>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>> + udelay(1); >>>> + >>>> + writel(0, sfctemp->regs); >>>> + /* wait t_pu(50us) + t_rst(100ns) */ >>>> + usleep_range(60, 200); >>>> + >>>> + /* de-assert reset */ >>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>>> + udelay(1); /* wait t_su(500ps) */ >>>> +} >>>> + >>>> +static void sfctemp_power_down(struct sfctemp *sfctemp) >>>> +{ >>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>> +} >>>> + >>>> +static void sfctemp_run_single(struct sfctemp *sfctemp) >>>> +{ >>>> + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); >>>> + udelay(1); >>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>> >>> The datasheet (or, rather, programming manual) does not appear >>> to be public, so I have to guess here. >>> >>> The code suggests that running a single conversion may be a choice, >>> not a requirement. If it is indeed a choice, the reasoning needs to be >>> explained since it adds a lot of complexity and dependencies to the >>> driver (for example, interrupt support is only mandatory or even needed >>> due to this choice). It also adds a significant delay to temperature >>> read operations, which may have practical impact on thermal control >>> software. >>> >>> If the chip only supports single temperature readings, that needs to be >>> explained as well (and why SFCTEMP_RUN has to be reset in that case). >> >> The chip supports continuous conversion. When you set SFCTEMP_RUN, the >> temperature raw data will be generated all the time. However, it will >> also generate interrupts all the time when the conversion is finished, >> because of the hardware limitation. So in this driver, we just support >> the single conversion. >> > > Sorry, I don't follow the logic. The interrupt is, for all practical > purposes, useless because there are no limits and exceeding any such > limits is therefore not supported. The only reason to have and enable > to interrupt is because continuous mode is disabled. > > The code could be simplified a lot if interrupt support would be > dropped and continuous mode would be enabled. If we enable continuous mode, which means SFCTEMP_RUN remains asserted, the conversion finished interrupt will be raised after each sample time (8.192 ms). Within a few minutes, a lot of interrupts are raised, as showed below. # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 1: 0 0 0 0 SiFive PLIC 1 Edge ccache_ecc 2: 1 0 0 0 SiFive PLIC 3 Edge ccache_ecc 3: 1 0 0 0 SiFive PLIC 4 Edge ccache_ecc 4: 0 0 0 0 SiFive PLIC 2 Edge ccache_ecc 5: 1116 1670 411 1466 RISC-V INTC 5 Edge riscv-timer 6: 32093 0 0 0 SiFive PLIC 81 Edge 120e0000.temperature-sensor 10: 1233 0 0 0 SiFive PLIC 32 Edge ttyS0 IPI0: 117 62 123 117 Rescheduling interrupts IPI1: 278 353 105 273 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 IRQ work interrupts IPI5: 0 0 0 0 Timer broadcast interrupts If we enable continuous mode and drop the interrupt support in the driver, the kernel will not know the interrupts but a lot of interrupts are still raised in hardware. Can we do such like that? Without the interrupt support, the temperature we read may be the value generated in the last cycle. I think the temperature has its value only when we read it, so we start conversion only when we read the temperature. Further more, it will consume more power if we enable continuous mode. Best regards, Hal
On 2/8/23 04:40, Hal Feng wrote: > On Mon, 6 Feb 2023 11:21:38 -0800, Guenter Roeck wrote: >> On 2/6/23 09:12, Hal Feng wrote: >>> On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote: >>>> On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: > [...] >>>>> diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c >>>>> new file mode 100644 >>>>> index 000000000000..e56716ad9587 >>>>> --- /dev/null >>>>> +++ b/drivers/hwmon/sfctemp.c >>>>> @@ -0,0 +1,350 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> >>>>> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >>>>> + */ >>>>> +#include <linux/clk.h> >>>>> +#include <linux/completion.h> >>>>> +#include <linux/delay.h> >>>>> +#include <linux/hwmon.h> >>>>> +#include <linux/interrupt.h> >>>>> +#include <linux/io.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/mutex.h> >>>>> +#include <linux/of.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/reset.h> >>>>> + >>>>> +/* >>>>> + * TempSensor reset. The RSTN can be de-asserted once the analog core has >>>>> + * powered up. Trst(min 100ns) >>>>> + * 0:reset 1:de-assert >>>>> + */ >>>>> +#define SFCTEMP_RSTN BIT(0) >>>> >>>> Missing include of linux/bits.h >>> >>> Will add it. Thanks. >>> >>>> >>>>> + >>>>> +/* >>>>> + * TempSensor analog core power down. The analog core will be powered up >>>>> + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the >>>>> + * analog core is powered up. >>>>> + * 0:power up 1:power down >>>>> + */ >>>>> +#define SFCTEMP_PD BIT(1) >>>>> + >>>>> +/* >>>>> + * TempSensor start conversion enable. >>>>> + * 0:disable 1:enable >>>>> + */ >>>>> +#define SFCTEMP_RUN BIT(2) >>>>> + >>>>> +/* >>>>> + * TempSensor conversion value output. >>>>> + * Temp(C)=DOUT*Y/4094 - K >>>>> + */ >>>>> +#define SFCTEMP_DOUT_POS 16 >>>>> +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) >>>>> + >>>>> +/* DOUT to Celcius conversion constants */ >>>>> +#define SFCTEMP_Y1000 237500L >>>>> +#define SFCTEMP_Z 4094L >>>>> +#define SFCTEMP_K1000 81100L >>>>> + >>>>> +struct sfctemp { >>>>> + /* serialize access to hardware register and enabled below */ >>>>> + struct mutex lock; >>>>> + struct completion conversion_done; >>>>> + void __iomem *regs; >>>>> + struct clk *clk_sense; >>>>> + struct clk *clk_bus; >>>>> + struct reset_control *rst_sense; >>>>> + struct reset_control *rst_bus; >>>>> + bool enabled; >>>>> +}; >>>>> + >>>>> +static irqreturn_t sfctemp_isr(int irq, void *data) >>>>> +{ >>>>> + struct sfctemp *sfctemp = data; >>>>> + >>>>> + complete(&sfctemp->conversion_done); >>>>> + return IRQ_HANDLED; >>>>> +} >>>>> + >>>>> +static void sfctemp_power_up(struct sfctemp *sfctemp) >>>>> +{ >>>>> + /* make sure we're powered down first */ >>>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>>> + udelay(1); >>>>> + >>>>> + writel(0, sfctemp->regs); >>>>> + /* wait t_pu(50us) + t_rst(100ns) */ >>>>> + usleep_range(60, 200); >>>>> + >>>>> + /* de-assert reset */ >>>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>>>> + udelay(1); /* wait t_su(500ps) */ >>>>> +} >>>>> + >>>>> +static void sfctemp_power_down(struct sfctemp *sfctemp) >>>>> +{ >>>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>>> +} >>>>> + >>>>> +static void sfctemp_run_single(struct sfctemp *sfctemp) >>>>> +{ >>>>> + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); >>>>> + udelay(1); >>>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>>> >>>> The datasheet (or, rather, programming manual) does not appear >>>> to be public, so I have to guess here. >>>> >>>> The code suggests that running a single conversion may be a choice, >>>> not a requirement. If it is indeed a choice, the reasoning needs to be >>>> explained since it adds a lot of complexity and dependencies to the >>>> driver (for example, interrupt support is only mandatory or even needed >>>> due to this choice). It also adds a significant delay to temperature >>>> read operations, which may have practical impact on thermal control >>>> software. >>>> >>>> If the chip only supports single temperature readings, that needs to be >>>> explained as well (and why SFCTEMP_RUN has to be reset in that case). >>> >>> The chip supports continuous conversion. When you set SFCTEMP_RUN, the >>> temperature raw data will be generated all the time. However, it will >>> also generate interrupts all the time when the conversion is finished, >>> because of the hardware limitation. So in this driver, we just support >>> the single conversion. >>> >> >> Sorry, I don't follow the logic. The interrupt is, for all practical >> purposes, useless because there are no limits and exceeding any such >> limits is therefore not supported. The only reason to have and enable >> to interrupt is because continuous mode is disabled. >> >> The code could be simplified a lot if interrupt support would be >> dropped and continuous mode would be enabled. > > If we enable continuous mode, which means SFCTEMP_RUN remains asserted, > the conversion finished interrupt will be raised after each sample > time (8.192 ms). Within a few minutes, a lot of interrupts are raised, > as showed below. > > # cat /proc/interrupts > CPU0 CPU1 CPU2 CPU3 > 1: 0 0 0 0 SiFive PLIC 1 Edge ccache_ecc > 2: 1 0 0 0 SiFive PLIC 3 Edge ccache_ecc > 3: 1 0 0 0 SiFive PLIC 4 Edge ccache_ecc > 4: 0 0 0 0 SiFive PLIC 2 Edge ccache_ecc > 5: 1116 1670 411 1466 RISC-V INTC 5 Edge riscv-timer > 6: 32093 0 0 0 SiFive PLIC 81 Edge 120e0000.temperature-sensor > 10: 1233 0 0 0 SiFive PLIC 32 Edge ttyS0 > IPI0: 117 62 123 117 Rescheduling interrupts > IPI1: 278 353 105 273 Function call interrupts > IPI2: 0 0 0 0 CPU stop interrupts > IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts > IPI4: 0 0 0 0 IRQ work interrupts > IPI5: 0 0 0 0 Timer broadcast interrupts > > If we enable continuous mode and drop the interrupt support in the > driver, the kernel will not know the interrupts but a lot of interrupts > are still raised in hardware. Can we do such like that? Why not ? It just stays raised. That happens a lot. > Without the interrupt support, the temperature we read may be the value > generated in the last cycle. That would be highly unusual and should be documented. > > I think the temperature has its value only when we read it, so we start "may be" ? "I think" ? That means you don't know ? Maybe test it, or ask the chip designers. > conversion only when we read the temperature. Further more, it will > consume more power if we enable continuous mode. > Usually that is not a concern, much less so than delaying each reader. Ultimately, sure, you can do whatever you want. I'll still accept the driver. I do expect you to explain your reasons (all of them) in the driver, though. If you don't _know_ if the temperature is updated in continuous mode, please state exactly that in the comments. Also explain how much power is saved by not running in continuous mode. I don't want anyone to come back later on and change the code because they don't know the reasons why it doesn't use continuous mode. Thanks, Guenter > Best regards, > Hal
On Wed, 8 Feb 2023 07:16:52 -0800, Guenter Roeck wrote: > On 2/8/23 04:40, Hal Feng wrote: >> On Mon, 6 Feb 2023 11:21:38 -0800, Guenter Roeck wrote: >>> On 2/6/23 09:12, Hal Feng wrote: >>>> On Tue, 3 Jan 2023 14:10:17 -0800, Guenter Roeck wrote: >>>>> On Tue, Jan 03, 2023 at 09:31:43AM +0800, Hal Feng wrote: >> [...] >>>>>> diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c >>>>>> new file mode 100644 >>>>>> index 000000000000..e56716ad9587 >>>>>> --- /dev/null >>>>>> +++ b/drivers/hwmon/sfctemp.c >>>>>> @@ -0,0 +1,350 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>>> +/* >>>>>> + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> >>>>>> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> >>>>>> + */ >>>>>> +#include <linux/clk.h> >>>>>> +#include <linux/completion.h> >>>>>> +#include <linux/delay.h> >>>>>> +#include <linux/hwmon.h> >>>>>> +#include <linux/interrupt.h> >>>>>> +#include <linux/io.h> >>>>>> +#include <linux/module.h> >>>>>> +#include <linux/mutex.h> >>>>>> +#include <linux/of.h> >>>>>> +#include <linux/platform_device.h> >>>>>> +#include <linux/reset.h> >>>>>> + >>>>>> +/* >>>>>> + * TempSensor reset. The RSTN can be de-asserted once the analog core has >>>>>> + * powered up. Trst(min 100ns) >>>>>> + * 0:reset 1:de-assert >>>>>> + */ >>>>>> +#define SFCTEMP_RSTN BIT(0) >>>>> >>>>> Missing include of linux/bits.h >>>> >>>> Will add it. Thanks. >>>> >>>>> >>>>>> + >>>>>> +/* >>>>>> + * TempSensor analog core power down. The analog core will be powered up >>>>>> + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the >>>>>> + * analog core is powered up. >>>>>> + * 0:power up 1:power down >>>>>> + */ >>>>>> +#define SFCTEMP_PD BIT(1) >>>>>> + >>>>>> +/* >>>>>> + * TempSensor start conversion enable. >>>>>> + * 0:disable 1:enable >>>>>> + */ >>>>>> +#define SFCTEMP_RUN BIT(2) >>>>>> + >>>>>> +/* >>>>>> + * TempSensor conversion value output. >>>>>> + * Temp(C)=DOUT*Y/4094 - K >>>>>> + */ >>>>>> +#define SFCTEMP_DOUT_POS 16 >>>>>> +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) >>>>>> + >>>>>> +/* DOUT to Celcius conversion constants */ >>>>>> +#define SFCTEMP_Y1000 237500L >>>>>> +#define SFCTEMP_Z 4094L >>>>>> +#define SFCTEMP_K1000 81100L >>>>>> + >>>>>> +struct sfctemp { >>>>>> + /* serialize access to hardware register and enabled below */ >>>>>> + struct mutex lock; >>>>>> + struct completion conversion_done; >>>>>> + void __iomem *regs; >>>>>> + struct clk *clk_sense; >>>>>> + struct clk *clk_bus; >>>>>> + struct reset_control *rst_sense; >>>>>> + struct reset_control *rst_bus; >>>>>> + bool enabled; >>>>>> +}; >>>>>> + >>>>>> +static irqreturn_t sfctemp_isr(int irq, void *data) >>>>>> +{ >>>>>> + struct sfctemp *sfctemp = data; >>>>>> + >>>>>> + complete(&sfctemp->conversion_done); >>>>>> + return IRQ_HANDLED; >>>>>> +} >>>>>> + >>>>>> +static void sfctemp_power_up(struct sfctemp *sfctemp) >>>>>> +{ >>>>>> + /* make sure we're powered down first */ >>>>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>>>> + udelay(1); >>>>>> + >>>>>> + writel(0, sfctemp->regs); >>>>>> + /* wait t_pu(50us) + t_rst(100ns) */ >>>>>> + usleep_range(60, 200); >>>>>> + >>>>>> + /* de-assert reset */ >>>>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>>>>> + udelay(1); /* wait t_su(500ps) */ >>>>>> +} >>>>>> + >>>>>> +static void sfctemp_power_down(struct sfctemp *sfctemp) >>>>>> +{ >>>>>> + writel(SFCTEMP_PD, sfctemp->regs); >>>>>> +} >>>>>> + >>>>>> +static void sfctemp_run_single(struct sfctemp *sfctemp) >>>>>> +{ >>>>>> + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); >>>>>> + udelay(1); >>>>>> + writel(SFCTEMP_RSTN, sfctemp->regs); >>>>> >>>>> The datasheet (or, rather, programming manual) does not appear >>>>> to be public, so I have to guess here. >>>>> >>>>> The code suggests that running a single conversion may be a choice, >>>>> not a requirement. If it is indeed a choice, the reasoning needs to be >>>>> explained since it adds a lot of complexity and dependencies to the >>>>> driver (for example, interrupt support is only mandatory or even needed >>>>> due to this choice). It also adds a significant delay to temperature >>>>> read operations, which may have practical impact on thermal control >>>>> software. >>>>> >>>>> If the chip only supports single temperature readings, that needs to be >>>>> explained as well (and why SFCTEMP_RUN has to be reset in that case). >>>> >>>> The chip supports continuous conversion. When you set SFCTEMP_RUN, the >>>> temperature raw data will be generated all the time. However, it will >>>> also generate interrupts all the time when the conversion is finished, >>>> because of the hardware limitation. So in this driver, we just support >>>> the single conversion. >>>> >>> >>> Sorry, I don't follow the logic. The interrupt is, for all practical >>> purposes, useless because there are no limits and exceeding any such >>> limits is therefore not supported. The only reason to have and enable >>> to interrupt is because continuous mode is disabled. >>> >>> The code could be simplified a lot if interrupt support would be >>> dropped and continuous mode would be enabled. >> >> If we enable continuous mode, which means SFCTEMP_RUN remains asserted, >> the conversion finished interrupt will be raised after each sample >> time (8.192 ms). Within a few minutes, a lot of interrupts are raised, >> as showed below. >> >> # cat /proc/interrupts >> CPU0 CPU1 CPU2 CPU3 >> 1: 0 0 0 0 SiFive PLIC 1 Edge ccache_ecc >> 2: 1 0 0 0 SiFive PLIC 3 Edge ccache_ecc >> 3: 1 0 0 0 SiFive PLIC 4 Edge ccache_ecc >> 4: 0 0 0 0 SiFive PLIC 2 Edge ccache_ecc >> 5: 1116 1670 411 1466 RISC-V INTC 5 Edge riscv-timer >> 6: 32093 0 0 0 SiFive PLIC 81 Edge 120e0000.temperature-sensor >> 10: 1233 0 0 0 SiFive PLIC 32 Edge ttyS0 >> IPI0: 117 62 123 117 Rescheduling interrupts >> IPI1: 278 353 105 273 Function call interrupts >> IPI2: 0 0 0 0 CPU stop interrupts >> IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts >> IPI4: 0 0 0 0 IRQ work interrupts >> IPI5: 0 0 0 0 Timer broadcast interrupts >> >> If we enable continuous mode and drop the interrupt support in the >> driver, the kernel will not know the interrupts but a lot of interrupts >> are still raised in hardware. Can we do such like that? > > Why not ? It just stays raised. That happens a lot. OK, I see. Thanks. > >> Without the interrupt support, the temperature we read may be the value >> generated in the last cycle. > > That would be highly unusual and should be documented. Because without the interrupt, there is no flag pointing out when the conversion is finished, and the temperature we read is the data generated before. With further consideration, the error is no more than one sample time (8.192 ms) in continuous mode while the error is no less than one sample time in single mode. In this respect, continuous mode is better than single mode. > > >> >> I think the temperature has its value only when we read it, so we start > > "may be" ? "I think" ? That means you don't know ? Maybe test it, or ask > the chip designers. Sorry for my inexact statement. I means the temperature data is useful to us only when we read it. If we don't read it, the temperature data will just be discarded every sample time. > >> conversion only when we read the temperature. Further more, it will >> consume more power if we enable continuous mode. >> > > Usually that is not a concern, much less so than delaying each reader. After some test, it's found that there is almost no difference between these two modes in power consumption. > > Ultimately, sure, you can do whatever you want. I'll still accept the driver. > I do expect you to explain your reasons (all of them) in the driver, though. > > If you don't _know_ if the temperature is updated in continuous mode, > please state exactly that in the comments. Also explain how much power > is saved by not running in continuous mode. I don't want anyone to come > back later on and change the code because they don't know the reasons > why it doesn't use continuous mode. It's OK to run the continuous mode instead and remove the "- interrupts" entry in "required:" of DT bindings. I will modify in the next version. Thank you for your suggestions. Best regards, Hal
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index c1d11cf13eef..f7ede608b6e3 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -179,6 +179,7 @@ Hardware Monitoring Kernel Drivers sch5627 sch5636 scpi-hwmon + sfctemp sht15 sht21 sht3x diff --git a/Documentation/hwmon/sfctemp.rst b/Documentation/hwmon/sfctemp.rst new file mode 100644 index 000000000000..9fbd5bb1f356 --- /dev/null +++ b/Documentation/hwmon/sfctemp.rst @@ -0,0 +1,33 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Kernel driver sfctemp +===================== + +Supported chips: + - StarFive JH7100 + - StarFive JH7110 + +Authors: + - Emil Renner Berthing <kernel@esmil.dk> + +Description +----------- + +This driver adds support for reading the built-in temperature sensor on the +JH7100 and JH7110 RISC-V SoCs by StarFive Technology Co. Ltd. + +``sysfs`` interface +------------------- + +The temperature sensor can be enabled, disabled and queried via the standard +hwmon interface in sysfs under ``/sys/class/hwmon/hwmonX`` for some value of +``X``: + +================ ==== ============================================= +Name Perm Description +================ ==== ============================================= +temp1_enable RW Enable or disable temperature sensor. + Automatically enabled by the driver, + but may be disabled to save power. +temp1_input RO Temperature reading in milli-degrees Celsius. +================ ==== ============================================= diff --git a/MAINTAINERS b/MAINTAINERS index 85e8f83161d7..ab3cd5827b26 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18661,6 +18661,14 @@ L: netdev@vger.kernel.org S: Supported F: drivers/net/ethernet/sfc/ +SFCTEMP HWMON DRIVER +M: Emil Renner Berthing <kernel@esmil.dk> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/hwmon/starfive,jh71x0-temp.yaml +F: Documentation/hwmon/sfctemp.rst +F: drivers/hwmon/sfctemp.c + SFF/SFP/SFP+ MODULE SUPPORT M: Russell King <linux@armlinux.org.uk> L: netdev@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 7ac3daaf59ce..c6bbfcca3a14 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1910,6 +1910,16 @@ config SENSORS_STTS751 This driver can also be built as a module. If so, the module will be called stts751. +config SENSORS_SFCTEMP + tristate "Starfive JH71x0 temperature sensor" + depends on SOC_STARFIVE || COMPILE_TEST + help + If you say yes here you get support for temperature sensor + on the Starfive JH71x0 SoCs. + + This driver can also be built as a module. If so, the module + will be called sfctemp. + config SENSORS_SMM665 tristate "Summit Microelectronics SMM665" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 11d076cad8a2..5a4a02c5535c 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_SBRMI) += sbrmi.o obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o obj-$(CONFIG_SENSORS_SCH5636) += sch5636.o +obj-$(CONFIG_SENSORS_SFCTEMP) += sfctemp.o obj-$(CONFIG_SENSORS_SL28CPLD) += sl28cpld-hwmon.o obj-$(CONFIG_SENSORS_SHT15) += sht15.o obj-$(CONFIG_SENSORS_SHT21) += sht21.o diff --git a/drivers/hwmon/sfctemp.c b/drivers/hwmon/sfctemp.c new file mode 100644 index 000000000000..e56716ad9587 --- /dev/null +++ b/drivers/hwmon/sfctemp.c @@ -0,0 +1,350 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> + * Copyright (C) 2021 Samin Guo <samin.guo@starfivetech.com> + */ +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/delay.h> +#include <linux/hwmon.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/reset.h> + +/* + * TempSensor reset. The RSTN can be de-asserted once the analog core has + * powered up. Trst(min 100ns) + * 0:reset 1:de-assert + */ +#define SFCTEMP_RSTN BIT(0) + +/* + * TempSensor analog core power down. The analog core will be powered up + * Tpu(min 50us) after PD is de-asserted. RSTN should be held low until the + * analog core is powered up. + * 0:power up 1:power down + */ +#define SFCTEMP_PD BIT(1) + +/* + * TempSensor start conversion enable. + * 0:disable 1:enable + */ +#define SFCTEMP_RUN BIT(2) + +/* + * TempSensor conversion value output. + * Temp(C)=DOUT*Y/4094 - K + */ +#define SFCTEMP_DOUT_POS 16 +#define SFCTEMP_DOUT_MSK GENMASK(27, 16) + +/* DOUT to Celcius conversion constants */ +#define SFCTEMP_Y1000 237500L +#define SFCTEMP_Z 4094L +#define SFCTEMP_K1000 81100L + +struct sfctemp { + /* serialize access to hardware register and enabled below */ + struct mutex lock; + struct completion conversion_done; + void __iomem *regs; + struct clk *clk_sense; + struct clk *clk_bus; + struct reset_control *rst_sense; + struct reset_control *rst_bus; + bool enabled; +}; + +static irqreturn_t sfctemp_isr(int irq, void *data) +{ + struct sfctemp *sfctemp = data; + + complete(&sfctemp->conversion_done); + return IRQ_HANDLED; +} + +static void sfctemp_power_up(struct sfctemp *sfctemp) +{ + /* make sure we're powered down first */ + writel(SFCTEMP_PD, sfctemp->regs); + udelay(1); + + writel(0, sfctemp->regs); + /* wait t_pu(50us) + t_rst(100ns) */ + usleep_range(60, 200); + + /* de-assert reset */ + writel(SFCTEMP_RSTN, sfctemp->regs); + udelay(1); /* wait t_su(500ps) */ +} + +static void sfctemp_power_down(struct sfctemp *sfctemp) +{ + writel(SFCTEMP_PD, sfctemp->regs); +} + +static void sfctemp_run_single(struct sfctemp *sfctemp) +{ + writel(SFCTEMP_RSTN | SFCTEMP_RUN, sfctemp->regs); + udelay(1); + writel(SFCTEMP_RSTN, sfctemp->regs); +} + +static int sfctemp_enable(struct sfctemp *sfctemp) +{ + int ret = 0; + + mutex_lock(&sfctemp->lock); + if (sfctemp->enabled) + goto done; + + ret = clk_prepare_enable(sfctemp->clk_bus); + if (ret) + goto err; + ret = reset_control_deassert(sfctemp->rst_bus); + if (ret) + goto err_disable_bus; + + ret = clk_prepare_enable(sfctemp->clk_sense); + if (ret) + goto err_assert_bus; + ret = reset_control_deassert(sfctemp->rst_sense); + if (ret) + goto err_disable_sense; + + sfctemp_power_up(sfctemp); + sfctemp->enabled = true; +done: + mutex_unlock(&sfctemp->lock); + return ret; + +err_disable_sense: + clk_disable_unprepare(sfctemp->clk_sense); +err_assert_bus: + reset_control_assert(sfctemp->rst_bus); +err_disable_bus: + clk_disable_unprepare(sfctemp->clk_bus); +err: + mutex_unlock(&sfctemp->lock); + return ret; +} + +static int sfctemp_disable(struct sfctemp *sfctemp) +{ + mutex_lock(&sfctemp->lock); + if (!sfctemp->enabled) + goto done; + + sfctemp_power_down(sfctemp); + reset_control_assert(sfctemp->rst_sense); + clk_disable_unprepare(sfctemp->clk_sense); + reset_control_assert(sfctemp->rst_bus); + clk_disable_unprepare(sfctemp->clk_bus); + sfctemp->enabled = false; +done: + mutex_unlock(&sfctemp->lock); + return 0; +} + +static void sfctemp_disable_action(void *data) +{ + sfctemp_disable(data); +} + +static int sfctemp_convert(struct sfctemp *sfctemp, long *val) +{ + int ret; + + mutex_lock(&sfctemp->lock); + if (!sfctemp->enabled) { + ret = -ENODATA; + goto out; + } + + sfctemp_run_single(sfctemp); + + ret = wait_for_completion_interruptible_timeout(&sfctemp->conversion_done, + msecs_to_jiffies(10)); + if (ret <= 0) { + if (ret == 0) + ret = -ETIMEDOUT; + goto out; + } + + /* calculate temperature in milli Celcius */ + *val = (long)((readl(sfctemp->regs) & SFCTEMP_DOUT_MSK) >> SFCTEMP_DOUT_POS) + * SFCTEMP_Y1000 / SFCTEMP_Z - SFCTEMP_K1000; + + ret = 0; +out: + mutex_unlock(&sfctemp->lock); + return ret; +} + +static umode_t sfctemp_is_visible(const void *data, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_temp: + switch (attr) { + case hwmon_temp_enable: + return 0644; + case hwmon_temp_input: + return 0444; + } + return 0; + default: + return 0; + } +} + +static int sfctemp_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct sfctemp *sfctemp = dev_get_drvdata(dev); + + switch (type) { + case hwmon_temp: + switch (attr) { + case hwmon_temp_enable: + *val = sfctemp->enabled; + return 0; + case hwmon_temp_input: + return sfctemp_convert(sfctemp, val); + } + return -EINVAL; + default: + return -EINVAL; + } +} + +static int sfctemp_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + struct sfctemp *sfctemp = dev_get_drvdata(dev); + + switch (type) { + case hwmon_temp: + switch (attr) { + case hwmon_temp_enable: + if (val == 0) + return sfctemp_disable(sfctemp); + if (val == 1) + return sfctemp_enable(sfctemp); + break; + } + return -EINVAL; + default: + return -EINVAL; + } +} + +static const struct hwmon_channel_info *sfctemp_info[] = { + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), + HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT), + NULL +}; + +static const struct hwmon_ops sfctemp_hwmon_ops = { + .is_visible = sfctemp_is_visible, + .read = sfctemp_read, + .write = sfctemp_write, +}; + +static const struct hwmon_chip_info sfctemp_chip_info = { + .ops = &sfctemp_hwmon_ops, + .info = sfctemp_info, +}; + +static int sfctemp_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device *hwmon_dev; + struct sfctemp *sfctemp; + int ret; + + sfctemp = devm_kzalloc(dev, sizeof(*sfctemp), GFP_KERNEL); + if (!sfctemp) + return -ENOMEM; + + dev_set_drvdata(dev, sfctemp); + mutex_init(&sfctemp->lock); + init_completion(&sfctemp->conversion_done); + + sfctemp->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(sfctemp->regs)) + return PTR_ERR(sfctemp->regs); + + sfctemp->clk_sense = devm_clk_get(dev, "sense"); + if (IS_ERR(sfctemp->clk_sense)) + return dev_err_probe(dev, PTR_ERR(sfctemp->clk_sense), + "error getting sense clock\n"); + + sfctemp->clk_bus = devm_clk_get(dev, "bus"); + if (IS_ERR(sfctemp->clk_bus)) + return dev_err_probe(dev, PTR_ERR(sfctemp->clk_bus), + "error getting bus clock\n"); + + sfctemp->rst_sense = devm_reset_control_get_exclusive(dev, "sense"); + if (IS_ERR(sfctemp->rst_sense)) + return dev_err_probe(dev, PTR_ERR(sfctemp->rst_sense), + "error getting sense reset\n"); + + sfctemp->rst_bus = devm_reset_control_get_exclusive(dev, "bus"); + if (IS_ERR(sfctemp->rst_bus)) + return dev_err_probe(dev, PTR_ERR(sfctemp->rst_bus), + "error getting busreset\n"); + + ret = reset_control_assert(sfctemp->rst_sense); + if (ret) + return dev_err_probe(dev, ret, "error asserting sense reset\n"); + + ret = reset_control_assert(sfctemp->rst_bus); + if (ret) + return dev_err_probe(dev, ret, "error asserting bus reset\n"); + + ret = platform_get_irq(pdev, 0); + if (ret < 0) + return ret; + + ret = devm_request_irq(dev, ret, sfctemp_isr, 0, pdev->name, sfctemp); + if (ret) + return dev_err_probe(dev, ret, "error requesting irq\n"); + + ret = devm_add_action(dev, sfctemp_disable_action, sfctemp); + if (ret) + return ret; + + ret = sfctemp_enable(sfctemp); + if (ret) + return dev_err_probe(dev, ret, "error enabling temperature sensor: %d\n", ret); + + hwmon_dev = devm_hwmon_device_register_with_info(dev, pdev->name, sfctemp, + &sfctemp_chip_info, NULL); + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static const struct of_device_id sfctemp_of_match[] = { + { .compatible = "starfive,jh7100-temp" }, + { .compatible = "starfive,jh7110-temp" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, sfctemp_of_match); + +static struct platform_driver sfctemp_driver = { + .probe = sfctemp_probe, + .driver = { + .name = "sfctemp", + .of_match_table = sfctemp_of_match, + }, +}; +module_platform_driver(sfctemp_driver); + +MODULE_AUTHOR("Emil Renner Berthing"); +MODULE_DESCRIPTION("StarFive JH71x0 temperature sensor driver"); +MODULE_LICENSE("GPL");