Re: [PATCH] iio: magnetometer: bmc150: simplify member access formatting

[Jonathan Cameron]

On Sun,  7 Jun 2026 14:21:20 +0000
Hungyu Lin <dennylin0707@xxxxxxxxx> wrote:

> Keep the structure member dereference on a single line to
> improve readability.
> 
> Signed-off-by: Hungyu Lin <dennylin0707@xxxxxxxxx>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index bf2551988008..d2075a18e6a7 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -311,8 +311,7 @@ static int bmc150_magn_set_odr(struct bmc150_magn_data *data, int val)
>  			ret = regmap_update_bits(data->regmap,
>  						 BMC150_MAGN_REG_OPMODE_ODR,
>  						 BMC150_MAGN_MASK_ODR,
> -						 bmc150_magn_samp_freq_table[i].
> -						 reg_val <<
> +						 bmc150_magn_samp_freq_table[i].reg_val <<
>  						 BMC150_MAGN_SHIFT_ODR);
That is indeed ugly!

However, preferred route if we are touching the code is to use field prep for these rather
than explicit shifts (allowing the shift declaration to be dropped!) If you want
to make that change though it needs to be driver wide rather than just for this
one case.

						FIELD_PREP(BMC150_MAGN_MASK_ODR,
							   bmc150_magn_samp_freq_table[i].reg_val),

Given that whole block is very long line, it may be worth looking to see if a broader
refactor can be done to make the whole thing more readable.
For this type of code there is a standard trick - invert the check in the loop
After a bit more tidying up (which is fine in one patch I think given this
is all about improving this function)

static int bmc150_magn_set_odr(struct bmc150_magn_data *data, int val)
{
	for (unsigned int i = 0; i < ARRAY_SIZE(bmc150_magn_samp_freq_table); i++) {
		if (bmc150_magn_samp_freq_table[i].freq != val)
			continue;

		return regmap_update_bits(data->regmap,
					  BMC150_MAGN_REG_OPMODE_ODR,
					  BMC150_MAGN_MASK_ODR,
					  FIELD_PREP(BMC150_MAGN_MASK_ODR,
						     bmc150_magn_samp_freq_table[i].reg_val));
	}

	return -EINVAL;
}

I don't really like the line going that near the absolute limit of 100 but
it seems to me that introducing a local variable would result in slightly
less readable code.

Anyhow, tidy it up along the lies of that code but also apply FIELD_PREP()
FIELD_GET() for all appropriate fields (and check for the relevant includes)

Thanks,

Jonathan



>  			if (ret < 0)
>  				return ret;