From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6EFD7194; Fri, 1 Dec 2023 10:17:25 -0800 (PST) Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Shh7j1n26z6K91p; Sat, 2 Dec 2023 02:15:45 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 6E79A1401E0; Sat, 2 Dec 2023 02:17:23 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Fri, 1 Dec 2023 18:17:22 +0000 Date: Fri, 1 Dec 2023 18:17:21 +0000 From: Jonathan Cameron To: Marek Vasut CC: Jonathan Cameron , , "Andy Shevchenko" , Matti Vaittinen , Alexander Stein , Andre Werner , Bjorn Helgaas , Conor Dooley , Fabio Estevam , Guenter Roeck , Krzysztof Kozlowski , Lars-Peter Clausen , Luca Ceresoli , Mark Brown , Naresh Solanki , Patrick Rudolph , Rob Herring , "Stefan Windfeldt-Prytz" , Vincent Tremblay , Subject: Re: [PATCH v5 2/2] iio: light: isl76682: Add ISL76682 driver Message-ID: <20231201181721.0000445c@Huawei.com> In-Reply-To: <9e73c450-2380-459a-9b41-a1b88f89548c@denx.de> References: <20231125222738.97875-1-marex@denx.de> <20231125222738.97875-2-marex@denx.de> <20231126181655.4e1040f9@jic23-huawei> <9e73c450-2380-459a-9b41-a1b88f89548c@denx.de> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-iio@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml100003.china.huawei.com (7.191.160.210) To lhrpeml500005.china.huawei.com (7.191.163.240) On Sun, 26 Nov 2023 23:09:36 +0100 Marek Vasut wrote: > On 11/26/23 19:16, Jonathan Cameron wrote: > > On Sat, 25 Nov 2023 23:26:23 +0100 > > Marek Vasut wrote: > > > >> The ISL76682 is very basic ALS which only supports ALS or IR mode > >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > >> other fancy functionality. > >> > >> Reviewed-by: Andy Shevchenko > >> Reviewed-by: Matti Vaittinen > >> Signed-off-by: Marek Vasut > > > > Hi Marek, > > > > One last question + a comment in general. Act on that if you like. > > > > Thanks, > > > > Jonathan > > > > > >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > > > > Why have an available attribute for a single value. Is it useful for anything? > > To report it to userspace, iio-sensor-proxy uses that to control the ALS > poll interval . It should use integration_time, not the associated available attribute. > > >> +static int isl76682_probe(struct i2c_client *client) > >> +{ > > > > ... > > > >> + indio_dev->info = &isl76682_info; > >> + indio_dev->channels = isl76682_channels; > >> + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); > >> + indio_dev->name = ISL76682_DRIVER_NAME; > > Trivial but I'm not a fan of using defines in cases like this. It just makes > > me go find the define when I could see the string directly here. > > > > In cases where matching or similar strictly requires the naming to be the same > > in various places a define is useful. In this case less so. > > > > Anyhow, it's a very minor comment so never mind if you prefer to leave it > > as it stands. > > I added it to V6 . > > I'll wait for the integration time reply above and then send V6 . >