On Sun, Oct 06, 2019 at 11:11:08AM -0400, William Breathitt Gray wrote:
> Utilize for_each_set_clump8 macro, and the bitmap_set_value8 and
> bitmap_get_value8 functions, where appropriate. In addition, remove the
> now unnecessary temp_mask and temp_shift members of the
> intel_soc_dts_sensor_entry structure.
Since it perhaps will be next version, I have few style comments here
(ignore them if you are not going to send a new version by some other reasons).
> int status;
> u32 temp_out;
> + unsigned long update_ptps;
I think it's better to put it one line below.
> u32 out;
> u32 store_ptps;
> u32 store_ptmc;
> - out = (store_ptps & ~(0xFF << (thres_index * 8)));
> - out |= (temp_out & 0xFF) << (thres_index * 8);
> + update_ptps = store_ptps;
> + bitmap_set_value8(&update_ptps, temp_out & 0xFF, thres_index * 8);
> + out = update_ptps;
+ blank line?
After this change it seems we may drop temp_out and use out instead.
> - out = (out & dts->temp_mask) >> dts->temp_shift;
> + temp_raw = out;
> + out = bitmap_get_value8(&temp_raw, dts->id * 8);
> out -= SOC_DTS_TJMAX_ENCODING;
> *temp = sensors->tj_max - out * 1000;
We may also join these together, though it's up to you.
> char name[10];
> int trip_count = 0;
> + int writable_trip_count = 0;
Perhaps move it after next line, or before previous one.
> int trip_mask = 0;
> u32 store_ptps;
> int ret;
> - int i;
> + unsigned long i;
We may skip this change, but if we go with it, better to place before
'int ret;' line.
> + unsigned long trip;
> + unsigned long ptps;
I would group each of these with relative group of definitions above.
> if (notification_support) {
> trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
> + writable_trip_count = trip_count - read_only_trip_cnt;
Maybe writable_trip_count -> writable_trip_cnt? (in align with r/o one).
> + trip_mask = GENMASK(writable_trip_count - 1, 0);
> }
--
With Best Regards,
Andy Shevchenko
