mbox series

[v1,0/4] Add support for Intel CRPS PSU

Message ID 20241212214927.3586509-1-ninad@linux.ibm.com
Headers show
Series Add support for Intel CRPS PSU | expand

Message

Ninad Palsule Dec. 12, 2024, 9:48 p.m. UTC 
Hello,

Please review the changes for Intel CRPS power supply driver.
I need to add label in the pmbus core as new application like PSUSensor
requires it.

Ninad Palsule (4):
  hwmon: pmbus-core: Add label for fan and temp
  hwmon: (pmbus/crps) Add Intel CRPS185 power supply
  dt-bindings: hwmon: intel,crps185: Add to trivial
  ARM: dts: aspeed: system1: Use crps PSU driver

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 Documentation/hwmon/crps.rst                  |  95 ++++++
 MAINTAINERS                                   |   7 +
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     |   8 +-
 drivers/hwmon/pmbus/Kconfig                   |   9 +
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/crps.c                    | 299 ++++++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c              |  14 +
 8 files changed, 431 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/hwmon/crps.rst
 create mode 100644 drivers/hwmon/pmbus/crps.c

Comments

Krzysztof Kozlowski Dec. 13, 2024, 11:08 a.m. UTC | #1 
On Thu, Dec 12, 2024 at 03:49:02PM -0600, Ninad Palsule wrote:
> Add INTEL Common Redundant Power Supply Versions crps185 bindings as
> trivial.

becuse they are trivial or you don't care? Some broader context would be
useful here.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Ninad Palsule Dec. 13, 2024, 4:12 p.m. UTC | #2 
Hello Guenter,

On 12/12/24 16:06, Guenter Roeck wrote:
> On 12/12/24 13:49, Ninad Palsule wrote:
>> Adding label files for fan and temperature sensors in the power supply.
>> The openbmc application dbus-sensor(psusensor) requires those files to
>> consfigure those sensors.
>> Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
>> as dbus-sensor(psusensor) application calculate index based on last
>> digit in the name so we do not want to make index double digit after
>> appending page index.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>
> We are not going to fix userspace problems in the kernel.
>
> Guenter
>

Thanks for the quick review.

Sorry I am not clear on this. I feel that it is better to support labels 
for temperature

sensors and fans like other. Are you saying we should not support these 
labels or

I need update in the patch to support them better?

Please let me know.

Thanks & Regards,

Ninad Palsule
Guenter Roeck Dec. 13, 2024, 5:08 p.m. UTC | #3 
On 12/13/24 08:12, Ninad Palsule wrote:
> Hello Guenter,
> 
> On 12/12/24 16:06, Guenter Roeck wrote:
>> On 12/12/24 13:49, Ninad Palsule wrote:
>>> Adding label files for fan and temperature sensors in the power supply.
>>> The openbmc application dbus-sensor(psusensor) requires those files to
>>> consfigure those sensors.
>>> Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
>>> as dbus-sensor(psusensor) application calculate index based on last
>>> digit in the name so we do not want to make index double digit after
>>> appending page index.
>>>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>
>> We are not going to fix userspace problems in the kernel.
>>
>> Guenter
>>
> 
> Thanks for the quick review.
> 
> Sorry I am not clear on this. I feel that it is better to support labels for temperature
> 
> sensors and fans like other. Are you saying we should not support these labels or
> 
> I need update in the patch to support them better?
> 

There should be no such labels. Labels are supposed to have specific meanings,
such as "this is the CPU temperature sensor", not vague meanings such as "tempA".

Guenter
Ninad Palsule Dec. 16, 2024, 5:53 p.m. UTC | #4 
Hi Guenter,


On 12/13/24 11:08, Guenter Roeck wrote:
> On 12/13/24 08:12, Ninad Palsule wrote:
>> Hello Guenter,
>>
>> On 12/12/24 16:06, Guenter Roeck wrote:
>>> On 12/12/24 13:49, Ninad Palsule wrote:
>>>> Adding label files for fan and temperature sensors in the power 
>>>> supply.
>>>> The openbmc application dbus-sensor(psusensor) requires those files to
>>>> consfigure those sensors.
>>>> Note that prefix for temp label is temp[A..C] used instead of 
>>>> temp[1..3]
>>>> as dbus-sensor(psusensor) application calculate index based on last
>>>> digit in the name so we do not want to make index double digit after
>>>> appending page index.
>>>>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>
>>> We are not going to fix userspace problems in the kernel.
>>>
>>> Guenter
>>>
>>
>> Thanks for the quick review.
>>
>> Sorry I am not clear on this. I feel that it is better to support 
>> labels for temperature
>>
>> sensors and fans like other. Are you saying we should not support 
>> these labels or
>>
>> I need update in the patch to support them better?
>>
>
> There should be no such labels. Labels are supposed to have specific 
> meanings,
> such as "this is the CPU temperature sensor", not vague meanings such 
> as "tempA".
>
> Guenter

Thanks for the quick response. I will remove these changes for now and 
will talk to you later about

better option.

Thanks & Regards,

Ninad Palsule