Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 24 Apr 2022 17:32:10 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Shreeya Patel <shreeya.patel@collabora.com>
Cc:     lars@metafoo.de, robh+dt@kernel.org, Zhigang.Shi@liteon.com,
        krzk@kernel.org, krisman@collabora.com, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, alvaro.soliverez@collabora.com
Subject: Re: [PATCH v2 3/3] iio: light: Add support for ltrf216a sensor
Message-ID: <20220424173210.554c8247@jic23-huawei>
In-Reply-To: <20220421140133.354498-4-shreeya.patel@collabora.com>
References: <20220421140133.354498-1-shreeya.patel@collabora.com>
        <20220421140133.354498-4-shreeya.patel@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 21 Apr 2022 19:31:33 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> From: Zhigang Shi <Zhigang.Shi@liteon.com>
> 
> Add initial support for ltrf216a ambient light sensor.
> 
> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf
> Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>

Hi Shreeya,

Looking pretty good.  Just a few minor things in here - I very nearly
just made the changes whilst applying but the one about reusing the
available array is slightly more complex than I like to do without
bouncing it back to the author.

Thanks,

Jonathan

> ---
> 
> Changes in v2
>   - Add support for 25ms and 50ms integration time.
>   - Rename some of the macros as per names given in datasheet
>   - Add a comment for the mutex lock
>   - Use read_avail callback instead of attributes and set the
>     appropriate _available bit.
>   - Use FIELD_PREP() at appropriate places.
>   - Add a constant lookup table for integration time and reg val
>   - Use BIT() macro for magic numbers.
>   - Improve error handling at few places.
>   - Use get_unaligned_le24() and div_u64()
>   - Use probe_new() callback and devm functions
>   - Return errors in probe using dev_err_probe()
>   - Use DEFINE_SIMPLE_DEV_PM_OPS()
>   - Correct the formula for lux to use 0.45 instead of 0.8
> 
> 
>  drivers/iio/light/Kconfig    |  10 +
>  drivers/iio/light/Makefile   |   1 +
>  drivers/iio/light/ltrf216a.c | 349 +++++++++++++++++++++++++++++++++++
>  3 files changed, 360 insertions(+)
>  create mode 100644 drivers/iio/light/ltrf216a.c
> 

> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> new file mode 100644
> index 000000000000..de6d2e2e7f08
> --- /dev/null
> +++ b/drivers/iio/light/ltrf216a.c
> @@ -0,0 +1,349 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * LTRF216A Ambient Light Sensor
> + *
> + * Copyright (C) 2021 Lite-On Technology Corp (Singapore)
> + * Author: Shi Zhigang <Zhigang.Shi@liteon.com>
> + *
> + * IIO driver for LTRF216A (7-bit I2C slave address 0x53).
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

I don't think you are using anything from iio/sysfs.h any more so please
drop this header (unless I'm missing something!)

> +#include <linux/mod_devicetable.h>
> +#include <linux/bitfield.h>
> +#include <linux/pm.h>
> +#include <linux/delay.h>
> +#include <asm/unaligned.h>
> +
Where no other reason to have a particular order for headers IIO preference is
3 blocks of headers.  First set are the non IIO related ones in alphabetical order.
Second block typically IIO specific ones.  Final block the asm includes

So here best order is something like

#include <linux/bitfield.h>
#include <linux/delay.h>
#include <linux/init.h>
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/mutex.h>
#include <linux/pm.h>

#include <linux/iio/iio.h>

#include <asm/unaligned.h>


> +#define LTRF216A_ALS_DATA_0		0x0D
> +
> +static const int int_time_mapping[] = { 400000, 200000, 100000, 50000, 25000 };

You should use the array below for the same matching purpose as this one and
avoid duplicating data.

> +
> +static const int ltrf216a_int_time_available[5][2] = {
> +	{0, 400000},
> +	{0, 200000},
> +	{0, 100000},
> +	{0, 50000},
> +	{0, 25000},
> +};
> +
> +static const int ltrf216a_int_time_reg[5][2] = {
> +	{400, 0x03},
> +	{200, 0x13},
> +	{100, 0x22},
> +	{50, 0x31},
> +	{25, 0x40},
> +};
> +
> +struct ltrf216a_data {
> +	struct i2c_client *client;
> +	u32 int_time;
> +	u8 int_time_fac;
> +	u8 als_gain_fac;
> +	struct mutex mutex; /* Protect read and write operations */

This could probably have been more descriptive. I think you are also
ensuring that the cached state and the device state are kept in sync.

> +};


> +
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata;
> +	u64 lux, div;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +	if (greendata < 0 || cleardata < 0) {
> +		return -EINVAL;

As this is an error case and you correctly return directly there is
no need to have else.  That will reduce indentation and
allow last line to simply be

return div_u64(lux, div);

> +
> +	} else {
> +		lux = greendata * 45 * WIN_FAC * 100;
> +		div = data->als_gain_fac * data->int_time_fac * 100;
> +		lux = div_u64(lux, div);
> +	}
> +
> +	return lux;
> +}
> +