Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <09a07473-2558-2b05-c9d5-8d439749bfa7@tweaklogic.com>
Date:   Tue, 10 Oct 2023 22:47:14 +1030
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
Subject: Re: [PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
To:     Matti Vaittinen <mazziesaccount@gmail.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paul Gazzillo <paul@pgazz.com>,
        Conor Dooley <conor+dt@kernel.org>,
        Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
References: <20231008154857.24162-1-subhajit.ghosh@tweaklogic.com>
 <20231008154857.24162-3-subhajit.ghosh@tweaklogic.com>
 <ae210957-4a72-24b3-2f11-8d5824041e85@gmail.com>
Content-Language: en-US
From:   Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
In-Reply-To: <ae210957-4a72-24b3-2f11-8d5824041e85@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 10/10/23 20:15, Matti Vaittinen wrote:

> 
> To my eyes this driver looks nice. Just spotted two minor things.
> 

Thanks Matti. Nice is one nice thing I heard after some time!

>> +    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 < 0;
> 
> Returning the truth value on purpose? :)

Nope, it's a bug. I'll fix it.

>> +static int get_device_id_lux_per_count(struct apds9306_data *data)
>> +{
>> +    int ret, part_id;
>> +
>> +    ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
>> +        data->nlux_per_count =
>> +            apds9306_part_id_nlux_per_count[0].nlux_per_count;
>> +    else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
>> +        data->nlux_per_count =
>> +            apds9306_part_id_nlux_per_count[1].nlux_per_count;
>> +    else
>> +        return -ENXIO;
> 
> I think we should be able to differentiate between the IC variants by DT compatible. (Commented that on bindings patch). Not sure if we need to support cases where the sensor is instantiated without device-tree. I am not super happy when code requires the part-id to be known if we have separate compatibles for variants. Can we in dt-case just print a warning if the part-ID is not what we expect - and proceed assuming the nlux_per_count based on the DT information? (Sometimes we see new variants with same part-IDs - or many part-IDs with no SW changes needed. Hence maintaining the part-ID lists may be tedious). This is just some pondering though, no strong requirements from my side

Yes, I agree with you. The purpose of DT is to provide proper hardware descriptions.
I will throw a warning as well as implement a compatibility match.

Regards,
Subhajit Ghosh