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

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

[Conor Dooley]

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(+)

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)
-- 
2.45.2

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

[Mukesh Kumar Savaliya]

On 4/30/2025 4:53 PM, Conor Dooley wrote:
> 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.
> 
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.

Also mention below limitations here .
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.

> 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(+)
> 
> diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c

[...]

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

[Andi Shyti]

Hi,

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>
> > 
> > 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 :-)

> Also mention below limitations here .
> 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?

Thanks,
Andi

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On Mon, May 05, 2025 at 10:04:27PM +0200, Andi Shyti wrote:
> Hi,
> 
> 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>
> > > 
> > > 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.

> > 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.
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.

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andi Shyti]

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

On Tue, May 06, 2025 at 02:04:55PM +0200, Andi Shyti wrote:
> 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?

I dunno, that's how it was provided, I'm a fan of self-determination in
that regard.

> > > > > 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.

Right, I wouldn't either. Sure, it could have been better and I
probably should have rewritten it when I sent it on - but I get more
than a bit pissed off and opt to push back when people who aren't
maintainers for some code come along with a review entirely about
cosmetic parts of a commit message.

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

I was just about to do this, but noticed you picked the patch up
already. Sorry for the delay there, I meant to do it yesterday but
crashed out early. I'd just have changed it to
"Add hardware support for the SMBUS commands smbus_quick, smbus_byte,
smbus_byte_data, smbus_word_data and smbus_block_data, replacing the
fallback to software emulation"
or similar. If you fancy rebasing, maybe use that?

> > > > 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? :-)

I wouldn't say no one, just neither Prashanth or I :-)

> > 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.

NW, thanks for picking it up.
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Andi Shyti]

Hi Conor,

> > (No need to resend—you can just reply to this mail with your
> > updated commit log.)
> 
> I was just about to do this, but noticed you picked the patch up
> already. Sorry for the delay there, I meant to do it yesterday but
> crashed out early. I'd just have changed it to
> "Add hardware support for the SMBUS commands smbus_quick, smbus_byte,
> smbus_byte_data, smbus_word_data and smbus_block_data, replacing the
> fallback to software emulation"
> or similar. If you fancy rebasing, maybe use that?

Done! Thanks for following up!

Andi

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

[Andi Shyti]

Hi Conor,

On Wed, Apr 30, 2025 at 12:23:39PM +0100, Conor Dooley wrote:
> 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>

merged to i2c/i2c-host.

Andi