linux-kernel - Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

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: <16c09207-48c6-4988-873f-772fa277f3b8@wanadoo.fr>
Date: Fri, 9 Aug 2024 14:07:05 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: jianping.shen@...bosch.com
Cc: Christian.Lorenz3@...bosch.com, Kai.Dolde@...bosch.com,
 Ulrike.Frauendorf@...bosch.com, conor+dt@...nel.org,
 devicetree@...r.kernel.org, dima.fedrau@...il.com, jic23@...nel.org,
 krzk+dt@...nel.org, lars@...afoo.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, marcelo.schmitt1@...il.com, robh@...nel.org
Subject: Re: [PATCH v2 2/2] iio: imu: smi240: imu driver

Le 09/08/2024 à 13:16, 
Jianping.Shen-V5te9oGctAVWk0Htik3J/w@...lic.gmane.org a écrit :
> From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen-V5te9oGctAVWk0Htik3J/w@...lic.gmane.org>
> 
> iio: imu: smi240: driver improvements
> Signed-off-by: Shen Jianping (ME-SE/EAD2) <Jianping.Shen-V5te9oGctAVWk0Htik3J/w@...lic.gmane.org>
> ---

Hi,

...

> diff --git a/drivers/iio/imu/smi240/smi240_core.c b/drivers/iio/imu/smi240/smi240_core.c
> new file mode 100644
> index 00000000000..4b2a4a290f3
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_core.c

...

> +static int smi240_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	int ret = 0;

No need to init (and in other places you don't)

> +	struct smi240_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +		ret = smi240_get_data(data, chan->type, chan->channel2, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		ret = smi240_get_low_pass_filter_freq(data, chan->type, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int smi240_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	int ret, i;
> +	struct smi240_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		for (i = 0; i < ARRAY_SIZE(smi240_low_pass_freqs); i++) {
> +			if (val == smi240_low_pass_freqs[i])
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(smi240_low_pass_freqs))
> +			return -EINVAL;
> +
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			data->accel_filter_freq = val;
> +			break;
> +		case IIO_ANGL_VEL:
> +			data->anglvel_filter_freq = val;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	// Write access to soft config is locked until hard/soft reset
> +	ret = smi240_soft_reset(data);
> +	if (ret)
> +		return ret;

Nitpick: missing new line?

> +	ret = smi240_soft_config(data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

...

> +int smi240_core_probe(struct device *dev, struct regmap *regmap)
> +{
> +	struct iio_dev *indio_dev;
> +	struct smi240_data *data;
> +	int ret, response;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Allocate iio device failed\n");

Usually messages related to memory allocation are not useful.
Just: return -ENOMEM?

> +
> +	data = iio_priv(indio_dev);
> +	dev_set_drvdata(dev, indio_dev);
> +	data->regmap = regmap;
> +	data->capture = 0;

No need to explicitly initialize 'capture', devm_iio_device_alloc() 
already zeroes the allocated emmory.
It doesn't hurt to be explicit, but why this field and not the other ones?

> +
> +	ret = regmap_read(data->regmap, SMI240_CHIP_ID_REG, &response);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Read chip id failed\n");
> +
> +	if (response != SMI240_CHIP_ID)
> +		dev_info(dev, "Unknown chip id: 0x%04x\n", response);

...

> diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
> new file mode 100644
> index 00000000000..ac9e37ffa37
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_spi.c

...

> +static int smi240_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	__be32 request;
> +	struct spi_device *spi = context;
> +	u8 reg_addr = ((u8 *)data)[0];
> +	u16 reg_data = ((u8 *)data)[2] << 8 | ((u8 *)data)[1];
> +
> +	request = SMI240_BUS_ID << 30;
> +	request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
> +	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
> +	request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
> +	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> +	request = cpu_to_be32(request);
> +
> +	return spi_write(spi, &request, sizeof(request));
> +}
> +
> +static struct regmap_bus smi240_regmap_bus = {

const?

> +	.read = smi240_regmap_spi_read,
> +	.write = smi240_regmap_spi_write,
> +};

...

CJ