[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241019153140.4f7a4f47@jic23-huawei>
Date: Sat, 19 Oct 2024 15:31:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Rishi Gupta <gupt21@...il.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: light: add support for veml3235
On Wed, 16 Oct 2024 23:39:32 +0200
Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
> The Vishay veml3235 is a low-power ambient light sensor with I2C
> interface. It provides a minimum detectable intensity of
> 0.0021 lx/cnt, configurable integration time and gain, and an additional
> white channel to distinguish between different light sources.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
A few minor things inline but looking good in general to me.
> diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c
> new file mode 100644
> index 000000000000..9bda42379456
> --- /dev/null
> +++ b/drivers/iio/light/veml3235.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML3235 Ambient Light Sensor
> + *
> + * Copyright (c) 2024, Javier Carrasco <javier.carrasco.cruz@...il.com>
> + *
> + * Datasheet: https://www.vishay.com/docs/80131/veml3235.pdf
> + * Appnote-80222: https://www.vishay.com/docs/80222/designingveml3235.pdf
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define VEML3235_REG_CONF 0x00
> +#define VEML3235_REG_WH_DATA 0x04
> +#define VEML3235_REG_ALS_DATA 0x05
> +#define VEML3235_REG_ID 0x09
> +
> +#define VEML3235_SD BIT(0)
> +#define VEML3235_SD0 BIT(8)
Name these so it's obvious they are in REG_CONF.
#define VEML3235_CONF_SD
etc.
> +
> +static int veml3235_set_gain(struct iio_dev *indio_dev, int val, int val2)
> +{
> + struct veml3235_data *data = iio_priv(indio_dev);
> + int ret, new_gain, gain_idx;
if (val2 !== 0)
return -EINVAL;
switch (val)
..
default:
return -EINVAL;
> +
> + if (val == 1 && val2 == 0) {
> + new_gain = 0x00;
> + gain_idx = 3;
> + } else if (val == 2 && val2 == 0) {
> + new_gain = 0x01;
> + gain_idx = 2;
> + } else if (val == 4 && val2 == 0) {
> + new_gain = 0x03;
> + gain_idx = 1;
> + } else if (val == 8 && val2 == 0) {
> + new_gain = 0x07;
> + gain_idx = 0;
> + } else {
> + return -EINVAL;
> + }
> +
> + ret = regmap_field_write(data->rf.gain, new_gain);
> + if (ret) {
> + dev_err(data->dev, "failed to set gain: %d\n", ret);
> + return ret;
> + }
> +
> + if (data->gain < gain_idx)
> + data->resolution <<= gain_idx - data->gain;
> + else if (data->gain > gain_idx)
> + data->resolution >>= data->gain - gain_idx;
> +
> + data->gain = gain_idx;
> +
> + return 0;
> +}
> +static int veml3235_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + *vals = (int *)&veml3235_it_times;
> + *length = 2 * ARRAY_SIZE(veml3235_it_times);
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)&veml3235_scale_vals;
> + *length = ARRAY_SIZE(veml3235_scale_vals);
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_LIST;
> + }
> +
> + return -EINVAL;
Push this into a default: as it lets static analysis tools be sure you intended
to only handle those two cases.
> +}
>
> +static int veml3235_read_id(struct veml3235_data *data)
> +{
> + int ret, reg;
> +
> + ret = regmap_field_read(data->rf.id, ®);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "failed to read ID\n");
> +
> + if (reg != 0x35)
> + return dev_err_probe(data->dev, -ENODEV, "Unknown ID %d\n", reg);
This looks to be breaking any potential use of fallback dt compatibles for
future devices that are fully backwards compatible.
Should just be a dev_info() to log that we aren't sure what the device is then
carry on anyway on basis the firmware tables should be correct.
> +
> + return 0;
> +}
Powered by blists - more mailing lists