diff mbox series

[v1] i2c: microchip-corei2c: add smbus support

Message ID 20250430-preview-dormitory-85191523283d@spud
State New
Headers show
Series [v1] i2c: microchip-corei2c: add smbus support | expand

Commit Message

Conor Dooley April 30, 2025, 11:23 a.m. UTC 
From: prashanth kumar burujukindi <prashanthkumar.burujukindi@microchip.com>

In this driver the supported SMBUS commands are smbus_quick,
smbus_byte, smbus_byte_data, smbus_word_data and smbus_block_data.

Signed-off-by: prashanth kumar burujukindi <prashanthkumar.burujukindi@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
smbus block read is not tested, due to lack of hardware, but is supported
by the controller, although we have tested smbus i2c block read.

CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Andi Shyti <andi.shyti@kernel.org>
CC: linux-i2c@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/i2c/busses/i2c-microchip-corei2c.c | 102 +++++++++++++++++++++
 1 file changed, 102 insertions(+)

Comments

Andi Shyti May 6, 2025, 12:04 p.m. UTC | #1 
Hi Conor,

On Tue, May 06, 2025 at 11:56:19AM +0100, Conor Dooley wrote:
> On Mon, May 05, 2025 at 10:04:27PM +0200, Andi Shyti wrote:
> > On Wed, Apr 30, 2025 at 05:06:09PM +0530, Mukesh Kumar Savaliya wrote:
> > > On 4/30/2025 4:53 PM, Conor Dooley wrote:
> > > > From: prashanth kumar burujukindi <prashanthkumar.burujukindi@microchip.com>

Do we want to keep lower case for names and surnames? Can I use
upper cases?

> > > > In this driver the supported SMBUS commands are smbus_quick,
> > > > smbus_byte, smbus_byte_data, smbus_word_data and smbus_block_data.
> > > > 
> > > Write completely in imperative mood. something like :
> > > 
> > > Add support for SMBUS commands in driver
> > > 
> > > Add support for SMBUS commands: smbus_quick, smbus_byte, smbus_byte_data,
> > > smbus_word_data, and smbus_block_data.
> > 
> > yes, I agree that the original commit log is a bit lazy written :-)
> 
> I don't personally think the suggested wording makes any meaningful
> difference, but I can rework it if required.

The point of using the imperative form is to clearly state what
the patch does. Saying "the supported commands are..." feels a
bit lazy, in my opinion, and requires a peek into the change to
fully understand what the patch introduces.

To be honest, I wouldn't reject the patch over this, but it
doesn't hurt to expand the log a little.

(No need to resend—you can just reply to this mail with your
updated commit log.)

> > > Also mention below limitations here .
> 
> I actually removed them from the commit message, since they're not
> limitations just what was and was not tested. I can put them back too
> if that's needed.
> 
> > > SMBUS block read is supported by the controller but has not been tested due
> > > to lack of hardware. However, SMBUS I2C block read has been tested.
> > 
> > Smbus i2c block has not been tested? If so, can we leave it out?
> > What is the interest to keep it in?
> 
> What's the interest in adding any feature? Someone might want to use it.

What's the point of adding a feature that no one uses? :-)

> We did not have a piece of hardware that uses it, so didn't do testing
> of that specific command, but a customer may well want to so we included
> it. Again, if you think removing it is the play, I can do that.

No worries, please leave it as it is if you think it will be
useful in the future. I just wanted to clarify.

Thanks,
Andi
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c
index 5db73429125c0..492bf4c34722c 100644
--- a/drivers/i2c/busses/i2c-microchip-corei2c.c
+++ b/drivers/i2c/busses/i2c-microchip-corei2c.c
@@ -76,6 +76,8 @@ 
 #define CORE_I2C_FREQ		(0x14)
 #define CORE_I2C_GLITCHREG	(0x18)
 #define CORE_I2C_SLAVE1_ADDR	(0x1c)
+#define CORE_I2C_SMBUS_MSG_WR	(0x0)
+#define CORE_I2C_SMBUS_MSG_RD	(0x1)
 
 #define PCLK_DIV_960	(CTRL_CR2)
 #define PCLK_DIV_256	(0)
@@ -424,9 +426,109 @@  static u32 mchp_corei2c_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }
 
