hwmon: a new driver for Synaptics AS370 PVT sensor
Subsystems (2)
cover hwmon: a new driver for Synaptics AS370 PVT sensor
Add a new driver for Synaptics AS370 PVT sensor. Currently only temperature is supported.
Jisheng Zhang (2):
hwmon: Add Synaptics AS370 PVT sensor driver
hwmon: (as370-hwmon) Add DT bindings for Synaptics AS370 PVT
.../devicetree/bindings/hwmon/as370.txt | 11 ++ drivers/hwmon/Kconfig | 10 ++ drivers/hwmon/Makefile | 1 + drivers/hwmon/as370-hwmon.c | 158 ++++++++++++++++++ 4 files changed, 180 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/as370.txt create mode 100644 drivers/hwmon/as370-hwmon.c
Signature · 1 line
2.23.0.rc1
PATCH 1/2 hwmon: Add Synaptics AS370 PVT sensor driver
Add a new driver for Synaptics AS370 PVT sensors. Currently, only temperature is supported.
Signed-off-by Jisheng Zhang <[email protected]>
--- drivers/hwmon/Kconfig | 10 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/as370-hwmon.c | 158 ++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+) create mode 100644 drivers/hwmon/as370-hwmon.c
drivers/hwmon/Kconfig +10 -0
@@ -246,6 +246,16 @@ config SENSORS_ADT7475 This driver can also be built as a module. If so, the module will be called adt7475. +config SENSORS_AS370 + tristate "Synaptics AS370 SoC hardware monitoring driver" + help + If you say yes here you get support for the PVT sensors of + the Synaptics AS370 SoC + + This driver can also be built as a module. If so, the module + will be called as370-hwmon. + + config SENSORS_ASC7621 tristate "Andigilog aSC7621" depends on I2C
drivers/hwmon/Makefile +1 -0
@@ -48,6 +48,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o +obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
drivers/hwmon/as370-hwmon.c new file +158 -0
158 lines not on this page · raw message
Signature · 1 line
2.23.0.rc1
Guenter Roeck
maintainerRe: [PATCH 1/2] hwmon: Add Synaptics AS370 PVT sensor driver
Quoted 16 lines · Jisheng Zhang
Add a new driver for Synaptics AS370 PVT sensors. Currently, only temperature is supported.
Signed-off-by Jisheng Zhang <[email protected]>--- drivers/hwmon/Kconfig | 10 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/as370-hwmon.c | 158 ++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+)create mode 100644
drivers/hwmon/as370-hwmon.cdrivers/hwmon/Kconfig +0
-0
drivers/hwmon/Kconfig config SENSORS_ADT7475 · line 250
Quoted 5 lines
drivers/hwmon/Kconfig +2
-0@@ -246,6 +246,16 @@ config SENSORS_ADT7475 This driver can also be built as a module. If so, the module will be called adt7475. +config SENSORS_AS370 + tristate "Synaptics AS370 SoC hardware monitoring driver"
I think this needs "depends on HAS_IOMEM".
Quoted 29 lines
drivers/hwmon/Kconfig +8
-0+ help + If you say yes here you get support for the PVT sensors of + the Synaptics AS370 SoC + + This driver can also be built as a module. If so, the module + will be called as370-hwmon. + + config SENSORS_ASC7621 tristate "Andigilog aSC7621" depends on I2Cdrivers/hwmon/Makefile +1
-0@@ -48,6 +48,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o +obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o obj-$(CONFIG_SENSORS_ATXP1) += atxp1.odrivers/hwmon/as370-hwmon.c new file +0
-0
drivers/hwmon/as370-hwmon.c line 10
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +10
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Synaptics AS370 SoC Hardware Monitoring Driver +* +* Copyright (C) 2018 Synaptics Incorporated +* Author: Jisheng Zhang <[email protected]> +*/ + +#include <linux/bitops.h> +#include <linux/delay.h>
Unnecessary include file.
Quoted 5 lines
drivers/hwmon/as370-hwmon.c new file +5
-0+#include <linux/hwmon.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h>
drivers/hwmon/as370-hwmon.c line 25
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +10
-0+ +#define CTRL 0x0 +#define PD BIT(0) +#define EN BIT(1) +#define T_SEL BIT(2) +#define V_SEL BIT(3) +#define NMOS_SEL BIT(8) +#define PMOS_SEL BIT(9) +#define STS 0x4 +#define BN_MASK (0xfff << 0)
How about using GENMASK() ?
Quoted 7 lines
drivers/hwmon/as370-hwmon.c new file +7
-0+#define EOC BIT(12) + +struct as370_hwmon { +void __iomem *base; +}; + +static void init_pvt (struct as370_hwmon *hwmon)
drivers/hwmon/as370-hwmon.c line 42
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +10
-0+{ +u32 val; +void __iomem *addr = hwmon->base +CTRL ; + + val =PD ; +writel_relaxed (val, addr); + val |=T_SEL ; + val &= ~V_SEL ; + val &= ~NMOS_SEL ; + val &= ~PMOS_SEL ;
What is the point of this ? We know that val == PD here.
drivers/hwmon/as370-hwmon.c line 53
Quoted 11 lines
drivers/hwmon/as370-hwmon.c new file +11
-0+writel_relaxed (val, addr); + val |=EN ; +writel_relaxed (val, addr); + val &= ~PD ; +writel_relaxed (val, addr); +} + +static int read_pvt (struct as370_hwmon *hwmon) +{ +return readl_relaxed (hwmon->base +STS ) &BN_MASK ; +}
Please fold into the calling code.
drivers/hwmon/as370-hwmon.c line 65
Quoted 12 lines
drivers/hwmon/as370-hwmon.c new file +12
-0+ +static int as370_hwmon_read (struct device *dev,enum hwmon_sensor_types type, +u32 attr,int channel,long *temp) +{ +int val; +struct as370_hwmon *hwmon =dev_get_drvdata (dev); + +switch (attr) { +case hwmon_temp_input: + val =read_pvt (hwmon); +if (val <0 ) +return val;
read_pvt() doesn't return a negative error code. This check is unnecessary.
drivers/hwmon/as370-hwmon.c line 66
Quoted 1 line
drivers/hwmon/as370-hwmon.c new file +1
-0+ *temp = val *251802 /4096 -85525 ;
This results in rounding down the reported temperature. It is ok if it is what you want; otherwise, I would suggest to use DIV_ROUND_CLOSEST().
Quoted 58 lines
drivers/hwmon/as370-hwmon.c new file +58
-0+break ; +default : +return -EOPNOTSUPP ; + } + +return 0 ; +} + +static umode_t +as370_hwmon_is_visible (const void *data,enum hwmon_sensor_types type, +u32 attr,int channel) +{ +if (type != hwmon_temp) +return 0 ; + +switch (attr) { +case hwmon_temp_input: +return 0444 ; +default : +return 0 ; + } +} + +static const u32 as370_hwmon_temp_config[] = { +HWMON_T_INPUT , +0 +}; + +static const struct hwmon_channel_info as370_hwmon_temp = { + .type = hwmon_temp, + .config = as370_hwmon_temp_config, +}; + +static const struct hwmon_channel_info *as370_hwmon_info[] = { + &as370_hwmon_temp, +NULL +}; + +static const struct hwmon_ops as370_hwmon_ops = { + .is_visible = as370_hwmon_is_visible, + .read = as370_hwmon_read, +}; + +static const struct hwmon_chip_info as370_chip_info = { + .ops = &as370_hwmon_ops, + .info = as370_hwmon_info, +}; + +static int as370_hwmon_probe (struct platform_device *pdev) +{ +struct resource *res; +struct device *hwmon_dev; +struct as370_hwmon *hwmon; +struct device *dev = &pdev->dev; + + hwmon =devm_kzalloc (dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ;
drivers/hwmon/as370-hwmon.c line 134
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +10
-0+ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + hwmon->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (hwmon->base)) +return PTR_ERR (hwmon->base); + +init_pvt (hwmon); + + hwmon_dev = devm_hwmon_device_register_with_info(dev, + "as370_hwmon",
The "_hwmon" seems unnecessary. It will show up in "sensors" as part of the sensor name. Is this really what you want ?
Quoted 25 lines
drivers/hwmon/as370-hwmon.c new file +24
-0+ hwmon, + &as370_chip_info, + NULL); +return PTR_ERR_OR_ZERO (hwmon_dev); +} + +static const struct of_device_id as370_hwmon_match[] = { + { .compatible ="syna,as370-hwmon" }, + {}, +}; +MODULE_DEVICE_TABLE (of, as370_hwmon_match); + +static struct platform_driver as370_hwmon_driver = { + .probe = as370_hwmon_probe, + .driver = { + .name ="as370-hwmon" , + .of_match_table = as370_hwmon_match, + }, +}; +module_platform_driver (as370_hwmon_driver); + +MODULE_AUTHOR ("Jisheng Zhang<[email protected]>" ); +MODULE_DESCRIPTION ("Synaptics AS370 SoC hardware monitor" ); +MODULE_LICENSE ("GPL v2" );
Re: [PATCH 1/2] hwmon: Add Synaptics AS370 PVT sensor driver
Hi Guenter,
Quoted 22 lines · On Mon, 26 Aug 2019 06:44:34 -0700 Guenter Roeck wrote:
Quoted 19 lines · Jisheng Zhang
Add a new driver for Synaptics AS370 PVT sensors. Currently, only temperature is supported.
Signed-off-by Jisheng Zhang <[email protected]>--- drivers/hwmon/Kconfig | 10 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/as370-hwmon.c | 158 ++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+)create mode 100644
drivers/hwmon/as370-hwmon.cdrivers/hwmon/Kconfig +0
-0@@ -246,6 +246,16 @@ config SENSORS_ADT7475 This driver can also be built as a module. If so, the module will be called adt7475.
Quoted 4 lines
drivers/hwmon/Kconfig config SENSORS_ADT7475 · line 250
Quoted 2 lines
drivers/hwmon/Kconfig +2
-0+config SENSORS_AS370 + tristate "Synaptics AS370 SoC hardware monitoring driver"I think this needs "depends on
HAS_IOMEM".
HWMON depends on HAS_IOMEM, so the dependency has been required by the common HWMON, we don't need it here.
Quoted 34 lines
Quoted 34 lines
drivers/hwmon/Kconfig +8
-0+ help + If you say yes here you get support for the PVT sensors of + the Synaptics AS370 SoC + + This driver can also be built as a module. If so, the module + will be called as370-hwmon. + + config SENSORS_ASC7621 tristate "Andigilog aSC7621" depends on I2Cdrivers/hwmon/Makefile +1
-0@@ -48,6 +48,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o +obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o obj-$(CONFIG_SENSORS_ATXP1) += atxp1.odrivers/hwmon/as370-hwmon.c new file +4
-0@@ -0,0 +1,158 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Synaptics AS370 SoC Hardware Monitoring Driver +*
Quoted 8 lines
drivers/hwmon/as370-hwmon.c line 10
Quoted 6 lines
drivers/hwmon/as370-hwmon.c new file +6
-0+* Copyright (C) 2018 Synaptics Incorporated +* Author: Jisheng Zhang <[email protected]> +*/ + +#include <linux/bitops.h> +#include <linux/delay.h> Unnecessary include file.
will remove it in newer version.
Quoted 10 lines
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +9
-0+#include <linux/hwmon.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> + +#define CTRL 0x0 +#define PD BIT(0) +#define EN BIT(1)
Quoted 8 lines
drivers/hwmon/as370-hwmon.c line 25
Quoted 6 lines
drivers/hwmon/as370-hwmon.c new file +6
-0+#define T_SEL BIT(2) +#define V_SEL BIT(3) +#define NMOS_SEL BIT(8) +#define PMOS_SEL BIT(9) +#define STS 0x4 +#define BN_MASK (0xfff << 0)How about using
GENMASK()?
Good idea.
Quoted 12 lines
Quoted 12 lines
drivers/hwmon/as370-hwmon.c new file +11
-0+#define EOC BIT(12) + +struct as370_hwmon { +void __iomem *base; +}; + +static void init_pvt (struct as370_hwmon *hwmon) +{ +u32 val; +void __iomem *addr = hwmon->base +CTRL ; +
Quoted 8 lines
drivers/hwmon/as370-hwmon.c line 42
Quoted 6 lines
drivers/hwmon/as370-hwmon.c new file +6
-0+ val =PD ; +writel_relaxed (val, addr); + val |=T_SEL ; + val &= ~V_SEL ; + val &= ~NMOS_SEL ; + val &= ~PMOS_SEL ;What is the point of this ? We know that val == PD here.
yes, could be removed
Quoted 30 lines
drivers/hwmon/as370-hwmon.c line 53
Quoted 12 lines
drivers/hwmon/as370-hwmon.c new file +11
-0+writel_relaxed (val, addr); + val |=EN ; +writel_relaxed (val, addr); + val &= ~PD ; +writel_relaxed (val, addr); +} + +static int read_pvt (struct as370_hwmon *hwmon) +{ +return readl_relaxed (hwmon->base +STS ) &BN_MASK ; +}Please fold into the calling code.
drivers/hwmon/as370-hwmon.c line 65
Quoted 12 lines
drivers/hwmon/as370-hwmon.c new file +12
-0+ +static int as370_hwmon_read (struct device *dev,enum hwmon_sensor_types type, +u32 attr,int channel,long *temp) +{ +int val; +struct as370_hwmon *hwmon =dev_get_drvdata (dev); + +switch (attr) { +case hwmon_temp_input: + val =read_pvt (hwmon); +if (val <0 ) +return val;
read_pvt()doesn't return a negative error code. This check is unnecessary.
Quoted 4 lines
drivers/hwmon/as370-hwmon.c line 66
Quoted 1 line
drivers/hwmon/as370-hwmon.c new file +1
-0+ *temp = val *251802 /4096 -85525 ;This results in rounding down the reported temperature. It is ok if it is what you want; otherwise, I would suggest to use
DIV_ROUND_CLOSEST().
Good idea.
Thanks for your review,
Jisheng
Quoted 99 lines
Quoted 59 lines
drivers/hwmon/as370-hwmon.c new file +58
-0+break ; +default : +return -EOPNOTSUPP ; + } + +return 0 ; +} + +static umode_t +as370_hwmon_is_visible (const void *data,enum hwmon_sensor_types type, +u32 attr,int channel) +{ +if (type != hwmon_temp) +return 0 ; + +switch (attr) { +case hwmon_temp_input: +return 0444 ; +default : +return 0 ; + } +} + +static const u32 as370_hwmon_temp_config[] = { +HWMON_T_INPUT , +0 +}; + +static const struct hwmon_channel_info as370_hwmon_temp = { + .type = hwmon_temp, + .config = as370_hwmon_temp_config, +}; + +static const struct hwmon_channel_info *as370_hwmon_info[] = { + &as370_hwmon_temp, +NULL +}; + +static const struct hwmon_ops as370_hwmon_ops = { + .is_visible = as370_hwmon_is_visible, + .read = as370_hwmon_read, +}; + +static const struct hwmon_chip_info as370_chip_info = { + .ops = &as370_hwmon_ops, + .info = as370_hwmon_info, +}; + +static int as370_hwmon_probe (struct platform_device *pdev) +{ +struct resource *res; +struct device *hwmon_dev; +struct as370_hwmon *hwmon; +struct device *dev = &pdev->dev; + + hwmon =devm_kzalloc (dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ;drivers/hwmon/as370-hwmon.c line 134
Quoted 10 lines
drivers/hwmon/as370-hwmon.c new file +10
-0+ + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + hwmon->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (hwmon->base)) +return PTR_ERR (hwmon->base); + +init_pvt (hwmon); + + hwmon_dev = devm_hwmon_device_register_with_info(dev, + "as370_hwmon",The "
_hwmon" seems unnecessary. It will show up in "sensors" as part of the sensor name. Is this really what you want ?Quoted 25 lines
drivers/hwmon/as370-hwmon.c new file +24
-0+ hwmon, + &as370_chip_info, + NULL); +return PTR_ERR_OR_ZERO (hwmon_dev); +} + +static const struct of_device_id as370_hwmon_match[] = { + { .compatible ="syna,as370-hwmon" }, + {}, +}; +MODULE_DEVICE_TABLE (of, as370_hwmon_match); + +static struct platform_driver as370_hwmon_driver = { + .probe = as370_hwmon_probe, + .driver = { + .name ="as370-hwmon" , + .of_match_table = as370_hwmon_match, + }, +}; +module_platform_driver (as370_hwmon_driver); + +MODULE_AUTHOR ("Jisheng Zhang<[email protected]>" ); +MODULE_DESCRIPTION ("Synaptics AS370 SoC hardware monitor" ); +MODULE_LICENSE ("GPL v2" );
Guenter Roeck
maintainerRe: [PATCH 1/2] hwmon: Add Synaptics AS370 PVT sensor driver
Quoted 26 lines · Jisheng Zhang
Hi Guenter,
Quoted 22 lines · On Mon, 26 Aug 2019 06:44:34 -0700 Guenter Roeck wrote:
Quoted 19 lines · Jisheng Zhang
Add a new driver for Synaptics AS370 PVT sensors. Currently, only temperature is supported.
Signed-off-by Jisheng Zhang <[email protected]>--- drivers/hwmon/Kconfig | 10 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/as370-hwmon.c | 158 ++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+)create mode 100644
drivers/hwmon/as370-hwmon.cdrivers/hwmon/Kconfig +0
-0@@ -246,6 +246,16 @@ config SENSORS_ADT7475 This driver can also be built as a module. If so, the module will be called adt7475.
Quoted 8 lines
Quoted 4 lines
drivers/hwmon/Kconfig config SENSORS_ADT7475 · line 250
Quoted 2 lines
drivers/hwmon/Kconfig +2
-0+config SENSORS_AS370 + tristate "Synaptics AS370 SoC hardware monitoring driver"I think this needs "depends on
HAS_IOMEM".HWMON depends on
HAS_IOMEM, so the dependency has been required by the common HWMON, we don't need it here.
This is so wrong :-(. As if I2C or SPI based sensor chips would require iomem. Oh well, no one complained in 12+ years, so I guess we are "fine", at least for the time being.
Thanks for noticing.
Guenter
PATCH 2/2 hwmon: (as370-hwmon) Add DT bindings for Synaptics AS370 PVT
In reply to Jisheng ZhangAdd device tree bindings for Synaptics AS370 PVT sensors.
Signed-off-by Jisheng Zhang <[email protected]>
--- Documentation/devicetree/bindings/hwmon/as370.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/as370.txt
Documentation/devicetree/bindings/hwmon/as370.txt new file +11 -0
@@ -0,0 +1,11 @@ +Bindings for Synaptics AS370 PVT sensors + +Required properties: +- compatible : "syna,as370-hwmon" +- reg : address and length of the register set. + +Example: + hwmon@ea0810 { + compatible = "syna,as370-hwmon"; + reg = <0xea0810 0xc>; + };
Signature · 1 line
2.23.0.rc1