| Message ID | 20171214103011.24713-4-miquel.raynal@free-electrons.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Miquel, On jeu., déc. 14 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > All Armada SoCs use one bit to declare if the sensor values are valid. > This bit moves across the versions of the IP. > > The method until then was to do both a shift and compare with an useless > flag of "0x1". It is clearer and quicker to directly save the value that > must be ANDed instead of the bit position and do a single bitwise AND > operation. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Thanks, Gregory > --- > drivers/thermal/armada_thermal.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > index 6c4af2622d4f..26698f2d3ca7 100644 > --- a/drivers/thermal/armada_thermal.c > +++ b/drivers/thermal/armada_thermal.c > @@ -24,8 +24,6 @@ > #include <linux/of_device.h> > #include <linux/thermal.h> > > -#define THERMAL_VALID_MASK 0x1 > - > /* Thermal Manager Control and Status Register */ > #define PMU_TDC0_SW_RST_MASK (0x1 << 1) > #define PMU_TM_DISABLE_OFFS 0 > @@ -67,7 +65,7 @@ struct armada_thermal_data { > /* Register shift and mask to access the sensor temperature */ > unsigned int temp_shift; > unsigned int temp_mask; > - unsigned int is_valid_shift; > + unsigned int is_valid_bit; > }; > > static void armadaxp_init_sensor(struct platform_device *pdev, > @@ -151,7 +149,7 @@ static bool armada_is_valid(struct armada_thermal_priv *priv) > { > unsigned long reg = readl_relaxed(priv->sensor); > > - return (reg >> priv->data->is_valid_shift) & THERMAL_VALID_MASK; > + return reg & priv->data->is_valid_bit; > } > > static int armada_get_temp(struct thermal_zone_device *thermal, > @@ -199,7 +197,7 @@ static const struct armada_thermal_data armadaxp_data = { > static const struct armada_thermal_data armada370_data = { > .is_valid = armada_is_valid, > .init_sensor = armada370_init_sensor, > - .is_valid_shift = 9, > + .is_valid_bit = BIT(9), > .temp_shift = 10, > .temp_mask = 0x1ff, > .coef_b = 3153000000UL, > @@ -210,7 +208,7 @@ static const struct armada_thermal_data armada370_data = { > static const struct armada_thermal_data armada375_data = { > .is_valid = armada_is_valid, > .init_sensor = armada375_init_sensor, > - .is_valid_shift = 10, > + .is_valid_bit = BIT(10), > .temp_shift = 0, > .temp_mask = 0x1ff, > .coef_b = 3171900000UL, > @@ -221,7 +219,7 @@ static const struct armada_thermal_data armada375_data = { > static const struct armada_thermal_data armada380_data = { > .is_valid = armada_is_valid, > .init_sensor = armada380_init_sensor, > - .is_valid_shift = 10, > + .is_valid_bit = BIT(10), > .temp_shift = 0, > .temp_mask = 0x3ff, > .coef_b = 1172499100UL, > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Miquel, On Thu, Dec 14, 2017 at 11:30:03AM +0100, Miquel Raynal wrote: > All Armada SoCs use one bit to declare if the sensor values are valid. > This bit moves across the versions of the IP. > > The method until then was to do both a shift and compare with an useless > flag of "0x1". It is clearer and quicker to directly save the value that > must be ANDed instead of the bit position and do a single bitwise AND > operation. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > drivers/thermal/armada_thermal.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > index 6c4af2622d4f..26698f2d3ca7 100644 > --- a/drivers/thermal/armada_thermal.c > +++ b/drivers/thermal/armada_thermal.c > @@ -24,8 +24,6 @@ > #include <linux/of_device.h> > #include <linux/thermal.h> > > -#define THERMAL_VALID_MASK 0x1 > - > /* Thermal Manager Control and Status Register */ > #define PMU_TDC0_SW_RST_MASK (0x1 << 1) > #define PMU_TM_DISABLE_OFFS 0 > @@ -67,7 +65,7 @@ struct armada_thermal_data { > /* Register shift and mask to access the sensor temperature */ > unsigned int temp_shift; > unsigned int temp_mask; > - unsigned int is_valid_shift; > + unsigned int is_valid_bit; Type should be u32 now, I think. > }; > > static void armadaxp_init_sensor(struct platform_device *pdev, > @@ -151,7 +149,7 @@ static bool armada_is_valid(struct armada_thermal_priv *priv) > { > unsigned long reg = readl_relaxed(priv->sensor); u32 here also, I think. But that's unrelated to this patch. > - return (reg >> priv->data->is_valid_shift) & THERMAL_VALID_MASK; > + return reg & priv->data->is_valid_bit; > } > > static int armada_get_temp(struct thermal_zone_device *thermal, > @@ -199,7 +197,7 @@ static const struct armada_thermal_data armadaxp_data = { > static const struct armada_thermal_data armada370_data = { > .is_valid = armada_is_valid, > .init_sensor = armada370_init_sensor, > - .is_valid_shift = 9, > + .is_valid_bit = BIT(9), > .temp_shift = 10, > .temp_mask = 0x1ff, > .coef_b = 3153000000UL, > @@ -210,7 +208,7 @@ static const struct armada_thermal_data armada370_data = { > static const struct armada_thermal_data armada375_data = { > .is_valid = armada_is_valid, > .init_sensor = armada375_init_sensor, > - .is_valid_shift = 10, > + .is_valid_bit = BIT(10), > .temp_shift = 0, > .temp_mask = 0x1ff, > .coef_b = 3171900000UL, > @@ -221,7 +219,7 @@ static const struct armada_thermal_data armada375_data = { > static const struct armada_thermal_data armada380_data = { > .is_valid = armada_is_valid, > .init_sensor = armada380_init_sensor, > - .is_valid_shift = 10, > + .is_valid_bit = BIT(10), > .temp_shift = 0, > .temp_mask = 0x3ff, > .coef_b = 1172499100UL, baruch
Hello Baruch, On Fri, 15 Dec 2017 10:33:16 +0200 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Miquel, > > On Thu, Dec 14, 2017 at 11:30:03AM +0100, Miquel Raynal wrote: > > All Armada SoCs use one bit to declare if the sensor values are > > valid. This bit moves across the versions of the IP. > > > > The method until then was to do both a shift and compare with an > > useless flag of "0x1". It is clearer and quicker to directly save > > the value that must be ANDed instead of the bit position and do a > > single bitwise AND operation. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > > --- > > drivers/thermal/armada_thermal.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/thermal/armada_thermal.c > > b/drivers/thermal/armada_thermal.c index 6c4af2622d4f..26698f2d3ca7 > > 100644 --- a/drivers/thermal/armada_thermal.c > > +++ b/drivers/thermal/armada_thermal.c > > @@ -24,8 +24,6 @@ > > #include <linux/of_device.h> > > #include <linux/thermal.h> > > > > -#define THERMAL_VALID_MASK 0x1 > > - > > /* Thermal Manager Control and Status Register */ > > #define PMU_TDC0_SW_RST_MASK (0x1 << 1) > > #define PMU_TM_DISABLE_OFFS 0 > > @@ -67,7 +65,7 @@ struct armada_thermal_data { > > /* Register shift and mask to access the sensor > > temperature */ unsigned int temp_shift; > > unsigned int temp_mask; > > - unsigned int is_valid_shift; > > + unsigned int is_valid_bit; > > Type should be u32 now, I think. Indeed, I'll change it in next version. > > > }; > > > > static void armadaxp_init_sensor(struct platform_device *pdev, > > @@ -151,7 +149,7 @@ static bool armada_is_valid(struct > > armada_thermal_priv *priv) { > > unsigned long reg = readl_relaxed(priv->sensor); > > u32 here also, I think. But that's unrelated to this patch. I completely agree, actually this is done in the next patch, moving all "unsigned long reg" to "u32 reg" while renaming the registers pointers, hope this is fine? > > > - return (reg >> priv->data->is_valid_shift) & > > THERMAL_VALID_MASK; > > + return reg & priv->data->is_valid_bit; > > } > > > > static int armada_get_temp(struct thermal_zone_device *thermal, > > @@ -199,7 +197,7 @@ static const struct armada_thermal_data > > armadaxp_data = { static const struct armada_thermal_data > > armada370_data = { .is_valid = armada_is_valid, > > .init_sensor = armada370_init_sensor, > > - .is_valid_shift = 9, > > + .is_valid_bit = BIT(9), > > .temp_shift = 10, > > .temp_mask = 0x1ff, > > .coef_b = 3153000000UL, > > @@ -210,7 +208,7 @@ static const struct armada_thermal_data > > armada370_data = { static const struct armada_thermal_data > > armada375_data = { .is_valid = armada_is_valid, > > .init_sensor = armada375_init_sensor, > > - .is_valid_shift = 10, > > + .is_valid_bit = BIT(10), > > .temp_shift = 0, > > .temp_mask = 0x1ff, > > .coef_b = 3171900000UL, > > @@ -221,7 +219,7 @@ static const struct armada_thermal_data > > armada375_data = { static const struct armada_thermal_data > > armada380_data = { .is_valid = armada_is_valid, > > .init_sensor = armada380_init_sensor, > > - .is_valid_shift = 10, > > + .is_valid_bit = BIT(10), > > .temp_shift = 0, > > .temp_mask = 0x3ff, > > .coef_b = 1172499100UL, > > baruch > Best regards, Miquèl
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c index 6c4af2622d4f..26698f2d3ca7 100644 --- a/drivers/thermal/armada_thermal.c +++ b/drivers/thermal/armada_thermal.c @@ -24,8 +24,6 @@ #include <linux/of_device.h> #include <linux/thermal.h> -#define THERMAL_VALID_MASK 0x1 - /* Thermal Manager Control and Status Register */ #define PMU_TDC0_SW_RST_MASK (0x1 << 1) #define PMU_TM_DISABLE_OFFS 0 @@ -67,7 +65,7 @@ struct armada_thermal_data { /* Register shift and mask to access the sensor temperature */ unsigned int temp_shift; unsigned int temp_mask; - unsigned int is_valid_shift; + unsigned int is_valid_bit; }; static void armadaxp_init_sensor(struct platform_device *pdev, @@ -151,7 +149,7 @@ static bool armada_is_valid(struct armada_thermal_priv *priv) { unsigned long reg = readl_relaxed(priv->sensor); - return (reg >> priv->data->is_valid_shift) & THERMAL_VALID_MASK; + return reg & priv->data->is_valid_bit; } static int armada_get_temp(struct thermal_zone_device *thermal, @@ -199,7 +197,7 @@ static const struct armada_thermal_data armadaxp_data = { static const struct armada_thermal_data armada370_data = { .is_valid = armada_is_valid, .init_sensor = armada370_init_sensor, - .is_valid_shift = 9, + .is_valid_bit = BIT(9), .temp_shift = 10, .temp_mask = 0x1ff, .coef_b = 3153000000UL, @@ -210,7 +208,7 @@ static const struct armada_thermal_data armada370_data = { static const struct armada_thermal_data armada375_data = { .is_valid = armada_is_valid, .init_sensor = armada375_init_sensor, - .is_valid_shift = 10, + .is_valid_bit = BIT(10), .temp_shift = 0, .temp_mask = 0x1ff, .coef_b = 3171900000UL, @@ -221,7 +219,7 @@ static const struct armada_thermal_data armada375_data = { static const struct armada_thermal_data armada380_data = { .is_valid = armada_is_valid, .init_sensor = armada380_init_sensor, - .is_valid_shift = 10, + .is_valid_bit = BIT(10), .temp_shift = 0, .temp_mask = 0x3ff, .coef_b = 1172499100UL,
All Armada SoCs use one bit to declare if the sensor values are valid. This bit moves across the versions of the IP. The method until then was to do both a shift and compare with an useless flag of "0x1". It is clearer and quicker to directly save the value that must be ANDed instead of the bit position and do a single bitwise AND operation. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- drivers/thermal/armada_thermal.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)