+static int mchp_corei2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned short flags,
+				   char read_write, u8 command,
+				   int size, union i2c_smbus_data *data)
+{
+	struct i2c_msg msgs[2];
+	struct mchp_corei2c_dev *idev = i2c_get_adapdata(adap);
+	u8 tx_buf[I2C_SMBUS_BLOCK_MAX + 2];
+	u8 rx_buf[I2C_SMBUS_BLOCK_MAX + 1];
+	int num_msgs = 1;
+
+	msgs[CORE_I2C_SMBUS_MSG_WR].addr = addr;
+	msgs[CORE_I2C_SMBUS_MSG_WR].flags = 0;
+
+	if (read_write == I2C_SMBUS_READ && size <= I2C_SMBUS_BYTE)
+		msgs[CORE_I2C_SMBUS_MSG_WR].flags = I2C_M_RD;
+
+	if (read_write == I2C_SMBUS_WRITE && size <= I2C_SMBUS_WORD_DATA)
+		msgs[CORE_I2C_SMBUS_MSG_WR].len = size;
+
+	if (read_write == I2C_SMBUS_WRITE && size > I2C_SMBUS_BYTE) {
+		msgs[CORE_I2C_SMBUS_MSG_WR].buf = tx_buf;
+		msgs[CORE_I2C_SMBUS_MSG_WR].buf[0] = command;
+	}
+
+	if (read_write == I2C_SMBUS_READ && size >= I2C_SMBUS_BYTE_DATA) {
+		msgs[CORE_I2C_SMBUS_MSG_WR].buf = tx_buf;
+		msgs[CORE_I2C_SMBUS_MSG_WR].buf[0] = command;
+		msgs[CORE_I2C_SMBUS_MSG_RD].addr = addr;
+		msgs[CORE_I2C_SMBUS_MSG_RD].flags = I2C_M_RD;
+		num_msgs = 2;
+	}
+
+	if (read_write == I2C_SMBUS_READ && size > I2C_SMBUS_QUICK)
+		msgs[CORE_I2C_SMBUS_MSG_WR].len = 1;
+
+	switch (size) {
+	case I2C_SMBUS_QUICK:
+		msgs[CORE_I2C_SMBUS_MSG_WR].buf = NULL;
+		return 0;
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE)
+			msgs[CORE_I2C_SMBUS_MSG_WR].buf = &command;
+		else
+			msgs[CORE_I2C_SMBUS_MSG_WR].buf = &data->byte;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			msgs[CORE_I2C_SMBUS_MSG_WR].buf[1] = data->byte;
+		} else {
+			msgs[CORE_I2C_SMBUS_MSG_RD].len = size - 1;
+			msgs[CORE_I2C_SMBUS_MSG_RD].buf = &data->byte;
+		}
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			msgs[CORE_I2C_SMBUS_MSG_WR].buf[1] = data->word & 0xFF;
+			msgs[CORE_I2C_SMBUS_MSG_WR].buf[2] = (data->word >> 8) & 0xFF;
+		} else {
+			msgs[CORE_I2C_SMBUS_MSG_RD].len = size - 1;
+			msgs[CORE_I2C_SMBUS_MSG_RD].buf = rx_buf;
+		}
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			int data_len;
+
+			data_len = data->block[0];
+			msgs[CORE_I2C_SMBUS_MSG_WR].len = data_len + 2;
+			for (int i = 0; i <= data_len; i++)
+				msgs[CORE_I2C_SMBUS_MSG_WR].buf[i + 1] = data->block[i];
+		} else {
+			msgs[CORE_I2C_SMBUS_MSG_RD].len = I2C_SMBUS_BLOCK_MAX + 1;
+			msgs[CORE_I2C_SMBUS_MSG_RD].buf = rx_buf;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	mchp_corei2c_xfer(&idev->adapter, msgs, num_msgs);
+	if (read_write == I2C_SMBUS_WRITE || size <= I2C_SMBUS_BYTE_DATA)
+		return 0;
+
+	switch (size) {
+	case I2C_SMBUS_WORD_DATA:
+		data->word = (rx_buf[0] | (rx_buf[1] << 8));
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		if (rx_buf[0] > I2C_SMBUS_BLOCK_MAX)
+			rx_buf[0] = I2C_SMBUS_BLOCK_MAX;
+		/* As per protocol first member of block is size of the block. */
+		for (int i = 0; i <= rx_buf[0]; i++)
+			data->block[i] = rx_buf[i];
+		break;
+	}
+
+	return 0;
+}
+
 static const struct i2c_algorithm mchp_corei2c_algo = {
 	.master_xfer = mchp_corei2c_xfer,
 	.functionality = mchp_corei2c_func,
+	.smbus_xfer = mchp_corei2c_smbus_xfer,
 };
 
 static int mchp_corei2c_probe(struct platform_device *pdev)