Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com>
Date:   Sun, 29 Oct 2023 17:51:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
Content-Language: en-US, en-GB
To:     Jonathan Cameron <jic23@kernel.org>,
        Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Matt Ranostay <matt@ranostay.sg>,
        Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231027074545.6055-1-subhajit.ghosh@tweaklogic.com>
 <20231027074545.6055-3-subhajit.ghosh@tweaklogic.com>
 <20231028162025.4259f1cc@jic23-huawei>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20231028162025.4259f1cc@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/28/23 18:20, Jonathan Cameron wrote:
> On Fri, 27 Oct 2023 18:15:45 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> 
>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
>> and clear channels with i2c interface. 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.
> 
> Hi Subhajit,
> 
> 
>>
> Change log below the ---
> 
> We don't generally want to end up with this information in the git log
> and anything above the --- is used for the commit message.
> 
> Note that if you want to keep notes in your local git it is fine adding
> 
> Signed-of-by...
> 
> ---
> 
> Version notes
> etc
> 
> 
> As then git am will drop them anyway when your patches are picked up.
> 
> 
>> v1 -> v2
>> - Renamed probe_new to probe
>> - Removed module id table
>>
>> v0 -> v1
>> - Fixed errors as per previous review
>> - Longer commit messages and descriptions
>> - Updated scale calculations as per iio gts scheme to export proper scale
>>    values and tables to userspace
>> - Removed processed attribute for the same channel for which raw is
>>    provided, instead, exporting proper scale and scale table to userspace so
>>    that userspace can do "(raw + offset) * scale" and derive Lux values
>> - Fixed IIO attribute range syntax
>> - Keeping the regmap lock enabled as the driver uses unlocked regfield
>>    accesses from interrupt handler
>> - Several levels of cleanups by placing guard mutexes in proper places and
>>    returning immediately in case of an error
>> - Using iio_device_claim_direct_mode() during raw reads so that
>>    configurations could not be changed during an adc conversion period
>> - In case of a powerdown error, returning immediately
>> - Removing the definition of direction of the hardware interrupt and
>>    leaving it on to device tree
>> - Adding the powerdown callback after doing device initialization
>> - Removed the regcache_cache_only() implementation
>>
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> 

...

> 
>> +static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
>> +{
>> +	int i, ret, time_sel, gain_sel;
>> +
>> +	/* Rounding up the last digit by one, otherwise matching table fails! */
> 
> Interesting.  Sounds like a question for Matti?

Sounds odd indeed. I assume this happens when scale setting is requested 
using one of the exact values advertised by the available scales from 
the GTS? This does not feel right and the +1 does not ring a bell to me. 
I need to investigate what's going on. It would help if you could 
provide the values used as val and val2 for the setting.

This will take a while from me though - I'll try to get to this next 
week. Thanks for pointing out the anomaly!

> 
>> +	if (val2 % 10)
>> +		val2 += 1;
>> +
>> +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> +				     data->intg_time_idx, val, val2, &gain_sel);
>> +	if (ret) {
>> +		for (i = 0; i < data->gts.num_itime; i++) {
>> +			time_sel = data->gts.itime_table[i].sel;
>> +
>> +			if (time_sel == data->intg_time_idx)
>> +				continue;
>> +
>> +			ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> +						time_sel, val, val2, &gain_sel);
>> +			if (!ret)
>> +				break;
>> +		}
>> +		if (ret)
>> +			return -EINVAL;
>> +
>> +		ret = apds9306_intg_time_set_hw(data, time_sel);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return apds9306_gain_set_hw(data, gain_sel);
>> +}

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~