linux-kernel - Re: [PATCH v8 2/2] iio: light: ROHM BH1745 colour sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2505dee2-c503-4566-a4ef-73103da9479d@gmail.com>
Date: Fri, 19 Jul 2024 18:02:34 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Mudit Sharma <muditsharma.info@...il.com>, jic23@...nel.org,
 lars@...afoo.de, krzk+dt@...nel.org, conor+dt@...nel.org, robh@...nel.org
Cc: linux-kernel@...r.kernel.org, mazziesaccount@...il.com,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 Ivan Orlov <ivan.orlov0322@...il.com>
Subject: Re: [PATCH v8 2/2] iio: light: ROHM BH1745 colour sensor

On 19/07/2024 00:02, Mudit Sharma wrote:
> Add support for BH1745, which is an I2C colour sensor with red, green,
> blue and clear channels. It has a programmable active low interrupt
> pin. Interrupt occurs when the signal from the selected interrupt
> source channel crosses set interrupt threshold high or low level.
> 
> Interrupt source for the device can be configured by enabling the
> corresponding event. Interrupt latch is always enabled when setting
> up interrupt.
> 
> Add myself as the maintainer for this driver in MAINTAINERS.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@...il.com>
> Reviewed-by: Ivan Orlov <ivan.orlov0322@...il.com>
> Reviewed-by: Javier Carrasco <javier.carrasco.cruz@...il.com>

Hi Mudit,

a couple of nitpicks inline.

...


> +static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2)
> +{
> +	struct device *dev = data->dev;
> +	int ret;
> +	int value;
> +	int current_int_time, current_hwgain_sel, current_hwgain;
> +	int new_hwgain, new_hwgain_sel, new_int_time_sel;
> +	int req_int_time = (1000000 * val) + val2;
> +
> +	if (!iio_gts_valid_time(&data->gts, req_int_time)) {
> +		dev_dbg(dev, "Unsupported integration time requested: %d\n",
> +			req_int_time);
> +		return -EINVAL;
> +	}
> +
> +	ret = bh1745_get_int_time(data, &current_int_time);
> +	if (ret)
> +		return ret;
> +
> +	if (current_int_time == req_int_time)
> +		return 0;
> +
> +	ret = regmap_read(data->regmap, BH1745_MODE_CTRL2, &value);
> +	if (ret)
> +		return ret;
> +
> +	current_hwgain_sel = FIELD_GET(BH1745_CTRL2_ADC_GAIN_MASK, value);
> +	current_hwgain = iio_gts_find_gain_by_sel(&data->gts, current_hwgain_sel);
> +	ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, current_hwgain,
> +						     current_int_time, req_int_time,
> +						     &new_hwgain);
> +	if (new_hwgain < 0) {

Typo in debug message: corresponding. I would recommend you to pass
codespell to checkpatch. It will often catch such things.

> +		dev_dbg(dev, "No corrosponding gain for requested integration time\n");
> +		return ret;
> +	}
> +

...

> +static int bh1745_write_raw_get_fmt(struct iio_dev *indio_dev,

Nit: the alignment seems to be a bit off here.

> +				      struct iio_chan_spec const *chan,
> +				      long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...


> +static int bh1745_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir, int state)
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int value;
> +
> +	if (state == 0)
> +		return regmap_clear_bits(data->regmap, BH1745_INTR, BH1745_INTR_ENABLE);
> +
> +	if (state == 1) {

Nit: empty line at the beginning of the scope.

> +
> +		/* Latch is always enabled when enabling interrupt */
> +		value = BH1745_INTR_ENABLE;
> +
> +		switch (chan->channel2) {
> +		case IIO_MOD_LIGHT_RED:
> +			return regmap_write(data->regmap, BH1745_INTR,
> +					    value | FIELD_PREP(BH1745_INTR_SOURCE_MASK,
> +							       BH1745_INTR_SOURCE_RED));
> +
> +		case IIO_MOD_LIGHT_GREEN:
> +			return regmap_write(data->regmap, BH1745_INTR,
> +					    value | FIELD_PREP(BH1745_INTR_SOURCE_MASK,
> +							       BH1745_INTR_SOURCE_GREEN));
> +
> +		case IIO_MOD_LIGHT_BLUE:
> +			return regmap_write(data->regmap, BH1745_INTR,
> +					    value | FIELD_PREP(BH1745_INTR_SOURCE_MASK,
> +							       BH1745_INTR_SOURCE_BLUE));
> +
> +		case IIO_MOD_LIGHT_CLEAR:
> +			return regmap_write(data->regmap, BH1745_INTR,
> +					    value | FIELD_PREP(BH1745_INTR_SOURCE_MASK,
> +							       BH1745_INTR_SOURCE_CLEAR));
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> 

Best regards,
Javier Carrasco