Re: [PATCH v2 02/10] hwmon: Add Qualcomm PMIC BCL driver

[Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>]

Hi Guenter,


On 7/30/2026 8:47 PM, Guenter Roeck wrote:
> On 7/30/26 04:48, Daniel Lezcano wrote:
>> On 7/24/26 02:04, Guenter Roeck wrote:
>>> On 7/23/26 12:16, Manaf Meethalavalappu Pallikunhi wrote:
>>>> Hi Guenter,
>>>>
>>>> On 7/23/2026 12:29 AM, Guenter Roeck wrote:
>>>>> On 7/22/26 11:19, Manaf Meethalavalappu Pallikunhi wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> On 7/22/2026 11:16 PM, Guenter Roeck wrote:
>>>>>>> On 7/22/26 10:38, Manaf Meethalavalappu Pallikunhi wrote:
>>>>>>> ...
>>>>>>>>>
>>>>>>>>> I am curious: Why not use static initialization and use the 
>>>>>>>>> is_visible
>>>>>>>>> function to determine if an attribute is visible or not ?
>>>>>>>>
>>>>>>>> ACK. There is already a comment in v1 suggesting the use of 
>>>>>>>> dynamic allocation based on the available attributes rather than 
>>>>>>>> static initialization. The intent is to avoid having to perform 
>>>>>>>> enable flag checks in multiple places.
>>>>>>>> . >
>>>>>>>
>>>>>>> You lost me, sorry. There is only a single is_visible function,
>>>>>>> and its intent is exactly to handle situations where some sensors
>>>>>>> are not always available.
>>>>>>>
>>>>>>> What would be those "multiple places" ?
>>>>>>
>>>>>> Understood, thanks for the clarification. I'll revert to static 
>>>>>> attribute initialization in the next revision and use is_visible() 
>>>>>> to control attribute visibility where needed.
>>>>>>
>>>>>>>
>>>>>>>>>  From the earlier exchange I had the impression that there is a 
>>>>>>>>> large
>>>>>>>>> number of current and voltage channels, but it looks like there 
>>>>>>>>> is only
>>>>>>>>> one each. That doesn't really warrant or need all this dynamic 
>>>>>>>>> code
>>>>>>>>
>>>>>>>> Yes, this hardware only supports one or two channels (voltage 
>>>>>>>> and current). What we discussed earlier was that each channel 
>>>>>>>> can have up to three threshold alarms (warning, critical, and 
>>>>>>>> emergency). However, the hwmon framework currently supports only 
>>>>>>>> two limit alarms for these sensor types. I have not added 
>>>>>>>> support for the third alarm threshold in this series to keep the 
>>>>>>>> initial driver support aligned with the existing hwmon 
>>>>>>>> capabilities. Once the basic driver support is accepted, I can 
>>>>>>>> queue a follow-up series to add support for the third limit alarm.
>>>>>>>>
>>>>>>>
>>>>>>> What does that have to do with attribute visibility ?
>>>>>>
>>>>>> I was only clarifying that the channel count has always been small 
>>>>>> (maximum of two channels) and has not changed since v1. The 
>>>>>> earlier discussion was primarily around the limit alarm 
>>>>>> attributes. That said, you're right that this is unrelated to 
>>>>>> attribute visibility.
>>>>>>
>>>>>
>>>>> Feel free to use (and test)
>>>>> https://lore.kernel.org/linux-hwmon/20260722185749.2313572-1- 
>>>>> linux@roeck-us.net/T/#u
>>>>
>>>> Thanks for working on this. I'll use that change as the base, rebase 
>>>> my series on top of it, and add support for the third alarm level.
>>>> By the way, for the voltage channel, the hardware monitors 
>>>> undervoltage conditions and provides three threshold levels. At the 
>>>> moment, I am mapping:
>>>>
>>>> LVL0 → min
>>>> LVL1 → lcrit
>>>>
>>>> For LVL2, would it be possible to introduce a corresponding low- 
>>>> voltage emergency threshold, such as lemergency (or another more 
>>>> appropriate name), to represent the third alarm level for voltage 
>>>> sensors ?
>>>>
>>>
>>> We can, but I really have no idea how to name it. lcrit is bad enough,
>>> but lemergency is even worse (or at least I think so). "lemerg" would
>>> be almost as bad, but at least it would kind of match "lcrit".
>>> I tried to ask Google AI, but it didn't give me any useful ideas.
>>> Any other suggestions or ideas ?
>>
>> 'lfatal' ?
>> After thinking about it, I'll stick with "lemergency", reason being that
> it matches
>      crit -> lcrit
>      emergency -> lemergency

I pulled in your patch [1] and extended the implementation to add 
lemergency support for the in sensor type. I validated the third-limit 
alarm support for both in and curr sensors.

The resulting sysfs interface now looks as follows:

/sys/class/hwmon/hwmon1# ls
curr1_crit curr1_emergency_alarm curr1_max device in0_lcrit 
in0_lemergency in0_min name power uevent
curr1_crit_alarm curr1_input curr1_max_alarm in0_input in0_lcrit_alarm 
in0_lemergency_alarm in0_min_alarm of_node subsystem

[1] https://lore.kernel.org/all/20260722192547.619281F000E9@smtp.kernel.org/

Could you please update your series to add lemergency support for the
in sensor type as well ?

Thanks,
Manaf


> 
> Guenter
>