Re: [PATCH v16 11/14] thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro

[Andy Shevchenko]

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