| Message ID | 20250430-preview-dormitory-85191523283d@spud |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] i2c: microchip-corei2c: add smbus support | expand |
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.
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
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
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.
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
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)