Messages in this thread | | | Date | Mon, 29 Apr 2024 06:49:21 -0700 | Subject | Re: [PATCH 1/4] hwmon (max6639): Use regmap | From | Guenter Roeck <> |
| |
On 4/29/24 01:19, Naresh Solanki wrote: > Hi Guenter, > > > On Sun, 28 Apr 2024 at 22:48, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 4/25/24 02:50, Naresh Solanki wrote: >> ... >>> This driver has 27 regmap accesses. Except volatile registers, others are >>> cached by regmap. >>> Some function which only access volatile registers will not be able to take >>> advantage of caching. This is also the case in various other drivers for similar >>> devices. >>> Also regmap offers bit handling which makes the code much cleaner. >>> >> >> Maybe I need to make it explicit in documentation. I will not accept regmap >> conversions unless local caching is dropped. Yes, that means that volatile >> registers will not be cached. I consider that a positive. > I agree with you. Regmap conversion wouldn't make sense if local caching > is present. > Correct me if I'm wrong, but in this context, local caching points to the > various variables in max6639_data ? > i.e., > bool valid; /* true if following fields are valid */ > unsigned long last_updated; /* In jiffies */ > > /* Register values sampled regularly */ > u16 temp[2]; /* Temperature, in 1/8 C, 0..255 C */ > bool temp_fault[2]; /* Detected temperature diode failure */ > u8 fan[2]; /* Register value: TACH count for fans >=30 */ > u8 status; /* Detected channel alarms and fan failures */ > > /* Register values only written to */ > u8 pwm[2]; /* Register value: Duty cycle 0..120 */ > u8 temp_therm[2]; /* THERM Temperature, 0..255 C (->_max) */ > u8 temp_alert[2]; /* ALERT Temperature, 0..255 C (->_crit) */ > u8 temp_ot[2]; /* OT Temperature, 0..255 C (->_emergency) */ > > /* Register values initialized only once */ > u8 ppr; /* Pulses per rotation 0..3 for 1..4 ppr */ > u8 rpm_range; /* Index in above rpm_ranges table */ > > Are you asking for removal of all these variables & each read sysfs > attribute read should access regmap cache directly ? >
Mostly yes. Note that "ppr" is only used in max6639_init_client(), and it is unnecessary to keep it in max6639_data to start with. rpm_range is ok to keep because it is a calculated initialization value. The fixed initialization of temp_therm, temp_alert, and temp_ot is questionable to start with because it overrides bios/rommon initialization values.
Guenter
| |