* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
@ 2023-03-15 13:11 ` Krzysztof Kozlowski
2023-03-15 14:02 ` Phinex Hung
2023-03-15 14:16 ` Guenter Roeck
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-15 13:11 UTC (permalink / raw)
To: phinex, jdelvare; +Cc: linux, linux-hwmon, linux-kernel
On 15/03/2023 13:16, phinex wrote:
> Support thermal zone so that we can just rely on dts to describe a
> thermal zone and do the cooling operations.
>
> You can define a comptible string "drivetemp,hdd-sensors" to enable
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.
You missed also DT list, which means this won't be tested by tools.
> this, such as
>
> sata_port0: sata-port@0 {
> compatible = "drivetemp,hdd-sensors";
> #thermal-sensor-cells = <0>;
> }
>
> Then define a thermal with this sensor to get it work.
>
> hdd_thermal: hdd-thermal {
> thermal-sensors = <&sata_port0>;
> }
>
> In most of the SoC systems, using dts to handle cooling is common.
> This can eliminate the usage of user space application to check
> the value exported in hwmon and then through sysfs to cooling.
>
> Signed-off-by: phinex <phinex@realtek.com>
Is phinex your full name or email alias?
>
> ---
> .../bindings/hwmon/drivetemp,hdd-sensors.yaml | 35 ++++++
Bindings are always separate.
> drivers/hwmon/drivetemp.c | 102 +++++++++++++++++-
> 2 files changed, 133 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> new file mode 100644
> index 000000000000..939d7a923e94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/drivetemp,hdd-sensors.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Drivetemp Temperature Monitor
> +
> +maintainers:
> + - Phinex <phinex@realtek.com>
> +
> +description: |
> + Drivetemp Temperature Monitor that support a single thermal zone
> + This single thermal zone can support multiple hard drives,
> + it uses maximal temperature of these hard drivers as its temp value.
> +
> +properties:
> + compatible:
> + enum:
> + - drivetemp,hdd-sensors
No such vendor prefix. Are you sure you are describing real hardware?
Also device specific part looks too generic. What device is it exactly?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 13:11 ` Krzysztof Kozlowski
@ 2023-03-15 14:02 ` Phinex Hung
2023-03-15 14:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-15 14:02 UTC (permalink / raw)
To: Krzysztof Kozlowski, jdelvare; +Cc: linux, linux-hwmon, linux-kernel
On 15/03/2023 21:11, "Krzysztof Kozlowski" <krzk@kernel.org <mailto:krzk@kernel.org>> wrote:
Hi Krzysztof,
Thanks for your comment.
>Please use scripts/get_maintainers.pl to get a list of necessary people
>and lists to CC. It might happen, that command when run on an older
>kernel, gives you outdated entries. Therefore please be sure you base
>your patches on recent Linux kernel.
I am working on git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
For master branch.
Is there any updated git repo that I should rebase my patch?
>Bindings are always separate.
Would separate in the next patch.
>No such vendor prefix. Are you sure you are describing real hardware?
>Also device specific part looks too generic. What device is it exactly?
It's a generic patch, just want to support thermal zone using a simple device tree.
Any SoC with SCSI interface and attached hard drives can benefit this change.
Normally a THERMAL_OF require a platform device to add a thermal zone.
Original drivetemp.c works quite well as a hwmon device, but no thermal zone support.
My patch just extend its capability to support a simple thermal zone created with this hwmon.
This is bind to any specific vendor, just to provide an easy way to handle cooling when hard drives' temperature gets high.
Best Regards,
Phinex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 14:02 ` Phinex Hung
@ 2023-03-15 14:06 ` Krzysztof Kozlowski
0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-15 14:06 UTC (permalink / raw)
To: Phinex Hung, jdelvare; +Cc: linux, linux-hwmon, linux-kernel
On 15/03/2023 15:02, Phinex Hung wrote:
>
>
> On 15/03/2023 21:11, "Krzysztof Kozlowski" <krzk@kernel.org <mailto:krzk@kernel.org>> wrote:
>
> Hi Krzysztof,
>
> Thanks for your comment.
>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
>
> I am working on git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
>
> For master branch.
>
> Is there any updated git repo that I should rebase my patch?
The mainline master. But even on that kernel you would get different
people to CC, so you did not run the command to get proper addresses.
>
>> No such vendor prefix. Are you sure you are describing real hardware?
>> Also device specific part looks too generic. What device is it exactly?
>
> It's a generic patch, just want to support thermal zone using a simple device tree.
> Any SoC with SCSI interface and attached hard drives can benefit this change.
Devicetree describes hardware, not generic patches. Please be more specific.
>
> Normally a THERMAL_OF require a platform device to add a thermal zone.
Whatever problem is (or is not) with THERMAL_OF does not really matter
to bindings.
>
> Original drivetemp.c works quite well as a hwmon device, but no thermal zone support.
>
> My patch just extend its capability to support a simple thermal zone created with this hwmon.
I understand. Still it is not describing any hardware. In all your
statements above you did not reference any specific devices or hardware
at all.
> This is bind to any specific vendor, just to provide an easy way to handle cooling when hard drives' temperature gets high.
Then either you need something more specific to hardware or Devicetree
is not the place to put it.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
2023-03-15 13:11 ` Krzysztof Kozlowski
@ 2023-03-15 14:16 ` Guenter Roeck
2023-03-15 15:06 ` kernel test robot
` (4 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2023-03-15 14:16 UTC (permalink / raw)
To: phinex, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/15/23 05:16, phinex wrote:
> Support thermal zone so that we can just rely on dts to describe a
> thermal zone and do the cooling operations.
>
> You can define a comptible string "drivetemp,hdd-sensors" to enable
> this, such as
>
> sata_port0: sata-port@0 {
> compatible = "drivetemp,hdd-sensors";
> #thermal-sensor-cells = <0>;
> }
>
> Then define a thermal with this sensor to get it work.
>
> hdd_thermal: hdd-thermal {
> thermal-sensors = <&sata_port0>;
> }
>
> In most of the SoC systems, using dts to handle cooling is common.
> This can eliminate the usage of user space application to check
> the value exported in hwmon and then through sysfs to cooling.
>
> Signed-off-by: phinex <phinex@realtek.com>
>
The driver registers drivetemp instances with the hwmon core using
HWMON_C_REGISTER_TZ. That means there should already be a thermal zone
for each instance. If that doesn't work, please find out why and fix it
instead of replicating what the hwmon core already is supposed to be
doing.
Guenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
2023-03-15 13:11 ` Krzysztof Kozlowski
2023-03-15 14:16 ` Guenter Roeck
@ 2023-03-15 15:06 ` kernel test robot
2023-03-15 15:36 ` Guenter Roeck
` (3 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2023-03-15 15:06 UTC (permalink / raw)
To: phinex, jdelvare; +Cc: oe-kbuild-all, linux, linux-hwmon, linux-kernel, phinex
Hi phinex,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.3-rc2 next-20230315]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20230315121606.GA71707%40threadripper
patch subject: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230315/202303152257.cVusXid9-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.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/1c53b683440a584685795fa8ff831379577081b0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
git checkout 1c53b683440a584685795fa8ff831379577081b0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303152257.cVusXid9-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/hwmon/drivetemp.c: In function 'hdd_read_temp':
>> drivers/hwmon/drivetemp.c:551:13: warning: variable 'err' set but not used [-Wunused-but-set-variable]
551 | int err;
| ^~~
drivers/hwmon/drivetemp.c: At top level:
>> drivers/hwmon/drivetemp.c:575:21: error: variable 'hdd_sensor_ops' has initializer but incomplete type
575 | static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/hwmon/drivetemp.c:576:10: error: 'const struct thermal_zone_of_device_ops' has no member named 'get_temp'
576 | .get_temp = hdd_read_temp,
| ^~~~~~~~
>> drivers/hwmon/drivetemp.c:576:21: warning: excess elements in struct initializer
576 | .get_temp = hdd_read_temp,
| ^~~~~~~~~~~~~
drivers/hwmon/drivetemp.c:576:21: note: (near initialization for 'hdd_sensor_ops')
drivers/hwmon/drivetemp.c: In function 'hdd_hwmon_probe':
>> drivers/hwmon/drivetemp.c:668:17: error: implicit declaration of function 'devm_thermal_zone_of_sensor_register'; did you mean 'devm_thermal_of_zone_register'? [-Werror=implicit-function-declaration]
668 | devm_thermal_zone_of_sensor_register(
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| devm_thermal_of_zone_register
drivers/hwmon/drivetemp.c: At top level:
>> drivers/hwmon/drivetemp.c:575:48: error: storage size of 'hdd_sensor_ops' isn't known
575 | static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/hdd_sensor_ops +575 drivers/hwmon/drivetemp.c
547
548 #if IS_ENABLED(CONFIG_THERMAL_OF)
549 static int hdd_read_temp(void *data, int *temp)
550 {
> 551 int err;
552 struct drivetemp_data *st = data;
553 long value, max = 0;
554
555 list_for_each_entry(st, &drivetemp_devlist, list) {
556 mutex_lock(&st->lock);
557 err = st->get_temp(st, hwmon_temp_input, &value);
558 mutex_unlock(&st->lock);
559
560 if (value > max)
561 max = value;
562 }
563
564 /*non-existent sensor or not ready*/
565 if (max == 0)
566 return -EAGAIN;
567
568 *temp = (int)max;
569
570 dev_dbg(st->dev, "%s, sensor read %d\n", __func__, *temp);
571
572 return 0;
573 }
574
> 575 static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
> 576 .get_temp = hdd_read_temp,
577 };
578
579 static const struct of_device_id hdd_of_match[] = {
580 {
581 .compatible = "drivetemp,hdd-sensors",
582 },
583 {},
584 };
585 MODULE_DEVICE_TABLE(of, hdd_of_match);
586 #endif
587
588 static const struct hwmon_ops drivetemp_ops = {
589 .is_visible = drivetemp_is_visible,
590 .read = drivetemp_read,
591 };
592
593 static const struct hwmon_chip_info drivetemp_chip_info = {
594 .ops = &drivetemp_ops,
595 .info = drivetemp_info,
596 };
597
598 /*
599 * The device argument points to sdev->sdev_dev. Its parent is
600 * sdev->sdev_gendev, which we can use to get the scsi_device pointer.
601 */
602 static int drivetemp_add(struct device *dev, struct class_interface *intf)
603 {
604 struct scsi_device *sdev = to_scsi_device(dev->parent);
605 struct drivetemp_data *st;
606 int err;
607 struct ata_port *ap;
608
609 st = kzalloc(sizeof(*st), GFP_KERNEL);
610 if (!st)
611 return -ENOMEM;
612
613 ap = ata_shost_to_port(sdev->host);
614
615 snprintf(st->drivename, MAX_NAME_LEN, "drivetemp_port%d", ap->port_no);
616
617 st->sdev = sdev;
618 st->dev = dev;
619 mutex_init(&st->lock);
620
621 if (drivetemp_identify(st)) {
622 err = -ENODEV;
623 goto abort;
624 }
625
626 st->hwdev = hwmon_device_register_with_info(
627 dev->parent, st->drivename, st, &drivetemp_chip_info, NULL);
628
629 if (IS_ERR(st->hwdev)) {
630 err = PTR_ERR(st->hwdev);
631 goto abort;
632 }
633
634 list_add(&st->list, &drivetemp_devlist);
635 return 0;
636
637 abort:
638 kfree(st);
639 return err;
640 }
641
642 static void drivetemp_remove(struct device *dev, struct class_interface *intf)
643 {
644 struct drivetemp_data *st, *tmp;
645
646 list_for_each_entry_safe(st, tmp, &drivetemp_devlist, list) {
647 if (st->dev == dev) {
648 list_del(&st->list);
649 hwmon_device_unregister(st->hwdev);
650 kfree(st);
651 break;
652 }
653 }
654 }
655
656 static struct class_interface drivetemp_interface = {
657 .add_dev = drivetemp_add,
658 .remove_dev = drivetemp_remove,
659 };
660
661 #if IS_ENABLED(CONFIG_THERMAL_OF)
662 static int hdd_hwmon_probe(struct platform_device *pdev)
663 {
664 if (list_empty(&drivetemp_devlist))
665 return -EPROBE_DEFER;
666
667 if (!tz)
> 668 devm_thermal_zone_of_sensor_register(
669 &pdev->dev, 0, &drivetemp_devlist, &hdd_sensor_ops);
670
671 return 0;
672 }
673
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
` (2 preceding siblings ...)
2023-03-15 15:06 ` kernel test robot
@ 2023-03-15 15:36 ` Guenter Roeck
2023-03-16 2:21 ` Phinex Hung
2023-03-15 21:15 ` kernel test robot
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-03-15 15:36 UTC (permalink / raw)
To: phinex, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/15/23 05:16, phinex wrote:
> Support thermal zone so that we can just rely on dts to describe a
> thermal zone and do the cooling operations.
>
> You can define a comptible string "drivetemp,hdd-sensors" to enable
> this, such as
>
> sata_port0: sata-port@0 {
> compatible = "drivetemp,hdd-sensors";
> #thermal-sensor-cells = <0>;
> }
>
> Then define a thermal with this sensor to get it work.
>
> hdd_thermal: hdd-thermal {
> thermal-sensors = <&sata_port0>;
> }
>
> In most of the SoC systems, using dts to handle cooling is common.
> This can eliminate the usage of user space application to check
> the value exported in hwmon and then through sysfs to cooling.
>
> Signed-off-by: phinex <phinex@realtek.com>
>
> ---
> .../bindings/hwmon/drivetemp,hdd-sensors.yaml | 35 ++++++
> drivers/hwmon/drivetemp.c | 102 +++++++++++++++++-
> 2 files changed, 133 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> new file mode 100644
> index 000000000000..939d7a923e94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/drivetemp,hdd-sensors.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/drivetemp,hdd-sensors.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Drivetemp Temperature Monitor
> +
> +maintainers:
> + - Phinex <phinex@realtek.com>
> +
> +description: |
> + Drivetemp Temperature Monitor that support a single thermal zone
> + This single thermal zone can support multiple hard drives,
> + it uses maximal temperature of these hard drivers as its temp value.
> +
> +properties:
> + compatible:
> + enum:
> + - drivetemp,hdd-sensors
> +
> + '#thermal-sensor-cells':
> + const: 0
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + sata_port0: sata-port@0 {
> + ompatible = "drivetemp,hdd-sensors";
> + #thermal-sensor-cells = <0>;
> + };
> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> index 1eb37106a220..9a60315d732c 100644
> --- a/drivers/hwmon/drivetemp.c
> +++ b/drivers/hwmon/drivetemp.c
> @@ -107,6 +107,14 @@
> #include <scsi/scsi_device.h>
> #include <scsi/scsi_driver.h>
> #include <scsi/scsi_proto.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +#include <linux/libata.h>
> +
> +/*A single thermal_zone for all HDD sensors */
> +static struct thermal_zone_device *tz;
This is conceptually wrong. It returns the maximum temperature from all drives,
not the temperature from a single drive.
This is not much different from collecting all temperatures from all sensors
in the system and declaring the maximum of those as single thermal zone.
If anything, each drive would have to reflect a thermal zone. The big question
is how to determine the associated devicetree property.
Also, essentially your patch claims that arch/arm/boot/dts/kirkwood-nsa310s.dts
doesn't work and no one ever noticed. I would like to see that confirmed.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 15:36 ` Guenter Roeck
@ 2023-03-16 2:21 ` Phinex Hung
2023-03-16 3:02 ` Guenter Roeck
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-16 2:21 UTC (permalink / raw)
To: Guenter Roeck, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/15/23 11:36, Guenter Roeck" <groeck7@gmail.com <mailto:groeck7@gmail.com> wrote:
>This is conceptually wrong. It returns the maximum temperature from all drives,
>not the temperature from a single drive.
>This is not much different from collecting all temperatures from all sensors
>in the system and declaring the maximum of those as single thermal zone.
>If anything, each drive would have to reflect a thermal zone. The big question
>is how to determine the associated devicetree property.
My basic idea is to use a single thermal zone for multiple disks.
In most of the systems, there might be only a single fan that used for cooling.
If each disk has its own thermal zone, we need to add almost the same dts entries for each thermal zone,
and do almost the same cooling operations.
That is why I am trying to using a single thermal zone for multiple disks.
In any case, if temperature of any disk goes high, cooling should take effect.
>Also, essentially your patch claims that arch/arm/boot/dts/kirkwood-nsa310s.dts
>doesn't work and no one ever noticed. I would like to see that confirmed.
To be honest, my first attempt to get your drivetemp.c works in our SoC was referring to a similar patch,
https://lore.kernel.org/linux-arm-kernel/CAJN1KkzR7NR8TguS7uDs6peDOpkFn0duVBqvKKzm3xnMs9iJ7A@mail.gmail.com/T/
By adding the similar entries in our dts but failed.
Two hwmon devices actually populated, a thermal zone described by my dts also was created.
But there is no link between the thermal zone described in dts and hwmons from drivetemp driver.
So that I traced the source and finding that the thermal zone registration failed due to the lack of a device node of a platform device.
That is why I am trying to use a platform device for registration again.
The original call to hwmon_device_register_with_info should call hwmon_thermal_register_sensors,
But int the last call to thermal_zone_of_sensor_register, dev->of_node would be checked.
This would cause the sensor registration to fail while hwmon still worked.
hwmon_device_register_with_info => __hwmon_device_register => hwmon_thermal_register_sensors =>
hwmon_thermal_add_sensor => devm_thermal_zone_of_sensor_register => thermal_zone_of_sensor_register =>
struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
const struct thermal_zone_of_device_ops *ops)
{
struct device_node *np, *child, *sensor_np;
struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
np = of_find_node_by_name(NULL, "thermal-zones");
if (!np)
return ERR_PTR(-ENODEV);
if (!dev || !dev->of_node) {
of_node_put(np);
return ERR_PTR(-ENODEV);
}
I am also curious why Kirkwood SoC works without specific patch.
In any case, if device tree binding is used, how could we associate drivetemp sensors with a specific thermal zone
if no compatible string is described for these drivetemp sensors in the dts file?
That is why I am adding a generic compatible string to match a platform sensors with a specific thermal zone.
Regards,
Phinex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 2:21 ` Phinex Hung
@ 2023-03-16 3:02 ` Guenter Roeck
2023-03-16 3:29 ` Phinex Hung
0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-03-16 3:02 UTC (permalink / raw)
To: Phinex Hung, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/15/23 19:21, Phinex Hung wrote:
>
> On 3/15/23 11:36, Guenter Roeck" <groeck7@gmail.com <mailto:groeck7@gmail.com> wrote:
>
>> This is conceptually wrong. It returns the maximum temperature from all drives,
>> not the temperature from a single drive.
>
>> This is not much different from collecting all temperatures from all sensors
>> in the system and declaring the maximum of those as single thermal zone.
>
>> If anything, each drive would have to reflect a thermal zone. The big question
>> is how to determine the associated devicetree property.
>
> My basic idea is to use a single thermal zone for multiple disks.
>
> In most of the systems, there might be only a single fan that used for cooling.
>
> If each disk has its own thermal zone, we need to add almost the same dts entries for each thermal zone,
>
> and do almost the same cooling operations.
>
> That is why I am trying to using a single thermal zone for multiple disks.
>
> In any case, if temperature of any disk goes high, cooling should take effect.
>
Sure. But your argument is inappropriate: You could as well argue that
this system with a single fan should bundle all its thermal sensors into
a single thermal zone, and that it should do so in the driver(s)
providing the thermal zone sensors to the thermal subsystem. This does
not take into account that there might be systems with dozens (or hundreds,
for that matter) of drives, in a system with multiple disk trays and fans
for each of those.
I don't know if and how the thermal subsystem deals with the situation
of having N thermal zone sensors and M << N cooling devices. This is
a general problem, not limited to disk drives. Just as we won't bundle
multiple thermal sensors on a multi-channel thermal sensor chip into a
single thermal zone, we won't bundle multiple disk drives into a single
thermal zone.
>> Also, essentially your patch claims that arch/arm/boot/dts/kirkwood-nsa310s.dts
>> doesn't work and no one ever noticed. I would like to see that confirmed.
>
> To be honest, my first attempt to get your drivetemp.c works in our SoC was referring to a similar patch,
>
> https://lore.kernel.org/linux-arm-kernel/CAJN1KkzR7NR8TguS7uDs6peDOpkFn0duVBqvKKzm3xnMs9iJ7A@mail.gmail.com/T/
>
> By adding the similar entries in our dts but failed.
>
> Two hwmon devices actually populated, a thermal zone described by my dts also was created.
>
> But there is no link between the thermal zone described in dts and hwmons from drivetemp driver.
>
> So that I traced the source and finding that the thermal zone registration failed due to the lack of a device node of a platform device.
>
> That is why I am trying to use a platform device for registration again.
>
> The original call to hwmon_device_register_with_info should call hwmon_thermal_register_sensors,
>
> But int the last call to thermal_zone_of_sensor_register, dev->of_node would be checked.
>
> This would cause the sensor registration to fail while hwmon still worked.
>
> hwmon_device_register_with_info => __hwmon_device_register => hwmon_thermal_register_sensors =>
>
> hwmon_thermal_add_sensor => devm_thermal_zone_of_sensor_register => thermal_zone_of_sensor_register =>
>
> struct thermal_zone_device *
> thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
> const struct thermal_zone_of_device_ops *ops)
> {
> struct device_node *np, *child, *sensor_np;
> struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
>
> np = of_find_node_by_name(NULL, "thermal-zones");
> if (!np)
> return ERR_PTR(-ENODEV);
>
> if (!dev || !dev->of_node) {
> of_node_put(np);
> return ERR_PTR(-ENODEV);
> }
>
> I am also curious why Kirkwood SoC works without specific patch.
>
> In any case, if device tree binding is used, how could we associate drivetemp sensors with a specific thermal zone
>
In theory it should work just like described in the kirkwood devicetree
files. If that doesn't work, the question is how to find the sata port
nodes from the drivetemp driver. I don't have a system with such nodes,
so I have no means to find or know the answer.
I also don't know how to attach more than one thermal sensor to a
single thermal zone, or if that is even possible. If it isn't, it
is a limitation of the thermal subsystem, and trying to hack around
it in a driver providing thermal sensors would be inappropriate.
Guenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 3:02 ` Guenter Roeck
@ 2023-03-16 3:29 ` Phinex Hung
2023-03-16 5:07 ` Guenter Roeck
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-16 3:29 UTC (permalink / raw)
To: Guenter Roeck, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/16/23 11:02, Guenter Roeck" <groeck7@gmail.com <mailto:groeck7@gmail.com> <mailto:groeck7@gmail.com <mailto:groeck7@gmail.com>> wrote:
>Sure. But your argument is inappropriate: You could as well argue that
>this system with a single fan should bundle all its thermal sensors into
>a single thermal zone, and that it should do so in the driver(s)
>providing the thermal zone sensors to the thermal subsystem. This does
>not take into account that there might be systems with dozens (or hundreds,
>for that matter) of drives, in a system with multiple disk trays and fans
>for each of those.
>I don't know if and how the thermal subsystem deals with the situation
>of having N thermal zone sensors and M << N cooling devices. This is
>a general problem, not limited to disk drives. Just as we won't bundle
>multiple thermal sensors on a multi-channel thermal sensor chip into a
>single thermal zone, we won't bundle multiple disk drives into a single
>thermal zone.
It's true, there are several different combinations.
Keep a single sensor with a single thermal might give more flexibility.
My idea is inspired from the comment of the following patch..
https://www.spinics.net/lists/devicetree/msg537186.html
By the comment in this patch, it said that "Thermal zone works only with first disks".
But it has two hard disk, each describes a thermal-sensors-cells.
/* Thermal zone works only with first disk */.
That is why I am trying to hack the original temp_read call back to support multiple sensors.
Anyway, keep a single mapping might be good and thanks for your comment.
>In theory it should work just like described in the kirkwood devicetree
>files. If that doesn't work, the question is how to find the sata port
>nodes from the drivetemp driver. I don't have a system with such nodes,
>so I have no means to find or know the answer.
>I also don't know how to attach more than one thermal sensor to a
>single thermal zone, or if that is even possible. If it isn't, it
>is a limitation of the thermal subsystem, and trying to hack around
>it in a driver providing thermal sensors would be inappropriate.
Thank you for this great module to make thermal and cooling control simpler.
That is why I am trying to get it work under our kernel 4.19 since our SoC currently works under 4.19 only.
Sata ports can be found without any issue, and the corresponding hwmon entries can be found, such as.
root@OpenWRT-Kylin:/sys/class/hwmon# ls -al
total 0
drwxr-xr-x 2 root root 0 Mar 11 08:13 .
drwxr-xr-x 42 root root 0 Mar 11 08:13 ..
lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon0 -> ../../devices/platform/soc/98000000.bus/9801b000.syscon/9801bc00.rtk_fan/hwmon/hwmon0
lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon1 -> ../../devices/platform/soc/98000000.bus/9803f000.sata/ata1/host0/target0:0:0/0:0:0:0/hwmon/hwmon1
lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon2 -> ../../devices/platform/soc/98000000.bus/9803f000.sata/ata2/host1/target1:0:0/1:0:0:0/hwmon/hwmon2
And I add the port number in the name of these hwmon devices so that I can differentiate them.
root@OpenWRT-Kylin:/sys/class/hwmon# cat hwmon1/name hwmon1/temp1_input
drivetemp_port0
52000
root@OpenWRT-Kylin:/sys/class/hwmon# cat hwmon2/name hwmon2/temp1_input
drivetemp_port1
49000
My initial problem is that "how could I associated these hwmon devices with thermal zones"?
By tracing the source code, finding that I probably need to register thermal_zone using a platform device.
Hacking into the thermal framework is not a good thing, so that is why I have this patch to hack your driver.
Any better idea to get thermal zone work without register a platform device and then registers the sensors with thermal zone again?
Thanks
Regards,
Phinex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 3:29 ` Phinex Hung
@ 2023-03-16 5:07 ` Guenter Roeck
2023-03-16 7:35 ` Phinex Hung
0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-03-16 5:07 UTC (permalink / raw)
To: Phinex Hung, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/15/23 20:29, Phinex Hung wrote:
>
> On 3/16/23 11:02, Guenter Roeck" <groeck7@gmail.com <mailto:groeck7@gmail.com> <mailto:groeck7@gmail.com <mailto:groeck7@gmail.com>> wrote:
>
>
>> Sure. But your argument is inappropriate: You could as well argue that
>> this system with a single fan should bundle all its thermal sensors into
>> a single thermal zone, and that it should do so in the driver(s)
>> providing the thermal zone sensors to the thermal subsystem. This does
>> not take into account that there might be systems with dozens (or hundreds,
>> for that matter) of drives, in a system with multiple disk trays and fans
>> for each of those.
>
>> I don't know if and how the thermal subsystem deals with the situation
>> of having N thermal zone sensors and M << N cooling devices. This is
>> a general problem, not limited to disk drives. Just as we won't bundle
>> multiple thermal sensors on a multi-channel thermal sensor chip into a
>> single thermal zone, we won't bundle multiple disk drives into a single
>> thermal zone.
>
> It's true, there are several different combinations.
>
> Keep a single sensor with a single thermal might give more flexibility.
>
> My idea is inspired from the comment of the following patch..
>
> https://www.spinics.net/lists/devicetree/msg537186.html
>
> By the comment in this patch, it said that "Thermal zone works only with first disks".
>
> But it has two hard disk, each describes a thermal-sensors-cells.
>
> /* Thermal zone works only with first disk */.
>
> That is why I am trying to hack the original temp_read call back to support multiple sensors.
>
> Anyway, keep a single mapping might be good and thanks for your comment.
>
>
>> In theory it should work just like described in the kirkwood devicetree
>> files. If that doesn't work, the question is how to find the sata port
>> nodes from the drivetemp driver. I don't have a system with such nodes,
>> so I have no means to find or know the answer.
>
>
>> I also don't know how to attach more than one thermal sensor to a
>> single thermal zone, or if that is even possible. If it isn't, it
>> is a limitation of the thermal subsystem, and trying to hack around
>> it in a driver providing thermal sensors would be inappropriate.
>
>
> Thank you for this great module to make thermal and cooling control simpler.
>
> That is why I am trying to get it work under our kernel 4.19 since our SoC currently works under 4.19 only.
>
> Sata ports can be found without any issue, and the corresponding hwmon entries can be found, such as.
>
> root@OpenWRT-Kylin:/sys/class/hwmon# ls -al
> total 0
> drwxr-xr-x 2 root root 0 Mar 11 08:13 .
> drwxr-xr-x 42 root root 0 Mar 11 08:13 ..
> lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon0 -> ../../devices/platform/soc/98000000.bus/9801b000.syscon/9801bc00.rtk_fan/hwmon/hwmon0
> lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon1 -> ../../devices/platform/soc/98000000.bus/9803f000.sata/ata1/host0/target0:0:0/0:0:0:0/hwmon/hwmon1
> lrwxrwxrwx 1 root root 0 Mar 11 08:13 hwmon2 -> ../../devices/platform/soc/98000000.bus/9803f000.sata/ata2/host1/target1:0:0/1:0:0:0/hwmon/hwmon2
>
> And I add the port number in the name of these hwmon devices so that I can differentiate them.
>
> root@OpenWRT-Kylin:/sys/class/hwmon# cat hwmon1/name hwmon1/temp1_input
> drivetemp_port0
> 52000
> root@OpenWRT-Kylin:/sys/class/hwmon# cat hwmon2/name hwmon2/temp1_input
> drivetemp_port1
> 49000
>
> My initial problem is that "how could I associated these hwmon devices with thermal zones"?
>
> By tracing the source code, finding that I probably need to register thermal_zone using a platform device.
>
Wrong conclusion. You have (or should have) devicetree node(s) such as
sata: sata@80000 {
compatible = "marvell,orion-sata";
reg = <0x80000 0x5000>;
interrupts = <21>;
clocks = <&gate_clk 14>, <&gate_clk 15>;
clock-names = "0", "1";
phys = <&sata_phy0>, <&sata_phy1>;
phy-names = "port0", "port1";
status = "disabled";
};
Those nodes should have devices associated with them (in this case instantiated
by drivers/ata/sata_mv.c). At the same time, the drivetemp driver callback
(drivetemp_add) gets called with a device pointer as parameter. The question
is how to get from the device pointer passed to drivetemp_add() to the device
created by the sata_mv driver. Is it dev ? or dev->parent ? Or dev->parent->parent ?
The devicetree node associated with that device is the one which should be used
to set the hwmon device devicetree node. Essentially you'll have to find out where
in the device list to find the of_node pointing to the above sata node. Then we
can discuss how to make that node available to the thermal subsystem.
Guenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 5:07 ` Guenter Roeck
@ 2023-03-16 7:35 ` Phinex Hung
2023-03-16 7:48 ` Phinex Hung
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-16 7:35 UTC (permalink / raw)
To: Guenter Roeck, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/16/23 13:08, "Guenter Roeck" <groeck7@gmail.com <mailto:groeck7@gmail.com> wrote:
>Wrong conclusion. You have (or should have) devicetree node(s) such as
>sata: sata@80000 {
>compatible = "marvell,orion-sata";
>reg = <0x80000 0x5000>;
>interrupts = <21>;
>clocks = <&gate_clk 14>, <&gate_clk 15>;
>clock-names = "0", "1";
>phys = <&sata_phy0>, <&sata_phy1>;
>phy-names = "port0", "port1";
>status = "disabled";
>};
I did see this, but just though that is a compatible match to marvell's sata driver.
Nothing to do with your drivetemp.
>Those nodes should have devices associated with them (in this case instantiated
>by drivers/ata/sata_mv.c). At the same time, the drivetemp driver callback
>(drivetemp_add) gets called with a device pointer as parameter. The question
>is how to get from the device pointer passed to drivetemp_add() to the device
>created by the sata_mv driver. Is it dev ? or dev->parent ? Or dev->parent->parent ?
>The devicetree node associated with that device is the one which should be used
>to set the hwmon device devicetree node. Essentially you'll have to find out where
>in the device list to find the of_node pointing to the above sata node. Then we
>can discuss how to make that node available to the thermal subsystem.
Thanks for the hint.
By checking marvell's SATA driver, it uses "ata_host_activate" to register a platform SATA driver.
In our SATA driver, we rely on "ahci_platform_init_host" to register a platform SATA driver.
Not sure whether this is the difference, but use the following patch can solve this issue.
@@ -540,9 +531,9 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
goto abort;
}
- st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
- st, &drivetemp_chip_info,
- NULL);
+ st->hwdev = hwmon_device_register_with_info(
+ dev->parent->parent->parent->parent->parent, "drivetemp", st,
+ &drivetemp_chip_info, NULL);
The problem is that in our case, dev is a scsi_device, dev->parent is a sd device,
dev->parent->parent is a scsi target, dev->parent->parent->parent is a scsi host
dev->parent->parent->parent->parent is an ATA device, while its parent is our platform device (ahci).
Besides, a change in dts is required to move #thermal-sensor-cells = <0> from a SATA port to an ahci host.
@@ -94,16 +94,15 @@
realtek,satawrap = <&sata_phy>;
#address-cells = <1>;
#size-cells = <0>;
+ #thermal-sensor-cells = <0>;
status = "okay";
sata_port0: sata-port@0 {
reg = <0>;
phys = <&sata_phy 0>;
resets = <&clkc RTD1295_RSTN_SATA_0>,
<&clkc RTD1295_RSTN_SATA_PHY_0>;
sata-gpios = <&misc_gpio 16 GPIO_ACTIVE_HIGH>;
- #thermal-sensor-cells = <0>;
status = "okay";
};
};
Without this change, sensor registration would still fail due to mismatch of the np node in thermal_zone_of_sensor_register function.
if (sensor_specs.np == sensor_np && id == sensor_id) {
tzd = thermal_zone_of_add_sensor(child, sensor_np,
data, ops);
if (!IS_ERR(tzd))
tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
of_node_put(sensor_specs.np);
of_node_put(child);
goto exit;
}
This might be a good solution for my initial problem, and it works perfectly for a single drive case.
Thank you again for the advice.
For a system with more than 1 drive, there is still a mapping between multiple hwmon devices and a thermal zone.
Since I move the #thermal-sensor-cells = <0> from SATA port to a AHCI host, thermal zone is associated with the host, not each SATA port.
Take the below case for example, it sounds that the thermal zone is associated with hwmon2 only.
root@OpenWRT-Kylin:/sys/class/thermal# cat thermal_zone1/temp
48000
root@OpenWRT-Kylin:/sys/class/thermal# cat ../hwmon/hwmon1/temp1_input
50000
root@OpenWRT-Kylin:/sys/class/thermal# cat ../hwmon/hwmon2/temp1_input
48000
Thanks
Regards,
Phinex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 7:35 ` Phinex Hung
@ 2023-03-16 7:48 ` Phinex Hung
2023-03-16 13:22 ` Guenter Roeck
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-16 7:48 UTC (permalink / raw)
To: Guenter Roeck, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/16/23 15:35 "Phinex Hung" <phinex@realtek.com <mailto:phinex@realtek.com> wrote:
>Not sure whether this is the difference, but use the following patch can solve this issue.
>@@ -540,9 +531,9 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
>goto abort;
>}
>
>
>- st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
>- st, &drivetemp_chip_info,
>- NULL);
>+ st->hwdev = hwmon_device_register_with_info(
>+ dev->parent->parent->parent->parent->parent, "drivetemp", st,
>+ &drivetemp_chip_info, NULL);
A more generic patch works in my case, listed below:
diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
index 1cf4f9015316..a4cddfea8d22 100644
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -525,6 +525,7 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
{
struct scsi_device *sdev = to_scsi_device(dev->parent);
struct drivetemp_data *st;
+ struct device *tdev = dev->parent;
int err;
st = kzalloc(sizeof(*st), GFP_KERNEL);
@@ -540,8 +541,11 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
goto abort;
}
- st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
- st, &drivetemp_chip_info,
+ while(!tdev->of_node)
+ tdev = tdev->parent;
+
+ st->hwdev = hwmon_device_register_with_info(tdev, "drivetemp", st,
+ &drivetemp_chip_info,
NULL);
if (IS_ERR(st->hwdev)) {
err = PTR_ERR(st->hwdev);
Thanks
Regards,
Phinex
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 7:48 ` Phinex Hung
@ 2023-03-16 13:22 ` Guenter Roeck
2023-03-16 14:17 ` Phinex Hung
0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-03-16 13:22 UTC (permalink / raw)
To: Phinex Hung, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/16/23 00:48, Phinex Hung wrote:
>
>
> On 3/16/23 15:35 "Phinex Hung" <phinex@realtek.com <mailto:phinex@realtek.com> wrote:
>
>> Not sure whether this is the difference, but use the following patch can solve this issue.
>
>> @@ -540,9 +531,9 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
>> goto abort;
>> }
>>
>>
>> - st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
>> - st, &drivetemp_chip_info,
>> - NULL);
>> + st->hwdev = hwmon_device_register_with_info(
>> + dev->parent->parent->parent->parent->parent, "drivetemp", st,
>> + &drivetemp_chip_info, NULL);
>
>
> A more generic patch works in my case, listed below:
>
> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> index 1cf4f9015316..a4cddfea8d22 100644
> --- a/drivers/hwmon/drivetemp.c
> +++ b/drivers/hwmon/drivetemp.c
> @@ -525,6 +525,7 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
> {
> struct scsi_device *sdev = to_scsi_device(dev->parent);
> struct drivetemp_data *st;
> + struct device *tdev = dev->parent;
> int err;
>
> st = kzalloc(sizeof(*st), GFP_KERNEL);
> @@ -540,8 +541,11 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
> goto abort;
> }
>
> - st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
> - st, &drivetemp_chip_info,
> + while(!tdev->of_node)
> + tdev = tdev->parent;
> +
That needs to be in the hwmon core. We can not change the device pointer
passed to hwmon_device_register_with_info() because that determines the
lifetime of the hwmon device.
Guenter
> + st->hwdev = hwmon_device_register_with_info(tdev, "drivetemp", st,
> + &drivetemp_chip_info,
> NULL);
> if (IS_ERR(st->hwdev)) {
> err = PTR_ERR(st->hwdev);
>
> Thanks
>
> Regards,
> Phinex
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 13:22 ` Guenter Roeck
@ 2023-03-16 14:17 ` Phinex Hung
2023-03-16 18:17 ` Guenter Roeck
0 siblings, 1 reply; 20+ messages in thread
From: Phinex Hung @ 2023-03-16 14:17 UTC (permalink / raw)
To: Guenter Roeck, jdelvare; +Cc: linux-hwmon, linux-kernel
On 3/16/23 22:48, Guenter Roeck wrote:
>That needs to be in the hwmon core. We can not change the device pointer
>passed to hwmon_device_register_with_info() because that determines the
>lifetime of the hwmon device.
>Guenter
Do you mean something like below?
Or is it reasonable that we just match a specific compatible string and assign the device node to the original dev->parent used in drivetemp_add function ?
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 33edb5c02f7d..a76beeada33e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -757,6 +757,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
struct hwmon_device *hwdev;
const char *label;
struct device *hdev;
+ struct device *tedv = dev;
int i, err, id;
/* Complain about invalid characters in hwmon name attribute */
@@ -826,7 +827,9 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
hwdev->name = name;
hdev->class = &hwmon_class;
hdev->parent = dev;
- hdev->of_node = dev ? dev->of_node : NULL;
+ while(!tdev->of_node)
+ tdev = tdev->parent;
+ hdev->of_node = tdev ? tdev->of_node : NULL;
hwdev->chip = chip;
dev_set_drvdata(hdev, drvdata);
@@ -838,7 +841,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
INIT_LIST_HEAD(&hwdev->tzdata);
- if (dev && dev->of_node && chip && chip->ops->read &&
+ if (tdev && tdev->of_node && chip && chip->ops->read &&
chip->info[0]->type == hwmon_chip &&
(chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
err = hwmon_thermal_register_sensors(hdev);
Regards,
Phinex
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 14:17 ` Phinex Hung
@ 2023-03-16 18:17 ` Guenter Roeck
2023-03-17 2:25 ` Phinex Hung
0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-03-16 18:17 UTC (permalink / raw)
To: Phinex Hung; +Cc: jdelvare, linux-hwmon, linux-kernel
On Thu, Mar 16, 2023 at 02:17:34PM +0000, Phinex Hung wrote:
>
>
> On 3/16/23 22:48, Guenter Roeck wrote:
>
>
> >That needs to be in the hwmon core. We can not change the device pointer
> >passed to hwmon_device_register_with_info() because that determines the
> >lifetime of the hwmon device.
>
>
> >Guenter
>
> Do you mean something like below?
>
Yes, except of course for the bugs (see below). That is much less
than perfect, of course, since we'd really want the device node
for the drive, not the controller, but it might be the best we can do.
> Or is it reasonable that we just match a specific compatible string and assign the device node to the original dev->parent used in drivetemp_add function ?
>
We can't add anything to the parent device node since we don't own it.
Also, I don't know if devicetree maintainers would accept the concept
of "virtual" device nodes (and I don't know how device nodes for drives
would or should look like either).
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 33edb5c02f7d..a76beeada33e 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -757,6 +757,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> struct hwmon_device *hwdev;
> const char *label;
> struct device *hdev;
> + struct device *tedv = dev;
tdev =
> int i, err, id;
>
> /* Complain about invalid characters in hwmon name attribute */
> @@ -826,7 +827,9 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> hwdev->name = name;
> hdev->class = &hwmon_class;
> hdev->parent = dev;
> - hdev->of_node = dev ? dev->of_node : NULL;
> + while(!tdev->of_node)
while (tdev && !tdev->of_node)
> + tdev = tdev->parent;
> + hdev->of_node = tdev ? tdev->of_node : NULL;
> hwdev->chip = chip;
> dev_set_drvdata(hdev, drvdata);
> @@ -838,7 +841,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>
> INIT_LIST_HEAD(&hwdev->tzdata);
>
> - if (dev && dev->of_node && chip && chip->ops->read &&
> + if (tdev && tdev->of_node && chip && chip->ops->read &&
This could probably be simplified to
if (hdev->of_node && chip && ..
> chip->info[0]->type == hwmon_chip &&
> (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
> err = hwmon_thermal_register_sensors(hdev);
>
> Regards,
> Phinex
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-16 18:17 ` Guenter Roeck
@ 2023-03-17 2:25 ` Phinex Hung
0 siblings, 0 replies; 20+ messages in thread
From: Phinex Hung @ 2023-03-17 2:25 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
On Friday, March 17 at 2023 2:18 AM , Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck wrote:
>Yes, except of course for the bugs (see below). That is much less than perfect, of course, since we'd really want the device node for the drive, not the controller, but it might be the best we can do.
See, thanks for the review, looks better and bug-less than my draft version.
Should I submit this patch again using the code snippet that we discussed? Or any suggestions ?
>> Or is it reasonable that we just match a specific compatible string and assign the device node to the original dev->parent used in drivetemp_add function ?
>>
>We can't add anything to the parent device node since we don't own it.
>Also, I don't know if devicetree maintainers would accept the concept of "virtual" device nodes (and I don't know how device nodes for drives would or should look like either).
What I am thinking about is "virtual" device nodes as well, such as..
@@ -99,6 +99,7 @@
status = "okay";
sata_port0: sata-port@0 {
+ compatible = "drivetemp,hdd-sensors";
reg = <0>;
phys = <&sata_phy 0>;
resets = <&clkc RTD1295_RSTN_SATA_0>,
@@ -110,6 +110,7 @@
};
sata_port1: sata-port@1 {
+ compatible = "drivetemp,hdd-sensors";
reg = <1>;
phys = <&sata_phy 1>;
resets = <&clkc (RTD1295_RSTN_SATA_1 | RTD1295_RSTN_REG_BANK_4)>,
And patches in the drivetemp.c itself..
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -107,6 +107,7 @@
#include <scsi/scsi_device.h>
#include <scsi/scsi_driver.h>
#include <scsi/scsi_proto.h>
+#include <linux/of.h>
struct drivetemp_data {
struct list_head list; /* list of instantiated devices */
@@ -525,6 +526,7 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
{
struct scsi_device *sdev = to_scsi_device(dev->parent);
struct drivetemp_data *st;
+ static struct device_node *node = NULL;
int err;
st = kzalloc(sizeof(*st), GFP_KERNEL);
@@ -540,6 +542,11 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf)
goto abort;
}
+ node = of_find_compatible_node(node, NULL, "drivetemp,hdd-sensors");
+
+ if(node)
+ dev->parent->of_node = node;
+
st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp",
st, &drivetemp_chip_info,
NULL);
Doing this can get my two HDD works for two thermal zones.
If "virtual" device node can be used, then we don't need to patch the hwmon.c core ?
>> - hdev->of_node = dev ? dev->of_node : NULL;
>> + while(!tdev->of_node)
> while (tdev && !tdev->of_node)
Thanks for this review, checking tdev can avoid endless loop. My fault for this.
> >- if (dev && dev->of_node && chip && chip->ops->read &&
> >+ if (tdev && tdev->of_node && chip && chip->ops->read &&
>This could probably be simplified to
> if (hdev->of_node && chip && ..
Looks better and simpiler.
Thanks
Regards,
Phinex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
` (3 preceding siblings ...)
2023-03-15 15:36 ` Guenter Roeck
@ 2023-03-15 21:15 ` kernel test robot
2023-03-15 22:28 ` kernel test robot
2023-03-21 6:02 ` [PATCH v2] hwmon: fix potential sensor registration fail if of_node is missing Phinex Hung
6 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2023-03-15 21:15 UTC (permalink / raw)
To: phinex, jdelvare
Cc: llvm, oe-kbuild-all, linux, linux-hwmon, linux-kernel, phinex
Hi phinex,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.3-rc2 next-20230315]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20230315121606.GA71707%40threadripper
patch subject: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
config: arm-randconfig-r012-20230312 (https://download.01.org/0day-ci/archive/20230316/202303160519.6Xosrf2g-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/1c53b683440a584685795fa8ff831379577081b0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
git checkout 1c53b683440a584685795fa8ff831379577081b0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/hwmon/ fs/xfs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303160519.6Xosrf2g-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/hwmon/drivetemp.c:551:6: warning: variable 'err' set but not used [-Wunused-but-set-variable]
int err;
^
>> drivers/hwmon/drivetemp.c:575:48: error: variable has incomplete type 'const struct thermal_zone_of_device_ops'
static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
^
drivers/hwmon/drivetemp.c:575:21: note: forward declaration of 'struct thermal_zone_of_device_ops'
static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
^
>> drivers/hwmon/drivetemp.c:668:3: error: call to undeclared function 'devm_thermal_zone_of_sensor_register'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
devm_thermal_zone_of_sensor_register(
^
drivers/hwmon/drivetemp.c:668:3: note: did you mean 'devm_thermal_of_zone_register'?
include/linux/thermal.h:303:29: note: 'devm_thermal_of_zone_register' declared here
struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
^
1 warning and 2 errors generated.
vim +575 drivers/hwmon/drivetemp.c
574
> 575 static const struct thermal_zone_of_device_ops hdd_sensor_ops = {
576 .get_temp = hdd_read_temp,
577 };
578
579 static const struct of_device_id hdd_of_match[] = {
580 {
581 .compatible = "drivetemp,hdd-sensors",
582 },
583 {},
584 };
585 MODULE_DEVICE_TABLE(of, hdd_of_match);
586 #endif
587
588 static const struct hwmon_ops drivetemp_ops = {
589 .is_visible = drivetemp_is_visible,
590 .read = drivetemp_read,
591 };
592
593 static const struct hwmon_chip_info drivetemp_chip_info = {
594 .ops = &drivetemp_ops,
595 .info = drivetemp_info,
596 };
597
598 /*
599 * The device argument points to sdev->sdev_dev. Its parent is
600 * sdev->sdev_gendev, which we can use to get the scsi_device pointer.
601 */
602 static int drivetemp_add(struct device *dev, struct class_interface *intf)
603 {
604 struct scsi_device *sdev = to_scsi_device(dev->parent);
605 struct drivetemp_data *st;
606 int err;
607 struct ata_port *ap;
608
609 st = kzalloc(sizeof(*st), GFP_KERNEL);
610 if (!st)
611 return -ENOMEM;
612
613 ap = ata_shost_to_port(sdev->host);
614
615 snprintf(st->drivename, MAX_NAME_LEN, "drivetemp_port%d", ap->port_no);
616
617 st->sdev = sdev;
618 st->dev = dev;
619 mutex_init(&st->lock);
620
621 if (drivetemp_identify(st)) {
622 err = -ENODEV;
623 goto abort;
624 }
625
626 st->hwdev = hwmon_device_register_with_info(
627 dev->parent, st->drivename, st, &drivetemp_chip_info, NULL);
628
629 if (IS_ERR(st->hwdev)) {
630 err = PTR_ERR(st->hwdev);
631 goto abort;
632 }
633
634 list_add(&st->list, &drivetemp_devlist);
635 return 0;
636
637 abort:
638 kfree(st);
639 return err;
640 }
641
642 static void drivetemp_remove(struct device *dev, struct class_interface *intf)
643 {
644 struct drivetemp_data *st, *tmp;
645
646 list_for_each_entry_safe(st, tmp, &drivetemp_devlist, list) {
647 if (st->dev == dev) {
648 list_del(&st->list);
649 hwmon_device_unregister(st->hwdev);
650 kfree(st);
651 break;
652 }
653 }
654 }
655
656 static struct class_interface drivetemp_interface = {
657 .add_dev = drivetemp_add,
658 .remove_dev = drivetemp_remove,
659 };
660
661 #if IS_ENABLED(CONFIG_THERMAL_OF)
662 static int hdd_hwmon_probe(struct platform_device *pdev)
663 {
664 if (list_empty(&drivetemp_devlist))
665 return -EPROBE_DEFER;
666
667 if (!tz)
> 668 devm_thermal_zone_of_sensor_register(
669 &pdev->dev, 0, &drivetemp_devlist, &hdd_sensor_ops);
670
671 return 0;
672 }
673
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
` (4 preceding siblings ...)
2023-03-15 21:15 ` kernel test robot
@ 2023-03-15 22:28 ` kernel test robot
2023-03-21 6:02 ` [PATCH v2] hwmon: fix potential sensor registration fail if of_node is missing Phinex Hung
6 siblings, 0 replies; 20+ messages in thread
From: kernel test robot @ 2023-03-15 22:28 UTC (permalink / raw)
To: phinex, jdelvare; +Cc: oe-kbuild-all, linux, linux-hwmon, linux-kernel, phinex
Hi phinex,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.3-rc2 next-20230315]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20230315121606.GA71707%40threadripper
patch subject: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of
config: i386-randconfig-a012 (https://download.01.org/0day-ci/archive/20230316/202303160645.7Nrjce3N-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/1c53b683440a584685795fa8ff831379577081b0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review phinex/hwmon-drivetemp-support-to-be-a-platform-driver-for-thermal_of/20230315-201903
git checkout 1c53b683440a584685795fa8ff831379577081b0
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303160645.7Nrjce3N-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/hwmon/drivetemp.c:116:36: warning: 'tz' defined but not used [-Wunused-variable]
116 | static struct thermal_zone_device *tz;
| ^~
vim +/tz +116 drivers/hwmon/drivetemp.c
114
115 /*A single thermal_zone for all HDD sensors */
> 116 static struct thermal_zone_device *tz;
117 #define MAX_NAME_LEN 255
118
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] hwmon: fix potential sensor registration fail if of_node is missing
2023-03-15 12:16 [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of phinex
` (5 preceding siblings ...)
2023-03-15 22:28 ` kernel test robot
@ 2023-03-21 6:02 ` Phinex Hung
6 siblings, 0 replies; 20+ messages in thread
From: Phinex Hung @ 2023-03-21 6:02 UTC (permalink / raw)
Cc: phinex, Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel
It is not sufficient to check of_node in current device.
In some cases, this would cause the sensor registration to fail.
This patch looks for device's ancestors to find a valid of_node if any.
Signed-off-by: Phinex Hung <phinex@realtek.com>
---
v2: assign of_node from its ancestor to support sensor registration
---
drivers/hwmon/hwmon.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 33edb5c02f7d..d193ed3cb35e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -757,6 +757,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
struct hwmon_device *hwdev;
const char *label;
struct device *hdev;
+ struct device *tdev = dev;
int i, err, id;
/* Complain about invalid characters in hwmon name attribute */
@@ -826,7 +827,9 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
hwdev->name = name;
hdev->class = &hwmon_class;
hdev->parent = dev;
- hdev->of_node = dev ? dev->of_node : NULL;
+ while (tdev && !tdev->of_node)
+ tdev = tdev->parent;
+ hdev->of_node = tdev ? tdev->of_node : NULL;
hwdev->chip = chip;
dev_set_drvdata(hdev, drvdata);
dev_set_name(hdev, HWMON_ID_FORMAT, id);
@@ -838,7 +841,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
INIT_LIST_HEAD(&hwdev->tzdata);
- if (dev && dev->of_node && chip && chip->ops->read &&
+ if (hdev->of_node && chip && chip->ops->read &&
chip->info[0]->type == hwmon_chip &&
(chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
err = hwmon_thermal_register_sensors(hdev);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread