Re: [PATCH v4 2/2] hwmon: add AMD Promontory 21 xHCI temperature sensor support

[Jihong Min <hurryman2212@icloud.com>]

 > Rather than using pm_ref as an output variable to indicate whether you
 > took a ref - how about you instead always take a ref on success and
 > return an error on fail? This would feel more logical to me.

Agreed. I changed the PM helper so success always means that a usage 
reference
is held, and the caller now unconditionally drops it with 
pm_runtime_put_noidle()
after the register access.

For the runtime PM disabled case, pm_runtime_get_if_active() returns 
-EINVAL.
In that case the helper now allows the read if the device is not marked
suspended, and uses pm_runtime_get_noresume() so the same success/put 
contract
is preserved without changing the runtime PM state.

 > guard(mutex) perhaps?

The private mutex is gone. Guenter pointed out that the hwmon core already
serializes the callbacks, so I removed the extra lock instead of 
converting it.

 > You only care about the first byte, right? Just use readb() and make
 > data a u8.

Done. The data register read now uses readb(), and the local data 
variable is
u8.

 > I personally don't really like changing the pointer when there is
 > potentially an error case with it for -ENODATA.

Done. The raw output pointer is only written after validating the read 
value.

 > Does 0xff actually happen with your runtime PM handling? Between my
 > suggestion above to use readb() this can turn into:
 >
 > if (!data)
 > return -ENODDATA;
 > *raw = data;
 >
 > return 0;

Done. I removed the 0xff invalid check. Suspended devices are rejected 
before
the MMIO access, so the read path now only treats zero as invalid and 
assigns
*raw after that check.

Sincerely,
Jihong Min