* [PATCH v1 1/4] lib: add generic polynomial calculation
2022-03-26 19:23 [PATCH v1 0/4] hwmon: add lan9668 driver Michael Walle
@ 2022-03-26 19:23 ` Michael Walle
2022-03-27 1:21 ` Guenter Roeck
2022-03-26 19:23 ` [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions Michael Walle
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Michael Walle @ 2022-03-26 19:23 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel, Michael Walle
Some temperature and voltage sensors use a polynomial to convert between
raw data points and actual temperature or voltage. The polynomial is
usually the result of a curve fitting of the diode characteristic.
The BT1 PVT hwmon driver already uses such a polynonmial calculation
which is rather generic. Move it to lib/ so other drivers can reuse it.
Signed-off-by: Michael Walle <michael@walle.cc>
---
include/linux/polynomial.h | 35 ++++++++++++++
lib/Kconfig | 3 ++
lib/Makefile | 2 +
lib/polynomial.c | 95 ++++++++++++++++++++++++++++++++++++++
4 files changed, 135 insertions(+)
create mode 100644 include/linux/polynomial.h
create mode 100644 lib/polynomial.c
diff --git a/include/linux/polynomial.h b/include/linux/polynomial.h
new file mode 100644
index 000000000000..9e074a0bb6fa
--- /dev/null
+++ b/include/linux/polynomial.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ */
+
+#ifndef _POLYNOMIAL_H
+#define _POLYNOMIAL_H
+
+/*
+ * struct polynomial_term - one term descriptor of a polynomial
+ * @deg: degree of the term.
+ * @coef: multiplication factor of the term.
+ * @divider: distributed divider per each degree.
+ * @divider_leftover: divider leftover, which couldn't be redistributed.
+ */
+struct polynomial_term {
+ unsigned int deg;
+ long coef;
+ long divider;
+ long divider_leftover;
+};
+
+/*
+ * struct polynomial - a polynomial descriptor
+ * @total_divider: total data divider.
+ * @terms: polynomial terms, last term must have degree of 0
+ */
+struct polynomial {
+ long total_divider;
+ struct polynomial_term terms[];
+};
+
+long polynomial_calc(const struct polynomial *poly, long data);
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index 087e06b4cdfd..6a843639814f 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -737,3 +737,6 @@ config PLDMFW
config ASN1_ENCODER
tristate
+
+config POLYNOMIAL
+ tristate
diff --git a/lib/Makefile b/lib/Makefile
index 6b9ffc1bd1ee..89fcae891361 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -263,6 +263,8 @@ obj-$(CONFIG_MEMREGION) += memregion.o
obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
obj-$(CONFIG_IRQ_POLL) += irq_poll.o
+obj-$(CONFIG_POLYNOMIAL) += polynomial.o
+
# stackdepot.c should not be instrumented or call instrumented functions.
# Prevent the compiler from calling builtins like memcmp() or bcmp() from this
# file.
diff --git a/lib/polynomial.c b/lib/polynomial.c
new file mode 100644
index 000000000000..63ea2bdc545f
--- /dev/null
+++ b/lib/polynomial.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Generic polynomial calculation using integer coefficients.
+ *
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Authors:
+ * Maxim Kaurkin <maxim.kaurkin@baikalelectronics.ru>
+ * Serge Semin <Sergey.Semin@baikalelectronics.ru>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/polynomial.h>
+
+/*
+ * Originally this was part of drivers/hwmon/bt1-pvt.c.
+ * There the following conversion is used and should serve as an example here:
+ *
+ * The original translation formulae of the temperature (in degrees of Celsius)
+ * to PVT data and vice-versa are following:
+ *
+ * N = 1.8322e-8*(T^4) + 2.343e-5*(T^3) + 8.7018e-3*(T^2) + 3.9269*(T^1) +
+ * 1.7204e2
+ * T = -1.6743e-11*(N^4) + 8.1542e-8*(N^3) + -1.8201e-4*(N^2) +
+ * 3.1020e-1*(N^1) - 4.838e1
+ *
+ * where T = [-48.380, 147.438]C and N = [0, 1023].
+ *
+ * They must be accordingly altered to be suitable for the integer arithmetics.
+ * The technique is called 'factor redistribution', which just makes sure the
+ * multiplications and divisions are made so to have a result of the operations
+ * within the integer numbers limit. In addition we need to translate the
+ * formulae to accept millidegrees of Celsius. Here what they look like after
+ * the alterations:
+ *
+ * N = (18322e-20*(T^4) + 2343e-13*(T^3) + 87018e-9*(T^2) + 39269e-3*T +
+ * 17204e2) / 1e4
+ * T = -16743e-12*(D^4) + 81542e-9*(D^3) - 182010e-6*(D^2) + 310200e-3*D -
+ * 48380
+ * where T = [-48380, 147438] mC and N = [0, 1023].
+ *
+ * static const struct polynomial poly_temp_to_N = {
+ * .total_divider = 10000,
+ * .terms = {
+ * {4, 18322, 10000, 10000},
+ * {3, 2343, 10000, 10},
+ * {2, 87018, 10000, 10},
+ * {1, 39269, 1000, 1},
+ * {0, 1720400, 1, 1}
+ * }
+ * };
+ *
+ * static const struct polynomial poly_N_to_temp = {
+ * .total_divider = 1,
+ * .terms = {
+ * {4, -16743, 1000, 1},
+ * {3, 81542, 1000, 1},
+ * {2, -182010, 1000, 1},
+ * {1, 310200, 1000, 1},
+ * {0, -48380, 1, 1}
+ * }
+ * };
+ */
+
+/*
+ * Here is the polynomial calculation function, which performs the
+ * redistributed terms calculations. It's pretty straightforward. We walk
+ * over each degree term up to the free one, and perform the redistributed
+ * multiplication of the term coefficient, its divider (as for the rationale
+ * fraction representation), data power and the rational fraction divider
+ * leftover. Then all of this is collected in a total sum variable, which
+ * value is normalized by the total divider before being returned.
+ */
+long polynomial_calc(const struct polynomial *poly, long data)
+{
+ const struct polynomial_term *term = poly->terms;
+ long total_divider = poly->total_divider ?: 1;
+ long tmp, ret = 0;
+ int deg;
+
+ do {
+ tmp = term->coef;
+ for (deg = 0; deg < term->deg; ++deg)
+ tmp = mult_frac(tmp, data, term->divider);
+ ret += tmp / term->divider_leftover;
+ } while ((term++)->deg);
+
+ return ret / total_divider;
+}
+EXPORT_SYMBOL_GPL(polynomial_calc);
+
+MODULE_DESCRIPTION("Generic polynomial calculations");
+MODULE_LICENSE("GPL");
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v1 1/4] lib: add generic polynomial calculation
2022-03-26 19:23 ` [PATCH v1 1/4] lib: add generic polynomial calculation Michael Walle
@ 2022-03-27 1:21 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2022-03-27 1:21 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel
On 3/26/22 12:23, Michael Walle wrote:
> Some temperature and voltage sensors use a polynomial to convert between
> raw data points and actual temperature or voltage. The polynomial is
> usually the result of a curve fitting of the diode characteristic.
>
> The BT1 PVT hwmon driver already uses such a polynonmial calculation
> which is rather generic. Move it to lib/ so other drivers can reuse it.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> include/linux/polynomial.h | 35 ++++++++++++++
> lib/Kconfig | 3 ++
> lib/Makefile | 2 +
> lib/polynomial.c | 95 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 135 insertions(+)
> create mode 100644 include/linux/polynomial.h
> create mode 100644 lib/polynomial.c
>
> diff --git a/include/linux/polynomial.h b/include/linux/polynomial.h
> new file mode 100644
> index 000000000000..9e074a0bb6fa
> --- /dev/null
> +++ b/include/linux/polynomial.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> + */
> +
> +#ifndef _POLYNOMIAL_H
> +#define _POLYNOMIAL_H
> +
> +/*
> + * struct polynomial_term - one term descriptor of a polynomial
> + * @deg: degree of the term.
> + * @coef: multiplication factor of the term.
> + * @divider: distributed divider per each degree.
> + * @divider_leftover: divider leftover, which couldn't be redistributed.
> + */
> +struct polynomial_term {
> + unsigned int deg;
> + long coef;
> + long divider;
> + long divider_leftover;
> +};
> +
> +/*
> + * struct polynomial - a polynomial descriptor
> + * @total_divider: total data divider.
> + * @terms: polynomial terms, last term must have degree of 0
> + */
> +struct polynomial {
> + long total_divider;
> + struct polynomial_term terms[];
> +};
> +
> +long polynomial_calc(const struct polynomial *poly, long data);
> +
> +#endif
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 087e06b4cdfd..6a843639814f 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -737,3 +737,6 @@ config PLDMFW
>
> config ASN1_ENCODER
> tristate
> +
> +config POLYNOMIAL
> + tristate
> diff --git a/lib/Makefile b/lib/Makefile
> index 6b9ffc1bd1ee..89fcae891361 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -263,6 +263,8 @@ obj-$(CONFIG_MEMREGION) += memregion.o
> obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
> obj-$(CONFIG_IRQ_POLL) += irq_poll.o
>
> +obj-$(CONFIG_POLYNOMIAL) += polynomial.o
> +
> # stackdepot.c should not be instrumented or call instrumented functions.
> # Prevent the compiler from calling builtins like memcmp() or bcmp() from this
> # file.
> diff --git a/lib/polynomial.c b/lib/polynomial.c
> new file mode 100644
> index 000000000000..63ea2bdc545f
> --- /dev/null
> +++ b/lib/polynomial.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic polynomial calculation using integer coefficients.
> + *
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> + *
> + * Authors:
> + * Maxim Kaurkin <maxim.kaurkin@baikalelectronics.ru>
> + * Serge Semin <Sergey.Semin@baikalelectronics.ru>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/polynomial.h>
> +
> +/*
> + * Originally this was part of drivers/hwmon/bt1-pvt.c.
> + * There the following conversion is used and should serve as an example here:
> + *
> + * The original translation formulae of the temperature (in degrees of Celsius)
> + * to PVT data and vice-versa are following:
> + *
> + * N = 1.8322e-8*(T^4) + 2.343e-5*(T^3) + 8.7018e-3*(T^2) + 3.9269*(T^1) +
> + * 1.7204e2
> + * T = -1.6743e-11*(N^4) + 8.1542e-8*(N^3) + -1.8201e-4*(N^2) +
> + * 3.1020e-1*(N^1) - 4.838e1
> + *
> + * where T = [-48.380, 147.438]C and N = [0, 1023].
> + *
> + * They must be accordingly altered to be suitable for the integer arithmetics.
> + * The technique is called 'factor redistribution', which just makes sure the
> + * multiplications and divisions are made so to have a result of the operations
> + * within the integer numbers limit. In addition we need to translate the
> + * formulae to accept millidegrees of Celsius. Here what they look like after
> + * the alterations:
> + *
> + * N = (18322e-20*(T^4) + 2343e-13*(T^3) + 87018e-9*(T^2) + 39269e-3*T +
> + * 17204e2) / 1e4
> + * T = -16743e-12*(D^4) + 81542e-9*(D^3) - 182010e-6*(D^2) + 310200e-3*D -
> + * 48380
> + * where T = [-48380, 147438] mC and N = [0, 1023].
> + *
> + * static const struct polynomial poly_temp_to_N = {
> + * .total_divider = 10000,
> + * .terms = {
> + * {4, 18322, 10000, 10000},
> + * {3, 2343, 10000, 10},
> + * {2, 87018, 10000, 10},
> + * {1, 39269, 1000, 1},
> + * {0, 1720400, 1, 1}
> + * }
> + * };
> + *
> + * static const struct polynomial poly_N_to_temp = {
> + * .total_divider = 1,
> + * .terms = {
> + * {4, -16743, 1000, 1},
> + * {3, 81542, 1000, 1},
> + * {2, -182010, 1000, 1},
> + * {1, 310200, 1000, 1},
> + * {0, -48380, 1, 1}
> + * }
> + * };
> + */
> +
> +/*
> + * Here is the polynomial calculation function, which performs the
This should be a proper doc string.
> + * redistributed terms calculations. It's pretty straightforward. We walk
> + * over each degree term up to the free one, and perform the redistributed
> + * multiplication of the term coefficient, its divider (as for the rationale
> + * fraction representation), data power and the rational fraction divider
> + * leftover. Then all of this is collected in a total sum variable, which
> + * value is normalized by the total divider before being returned.
> + */
> +long polynomial_calc(const struct polynomial *poly, long data)
> +{
> + const struct polynomial_term *term = poly->terms;
> + long total_divider = poly->total_divider ?: 1;
> + long tmp, ret = 0;
> + int deg;
> +
> + do {
> + tmp = term->coef;
> + for (deg = 0; deg < term->deg; ++deg)
> + tmp = mult_frac(tmp, data, term->divider);
> + ret += tmp / term->divider_leftover;
> + } while ((term++)->deg);
> +
> + return ret / total_divider;
> +}
> +EXPORT_SYMBOL_GPL(polynomial_calc);
> +
> +MODULE_DESCRIPTION("Generic polynomial calculations");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions
2022-03-26 19:23 [PATCH v1 0/4] hwmon: add lan9668 driver Michael Walle
2022-03-26 19:23 ` [PATCH v1 1/4] lib: add generic polynomial calculation Michael Walle
@ 2022-03-26 19:23 ` Michael Walle
2022-03-27 0:49 ` kernel test robot
` (2 more replies)
2022-03-26 19:23 ` [PATCH v1 3/4] dt-bindings: hwmon: add Microchip LAN966x bindings Michael Walle
2022-03-26 19:23 ` [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC Michael Walle
3 siblings, 3 replies; 16+ messages in thread
From: Michael Walle @ 2022-03-26 19:23 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel, Michael Walle
The polynomial calculation function was moved into lib/ to be able to
reuse it. Move over to this one.
Signed-off-by: Michael Walle <michael@walle.cc>
---
drivers/hwmon/bt1-pvt.c | 50 +++++++++++------------------------------
1 file changed, 13 insertions(+), 37 deletions(-)
diff --git a/drivers/hwmon/bt1-pvt.c b/drivers/hwmon/bt1-pvt.c
index 74ce5211eb75..21ab172774ec 100644
--- a/drivers/hwmon/bt1-pvt.c
+++ b/drivers/hwmon/bt1-pvt.c
@@ -26,6 +26,7 @@
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/polynomial.h>
#include <linux/seqlock.h>
#include <linux/sysfs.h>
#include <linux/types.h>
@@ -65,7 +66,7 @@ static const struct pvt_sensor_info pvt_info[] = {
* 48380,
* where T = [-48380, 147438] mC and N = [0, 1023].
*/
-static const struct pvt_poly __maybe_unused poly_temp_to_N = {
+static const struct polynomial __maybe_unused poly_temp_to_N = {
.total_divider = 10000,
.terms = {
{4, 18322, 10000, 10000},
@@ -76,7 +77,7 @@ static const struct pvt_poly __maybe_unused poly_temp_to_N = {
}
};
-static const struct pvt_poly poly_N_to_temp = {
+static const struct polynomial poly_N_to_temp = {
.total_divider = 1,
.terms = {
{4, -16743, 1000, 1},
@@ -97,7 +98,7 @@ static const struct pvt_poly poly_N_to_temp = {
* N = (18658e-3*V - 11572) / 10,
* V = N * 10^5 / 18658 + 11572 * 10^4 / 18658.
*/
-static const struct pvt_poly __maybe_unused poly_volt_to_N = {
+static const struct polynomial __maybe_unused poly_volt_to_N = {
.total_divider = 10,
.terms = {
{1, 18658, 1000, 1},
@@ -105,7 +106,7 @@ static const struct pvt_poly __maybe_unused poly_volt_to_N = {
}
};
-static const struct pvt_poly poly_N_to_volt = {
+static const struct polynomial poly_N_to_volt = {
.total_divider = 10,
.terms = {
{1, 100000, 18658, 1},
@@ -113,31 +114,6 @@ static const struct pvt_poly poly_N_to_volt = {
}
};
-/*
- * Here is the polynomial calculation function, which performs the
- * redistributed terms calculations. It's pretty straightforward. We walk
- * over each degree term up to the free one, and perform the redistributed
- * multiplication of the term coefficient, its divider (as for the rationale
- * fraction representation), data power and the rational fraction divider
- * leftover. Then all of this is collected in a total sum variable, which
- * value is normalized by the total divider before being returned.
- */
-static long pvt_calc_poly(const struct pvt_poly *poly, long data)
-{
- const struct pvt_poly_term *term = poly->terms;
- long tmp, ret = 0;
- int deg;
-
- do {
- tmp = term->coef;
- for (deg = 0; deg < term->deg; ++deg)
- tmp = mult_frac(tmp, data, term->divider);
- ret += tmp / term->divider_leftover;
- } while ((term++)->deg);
-
- return ret / poly->total_divider;
-}
-
static inline u32 pvt_update(void __iomem *reg, u32 mask, u32 data)
{
u32 old;
@@ -324,9 +300,9 @@ static int pvt_read_data(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
} while (read_seqretry(&cache->data_seqlock, seq));
if (type == PVT_TEMP)
- *val = pvt_calc_poly(&poly_N_to_temp, data);
+ *val = polynomial_calc(&poly_N_to_temp, data);
else
- *val = pvt_calc_poly(&poly_N_to_volt, data);
+ *val = polynomial_calc(&poly_N_to_volt, data);
return 0;
}
@@ -345,9 +321,9 @@ static int pvt_read_limit(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
data = FIELD_GET(PVT_THRES_HI_MASK, data);
if (type == PVT_TEMP)
- *val = pvt_calc_poly(&poly_N_to_temp, data);
+ *val = polynomial_calc(&poly_N_to_temp, data);
else
- *val = pvt_calc_poly(&poly_N_to_volt, data);
+ *val = polynomial_calc(&poly_N_to_volt, data);
return 0;
}
@@ -360,10 +336,10 @@ static int pvt_write_limit(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
if (type == PVT_TEMP) {
val = clamp(val, PVT_TEMP_MIN, PVT_TEMP_MAX);
- data = pvt_calc_poly(&poly_temp_to_N, val);
+ data = polynomial_calc(&poly_temp_to_N, val);
} else {
val = clamp(val, PVT_VOLT_MIN, PVT_VOLT_MAX);
- data = pvt_calc_poly(&poly_volt_to_N, val);
+ data = polynomial_calc(&poly_volt_to_N, val);
}
/* Serialize limit update, since a part of the register is changed. */
@@ -522,9 +498,9 @@ static int pvt_read_data(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
return -ETIMEDOUT;
if (type == PVT_TEMP)
- *val = pvt_calc_poly(&poly_N_to_temp, data);
+ *val = polynomial_calc(&poly_N_to_temp, data);
else
- *val = pvt_calc_poly(&poly_N_to_volt, data);
+ *val = polynomial_calc(&poly_N_to_volt, data);
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions
2022-03-26 19:23 ` [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions Michael Walle
@ 2022-03-27 0:49 ` kernel test robot
2022-03-27 1:10 ` kernel test robot
2022-03-27 1:22 ` Guenter Roeck
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2022-03-27 0:49 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski
Cc: kbuild-all, linux-hwmon, devicetree, linux-kernel, Michael Walle
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on robh/for-next linus/master v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20220327/202203270834.xEWniPdY-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/51d4d93c8777a757e21386aa76ccc128a6bd85af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
git checkout 51d4d93c8777a757e21386aa76ccc128a6bd85af
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/hwmon/bt1-pvt.o: in function `pvt_read_data':
bt1-pvt.c:(.text+0xa32): undefined reference to `polynomial_calc'
ia64-linux-ld: drivers/hwmon/bt1-pvt.o: in function `pvt_write_limit':
bt1-pvt.c:(.text+0xae2): undefined reference to `polynomial_calc'
>> ia64-linux-ld: bt1-pvt.c:(.text+0xb22): undefined reference to `polynomial_calc'
ia64-linux-ld: drivers/hwmon/bt1-pvt.o: in function `pvt_hwmon_read':
bt1-pvt.c:(.text+0x2532): undefined reference to `polynomial_calc'
ia64-linux-ld: bt1-pvt.c:(.text+0x2732): undefined reference to `polynomial_calc'
ia64-linux-ld: drivers/hwmon/bt1-pvt.o:bt1-pvt.c:(.text+0x27e2): more undefined references to `polynomial_calc' follow
`adc3xxx_i2c_remove' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions
2022-03-26 19:23 ` [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions Michael Walle
2022-03-27 0:49 ` kernel test robot
@ 2022-03-27 1:10 ` kernel test robot
2022-03-27 1:22 ` Guenter Roeck
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2022-03-27 1:10 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski
Cc: kbuild-all, linux-hwmon, devicetree, linux-kernel, Michael Walle
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on robh/for-next linus/master v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: ia64-buildonly-randconfig-r002-20220327 (https://download.01.org/0day-ci/archive/20220327/202203270925.NrqU6SE8-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/51d4d93c8777a757e21386aa76ccc128a6bd85af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
git checkout 51d4d93c8777a757e21386aa76ccc128a6bd85af
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/hwmon/bt1-pvt.o: in function `pvt_read_data':
>> bt1-pvt.c:(.text+0xeb2): undefined reference to `polynomial_calc'
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions
2022-03-26 19:23 ` [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions Michael Walle
2022-03-27 0:49 ` kernel test robot
2022-03-27 1:10 ` kernel test robot
@ 2022-03-27 1:22 ` Guenter Roeck
2 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2022-03-27 1:22 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel
On 3/26/22 12:23, Michael Walle wrote:
> The polynomial calculation function was moved into lib/ to be able to
> reuse it. Move over to this one.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
select POLYNOMIAL missing in Kconfig
> drivers/hwmon/bt1-pvt.c | 50 +++++++++++------------------------------
> 1 file changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/hwmon/bt1-pvt.c b/drivers/hwmon/bt1-pvt.c
> index 74ce5211eb75..21ab172774ec 100644
> --- a/drivers/hwmon/bt1-pvt.c
> +++ b/drivers/hwmon/bt1-pvt.c
> @@ -26,6 +26,7 @@
> #include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/polynomial.h>
> #include <linux/seqlock.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
> @@ -65,7 +66,7 @@ static const struct pvt_sensor_info pvt_info[] = {
> * 48380,
> * where T = [-48380, 147438] mC and N = [0, 1023].
> */
> -static const struct pvt_poly __maybe_unused poly_temp_to_N = {
> +static const struct polynomial __maybe_unused poly_temp_to_N = {
> .total_divider = 10000,
> .terms = {
> {4, 18322, 10000, 10000},
> @@ -76,7 +77,7 @@ static const struct pvt_poly __maybe_unused poly_temp_to_N = {
> }
> };
>
> -static const struct pvt_poly poly_N_to_temp = {
> +static const struct polynomial poly_N_to_temp = {
> .total_divider = 1,
> .terms = {
> {4, -16743, 1000, 1},
> @@ -97,7 +98,7 @@ static const struct pvt_poly poly_N_to_temp = {
> * N = (18658e-3*V - 11572) / 10,
> * V = N * 10^5 / 18658 + 11572 * 10^4 / 18658.
> */
> -static const struct pvt_poly __maybe_unused poly_volt_to_N = {
> +static const struct polynomial __maybe_unused poly_volt_to_N = {
> .total_divider = 10,
> .terms = {
> {1, 18658, 1000, 1},
> @@ -105,7 +106,7 @@ static const struct pvt_poly __maybe_unused poly_volt_to_N = {
> }
> };
>
> -static const struct pvt_poly poly_N_to_volt = {
> +static const struct polynomial poly_N_to_volt = {
> .total_divider = 10,
> .terms = {
> {1, 100000, 18658, 1},
> @@ -113,31 +114,6 @@ static const struct pvt_poly poly_N_to_volt = {
> }
> };
>
> -/*
> - * Here is the polynomial calculation function, which performs the
> - * redistributed terms calculations. It's pretty straightforward. We walk
> - * over each degree term up to the free one, and perform the redistributed
> - * multiplication of the term coefficient, its divider (as for the rationale
> - * fraction representation), data power and the rational fraction divider
> - * leftover. Then all of this is collected in a total sum variable, which
> - * value is normalized by the total divider before being returned.
> - */
> -static long pvt_calc_poly(const struct pvt_poly *poly, long data)
> -{
> - const struct pvt_poly_term *term = poly->terms;
> - long tmp, ret = 0;
> - int deg;
> -
> - do {
> - tmp = term->coef;
> - for (deg = 0; deg < term->deg; ++deg)
> - tmp = mult_frac(tmp, data, term->divider);
> - ret += tmp / term->divider_leftover;
> - } while ((term++)->deg);
> -
> - return ret / poly->total_divider;
> -}
> -
> static inline u32 pvt_update(void __iomem *reg, u32 mask, u32 data)
> {
> u32 old;
> @@ -324,9 +300,9 @@ static int pvt_read_data(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
> } while (read_seqretry(&cache->data_seqlock, seq));
>
> if (type == PVT_TEMP)
> - *val = pvt_calc_poly(&poly_N_to_temp, data);
> + *val = polynomial_calc(&poly_N_to_temp, data);
> else
> - *val = pvt_calc_poly(&poly_N_to_volt, data);
> + *val = polynomial_calc(&poly_N_to_volt, data);
>
> return 0;
> }
> @@ -345,9 +321,9 @@ static int pvt_read_limit(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
> data = FIELD_GET(PVT_THRES_HI_MASK, data);
>
> if (type == PVT_TEMP)
> - *val = pvt_calc_poly(&poly_N_to_temp, data);
> + *val = polynomial_calc(&poly_N_to_temp, data);
> else
> - *val = pvt_calc_poly(&poly_N_to_volt, data);
> + *val = polynomial_calc(&poly_N_to_volt, data);
>
> return 0;
> }
> @@ -360,10 +336,10 @@ static int pvt_write_limit(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
>
> if (type == PVT_TEMP) {
> val = clamp(val, PVT_TEMP_MIN, PVT_TEMP_MAX);
> - data = pvt_calc_poly(&poly_temp_to_N, val);
> + data = polynomial_calc(&poly_temp_to_N, val);
> } else {
> val = clamp(val, PVT_VOLT_MIN, PVT_VOLT_MAX);
> - data = pvt_calc_poly(&poly_volt_to_N, val);
> + data = polynomial_calc(&poly_volt_to_N, val);
> }
>
> /* Serialize limit update, since a part of the register is changed. */
> @@ -522,9 +498,9 @@ static int pvt_read_data(struct pvt_hwmon *pvt, enum pvt_sensor_type type,
> return -ETIMEDOUT;
>
> if (type == PVT_TEMP)
> - *val = pvt_calc_poly(&poly_N_to_temp, data);
> + *val = polynomial_calc(&poly_N_to_temp, data);
> else
> - *val = pvt_calc_poly(&poly_N_to_volt, data);
> + *val = polynomial_calc(&poly_N_to_volt, data);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 3/4] dt-bindings: hwmon: add Microchip LAN966x bindings
2022-03-26 19:23 [PATCH v1 0/4] hwmon: add lan9668 driver Michael Walle
2022-03-26 19:23 ` [PATCH v1 1/4] lib: add generic polynomial calculation Michael Walle
2022-03-26 19:23 ` [PATCH v1 2/4] hwmon: (bt1-pvt) use generic polynomial functions Michael Walle
@ 2022-03-26 19:23 ` Michael Walle
2022-03-27 11:17 ` Krzysztof Kozlowski
2022-03-26 19:23 ` [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC Michael Walle
3 siblings, 1 reply; 16+ messages in thread
From: Michael Walle @ 2022-03-26 19:23 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel, Michael Walle
Add a binding for the temperature sensor and the fan controller on the
Microchip LAN966x family.
Signed-off-by: Michael Walle <michael@walle.cc>
---
.../bindings/hwmon/microchip,lan966x.yaml | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,lan966x.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/microchip,lan966x.yaml b/Documentation/devicetree/bindings/hwmon/microchip,lan966x.yaml
new file mode 100644
index 000000000000..390dd6755ff5
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/microchip,lan966x.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/microchip,lan966x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip LAN966x Hardware Monitor
+
+maintainers:
+ - Michael Walle <michael@walle.cc>
+
+description: |
+ Microchip LAN966x temperature monitor and fan controller
+
+properties:
+ compatible:
+ enum:
+ - microchip,lan9668-hwmon
+
+ reg:
+ items:
+ - description: PVT registers
+ - description: FAN registers
+
+ reg-names:
+ items:
+ - const: pvt
+ - const: fan
+
+ clocks:
+ maxItems: 1
+
+ '#thermal-sensor-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ hwmon: hwmon@e2010180 {
+ compatible = "microchip,lan9668-hwmon";
+ reg = <0xe2010180 0xc>,
+ <0xe20042a8 0xc>;
+ reg-names = "pvt", "fan";
+ clocks = <&sys_clk>;
+ #thermal-sensor-cells = <0>;
+ };
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v1 3/4] dt-bindings: hwmon: add Microchip LAN966x bindings
2022-03-26 19:23 ` [PATCH v1 3/4] dt-bindings: hwmon: add Microchip LAN966x bindings Michael Walle
@ 2022-03-27 11:17 ` Krzysztof Kozlowski
0 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-27 11:17 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel
On 26/03/2022 20:23, Michael Walle wrote:
> Add a binding for the temperature sensor and the fan controller on the
> Microchip LAN966x family.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> .../bindings/hwmon/microchip,lan966x.yaml | 53 +++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,lan966x.yaml
>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-26 19:23 [PATCH v1 0/4] hwmon: add lan9668 driver Michael Walle
` (2 preceding siblings ...)
2022-03-26 19:23 ` [PATCH v1 3/4] dt-bindings: hwmon: add Microchip LAN966x bindings Michael Walle
@ 2022-03-26 19:23 ` Michael Walle
2022-03-27 1:34 ` Guenter Roeck
2022-03-27 3:13 ` kernel test robot
3 siblings, 2 replies; 16+ messages in thread
From: Michael Walle @ 2022-03-26 19:23 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel, Michael Walle
Add support for the temperatur sensor and the fan controller on the
Microchip LAN966x SoC. Apparently, an Analog Bits PVT sensor is used
which can measure temperature and process voltages. But only a forumlae
for the temperature sensor is known. Additionally, the SoC support a fan
tacho input as well as a PWM signal to control the fan.
Signed-off-by: Michael Walle <michael@walle.cc>
---
drivers/hwmon/Kconfig | 12 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/lan966x-hwmon.c | 384 ++++++++++++++++++++++++++++++++++
3 files changed, 397 insertions(+)
create mode 100644 drivers/hwmon/lan966x-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 68a8a27ab3b7..4df8521a6f9d 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -814,6 +814,18 @@ config SENSORS_POWR1220
This driver can also be built as a module. If so, the module
will be called powr1220.
+config SENSORS_LAN966X
+ tristate "Microchip LAN966x Hardware Monitoring"
+ depends on SOC_LAN966 || COMPILE_TEST
+ depends on REGMAP
+ select POLYNOMIAL
+ help
+ If you say yes here you get support for temperature monitoring
+ on the Microchip LAN966x SoC.
+
+ This driver can also be built as a module. If so, the module
+ will be called lan966x-hwmon.
+
config SENSORS_LINEAGE
tristate "Lineage Compact Power Line Power Entry Module"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8a03289e2aa4..51ca6956f8b7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_SENSORS_IT87) += it87.o
obj-$(CONFIG_SENSORS_JC42) += jc42.o
obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
obj-$(CONFIG_SENSORS_K10TEMP) += k10temp.o
+obj-$(CONFIG_SENSORS_LAN966X) += lan966x-hwmon.o
obj-$(CONFIG_SENSORS_LINEAGE) += lineage-pem.o
obj-$(CONFIG_SENSORS_LOCHNAGAR) += lochnagar-hwmon.o
obj-$(CONFIG_SENSORS_LM63) += lm63.o
diff --git a/drivers/hwmon/lan966x-hwmon.c b/drivers/hwmon/lan966x-hwmon.c
new file mode 100644
index 000000000000..e53b47f501ef
--- /dev/null
+++ b/drivers/hwmon/lan966x-hwmon.c
@@ -0,0 +1,384 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/polynomial.h>
+#include <linux/regmap.h>
+
+/*
+ * The original translation formulae of the temperature (in degrees of Celsius)
+ * are as follows:
+ *
+ * T = -3.4627e-11*(N^4) + 1.1023e-7*(N^3) + -1.9165e-4*(N^2) +
+ * 3.0604e-1*(N^1) + -5.6197e1
+ *
+ * where [-56.197, 136.402]C and N = [0, 1023].
+ *
+ * They must be accordingly altered to be suitable for the integer arithmetics.
+ * The technique is called 'factor redistribution', which just makes sure the
+ * multiplications and divisions are made so to have a result of the operations
+ * within the integer numbers limit. In addition we need to translate the
+ * formulae to accept millidegrees of Celsius. Here what it looks like after
+ * the alterations:
+ *
+ * T = -34627e-12*(N^4) + 110230e-9*(N^3) + -191650e-6*(N^2) +
+ * 306040e-3*(N^1) + -56197
+ *
+ * where T = [-56197, 136402]mC and N = [0, 1023].
+ */
+
+static const struct polynomial poly_N_to_temp = {
+ .terms = {
+ {4, -34627, 1000, 1},
+ {3, 110230, 1000, 1},
+ {2, -191650, 1000, 1},
+ {1, 306040, 1000, 1},
+ {0, -56197, 1, 1}
+ }
+};
+
+#define PVT_SENSOR_CTRL 0x0 /* unused */
+#define PVT_SENSOR_CFG 0x4
+#define SENSOR_CFG_CLK_CFG GENMASK(27, 20)
+#define SENSOR_CFG_TRIM_VAL GENMASK(13, 9)
+#define SENSOR_CFG_SAMPLE_ENA BIT(8)
+#define SENSOR_CFG_START_CAPTURE BIT(7)
+#define SENSOR_CFG_CONTINIOUS_MODE BIT(6)
+#define SENSOR_CFG_PSAMPLE_ENA GENMASK(1, 0)
+#define PVT_SENSOR_STAT 0x8
+#define SENSOR_STAT_DATA_VALID BIT(10)
+#define SENSOR_STAT_DATA GENMASK(9, 0)
+
+#define FAN_CFG 0x0
+#define FAN_CFG_DUTY_CYCLE GENMASK(23, 16)
+#define INV_POL BIT(3)
+#define GATE_ENA BIT(2)
+#define PWM_OPEN_COL_ENA BIT(1)
+#define FAN_STAT_CFG BIT(0)
+#define FAN_PWM_FREQ 0x4
+#define FAN_PWM_CYC_10US GENMASK(25, 15)
+#define FAN_PWM_FREQ_FREQ GENMASK(14, 0)
+#define FAN_CNT 0xc
+#define FAN_CNT_DATA GENMASK(15, 0)
+
+struct lan966x_hwmon {
+ struct regmap *regmap_pvt;
+ struct regmap *regmap_fan;
+ struct clk *clk;
+};
+
+static int lan966x_hwmon_read_temp(struct device *dev, long *val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+ unsigned int data;
+ int ret;
+
+ ret = regmap_read(hwmon->regmap_pvt, PVT_SENSOR_STAT, &data);
+ if (ret < 0)
+ return ret;
+
+ if (!(data & SENSOR_STAT_DATA_VALID))
+ return -EINVAL;
+
+ *val = polynomial_calc(&poly_N_to_temp,
+ FIELD_GET(SENSOR_STAT_DATA, data));
+
+ return 0;
+}
+
+static int lan966x_hwmon_read_fan(struct device *dev, long *val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+ unsigned int data;
+ int ret;
+
+ ret = regmap_read(hwmon->regmap_fan, FAN_CNT, &data);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Data is given in pulses per second. According to the hwmon ABI we
+ * have to assume two pulses per revolution.
+ */
+ *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
+
+ return 0;
+}
+
+static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+ unsigned int data;
+ int ret;
+
+ ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
+ if (ret < 0)
+ return ret;
+
+ *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
+
+ return 0;
+}
+
+static int lan966x_hwmon_read_pwm_freq(struct device *dev, long *val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+ unsigned long rate = clk_get_rate(hwmon->clk);
+ unsigned int data;
+ int ret;
+
+ ret = regmap_read(hwmon->regmap_fan, FAN_PWM_FREQ, &data);
+ if (ret < 0)
+ return ret;
+
+ data = FIELD_GET(FAN_PWM_FREQ_FREQ, data);
+ *val = DIV_ROUND_CLOSEST(rate, 256);
+ *val = DIV_ROUND_CLOSEST(*val, data + 1);
+
+ return 0;
+}
+
+static int lan966x_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+
+ switch (type) {
+ case hwmon_temp:
+ return lan966x_hwmon_read_temp(dev, val);
+ case hwmon_fan:
+ return lan966x_hwmon_read_fan(dev, val);
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return lan966x_hwmon_read_pwm(dev, val);
+ case hwmon_pwm_freq:
+ return lan966x_hwmon_read_pwm_freq(dev, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int lan966x_hwmon_write_pwm(struct device *dev, long val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+
+ if (val < 0 || val > 255)
+ return -EINVAL;
+
+ return regmap_update_bits(hwmon->regmap_fan, FAN_CFG,
+ FAN_CFG_DUTY_CYCLE,
+ FIELD_PREP(FAN_CFG_DUTY_CYCLE, val));
+}
+
+static int lan966x_hwmon_write_pwm_freq(struct device *dev, long val)
+{
+ struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
+ unsigned long rate = clk_get_rate(hwmon->clk);
+
+ val = DIV_ROUND_CLOSEST(rate, val);
+ val = DIV_ROUND_CLOSEST(val, 256) - 1;
+ val = clamp_val(val, 0, FAN_PWM_FREQ_FREQ);
+
+ return regmap_update_bits(hwmon->regmap_fan, FAN_PWM_FREQ,
+ FAN_PWM_FREQ_FREQ,
+ FIELD_PREP(FAN_PWM_FREQ_FREQ, val));
+}
+
+static int lan966x_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return lan966x_hwmon_write_pwm(dev, val);
+ case hwmon_pwm_freq:
+ return lan966x_hwmon_write_pwm_freq(dev, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t lan966x_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ umode_t mode = 0;
+
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_input:
+ mode = 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ mode = 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ case hwmon_pwm_freq:
+ mode = 0644;
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return mode;
+}
+
+
+static const struct hwmon_channel_info *lan966x_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_FREQ),
+ NULL
+};
+
+static const struct hwmon_ops lan966x_hwmon_ops = {
+ .is_visible = lan966x_hwmon_is_visible,
+ .read = lan966x_hwmon_read,
+ .write = lan966x_hwmon_write,
+};
+
+static const struct hwmon_chip_info lan966x_hwmon_chip_info = {
+ .ops = &lan966x_hwmon_ops,
+ .info = lan966x_hwmon_info,
+};
+
+static int lan966x_hwmon_enable(struct lan966x_hwmon *hwmon)
+{
+ unsigned int mask = SENSOR_CFG_SAMPLE_ENA |
+ SENSOR_CFG_START_CAPTURE |
+ SENSOR_CFG_CONTINIOUS_MODE |
+ SENSOR_CFG_PSAMPLE_ENA;
+ unsigned int val;
+
+ /* enable continuous mode */
+ val = SENSOR_CFG_SAMPLE_ENA | SENSOR_CFG_CONTINIOUS_MODE;
+
+ return regmap_update_bits(hwmon->regmap_pvt, PVT_SENSOR_CFG,
+ mask, val);
+}
+
+static struct regmap *lan966x_init_regmap(struct platform_device *pdev,
+ const char *name)
+{
+ struct regmap_config regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ };
+ void __iomem *base;
+
+ base = devm_platform_ioremap_resource_byname(pdev, name);
+ if (IS_ERR(base))
+ return base;
+
+ regmap_config.name = name;
+
+ return devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
+}
+
+static void lan966x_clk_disable(void *data)
+{
+ struct lan966x_hwmon *hwmon = data;
+
+ clk_disable_unprepare(hwmon->clk);
+}
+
+static int lan966x_clk_enable(struct device *dev, struct lan966x_hwmon *hwmon)
+{
+ int ret;
+
+ ret = clk_prepare_enable(hwmon->clk);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, lan966x_clk_disable, hwmon);
+}
+
+static int lan966x_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct lan966x_hwmon *hwmon;
+ struct device *hwmon_dev;
+ int ret;
+
+ hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return -ENOMEM;
+
+ hwmon->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(hwmon->clk))
+ return dev_err_probe(dev, PTR_ERR(hwmon->clk),
+ "failed to get clock\n");
+
+ ret = lan966x_clk_enable(dev, hwmon);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable clock\n");
+
+ hwmon->regmap_pvt = lan966x_init_regmap(pdev, "pvt");
+ if (IS_ERR(hwmon->regmap_pvt))
+ return dev_err_probe(dev, PTR_ERR(hwmon->regmap_pvt),
+ "failed to get regmap for PVT registers\n");
+
+ hwmon->regmap_fan = lan966x_init_regmap(pdev, "fan");
+ if (IS_ERR(hwmon->regmap_fan))
+ return dev_err_probe(dev, PTR_ERR(hwmon->regmap_fan),
+ "failed to get regmap for fan registers\n");
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+ "lan966x_hwmon", hwmon,
+ &lan966x_hwmon_chip_info, NULL);
+ if (IS_ERR(hwmon_dev))
+ return dev_err_probe(dev, PTR_ERR(hwmon_dev),
+ "failed to register hwmon device\n");
+
+ return lan966x_hwmon_enable(hwmon);
+}
+
+static const struct of_device_id lan966x_hwmon_of_match[] = {
+ { .compatible = "microchip,lan9668-hwmon" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, lan966x_hwmon_of_match);
+
+static struct platform_driver lan966x_hwmon_driver = {
+ .probe = lan966x_hwmon_probe,
+ .driver = {
+ .name = "lan966x-hwmon",
+ .of_match_table = lan966x_hwmon_of_match,
+ },
+};
+module_platform_driver(lan966x_hwmon_driver);
+
+MODULE_DESCRIPTION("LAN966x Hardware Monitoring Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-26 19:23 ` [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC Michael Walle
@ 2022-03-27 1:34 ` Guenter Roeck
2022-03-27 14:18 ` Michael Walle
2022-03-27 3:13 ` kernel test robot
1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2022-03-27 1:34 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Rob Herring, Krzysztof Kozlowski
Cc: linux-hwmon, devicetree, linux-kernel
On 3/26/22 12:23, Michael Walle wrote:
> Add support for the temperatur sensor and the fan controller on the
> Microchip LAN966x SoC. Apparently, an Analog Bits PVT sensor is used
> which can measure temperature and process voltages. But only a forumlae
> for the temperature sensor is known. Additionally, the SoC support a fan
> tacho input as well as a PWM signal to control the fan.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> drivers/hwmon/Kconfig | 12 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/lan966x-hwmon.c | 384 ++++++++++++++++++++++++++++++++++
Documentation missing
> 3 files changed, 397 insertions(+)
> create mode 100644 drivers/hwmon/lan966x-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 68a8a27ab3b7..4df8521a6f9d 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -814,6 +814,18 @@ config SENSORS_POWR1220
> This driver can also be built as a module. If so, the module
> will be called powr1220.
>
> +config SENSORS_LAN966X
> + tristate "Microchip LAN966x Hardware Monitoring"
> + depends on SOC_LAN966 || COMPILE_TEST
> + depends on REGMAP
> + select POLYNOMIAL
> + help
> + If you say yes here you get support for temperature monitoring
> + on the Microchip LAN966x SoC.
> +
> + This driver can also be built as a module. If so, the module
> + will be called lan966x-hwmon.
> +
> config SENSORS_LINEAGE
> tristate "Lineage Compact Power Line Power Entry Module"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8a03289e2aa4..51ca6956f8b7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_SENSORS_IT87) += it87.o
> obj-$(CONFIG_SENSORS_JC42) += jc42.o
> obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
> obj-$(CONFIG_SENSORS_K10TEMP) += k10temp.o
> +obj-$(CONFIG_SENSORS_LAN966X) += lan966x-hwmon.o
> obj-$(CONFIG_SENSORS_LINEAGE) += lineage-pem.o
> obj-$(CONFIG_SENSORS_LOCHNAGAR) += lochnagar-hwmon.o
> obj-$(CONFIG_SENSORS_LM63) += lm63.o
> diff --git a/drivers/hwmon/lan966x-hwmon.c b/drivers/hwmon/lan966x-hwmon.c
> new file mode 100644
> index 000000000000..e53b47f501ef
> --- /dev/null
> +++ b/drivers/hwmon/lan966x-hwmon.c
> @@ -0,0 +1,384 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/polynomial.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * The original translation formulae of the temperature (in degrees of Celsius)
> + * are as follows:
> + *
> + * T = -3.4627e-11*(N^4) + 1.1023e-7*(N^3) + -1.9165e-4*(N^2) +
> + * 3.0604e-1*(N^1) + -5.6197e1
> + *
> + * where [-56.197, 136.402]C and N = [0, 1023].
> + *
> + * They must be accordingly altered to be suitable for the integer arithmetics.
> + * The technique is called 'factor redistribution', which just makes sure the
> + * multiplications and divisions are made so to have a result of the operations
> + * within the integer numbers limit. In addition we need to translate the
> + * formulae to accept millidegrees of Celsius. Here what it looks like after
> + * the alterations:
> + *
> + * T = -34627e-12*(N^4) + 110230e-9*(N^3) + -191650e-6*(N^2) +
> + * 306040e-3*(N^1) + -56197
> + *
> + * where T = [-56197, 136402]mC and N = [0, 1023].
> + */
> +
> +static const struct polynomial poly_N_to_temp = {
> + .terms = {
> + {4, -34627, 1000, 1},
> + {3, 110230, 1000, 1},
> + {2, -191650, 1000, 1},
> + {1, 306040, 1000, 1},
> + {0, -56197, 1, 1}
> + }
> +};
> +
> +#define PVT_SENSOR_CTRL 0x0 /* unused */
> +#define PVT_SENSOR_CFG 0x4
> +#define SENSOR_CFG_CLK_CFG GENMASK(27, 20)
> +#define SENSOR_CFG_TRIM_VAL GENMASK(13, 9)
> +#define SENSOR_CFG_SAMPLE_ENA BIT(8)
> +#define SENSOR_CFG_START_CAPTURE BIT(7)
> +#define SENSOR_CFG_CONTINIOUS_MODE BIT(6)
> +#define SENSOR_CFG_PSAMPLE_ENA GENMASK(1, 0)
> +#define PVT_SENSOR_STAT 0x8
> +#define SENSOR_STAT_DATA_VALID BIT(10)
> +#define SENSOR_STAT_DATA GENMASK(9, 0)
> +
> +#define FAN_CFG 0x0
> +#define FAN_CFG_DUTY_CYCLE GENMASK(23, 16)
> +#define INV_POL BIT(3)
> +#define GATE_ENA BIT(2)
> +#define PWM_OPEN_COL_ENA BIT(1)
> +#define FAN_STAT_CFG BIT(0)
> +#define FAN_PWM_FREQ 0x4
> +#define FAN_PWM_CYC_10US GENMASK(25, 15)
> +#define FAN_PWM_FREQ_FREQ GENMASK(14, 0)
> +#define FAN_CNT 0xc
> +#define FAN_CNT_DATA GENMASK(15, 0)
> +
> +struct lan966x_hwmon {
> + struct regmap *regmap_pvt;
> + struct regmap *regmap_fan;
> + struct clk *clk;
> +};
> +
> +static int lan966x_hwmon_read_temp(struct device *dev, long *val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> + unsigned int data;
> + int ret;
> +
> + ret = regmap_read(hwmon->regmap_pvt, PVT_SENSOR_STAT, &data);
> + if (ret < 0)
> + return ret;
> +
> + if (!(data & SENSOR_STAT_DATA_VALID))
> + return -EINVAL;
-ENODATA. The user did not do anything wrong.
> +
> + *val = polynomial_calc(&poly_N_to_temp,
> + FIELD_GET(SENSOR_STAT_DATA, data));
> +
> + return 0;
> +}
> +
> +static int lan966x_hwmon_read_fan(struct device *dev, long *val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> + unsigned int data;
> + int ret;
> +
> + ret = regmap_read(hwmon->regmap_fan, FAN_CNT, &data);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * Data is given in pulses per second. According to the hwmon ABI we
> + * have to assume two pulses per revolution.
The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
that is all. Pulses per revolution is a fan property.
> + */
> + *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
> +
> + return 0;
> +}
> +
> +static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> + unsigned int data;
> + int ret;
> +
> + ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
> + if (ret < 0)
> + return ret;
> +
> + *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
> +
> + return 0;
> +}
> +
> +static int lan966x_hwmon_read_pwm_freq(struct device *dev, long *val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> + unsigned long rate = clk_get_rate(hwmon->clk);
Is that a dynamic frequency ? If not, it would be better to read it once
and store it in struct lan966x_hwmon.
> + unsigned int data;
> + int ret;
> +
> + ret = regmap_read(hwmon->regmap_fan, FAN_PWM_FREQ, &data);
> + if (ret < 0)
> + return ret;
> +
> + data = FIELD_GET(FAN_PWM_FREQ_FREQ, data);
> + *val = DIV_ROUND_CLOSEST(rate, 256);
The result of above operation should be stored in a temporary variable.
> + *val = DIV_ROUND_CLOSEST(*val, data + 1);
> +
> + return 0;
> +}
> +
> +static int lan966x_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> +
> + switch (type) {
> + case hwmon_temp:
> + return lan966x_hwmon_read_temp(dev, val);
> + case hwmon_fan:
> + return lan966x_hwmon_read_fan(dev, val);
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + return lan966x_hwmon_read_pwm(dev, val);
> + case hwmon_pwm_freq:
> + return lan966x_hwmon_read_pwm_freq(dev, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int lan966x_hwmon_write_pwm(struct device *dev, long val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> +
> + if (val < 0 || val > 255)
> + return -EINVAL;
> +
> + return regmap_update_bits(hwmon->regmap_fan, FAN_CFG,
> + FAN_CFG_DUTY_CYCLE,
> + FIELD_PREP(FAN_CFG_DUTY_CYCLE, val));
> +}
> +
> +static int lan966x_hwmon_write_pwm_freq(struct device *dev, long val)
> +{
> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
> + unsigned long rate = clk_get_rate(hwmon->clk);
> +
> + val = DIV_ROUND_CLOSEST(rate, val);
> + val = DIV_ROUND_CLOSEST(val, 256) - 1;
> + val = clamp_val(val, 0, FAN_PWM_FREQ_FREQ);
> +
> + return regmap_update_bits(hwmon->regmap_fan, FAN_PWM_FREQ,
> + FAN_PWM_FREQ_FREQ,
> + FIELD_PREP(FAN_PWM_FREQ_FREQ, val));
> +}
> +
> +static int lan966x_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + return lan966x_hwmon_write_pwm(dev, val);
> + case hwmon_pwm_freq:
> + return lan966x_hwmon_write_pwm_freq(dev, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static umode_t lan966x_hwmon_is_visible(const void *data,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + umode_t mode = 0;
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_input:
> + mode = 0444;
> + break;
> + default:
> + break;
> + }
> + break;
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + mode = 0444;
> + break;
> + default:
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + case hwmon_pwm_freq:
> + mode = 0644;
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return mode;
> +}
> +
> +
> +static const struct hwmon_channel_info *lan966x_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_FREQ),
> + NULL
> +};
> +
> +static const struct hwmon_ops lan966x_hwmon_ops = {
> + .is_visible = lan966x_hwmon_is_visible,
> + .read = lan966x_hwmon_read,
> + .write = lan966x_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info lan966x_hwmon_chip_info = {
> + .ops = &lan966x_hwmon_ops,
> + .info = lan966x_hwmon_info,
> +};
> +
> +static int lan966x_hwmon_enable(struct lan966x_hwmon *hwmon)
> +{
> + unsigned int mask = SENSOR_CFG_SAMPLE_ENA |
> + SENSOR_CFG_START_CAPTURE |
> + SENSOR_CFG_CONTINIOUS_MODE |
> + SENSOR_CFG_PSAMPLE_ENA;
> + unsigned int val;
> +
> + /* enable continuous mode */
> + val = SENSOR_CFG_SAMPLE_ENA | SENSOR_CFG_CONTINIOUS_MODE;
> +
> + return regmap_update_bits(hwmon->regmap_pvt, PVT_SENSOR_CFG,
> + mask, val);
> +}
> +
> +static struct regmap *lan966x_init_regmap(struct platform_device *pdev,
> + const char *name)
> +{
> + struct regmap_config regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + };
> + void __iomem *base;
> +
> + base = devm_platform_ioremap_resource_byname(pdev, name);
> + if (IS_ERR(base))
> + return base;
> +
> + regmap_config.name = name;
> +
> + return devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
> +}
> +
> +static void lan966x_clk_disable(void *data)
> +{
> + struct lan966x_hwmon *hwmon = data;
> +
> + clk_disable_unprepare(hwmon->clk);
> +}
> +
> +static int lan966x_clk_enable(struct device *dev, struct lan966x_hwmon *hwmon)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(hwmon->clk);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, lan966x_clk_disable, hwmon);
> +}
> +
> +static int lan966x_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct lan966x_hwmon *hwmon;
> + struct device *hwmon_dev;
> + int ret;
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> +
> + hwmon->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(hwmon->clk))
> + return dev_err_probe(dev, PTR_ERR(hwmon->clk),
> + "failed to get clock\n");
> +
> + ret = lan966x_clk_enable(dev, hwmon);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable clock\n");
> +
> + hwmon->regmap_pvt = lan966x_init_regmap(pdev, "pvt");
> + if (IS_ERR(hwmon->regmap_pvt))
> + return dev_err_probe(dev, PTR_ERR(hwmon->regmap_pvt),
> + "failed to get regmap for PVT registers\n");
> +
> + hwmon->regmap_fan = lan966x_init_regmap(pdev, "fan");
> + if (IS_ERR(hwmon->regmap_fan))
> + return dev_err_probe(dev, PTR_ERR(hwmon->regmap_fan),
> + "failed to get regmap for fan registers\n");
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> + "lan966x_hwmon", hwmon,
> + &lan966x_hwmon_chip_info, NULL);
> + if (IS_ERR(hwmon_dev))
> + return dev_err_probe(dev, PTR_ERR(hwmon_dev),
> + "failed to register hwmon device\n");
> +
> + return lan966x_hwmon_enable(hwmon);
> +}
> +
> +static const struct of_device_id lan966x_hwmon_of_match[] = {
> + { .compatible = "microchip,lan9668-hwmon" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lan966x_hwmon_of_match);
> +
> +static struct platform_driver lan966x_hwmon_driver = {
> + .probe = lan966x_hwmon_probe,
> + .driver = {
> + .name = "lan966x-hwmon",
> + .of_match_table = lan966x_hwmon_of_match,
> + },
> +};
> +module_platform_driver(lan966x_hwmon_driver);
> +
> +MODULE_DESCRIPTION("LAN966x Hardware Monitoring Driver");
> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-27 1:34 ` Guenter Roeck
@ 2022-03-27 14:18 ` Michael Walle
2022-03-27 18:22 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Michael Walle @ 2022-03-27 14:18 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, linux-hwmon,
devicetree, linux-kernel
Am 2022-03-27 03:34, schrieb Guenter Roeck:
>> + /*
>> + * Data is given in pulses per second. According to the hwmon ABI we
>> + * have to assume two pulses per revolution.
>
> The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
> that is all. Pulses per revolution is a fan property.
There is fanY_pulses according to
Documentation/ABI/testing/sysfs-class-hwmon:
Should only be created if the chip has a register to configure
the number of pulses. In the absence of such a register (and
thus attribute) the value assumed by all devices is 2 pulses
per fan revolution.
The hardware returns just the pulses per second. Doesn't that
mean I have to divide that value by two?
>> + */
>> + *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
.. otherwise this should then be
*val = FIELD_GET(FAN_CNT_DATA, data) * 60;
>> +
>> + return 0;
>> +}
>> +
>> +static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
>> +{
>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>> + unsigned int data;
>> + int ret;
>> +
>> + ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
>> +
>> + return 0;
>> +}
>> +
>> +static int lan966x_hwmon_read_pwm_freq(struct device *dev, long *val)
>> +{
>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>> + unsigned long rate = clk_get_rate(hwmon->clk);
>
> Is that a dynamic frequency ? If not, it would be better to read it
> once
> and store it in struct lan966x_hwmon.
yes it is configurable, actually. See lan966x_hwmon_write_pwm_freq().
-michael
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-27 14:18 ` Michael Walle
@ 2022-03-27 18:22 ` Guenter Roeck
2022-03-28 11:28 ` Michael Walle
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2022-03-27 18:22 UTC (permalink / raw)
To: Michael Walle
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, linux-hwmon,
devicetree, linux-kernel
On 3/27/22 07:18, Michael Walle wrote:
> Am 2022-03-27 03:34, schrieb Guenter Roeck:
>
>>> + /*
>>> + * Data is given in pulses per second. According to the hwmon ABI we
>>> + * have to assume two pulses per revolution.
>>
>> The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
>> that is all. Pulses per revolution is a fan property.
>
> There is fanY_pulses according to Documentation/ABI/testing/sysfs-class-hwmon:
>
> Should only be created if the chip has a register to configure
> the number of pulses. In the absence of such a register (and
> thus attribute) the value assumed by all devices is 2 pulses
> per fan revolution.
>
> The hardware returns just the pulses per second. Doesn't that
> mean I have to divide that value by two?
>
The above refers to hardware which reports RPM.
It is up to the driver to calculate and return RPM. How you do it is your
decision. Drivers should report the most likely correct RPM value to
userspace, one that rarely needs manual adjustment. Almost all fans
report two pulses per revolution, so normally that assumption is used
to convert PPM to RPM. That isn't mandated (or supposed to be mandated)
by the ABI. I would call it common sense.
I'll be happy to accept a patch clarifying this.
>>> + */
>>> + *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
>
> .. otherwise this should then be
> *val = FIELD_GET(FAN_CNT_DATA, data) * 60;
>
If you really want to do this, make sure it is well documented that users
will need to adjust the fan speed via sensors3.conf to get the real fan speed.
>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
>>> +{
>>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>> + unsigned int data;
>>> + int ret;
>>> +
>>> + ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int lan966x_hwmon_read_pwm_freq(struct device *dev, long *val)
>>> +{
>>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>> + unsigned long rate = clk_get_rate(hwmon->clk);
>>
>> Is that a dynamic frequency ? If not, it would be better to read it once
>> and store it in struct lan966x_hwmon.
>
> yes it is configurable, actually. See lan966x_hwmon_write_pwm_freq().
>
That is the pwm frequency, not the clock frequency. I don't see any
code which updates the clock frequency reported by clk_get_rate(hwmon->clk),
ie I don't see a call to clk_set_rate().
Thanks,
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-27 18:22 ` Guenter Roeck
@ 2022-03-28 11:28 ` Michael Walle
2022-03-28 16:37 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Michael Walle @ 2022-03-28 11:28 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, linux-hwmon,
devicetree, linux-kernel
Am 2022-03-27 20:22, schrieb Guenter Roeck:
> On 3/27/22 07:18, Michael Walle wrote:
>> Am 2022-03-27 03:34, schrieb Guenter Roeck:
>>
>>>> + /*
>>>> + * Data is given in pulses per second. According to the hwmon
>>>> ABI we
>>>> + * have to assume two pulses per revolution.
>>>
>>> The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
>>> that is all. Pulses per revolution is a fan property.
>>
>> There is fanY_pulses according to
>> Documentation/ABI/testing/sysfs-class-hwmon:
>>
>> Should only be created if the chip has a register to configure
>> the number of pulses. In the absence of such a register (and
>> thus attribute) the value assumed by all devices is 2 pulses
>> per fan revolution.
>>
>> The hardware returns just the pulses per second. Doesn't that
>> mean I have to divide that value by two?
>>
>
> The above refers to hardware which reports RPM.
>
> It is up to the driver to calculate and return RPM. How you do it is
> your
> decision. Drivers should report the most likely correct RPM value to
> userspace, one that rarely needs manual adjustment. Almost all fans
> report two pulses per revolution, so normally that assumption is used
> to convert PPM to RPM. That isn't mandated (or supposed to be mandated)
> by the ABI. I would call it common sense.
>
> I'll be happy to accept a patch clarifying this.
Where would that go? into the sysfs abi description of the
fanY_input?
>>>> + */
>>>> + *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
>>
>> .. otherwise this should then be
>> *val = FIELD_GET(FAN_CNT_DATA, data) * 60;
>>
>
> If you really want to do this, make sure it is well documented that
> users
> will need to adjust the fan speed via sensors3.conf to get the real fan
> speed.
>
>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
>>>> +{
>>>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>>> + unsigned int data;
>>>> + int ret;
>>>> +
>>>> + ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int lan966x_hwmon_read_pwm_freq(struct device *dev, long
>>>> *val)
>>>> +{
>>>> + struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>>> + unsigned long rate = clk_get_rate(hwmon->clk);
>>>
>>> Is that a dynamic frequency ? If not, it would be better to read it
>>> once
>>> and store it in struct lan966x_hwmon.
>>
>> yes it is configurable, actually. See lan966x_hwmon_write_pwm_freq().
>>
>
> That is the pwm frequency, not the clock frequency. I don't see any
> code which updates the clock frequency reported by
> clk_get_rate(hwmon->clk),
> ie I don't see a call to clk_set_rate().
Ahh sorry, missunderstood you. Yeah, in v2 it will be read
during probe.
-michael
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-28 11:28 ` Michael Walle
@ 2022-03-28 16:37 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2022-03-28 16:37 UTC (permalink / raw)
To: Michael Walle
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, linux-hwmon,
devicetree, linux-kernel
On 3/28/22 04:28, Michael Walle wrote:
> Am 2022-03-27 20:22, schrieb Guenter Roeck:
>> On 3/27/22 07:18, Michael Walle wrote:
>>> Am 2022-03-27 03:34, schrieb Guenter Roeck:
>>>
>>>>> + /*
>>>>> + * Data is given in pulses per second. According to the hwmon ABI we
>>>>> + * have to assume two pulses per revolution.
>>>>
>>>> The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
>>>> that is all. Pulses per revolution is a fan property.
>>>
>>> There is fanY_pulses according to Documentation/ABI/testing/sysfs-class-hwmon:
>>>
>>> Should only be created if the chip has a register to configure
>>> the number of pulses. In the absence of such a register (and
>>> thus attribute) the value assumed by all devices is 2 pulses
>>> per fan revolution.
>>>
>>> The hardware returns just the pulses per second. Doesn't that
>>> mean I have to divide that value by two?
>>>
>>
>> The above refers to hardware which reports RPM.
>>
>> It is up to the driver to calculate and return RPM. How you do it is your
>> decision. Drivers should report the most likely correct RPM value to
>> userspace, one that rarely needs manual adjustment. Almost all fans
>> report two pulses per revolution, so normally that assumption is used
>> to convert PPM to RPM. That isn't mandated (or supposed to be mandated)
>> by the ABI. I would call it common sense.
>>
>> I'll be happy to accept a patch clarifying this.
>
> Where would that go? into the sysfs abi description of the
> fanY_input?
>
For example.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC
2022-03-26 19:23 ` [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC Michael Walle
2022-03-27 1:34 ` Guenter Roeck
@ 2022-03-27 3:13 ` kernel test robot
1 sibling, 0 replies; 16+ messages in thread
From: kernel test robot @ 2022-03-27 3:13 UTC (permalink / raw)
To: Michael Walle, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski
Cc: kbuild-all, linux-hwmon, devicetree, linux-kernel, Michael Walle
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: sparc64-randconfig-s031-20220327 (https://download.01.org/0day-ci/archive/20220327/202203271141.S44Wx3yF-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/intel-lab-lkp/linux/commit/cfef456b1f1b1ab545a03f098e209aff8ae507b7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Walle/hwmon-add-lan9668-driver/20220327-032606
git checkout cfef456b1f1b1ab545a03f098e209aff8ae507b7
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/hwmon/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/lan966x-hwmon.c:302:24: sparse: sparse: incorrect type in return expression (different address spaces) @@ expected struct regmap * @@ got void [noderef] __iomem *[assigned] base @@
drivers/hwmon/lan966x-hwmon.c:302:24: sparse: expected struct regmap *
drivers/hwmon/lan966x-hwmon.c:302:24: sparse: got void [noderef] __iomem *[assigned] base
vim +302 drivers/hwmon/lan966x-hwmon.c
289
290 static struct regmap *lan966x_init_regmap(struct platform_device *pdev,
291 const char *name)
292 {
293 struct regmap_config regmap_config = {
294 .reg_bits = 32,
295 .reg_stride = 4,
296 .val_bits = 32,
297 };
298 void __iomem *base;
299
300 base = devm_platform_ioremap_resource_byname(pdev, name);
301 if (IS_ERR(base))
> 302 return base;
303
304 regmap_config.name = name;
305
306 return devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
307 }
308
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 16+ messages in thread