[v10,10/10] thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro
diff mbox series

Message ID a240216a143782e3cfecf6f963578067b68b6007.1552566114.git.vilhelm.gray@gmail.com
State New
Headers show
Series
  • Introduce the for_each_set_clump8 macro
Related show

Commit Message

William Breathitt Gray March 14, 2019, 12:32 p.m. UTC
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.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 drivers/thermal/intel/intel_soc_dts_iosf.c | 29 +++++++++++++---------
 drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
 2 files changed, 17 insertions(+), 14 deletions(-)

Comments

Andy Shevchenko March 14, 2019, 2:26 p.m. UTC | #1
On Thu, Mar 14, 2019 at 09:32:57PM +0900, 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.

I think you need to Cc thermal maintainers.

I will test this next week.
Thanks!

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Suggested-by?

> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  drivers/thermal/intel/intel_soc_dts_iosf.c | 29 +++++++++++++---------
>  drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> index e0813dfaa278..b66f554670f7 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> @@ -15,6 +15,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/bitops.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
> @@ -111,6 +112,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
>  {
>  	int status;
>  	u32 temp_out;
> +	unsigned long update_ptps;
>  	u32 out;
>  	u32 store_ptps;
>  	u32 store_ptmc;
> @@ -129,8 +131,9 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
>  	if (status)
>  		return status;
>  
> -	out = (store_ptps & ~(0xFF << (thres_index * 8)));
> -	out |= (temp_out & 0xFF) << (thres_index * 8);
> +	update_ptps = store_ptps;
> +	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);
> +	out = update_ptps;
>  	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>  				SOC_DTS_OFFSET_PTPS, out);
>  	if (status)
> @@ -232,6 +235,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>  	u32 out;
>  	struct intel_soc_dts_sensor_entry *dts;
>  	struct intel_soc_dts_sensors *sensors;
> +	unsigned long temp_raw;
>  
>  	dts = tzd->devdata;
>  	sensors = dts->sensors;
> @@ -240,7 +244,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>  	if (status)
>  		return status;
>  
> -	out = (out & dts->temp_mask) >> dts->temp_shift;
> +	temp_raw = out;
> +	out = bitmap_get_value8(&temp_raw, 32, dts->id * 8);
>  	out -= SOC_DTS_TJMAX_ENCODING;
>  	*temp = sensors->tj_max - out * 1000;
>  
> @@ -290,10 +295,13 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  {
>  	char name[10];
>  	int trip_count = 0;
> +	int writable_trip_count = 0;
>  	int trip_mask = 0;
>  	u32 store_ptps;
>  	int ret;
> -	int i;
> +	unsigned int i;
> +	unsigned long trip;
> +	unsigned long ptps;
>  
>  	/* Store status to restor on exit */
>  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> @@ -302,11 +310,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  		goto err_ret;
>  
>  	dts->id = id;
> -	dts->temp_mask = 0x00FF << (id * 8);
> -	dts->temp_shift = id * 8;
>  	if (notification_support) {
>  		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
> -		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
> +		writable_trip_count = trip_count - read_only_trip_cnt;
> +		trip_mask = GENMASK(writable_trip_count - 1, 0);
>  	}
>  
>  	/* Check if the writable trip we provide is not used by BIOS */
> @@ -315,11 +322,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  	if (ret)
>  		trip_mask = 0;
>  	else {
> -		for (i = 0; i < trip_count; ++i) {
> -			if (trip_mask & BIT(i))
> -				if (store_ptps & (0xff << (i * 8)))
> -					trip_mask &= ~BIT(i);
> -		}
> +		ptps = store_ptps;
> +		for_each_set_clump8(i, trip, ptps, writable_trip_count * 8)
> +			trip_mask &= ~BIT(i / 8);
>  	}
>  	dts->trip_mask = trip_mask;
>  	dts->trip_count = trip_count;
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> index 625e37bf93dc..d0362d7acdef 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> @@ -33,8 +33,6 @@ struct intel_soc_dts_sensors;
>  
>  struct intel_soc_dts_sensor_entry {
>  	int id;
> -	u32 temp_mask;
> -	u32 temp_shift;
>  	u32 store_status;
>  	u32 trip_mask;
>  	u32 trip_count;
> -- 
> 2.21.0
>
William Breathitt Gray March 14, 2019, 2:39 p.m. UTC | #2
On Thu, Mar 14, 2019 at 04:26:40PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 14, 2019 at 09:32:57PM +0900, 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.
> 
> I think you need to Cc thermal maintainers.

Ah, you're right, I overlooked them. Should I resend this patchset, or
add them to the CC list on the next version?

William Breathitt Gray

> 
> I will test this next week.
> Thanks!
> 
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Suggested-by?
> 
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >  drivers/thermal/intel/intel_soc_dts_iosf.c | 29 +++++++++++++---------
> >  drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
> >  2 files changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> > index e0813dfaa278..b66f554670f7 100644
> > --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> > +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> > @@ -15,6 +15,7 @@
> >  
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> > +#include <linux/bitops.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > @@ -111,6 +112,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >  {
> >  	int status;
> >  	u32 temp_out;
> > +	unsigned long update_ptps;
> >  	u32 out;
> >  	u32 store_ptps;
> >  	u32 store_ptmc;
> > @@ -129,8 +131,9 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >  	if (status)
> >  		return status;
> >  
> > -	out = (store_ptps & ~(0xFF << (thres_index * 8)));
> > -	out |= (temp_out & 0xFF) << (thres_index * 8);
> > +	update_ptps = store_ptps;
> > +	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);
> > +	out = update_ptps;
> >  	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> >  				SOC_DTS_OFFSET_PTPS, out);
> >  	if (status)
> > @@ -232,6 +235,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> >  	u32 out;
> >  	struct intel_soc_dts_sensor_entry *dts;
> >  	struct intel_soc_dts_sensors *sensors;
> > +	unsigned long temp_raw;
> >  
> >  	dts = tzd->devdata;
> >  	sensors = dts->sensors;
> > @@ -240,7 +244,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> >  	if (status)
> >  		return status;
> >  
> > -	out = (out & dts->temp_mask) >> dts->temp_shift;
> > +	temp_raw = out;
> > +	out = bitmap_get_value8(&temp_raw, 32, dts->id * 8);
> >  	out -= SOC_DTS_TJMAX_ENCODING;
> >  	*temp = sensors->tj_max - out * 1000;
> >  
> > @@ -290,10 +295,13 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  {
> >  	char name[10];
> >  	int trip_count = 0;
> > +	int writable_trip_count = 0;
> >  	int trip_mask = 0;
> >  	u32 store_ptps;
> >  	int ret;
> > -	int i;
> > +	unsigned int i;
> > +	unsigned long trip;
> > +	unsigned long ptps;
> >  
> >  	/* Store status to restor on exit */
> >  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > @@ -302,11 +310,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  		goto err_ret;
> >  
> >  	dts->id = id;
> > -	dts->temp_mask = 0x00FF << (id * 8);
> > -	dts->temp_shift = id * 8;
> >  	if (notification_support) {
> >  		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
> > -		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
> > +		writable_trip_count = trip_count - read_only_trip_cnt;
> > +		trip_mask = GENMASK(writable_trip_count - 1, 0);
> >  	}
> >  
> >  	/* Check if the writable trip we provide is not used by BIOS */
> > @@ -315,11 +322,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  	if (ret)
> >  		trip_mask = 0;
> >  	else {
> > -		for (i = 0; i < trip_count; ++i) {
> > -			if (trip_mask & BIT(i))
> > -				if (store_ptps & (0xff << (i * 8)))
> > -					trip_mask &= ~BIT(i);
> > -		}
> > +		ptps = store_ptps;
> > +		for_each_set_clump8(i, trip, ptps, writable_trip_count * 8)
> > +			trip_mask &= ~BIT(i / 8);
> >  	}
> >  	dts->trip_mask = trip_mask;
> >  	dts->trip_count = trip_count;
> > diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> > index 625e37bf93dc..d0362d7acdef 100644
> > --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> > +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> > @@ -33,8 +33,6 @@ struct intel_soc_dts_sensors;
> >  
> >  struct intel_soc_dts_sensor_entry {
> >  	int id;
> > -	u32 temp_mask;
> > -	u32 temp_shift;
> >  	u32 store_status;
> >  	u32 trip_mask;
> >  	u32 trip_count;
> > -- 
> > 2.21.0
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko March 22, 2019, 7:02 p.m. UTC | #3
On Thu, Mar 14, 2019 at 09:32:57PM +0900, 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.

One comment below, otherwise
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thanks!

> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  drivers/thermal/intel/intel_soc_dts_iosf.c | 29 +++++++++++++---------
>  drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> index e0813dfaa278..b66f554670f7 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> @@ -15,6 +15,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/bitops.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
> @@ -111,6 +112,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
>  {
>  	int status;
>  	u32 temp_out;
> +	unsigned long update_ptps;
>  	u32 out;
>  	u32 store_ptps;
>  	u32 store_ptmc;
> @@ -129,8 +131,9 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
>  	if (status)
>  		return status;
>  
> -	out = (store_ptps & ~(0xFF << (thres_index * 8)));
> -	out |= (temp_out & 0xFF) << (thres_index * 8);
> +	update_ptps = store_ptps;
> +	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);
> +	out = update_ptps;
>  	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>  				SOC_DTS_OFFSET_PTPS, out);
>  	if (status)
> @@ -232,6 +235,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>  	u32 out;
>  	struct intel_soc_dts_sensor_entry *dts;
>  	struct intel_soc_dts_sensors *sensors;
> +	unsigned long temp_raw;
>  
>  	dts = tzd->devdata;
>  	sensors = dts->sensors;
> @@ -240,7 +244,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>  	if (status)
>  		return status;
>  
> -	out = (out & dts->temp_mask) >> dts->temp_shift;
> +	temp_raw = out;
> +	out = bitmap_get_value8(&temp_raw, 32, dts->id * 8);
>  	out -= SOC_DTS_TJMAX_ENCODING;
>  	*temp = sensors->tj_max - out * 1000;
>  
> @@ -290,10 +295,13 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  {
>  	char name[10];
>  	int trip_count = 0;
> +	int writable_trip_count = 0;
>  	int trip_mask = 0;
>  	u32 store_ptps;
>  	int ret;
> -	int i;
> +	unsigned int i;
> +	unsigned long trip;
> +	unsigned long ptps;
>  
>  	/* Store status to restor on exit */
>  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> @@ -302,11 +310,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  		goto err_ret;
>  
>  	dts->id = id;
> -	dts->temp_mask = 0x00FF << (id * 8);
> -	dts->temp_shift = id * 8;
>  	if (notification_support) {
>  		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
> -		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
> +		writable_trip_count = trip_count - read_only_trip_cnt;
> +		trip_mask = GENMASK(writable_trip_count - 1, 0);
>  	}
>  
>  	/* Check if the writable trip we provide is not used by BIOS */
> @@ -315,11 +322,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>  	if (ret)
>  		trip_mask = 0;
>  	else {
> -		for (i = 0; i < trip_count; ++i) {
> -			if (trip_mask & BIT(i))
> -				if (store_ptps & (0xff << (i * 8)))
> -					trip_mask &= ~BIT(i);
> -		}
> +		ptps = store_ptps;
> +		for_each_set_clump8(i, trip, ptps, writable_trip_count * 8)

It seems you didn't notice gcc warnings, the ptps should be pointer, i.e. &ptps.
After changing here manually seems working.

> +			trip_mask &= ~BIT(i / 8);
>  	}
>  	dts->trip_mask = trip_mask;
>  	dts->trip_count = trip_count;
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> index 625e37bf93dc..d0362d7acdef 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> @@ -33,8 +33,6 @@ struct intel_soc_dts_sensors;
>  
>  struct intel_soc_dts_sensor_entry {
>  	int id;
> -	u32 temp_mask;
> -	u32 temp_shift;
>  	u32 store_status;
>  	u32 trip_mask;
>  	u32 trip_count;
> -- 
> 2.21.0
>
William Breathitt Gray March 24, 2019, 3:38 a.m. UTC | #4
On Fri, Mar 22, 2019 at 09:02:43PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 14, 2019 at 09:32:57PM +0900, 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.
> 
> One comment below, otherwise
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Thanks!

Thank you for testing out the patch. I'll fix the bug your pointed out
and add the Tested-by line.

I have a couple minor comments below.

> 
> > 
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >  drivers/thermal/intel/intel_soc_dts_iosf.c | 29 +++++++++++++---------
> >  drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
> >  2 files changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> > index e0813dfaa278..b66f554670f7 100644
> > --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> > +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> > @@ -15,6 +15,7 @@
> >  
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> > +#include <linux/bitops.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > @@ -111,6 +112,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >  {
> >  	int status;
> >  	u32 temp_out;
> > +	unsigned long update_ptps;
> >  	u32 out;
> >  	u32 store_ptps;
> >  	u32 store_ptmc;
> > @@ -129,8 +131,9 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >  	if (status)
> >  		return status;
> >  
> > -	out = (store_ptps & ~(0xFF << (thres_index * 8)));
> > -	out |= (temp_out & 0xFF) << (thres_index * 8);
> > +	update_ptps = store_ptps;
> > +	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);

I chose to hardcode a bitmap width of 32 here because that is the width
of the store_ptps variables, but I don't think we necessarily use all 32
bits here. Would the actual width of store_ptps be the number of
writable trip count bits (writable_trip_count * 8)?

> > +	out = update_ptps;
> >  	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> >  				SOC_DTS_OFFSET_PTPS, out);
> >  	if (status)
> > @@ -232,6 +235,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> >  	u32 out;
> >  	struct intel_soc_dts_sensor_entry *dts;
> >  	struct intel_soc_dts_sensors *sensors;
> > +	unsigned long temp_raw;
> >  
> >  	dts = tzd->devdata;
> >  	sensors = dts->sensors;
> > @@ -240,7 +244,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> >  	if (status)
> >  		return status;
> >  
> > -	out = (out & dts->temp_mask) >> dts->temp_shift;
> > +	temp_raw = out;
> > +	out = bitmap_get_value8(&temp_raw, 32, dts->id * 8);

Same question as above: what is the maximum bitmap width here if not 32?

William Breathitt Gray

> >  	out -= SOC_DTS_TJMAX_ENCODING;
> >  	*temp = sensors->tj_max - out * 1000;
> >  
> > @@ -290,10 +295,13 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  {
> >  	char name[10];
> >  	int trip_count = 0;
> > +	int writable_trip_count = 0;
> >  	int trip_mask = 0;
> >  	u32 store_ptps;
> >  	int ret;
> > -	int i;
> > +	unsigned int i;
> > +	unsigned long trip;
> > +	unsigned long ptps;
> >  
> >  	/* Store status to restor on exit */
> >  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > @@ -302,11 +310,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  		goto err_ret;
> >  
> >  	dts->id = id;
> > -	dts->temp_mask = 0x00FF << (id * 8);
> > -	dts->temp_shift = id * 8;
> >  	if (notification_support) {
> >  		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
> > -		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
> > +		writable_trip_count = trip_count - read_only_trip_cnt;
> > +		trip_mask = GENMASK(writable_trip_count - 1, 0);
> >  	}
> >  
> >  	/* Check if the writable trip we provide is not used by BIOS */
> > @@ -315,11 +322,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >  	if (ret)
> >  		trip_mask = 0;
> >  	else {
> > -		for (i = 0; i < trip_count; ++i) {
> > -			if (trip_mask & BIT(i))
> > -				if (store_ptps & (0xff << (i * 8)))
> > -					trip_mask &= ~BIT(i);
> > -		}
> > +		ptps = store_ptps;
> > +		for_each_set_clump8(i, trip, ptps, writable_trip_count * 8)
> 
> It seems you didn't notice gcc warnings, the ptps should be pointer, i.e. &ptps.
> After changing here manually seems working.
> 
> > +			trip_mask &= ~BIT(i / 8);
> >  	}
> >  	dts->trip_mask = trip_mask;
> >  	dts->trip_count = trip_count;
> > diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> > index 625e37bf93dc..d0362d7acdef 100644
> > --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> > +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> > @@ -33,8 +33,6 @@ struct intel_soc_dts_sensors;
> >  
> >  struct intel_soc_dts_sensor_entry {
> >  	int id;
> > -	u32 temp_mask;
> > -	u32 temp_shift;
> >  	u32 store_status;
> >  	u32 trip_mask;
> >  	u32 trip_count;
> > -- 
> > 2.21.0
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko March 24, 2019, 1:52 p.m. UTC | #5
On Sun, Mar 24, 2019 at 12:38:29PM +0900, William Breathitt Gray wrote:
> On Fri, Mar 22, 2019 at 09:02:43PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 14, 2019 at 09:32:57PM +0900, 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.
> > 
> > One comment below, otherwise
> > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Thanks!
> 
> Thank you for testing out the patch. I'll fix the bug your pointed out
> and add the Tested-by line.
> 
> I have a couple minor comments below.

> > >  	u32 store_ptps;

> > > -	out = (store_ptps & ~(0xFF << (thres_index * 8)));
> > > -	out |= (temp_out & 0xFF) << (thres_index * 8);
> > > +	update_ptps = store_ptps;
> > > +	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);
> 
> I chose to hardcode a bitmap width of 32 here because that is the width
> of the store_ptps variables, but I don't think we necessarily use all 32
> bits here. Would the actual width of store_ptps be the number of
> writable trip count bits (writable_trip_count * 8)?

I think 32 is okay, but sizeof(store_ptps) would be better. It's easy to read
since no need to check what is the actual size inside this variable is used.


Same for below.

Patch
diff mbox series

diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
index e0813dfaa278..b66f554670f7 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.c
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
@@ -15,6 +15,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/bitops.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -111,6 +112,7 @@  static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
 {
 	int status;
 	u32 temp_out;
+	unsigned long update_ptps;
 	u32 out;
 	u32 store_ptps;
 	u32 store_ptmc;
@@ -129,8 +131,9 @@  static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
 	if (status)
 		return status;
 
-	out = (store_ptps & ~(0xFF << (thres_index * 8)));
-	out |= (temp_out & 0xFF) << (thres_index * 8);
+	update_ptps = store_ptps;
+	bitmap_set_value8(&update_ptps, 32, temp_out & 0xFF, thres_index * 8);
+	out = update_ptps;
 	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
 				SOC_DTS_OFFSET_PTPS, out);
 	if (status)
@@ -232,6 +235,7 @@  static int sys_get_curr_temp(struct thermal_zone_device *tzd,
 	u32 out;
 	struct intel_soc_dts_sensor_entry *dts;
 	struct intel_soc_dts_sensors *sensors;
+	unsigned long temp_raw;
 
 	dts = tzd->devdata;
 	sensors = dts->sensors;
@@ -240,7 +244,8 @@  static int sys_get_curr_temp(struct thermal_zone_device *tzd,
 	if (status)
 		return status;
 
-	out = (out & dts->temp_mask) >> dts->temp_shift;
+	temp_raw = out;
+	out = bitmap_get_value8(&temp_raw, 32, dts->id * 8);
 	out -= SOC_DTS_TJMAX_ENCODING;
 	*temp = sensors->tj_max - out * 1000;
 
@@ -290,10 +295,13 @@  static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 {
 	char name[10];
 	int trip_count = 0;
+	int writable_trip_count = 0;
 	int trip_mask = 0;
 	u32 store_ptps;
 	int ret;
-	int i;
+	unsigned int i;
+	unsigned long trip;
+	unsigned long ptps;
 
 	/* Store status to restor on exit */
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
@@ -302,11 +310,10 @@  static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 		goto err_ret;
 
 	dts->id = id;
-	dts->temp_mask = 0x00FF << (id * 8);
-	dts->temp_shift = id * 8;
 	if (notification_support) {
 		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
-		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
+		writable_trip_count = trip_count - read_only_trip_cnt;
+		trip_mask = GENMASK(writable_trip_count - 1, 0);
 	}
 
 	/* Check if the writable trip we provide is not used by BIOS */
@@ -315,11 +322,9 @@  static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 	if (ret)
 		trip_mask = 0;
 	else {
-		for (i = 0; i < trip_count; ++i) {
-			if (trip_mask & BIT(i))
-				if (store_ptps & (0xff << (i * 8)))
-					trip_mask &= ~BIT(i);
-		}
+		ptps = store_ptps;
+		for_each_set_clump8(i, trip, ptps, writable_trip_count * 8)
+			trip_mask &= ~BIT(i / 8);
 	}
 	dts->trip_mask = trip_mask;
 	dts->trip_count = trip_count;
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
index 625e37bf93dc..d0362d7acdef 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.h
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
@@ -33,8 +33,6 @@  struct intel_soc_dts_sensors;
 
 struct intel_soc_dts_sensor_entry {
 	int id;
-	u32 temp_mask;
-	u32 temp_shift;
 	u32 store_status;
 	u32 trip_mask;
 	u32 trip_count;