Message-ID: <47739cca-db41-4182-9f80-ff138f5b8ec8@tweaklogic.com>
Date: Thu, 29 Feb 2024 22:21:52 +1030
From: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To: Matti Vaittinen <mazziesaccount@...il.com>,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Matt Ranostay <matt@...ostay.sg>,
 Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 5/5] iio: light: Add support for APDS9306 Light Sensor

On 28/2/24 23:38, Matti Vaittinen wrote:
> On 2/28/24 14:24, Subhajit Ghosh wrote:
>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
>> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
>> channel approximates the response of the human-eye providing direct
>> read out where the output count is proportional to ambient light levels.
>> It is internally temperature compensated and rejects 50Hz and 60Hz flicker
>> caused by artificial light sources. Hardware interrupt configuration is
>> optional. It is a low power device with 20 bit resolution and has
>> configurable adaptive interrupt mode and interrupt persistence mode.
>> The device also features inbuilt hardware gain, multiple integration time
>> selection options and sampling frequency selection options.
>>
>> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
>> Scales, Gains and Integration time implementation.
>>
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
>> ---
>> v7 -> v8:
>>   - Renamed APDS9306_INT_CH_CLEAR to APDS9306_INT_SRC_CLEAR macro for higher
>>     readability
>>   - Removed APDS9306_CHANNEL macro for higher readability
>>   - Updated iio_push_event() functions with correct type of events (Light or Intensity)
>>   - Updated variable name "event_ch_is_light" to "int_src" and change as per
>>     review to fix compiler warning
>>   - Used scope for guard() functions
>>   - Other fixes as per reviews
>>     https://lore.kernel.org/all/20240224151340.3f2f51e8@jic23-huawei/
>>     https://lore.kernel.org/all/ZdycR6nr3rtrnuth@smile.fi.intel.com/
>>
..

Hi Matti,
>> ---
> 
> Hi Subhajit,
> 
> I just happened to notice couple of minor things. I see the series is already in a v8 and don't want to cause extra re-spins. So, perhaps consider these points if you need to do v9 but I am sending these only as 'nits'. I don't think any of my findings are very serious.
>Thank for reviewing. I will do as many re-spins as it takes to get things correct if required.
It is best possible source of learning for me.
> ...
> 
>> +static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
>> +{
>> +    struct device *dev = data->dev;
>> +    struct apds9306_regfields *rf = &data->rf;
>> +    int ret, intg_old, gain_old, gain_new, gain_new_closest, intg_time_idx;
>> +    int gain_idx;
>> +    bool ok;
>> +
>> +    if (!iio_gts_valid_time(&data->gts, val2)) {
>> +        dev_err_ratelimited(dev, "Unsupported integration time %u\n", val2);
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = regmap_field_read(rf->intg_time, &intg_time_idx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_field_read(rf->gain, &gain_idx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    intg_old = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    if (intg_old == val2)
>> +        return 0;
>> +
>> +    gain_old = iio_gts_find_gain_by_sel(&data->gts, gain_idx);
>> +    if (gain_old < 0)
>> +        return gain_old;
>> +
>> +    ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
>> +                             intg_old, val2, &gain_new);
> 
> You don't use the 'ret' here, so maybe for the clarity, not assign it.
> Or, maybe you wan't to try to squeeze out few cycles for succesful case and check the ret for '0' - in which case you should be able to omit the check right below as well as the call to iio_find_closest_gain_low(). OTOH, this is likely not a "hot path" so I don't care too much about the extra call if you think code is clearer this way.
I will stick to the first option and remove the unused ret. The code looks linear and clearer
that way. Although it depends upon further reviews.

Regards,
Subhajit Ghosh
> 
>> +    if (gain_new < 0) {
>> +        dev_err_ratelimited(dev, "Unsupported gain with time\n");
>> +        return gain_new;
>> +    }
>> +
>> +    gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
>> +    if (gain_new_closest < 0) {
>> +        gain_new_closest = iio_gts_get_min_gain(&data->gts);
>> +        if (gain_new_closest < 0)
>> +            return gain_new_closest;
>> +    }
>> +    if (!ok)
>> +        dev_dbg(dev, "Unable to find optimum gain, setting minimum");
>> +
>> +    ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = regmap_field_write(rf->intg_time, ret);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return regmap_field_write(rf->gain, ret);
>> +}
> 
> 

Powered by blists - more mailing lists