diff mbox series

iio: pac1921: remove unnecessary explicit casts

Message ID 20240916-iio-pac1921-nocast-v1-1-a0f96d321eee@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: pac1921: remove unnecessary explicit casts | expand

Commit Message

Matteo Martelli Sept. 16, 2024, noon UTC 
Many explicit casts were introduced to address Wconversion and
Wsign-compare warnings. Remove them to improve readability.

Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
Link: https://lore.kernel.org/linux-iio/1fa4ab12-0939-477d-bc92-306fd32e4fd9@stanley.mountain/
---
 drivers/iio/adc/pac1921.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)


---
base-commit: fec496684388685647652ab4213454fbabdab099
change-id: 20240911-iio-pac1921-nocast-5c98cdeec059

Best regards,

Comments

Jonathan Cameron Sept. 28, 2024, 3:28 p.m. UTC | #1 
On Mon, 16 Sep 2024 14:00:05 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Many explicit casts were introduced to address Wconversion and
> Wsign-compare warnings. Remove them to improve readability.
> 
> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")

No fixes tag on this one. Its not a bug, just a readability improvement.

> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
There are a few cases in here where I think the cast is about ensuring
we don't overflow in the maths rather than for warning suppression.

We all love 32 bit architectures after all ;)

Jonathan

> ---
> Link: https://lore.kernel.org/linux-iio/1fa4ab12-0939-477d-bc92-306fd32e4fd9@stanley.mountain/
> ---
>  drivers/iio/adc/pac1921.c | 43 +++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index 4c2a1c07bc39..de69a1619a9e 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -240,8 +240,8 @@ static inline void pac1921_calc_scale(int dividend, int divisor, int *val,
>  {
>  	s64 tmp;
>  
> -	tmp = div_s64(dividend * (s64)NANO, divisor);
> -	*val = (int)div_s64_rem(tmp, NANO, val2);
> +	tmp = div_s64(dividend * NANO, divisor);

For this one, NANO is an unsigned long and dividend just an int.
Either the s64 cast is needed because dividend * NANO might go out of
unsigned long range which might (I think) be 32 bit on a 32bit machine
or it doesn't.  If it does, then you need the cast, if not you don't
need to use div_s64 as it's not that large.

> +	*val = div_s64_rem(tmp, NANO, val2);
>  }
>  
>  /*
> @@ -260,7 +260,7 @@ static void pac1921_calc_current_scales(struct pac1921_priv *priv)
>  		int max = (PAC1921_MAX_VSENSE_MV * MICRO) >> i;
>  		int vsense_lsb = DIV_ROUND_CLOSEST(max, PAC1921_RES_RESOLUTION);
>  
> -		pac1921_calc_scale(vsense_lsb, (int)priv->rshunt_uohm,
> +		pac1921_calc_scale(vsense_lsb, priv->rshunt_uohm,
>  				   &priv->current_scales[i][0],
>  				   &priv->current_scales[i][1]);
>  	}
> @@ -314,7 +314,7 @@ static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp)
>  			       timestamp);
>  	}
>  
> -	priv->prev_ovf_flags = (u8)flags;
> +	priv->prev_ovf_flags = flags;
>  
>  	return 0;
>  }
> @@ -329,8 +329,7 @@ static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp)
>  static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg,
>  			    u16 *val)
>  {
> -	int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, val,
> -				   sizeof(*val));
> +	int ret = regmap_bulk_read(priv->regmap, reg, val, sizeof(*val));
>  	if (ret)
>  		return ret;
>  
> @@ -366,7 +365,7 @@ static int pac1921_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		*val = (int)res_val;
> +		*val = res_val;
>  
>  		return IIO_VAL_INT;
>  	}
> @@ -397,13 +396,13 @@ static int pac1921_read_raw(struct iio_dev *indio_dev,
>  			int *curr_scale = priv->current_scales[priv->di_gain];
>  
>  			/* Convert current_scale from INT_PLUS_NANO to INT */
> -			s64 tmp = curr_scale[0] * (s64)NANO + curr_scale[1];
> +			s64 tmp = curr_scale[0] * NANO + curr_scale[1];
Same potential issue as above. NANO and curr_scale might be 32 bit in which
case I believe this can overflow?

>  
>  			/* Multiply by max_vbus (V) / dv_gain */
> -			tmp *= PAC1921_MAX_VBUS_V >> (int)priv->dv_gain;
> +			tmp *= PAC1921_MAX_VBUS_V >> priv->dv_gain;
>  
>  			/* Convert back to INT_PLUS_NANO */
> -			*val = (int)div_s64_rem(tmp, NANO, val2);
> +			*val = div_s64_rem(tmp, NANO, val2);
>  
>  			return IIO_VAL_INT_PLUS_NANO;
>  		}
...

