Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <659d5d74-4861-45c4-8b95-218f3a7e60cf@gmail.com>
Date:   Sat, 25 Nov 2023 17:17:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/3] iio: light: add VEML6075 UVA and UVB light sensor
 driver
To:     Jonathan Cameron <jic23@kernel.org>
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>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org
References: <20231110-veml6075-v2-0-d04efbc8bf51@gmail.com>
 <20231110-veml6075-v2-3-d04efbc8bf51@gmail.com>
 <20231125151116.6d9b6e46@jic23-huawei>
Content-Language: en-US
From:   Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <20231125151116.6d9b6e46@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 25.11.23 16:11, Jonathan Cameron wrote:
> On Sat, 25 Nov 2023 12:56:57 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> The Vishay VEMl6075 is a low power, 16-bit resolution UVA and UVB
>> light sensor with I2C interface and noise compensation (visible and
>> infrarred).
>>
>> Every UV channel generates an output signal measured in counts per
>> integration period, where the integration time is configurable.
>>
>> This driver adds support for both UV channels and the ultraviolet
>> index (UVI) inferred from them according to the device application note
>> with open-air (no teflon) coefficients.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> 
> Hi Javier,
> 
> A few more minor things. Looks good in general.
> 
> Jonathan
> 
>> +struct veml6075_data {
>> +	struct i2c_client *client;
>> +	struct regmap *regmap;
>> +	struct mutex lock; /* integration time/measurement trigger lock */
> 
> Could perhaps be clearer. Maybe something like
> 	/* Prevent integration time changing during a measurement */
> 
It prevents integration time changing as well as measurement triggers
while a measurement is underway. I just wanted to cover both usages with
a short comment in the same line.
>> +
>> +static int veml6075_uva_comp(int raw_uva, int comp1, int comp2)
>> +{
>> +	int comp1a_c, comp2a_c, uva_comp;
>> +
>> +	comp1a_c = (comp1 * VEML6075_A_COEF) / 1000U;
>> +	comp2a_c = (comp2 * VEML6075_B_COEF) / 1000U;
>> +	uva_comp = raw_uva - comp1a_c - comp2a_c;
>> +
>> +	return clamp_val(uva_comp, 0, U16_MAX);
>> +}
>> +
>> +static int veml6075_uvb_comp(int raw_uvb, int comp1, int comp2)
>> +{
>> +	int comp1b_c, comp2b_c, uvb_comp;
>> +
>> +	comp1b_c = (comp1 * VEML6075_C_COEF) / 1000U;
> 
> Any of units.h appropriate here?  I'm not sure if the / 1000U is a units
> thing or not.
> 
These divisions are used to scale the coefficients down, as they are
defined as entire numbers. These coefficients have no units and the
resulting value is a count.

I have nothing to add to the rest of your comments. I will start working
on v3.

Thanks again for your thorough review.

Best regards,
Javier Carrasco