Message-ID:
 <AM8PR10MB47217960E30212DC62ED7821CD712@AM8PR10MB4721.EURPRD10.PROD.OUTLOOK.COM>
Date: Thu, 3 Oct 2024 21:44:42 +0000
From: "Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@...bosch.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "lars@...afoo.de" <lars@...afoo.de>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "dima.fedrau@...il.com" <dima.fedrau@...il.com>,
	"marcelo.schmitt1@...il.com" <marcelo.schmitt1@...il.com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Lorenz
 Christian (ME-SE/EAD2)" <Christian.Lorenz3@...bosch.com>, "Frauendorf Ulrike
 (ME/PJ-SW3)" <Ulrike.Frauendorf@...bosch.com>, "Dolde Kai (ME-SE/PAE-A3)"
	<Kai.Dolde@...bosch.com>
Subject: RE: [PATCH v8 2/2] iio: imu: smi240: add driver

>> +
>> +static int smi240_regmap_spi_read(void *context, const void *reg_buf,
>> +				  size_t reg_size, void *val_buf,
>> +				  size_t val_size)
>> +{
>> +	int ret;
>> +	u32 request, response;
>> +	u16 *val = val_buf;
>> +	struct spi_device *spi = context;
>> +	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
>> +	struct smi240_data *iio_priv_data = iio_priv(indio_dev);
>> +
>> +	if (reg_size != 1 || val_size != 2)
>> +		return -EINVAL;
>> +
>> +	request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
>> +	request |= FIELD_PREP(SMI240_WRITE_CAP_BIT_MASK, iio_priv_data-
>>capture);
>> +	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf);
>> +	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
>> +
>> +	iio_priv_data->spi_buf = cpu_to_be32(request);
>> +
>> +	/*
>> +	 * SMI240 module consists of a 32Bit Out Of Frame (OOF)
>> +	 * SPI protocol, where the slave interface responds to
>> +	 * the Master request in the next frame.
>> +	 * CS signal must toggle (> 700 ns) between the frames.
>> +	 */
>> +	ret = spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = spi_read(spi, &iio_priv_data->spi_buf, sizeof(response));
>> +	if (ret)
>> +		return ret;
>> +
>> +	response = be32_to_cpu(iio_priv_data->spi_buf);
>> +
>> +	if (!smi240_sensor_data_is_valid(response))
>> +		return -EIO;
>> +
>> +	*val = cpu_to_le16(FIELD_GET(SMI240_READ_DATA_MASK, response));
>So this is line sparse doesn't like which is reasonable given you are forcing an le16
>value into a u16.
>Minimal fix is just to change type of val to __le16 *
>
>I still find the endian handling in here mess and am not convinced the complexity is
>strictly necessary or correct.
>
>I'd expect the requirements of reordering to be same in read and write directions
>(unless device is really crazy), so why do we need a conversion to le16 here but not
>one from le16 in the write?

Hello Jonathan,

yes, you are right. The "cpu_to_le16" is not required at all.  SMI240 does not use the standard SPI protocol, on the other side the regmap is designed to use standard SPI protocol (by default) and may flip the register value dependent on "val_format_endian". When the both work together, it may lead to confusing.  Let me make it clear.

In the SMI240, the register address is 8 bit and each register is 16 bit. We do not have any register value, which is bigger than 16 bit and need to be stored in multiple registers.  Therefore the device does not need endian. Neither big endian nor Little Endian.   To access the register, it is important to prepare the request frame according to the specification. 

A request is 32 bit

	ID	ADR	W	CAP	*	WDATA	CRC
	31-30	29-22	21	20	19	18-3		2-0

ID: device id (if more than 1 device)
ADR: reg address
W: write/read
CAP: capture mode on/off
*: reserved
WDATA: reg value to write
CRC: check sum

To prepare the request properly, the bit order is here critical. We need to put each part in its bit position. The request is created as a local u32, with help of FIELD_PREP, we put the value of each part to its bit position. FIELD_PREP will take care of the cpu endian and always put the value to the correct bit position.  Before we send the request via SPI, a cpu endian to big endian conversion is required. Since the spi bus transfers data using big endian. When we get the response from spi, the response is big endian and need to be converted in cpu endian.  Any other manually endian conversion is not required. 

The SPI read in next version look like that

1. Prepare request
2. Convert request from cpu endian to big endian and send via spi
3. Get response 
4. Convert response from big endian to cpu endian  and take the reg value from converted response
As you mentioned, an additional cpu_to_le16 is not required. Since the response is already converted to cpu endian.


static int smi240_regmap_spi_read(void *context, const void *reg_buf,
				  size_t reg_size, void *val_buf,
				  size_t val_size)
{
	int ret;
	u32 request, response;
	u16 *val = val_buf;
	struct spi_device *spi = context;
	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
	struct smi240_data *iio_priv_data = iio_priv(indio_dev);

	if (reg_size != 1 || val_size != 2)
		return -EINVAL;

	request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
	request |= FIELD_PREP(SMI240_WRITE_CAP_BIT_MASK, iio_priv_data->capture);
	request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf);
	request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);

	iio_priv_data->spi_buf = cpu_to_be32(request);

	ret = spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
	if (ret)
		return ret;

	ret = spi_read(spi, &iio_priv_data->spi_buf, sizeof(response));
	if (ret)
		return ret;

	response = be32_to_cpu(iio_priv_data->spi_buf);

	if (!smi240_sensor_data_is_valid(response))
		return -EIO;

	*val = FIELD_GET(SMI240_READ_DATA_MASK, response);

	return 0;
}


The SPI write in next version look like that

1. Prepare request
2. Convert request from cpu endian to big endian and send via spi

The critical part here is the reg value (WDATA).  As part of the request, the reg value shall be put to bit 18-3 WITHOUT TOUCHING IT. The reg value as a local u16, shall use the same cpu endian as the request. This is to keep the correct bit order for reg value and also for the request.  Nevertheless reg value is passed by regmap_write.  Regmap core may flip the reg value when val_format_endian != cpu_endian. If this happens, then regmap core has actually changed the reg value.  To prevent regmap to flip the reg value we use REGMAP_ENDIAN_NATIVE as val_format_endian. 

static int smi240_regmap_spi_write(void *context, const void *data,
				   size_t count)
{
	u8 reg_addr;
	u16 reg_data;
	u32 request;
	const u8 *data_ptr = data;
	struct spi_device *spi = context;
	struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
	struct smi240_data *iio_priv_data = iio_priv(indio_dev);

	if (count < 2)
		return -EINVAL;

	reg_addr = data_ptr[0];
	memcpy(&reg_data, &data_ptr[1], sizeof(reg_data));

	request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
	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);

	iio_priv_data->spi_buf = cpu_to_be32(request);

	return spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
}

static const struct regmap_config smi240_regmap_config = {
	.reg_bits = 8,
	.val_bits = 16,
	.val_format_endian = REGMAP_ENDIAN_NATIVE,
};


Fazit:

The bit order in request is critical to us.   FIELD_PREP will take care of the byte order (big / little endian) for us, and always put the value of each part to the correct bit position. We shall never manually change the cpu endian to each part. Just convert the whole request to/from big endian when sending / receiving via spi.

I hope this make the endian handling clear to you.

Best Regards

Jianping Shen

Powered by blists - more mailing lists