> @@ -607,7 +606,7 @@ static int pac1921_update_int_num_samples(struct pac1921_priv *priv,
>  	if (ret < 0)
>  		return ret;
>  
> -	n_samples = (u8)ret;
> +	n_samples = ret;
>  
>  	if (priv->n_samples == n_samples)
>  		return 0;
> @@ -770,7 +769,7 @@ static ssize_t pac1921_read_shunt_resistor(struct iio_dev *indio_dev,
>  
>  	guard(mutex)(&priv->lock);
>  
> -	vals[0] = (int)priv->rshunt_uohm;
> +	vals[0] = priv->rshunt_uohm;
>  	vals[1] = MICRO;
>  
>  	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
> @@ -793,13 +792,13 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> -	rshunt_uohm = (u32)val * MICRO + (u32)val_fract;
> +	rshunt_uohm = val * MICRO + val_fract;
>  	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
>  		return -EINVAL;
>  
>  	guard(mutex)(&priv->lock);
>  
> -	priv->rshunt_uohm = (u32)rshunt_uohm;
> +	priv->rshunt_uohm = rshunt_uohm;
>  
>  	pac1921_calc_current_scales(priv);
>  
> @@ -1168,7 +1167,7 @@ static int pac1921_probe(struct i2c_client *client)
>  
>  	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
>  	if (IS_ERR(priv->regmap))
> -		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
>  				     "Cannot initialize register map\n");
>  
>  	devm_mutex_init(dev, &priv->lock);
> @@ -1191,7 +1190,7 @@ static int pac1921_probe(struct i2c_client *client)
>  
>  	priv->vdd = devm_regulator_get(dev, "vdd");
>  	if (IS_ERR(priv->vdd))
> -		return dev_err_probe(dev, (int)PTR_ERR(priv->vdd),
> +		return dev_err_probe(dev, PTR_ERR(priv->vdd),
>  				     "Cannot get vdd regulator\n");
>  
>  	ret = regulator_enable(priv->vdd);
> 
> ---
> base-commit: fec496684388685647652ab4213454fbabdab099
> change-id: 20240911-iio-pac1921-nocast-5c98cdeec059
> 
> Best regards,
Matteo Martelli Sept. 30, 2024, 9:32 a.m. UTC | #2 
Quoting Jonathan Cameron (2024-09-28 17:28:43)
> On Mon, 16 Sep 2024 14:00:05 +0200
> Matteo Martelli <matteomartelli3@gmail.com> wrote:
> 
> > Many explicit casts were introduced to address Wconversion and
> > Wsign-compare warnings. Remove them to improve readability.
> > 
> > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> 
> No fixes tag on this one. Its not a bug, just a readability improvement.
> 
> > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> There are a few cases in here where I think the cast is about ensuring
> we don't overflow in the maths rather than for warning suppression.
> 
> We all love 32 bit architectures after all ;)
> 
> Jonathan
> 
> > ---
> > Link: https://lore.kernel.org/linux-iio/1fa4ab12-0939-477d-bc92-306fd32e4fd9@stanley.mountain/
> > ---
> >  drivers/iio/adc/pac1921.c | 43 +++++++++++++++++++++----------------------
> >  1 file changed, 21 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > index 4c2a1c07bc39..de69a1619a9e 100644
> > --- a/drivers/iio/adc/pac1921.c
> > +++ b/drivers/iio/adc/pac1921.c
> > @@ -240,8 +240,8 @@ static inline void pac1921_calc_scale(int dividend, int divisor, int *val,
> >  {
> >       s64 tmp;
> >  
> > -     tmp = div_s64(dividend * (s64)NANO, divisor);
> > -     *val = (int)div_s64_rem(tmp, NANO, val2);
> > +     tmp = div_s64(dividend * NANO, divisor);
> 
> For this one, NANO is an unsigned long and dividend just an int.
> Either the s64 cast is needed because dividend * NANO might go out of
> unsigned long range which might (I think) be 32 bit on a 32bit machine
> or it doesn't.  If it does, then you need the cast, if not you don't
> need to use div_s64 as it's not that large.

Oops! While removing the casts I was only thinking about the sign change
for this case, which would not be an issue being the dividend always
positive. However you are indeed right, this would overflow in 32-bit
architectures. Thanks for catching it, I am going to reintroduce this
cast and the other similar one in the next patch version.

...

Thanks,
Matteo Martelli
diff mbox series

Patch

diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index 4c2a1c07bc39..de69a1619a9e 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -240,8 +240,8 @@  static inline void pac1921_calc_scale(int dividend, int divisor, int *val,
 {
 	s64 tmp;
 
-	tmp = div_s64(dividend * (s64)NANO, divisor);
-	*val = (int)div_s64_rem(tmp, NANO, val2);
+	tmp = div_s64(dividend * NANO, divisor);
+	*val = div_s64_rem(tmp, NANO, val2);
 }
 
 /*
@@ -260,7 +260,7 @@  static void pac1921_calc_current_scales(struct pac1921_priv *priv)
 		int max = (PAC1921_MAX_VSENSE_MV * MICRO) >> i;
 		int vsense_lsb = DIV_ROUND_CLOSEST(max, PAC1921_RES_RESOLUTION);
 
-		pac1921_calc_scale(vsense_lsb, (int)priv->rshunt_uohm,
+		pac1921_calc_scale(vsense_lsb, priv->rshunt_uohm,
 				   &priv->current_scales[i][0],
 				   &priv->current_scales[i][1]);
 	}
@@ -314,7 +314,7 @@  static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp)
 			       timestamp);
 	}
 
-	priv->prev_ovf_flags = (u8)flags;
+	priv->prev_ovf_flags = flags;
 
 	return 0;
 }
@@ -329,8 +329,7 @@  static int pac1921_check_push_overflow(struct iio_dev *indio_dev, s64 timestamp)
 static int pac1921_read_res(struct pac1921_priv *priv, unsigned long reg,
 			    u16 *val)
 {
-	int ret = regmap_bulk_read(priv->regmap, (unsigned int)reg, val,
-				   sizeof(*val));
+	int ret = regmap_bulk_read(priv->regmap, reg, val, sizeof(*val));
 	if (ret)
 		return ret;
 
@@ -366,7 +365,7 @@  static int pac1921_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		*val = (int)res_val;
+		*val = res_val;
 
 		return IIO_VAL_INT;
 	}
@@ -397,13 +396,13 @@  static int pac1921_read_raw(struct iio_dev *indio_dev,
 			int *curr_scale = priv->current_scales[priv->di_gain];
 
 			/* Convert current_scale from INT_PLUS_NANO to INT */
-			s64 tmp = curr_scale[0] * (s64)NANO + curr_scale[1];
+			s64 tmp = curr_scale[0] * NANO + curr_scale[1];
 
 			/* Multiply by max_vbus (V) / dv_gain */
-			tmp *= PAC1921_MAX_VBUS_V >> (int)priv->dv_gain;
+			tmp *= PAC1921_MAX_VBUS_V >> priv->dv_gain;
 
 			/* Convert back to INT_PLUS_NANO */
-			*val = (int)div_s64_rem(tmp, NANO, val2);
+			*val = div_s64_rem(tmp, NANO, val2);
 
 			return IIO_VAL_INT_PLUS_NANO;
 		}
@@ -426,7 +425,7 @@  static int pac1921_read_raw(struct iio_dev *indio_dev,
 		 * 1/(integr_period_usecs/MICRO) = MICRO/integr_period_usecs
 		 */
 		*val = MICRO;
-		*val2 = (int)priv->integr_period_usecs;
+		*val2 = priv->integr_period_usecs;
 		return IIO_VAL_FRACTIONAL;
 
 	default:
@@ -503,7 +502,7 @@  static int pac1921_lookup_scale(const int (*const scales_tbl)[2], size_t size,
 	for (unsigned int i = 0; i < size; i++)
 		if (scales_tbl[i][0] == scale_val &&
 		    scales_tbl[i][1] == scale_val2)
-			return (int)i;
+			return i;
 
 	return -EINVAL;
 }
@@ -553,7 +552,7 @@  static int pac1921_update_gain_from_scale(struct pac1921_priv *priv,
 		if (ret < 0)
 			return ret;
 
-		return pac1921_update_gain(priv, &priv->dv_gain, (u8)ret,
+		return pac1921_update_gain(priv, &priv->dv_gain, ret,
 					   PAC1921_GAIN_DV_GAIN_MASK);
 	case PAC1921_CHAN_VSENSE:
 		ret = pac1921_lookup_scale(pac1921_vsense_scales,
@@ -562,7 +561,7 @@  static int pac1921_update_gain_from_scale(struct pac1921_priv *priv,
 		if (ret < 0)
 			return ret;
 
-		return pac1921_update_gain(priv, &priv->di_gain, (u8)ret,
+		return pac1921_update_gain(priv, &priv->di_gain, ret,
 					   PAC1921_GAIN_DI_GAIN_MASK);
 	case PAC1921_CHAN_CURRENT:
 		ret = pac1921_lookup_scale(priv->current_scales,
@@ -571,7 +570,7 @@  static int pac1921_update_gain_from_scale(struct pac1921_priv *priv,
 		if (ret < 0)
 			return ret;
 
-		return pac1921_update_gain(priv, &priv->di_gain, (u8)ret,
+		return pac1921_update_gain(priv, &priv->di_gain, ret,
 					   PAC1921_GAIN_DI_GAIN_MASK);
 	default:
 		return -EINVAL;
@@ -586,7 +585,7 @@  static int pac1921_lookup_int_num_samples(int num_samples)
 {
 	for (unsigned int i = 0; i < ARRAY_SIZE(pac1921_int_num_samples); i++)
 		if (pac1921_int_num_samples[i] == num_samples)
-			return (int)i;
+			return i;
 
 	return -EINVAL;
 }
@@ -607,7 +606,7 @@  static int pac1921_update_int_num_samples(struct pac1921_priv *priv,
 	if (ret < 0)
 		return ret;
 
-	n_samples = (u8)ret;
+	n_samples = ret;
 
 	if (priv->n_samples == n_samples)
 		return 0;
@@ -770,7 +769,7 @@  static ssize_t pac1921_read_shunt_resistor(struct iio_dev *indio_dev,
 
 	guard(mutex)(&priv->lock);
 
-	vals[0] = (int)priv->rshunt_uohm;
+	vals[0] = priv->rshunt_uohm;
 	vals[1] = MICRO;
 
 	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
@@ -793,13 +792,13 @@  static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 
-	rshunt_uohm = (u32)val * MICRO + (u32)val_fract;
+	rshunt_uohm = val * MICRO + val_fract;
 	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
 		return -EINVAL;
 
 	guard(mutex)(&priv->lock);
 
-	priv->rshunt_uohm = (u32)rshunt_uohm;
+	priv->rshunt_uohm = rshunt_uohm;
 
 	pac1921_calc_current_scales(priv);
 
@@ -1168,7 +1167,7 @@  static int pac1921_probe(struct i2c_client *client)
 
 	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
 	if (IS_ERR(priv->regmap))
-		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
+		return dev_err_probe(dev, PTR_ERR(priv->regmap),
 				     "Cannot initialize register map\n");
 
 	devm_mutex_init(dev, &priv->lock);
@@ -1191,7 +1190,7 @@  static int pac1921_probe(struct i2c_client *client)
 
 	priv->vdd = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(priv->vdd))
-		return dev_err_probe(dev, (int)PTR_ERR(priv->vdd),
+		return dev_err_probe(dev, PTR_ERR(priv->vdd),
 				     "Cannot get vdd regulator\n");
 
 	ret = regulator_enable(priv->vdd);