RE: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of

[Phinex Hung <phinex@xxxxxxxxxxx>]

On Friday, March 17 at 2023 2:18 AM , Guenter Roeck <groeck7@xxxxxxxxx> 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