Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sun, 22 May 2022 11:44:33 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Tomas Melin <tomas.melin@vaisala.com>
Cc:     LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>, lars@metafoo.de,
        robh+dt@kernel.org, andy.shevchenko@gmail.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org
Subject: Re: [PATCH V6 3/5] iio: accel: sca3300: modified to support multi
 chips
Message-ID: <20220522114433.6d3257e1@jic23-huawei>
In-Reply-To: <cd5864b3-c436-4a22-663b-703377bf8521@vaisala.com>
References: <20220513124135.1295822-1-Qing-wu.Li@leica-geosystems.com.cn>
        <20220513124135.1295822-4-Qing-wu.Li@leica-geosystems.com.cn>
        <20220514151003.42fa5044@jic23-huawei>
        <cd5864b3-c436-4a22-663b-703377bf8521@vaisala.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

> >> +static int sca3300_set_frequency(struct sca3300_data *data, int val)
> >> +{
> >> +	const struct sca3300_chip_info *chip = data->chip;
> >> +	unsigned int index;
> >> +	unsigned int i;
> >> +
> >> +	if (sca3300_get_op_mode(data, &index))
> >> +		return -EINVAL;
> >> +
> >> +	for (i = 0; i < chip->num_avail_modes; i++) {
> >> +		if ((val == chip->freq_table[chip->freq_map[i]]) &&  
> > 
> > The conditions being checked here are far from obvious, so I think this would benefit
> > from an explanatory comment.
> > 
> > Something along the lines of,
> > "Find a mode in which the requested sampling frequency is available
> >  and the scaling currently set is retained".  
> 
> 
> In addition to a comment, how about small restructure of loop and giving
> local variables that tell the purpose, something like
> 
> 
> ...
> 
> unsigned int opmode_scale, new_scale;
> 
> opmode_scale = chip->accel_scale[chip->accel_scale_map[index]];
> 
> /*
> * Find a mode in which the requested sampling frequency is available
> * and the scaling currently set is retained
> */
> for (i = 0; i < chip->num_avail_modes; i++) {
>     if (val == chip->freq_table[chip->freq_map[i]]) {
>         new_scale = chip->accel_scale[chip->accel_scale_map[i]]);	
>         if (opmode_scale == new_scale)
>             break;
>     }
> }
> 
> 
> That way it's IMHO more clear what we are comparing.
LGTM.

Thanks,

Jonathan

> 
> Thanks,
> Tomas