On Wed, Aug 16, 2023 at 09:42:17PM +0530, Kalesh Anakkur Purayil wrote: > On Wed, Aug 16, 2023 at 5:43 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > > On Wed, Aug 16, 2023 at 03:58:34PM +0530, Kalesh Anakkur Purayil wrote: > > > Thank you Guenter for the review and the suggestions. > > > > > > Please see my response inline. > > > > > > On Tue, Aug 15, 2023 at 8:35 PM Guenter Roeck <linux@xxxxxxxxxxxx> > > wrote: > > > > > [ ... ] > > > > > > > > > > Hmm, that isn't really the purpose of alarm attributes. The expectation > > > > would be that the chip sets alarm flags and the driver reports it. > > > > I guess there is some value in having it, so I won't object. > > > > > > > > Anyway, the ordering is wrong. max_alarm should be the lowest > > > > alarm level, followed by crit and emergency. So > > > > max_alarm -> temp >= bp->warn_thresh_temp > > > > crit_alarm -> temp >= bp->crit_thresh_temp > > > > emergency_alarm -> temp >= bp->fatal_thresh_temp > > > > or temp >= bp->shutdown_thresh_temp > > > > > > > > There are only three levels of upper temperature alarms. > > > > Abusing lcrit as 4th upper alarm is most definitely wrong. > > > > > > > [Kalesh]: Thank you for the clarification. > > > bnxt_en driver wants to expose 4 threshold temperatures to the user > > through > > > hwmon sysfs. > > > 1. warning threshold temperature > > > 2. critical threshold temperature > > > 3. fatal threshold temperature > > > 4. shutdown threshold temperature > > > > > > I will use the following mapping: > > > > > > hwmon_temp_max : warning threshold temperature > > > hwmon_temp_crit : critical threshold temperature > > > hwmon_temp_emergency : fatal threshold temperature > > > > > > hwmon_temp_max_alarm : temp >= bp->warn_thresh_temp > > > hwmon_temp_crit_alarm : temp >= bp->crit_thresh_temp > > > hwmon_temp_emergency_alarm : temp >= bp->fatal_thresh_temp > > > > > > Is it OK to map the shutdown threshold temperature to "hwmon_temp_fault"? > > > > That is a flag, not a temperature, and it is intended to signal > > a problem ith the sensor. > > > > > If not, can you please suggest an alternative? > > > > > > > The only one I can think of is to add non-standard attributes > > such as temp1_shutdown and temp1_shutdown_alarm. > > > [Kalesh]: Sorry, I don't quite get this part. I was looking at the kernel > hwmon code, but could not find any reference. > It would be non-standard attributes, so, correct, there is no reference. > Can we add new attributes "shutdown" and "shutdown_alarm" for tempX? For > example: > > #define HWMON_T_SHUTDOWN BIT(hwmon_temp_shutdown) > Not for a single driver. You can implement the sysfs attributes directly in the driver and pass an extra attribute group to the hwmon core when registering the hwmon device. Guenter
