Re: [PATCH v2 2/3] iio: light: ltr390: Add ALS channel and support for gain and resolution

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 28 Jul 2024 20:49:55 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

>     Add new ALS channel and allow reading raw and scale values.
>     Also provide gain and resolution configuration for ALS channel.
>     Add automatic mode switching between the UVS and ALS channel
>     based on which channel is being accessed.
>     The default mode in which the sensor start is ALS mode.
Same issue with indent as in patch 1.

> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
Later patches should not be in reply to the first one.
Use a cover letter.

Minor comments inline on using the enum to maintain the type of the mode
setting.


> ---
>  drivers/iio/light/ltr390.c | 111 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 96 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 9f00661c3..d1a259141 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -62,11 +62,17 @@
>   */
>  #define LTR390_WINDOW_FACTOR 1
>  
> +enum ltr390_mode {
> +	LTR390_SET_ALS_MODE,
> +	LTR390_SET_UVS_MODE,
> +};
> +
>  struct ltr390_data {
>  	struct regmap *regmap;
>  	struct i2c_client *client;
>  	/* Protects device from simulataneous reads */
>  	struct mutex lock;
> +	int mode;
enum ltr380_mode mode;

>  	int gain;
>  	int int_time_us;
>  };
> @@ -94,6 +100,28 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
>  	return get_unaligned_le24(recieve_buffer);
>  }
>  
> +static int ltr390_set_mode(struct ltr390_data *data, int mode)

pass in the enum lt380_mode.

> +{
> +	if (data->mode == mode)
> +		return 0;
> +
> +	switch (mode) {
> +	case LTR390_SET_ALS_MODE:
> +		regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> +		break;
> +
> +	case LTR390_SET_UVS_MODE:
> +		regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> +		break;
> +
> +	default:
> +		return -EINVAL;
Should be able to drop this if passing in the enum as the compiler can see you
have cases for every value.

> +	}
> +
> +	data->mode = mode;
> +	return 0;
> +}
> +