| Message ID | 20210922160649.28449-1-andrew_gabbasov@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: rcar: add SMBus block read support | expand |
Hi Andrew, On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov <andrew_gabbasov@mentor.com> wrote: > The smbus block read is not currently supported for rcar i2c devices. > This patchset adds the support to rcar i2c bus so that blocks of data > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > function from the i2c-core-smbus.c). > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > This patch (adapted) was tested with v4.14, but due to lack of real > hardware with SMBus block read operations support, using "simulation", > that is manual analysis of data, read from plain I2C devices with > SMBus block read request. > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > /* > * The last two bytes needs to be fetched using PIO in > * order for the STOP phase to work. > + * > + * For SMBus block read the first byte was received using PIO. So it might be easier to read, and more maintainable, to keep the old assignments: buf = priv->msg->buf; len = priv->msg->len - 2; and adjust them for SMBus afterwards: if (block_data) { /* For SMBus block read the first byte was received using PIO */ buf++; len--; } ? > */ > - buf = priv->msg->buf; > - len = priv->msg->len - 2; > + if (block_data) { > + buf = priv->msg->buf + 1; > + len = priv->msg->len - 3; > + } else { > + buf = priv->msg->buf; > + len = priv->msg->len - 2; > + } > } else { > /* > * First byte in message was sent using PIO. And below we have another case handling buf and len :-( So perhaps: buf = priv->msg->buf; len = priv->msg->len; if (read) { /* * The last two bytes needs to be fetched using PIO in * order for the STOP phase to work. */ len -= 2; } if (!read || block_data) { /* First byte in message was sent using PIO * buf++; len--; } Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for your review! > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, October 05, 2021 4:32 PM > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support > > Hi Andrew, > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov > <andrew_gabbasov@mentor.com> wrote: > > The smbus block read is not currently supported for rcar i2c devices. > > This patchset adds the support to rcar i2c bus so that blocks of data > > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > > function from the i2c-core-smbus.c). > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > > > This patch (adapted) was tested with v4.14, but due to lack of real > > hardware with SMBus block read operations support, using "simulation", > > that is manual analysis of data, read from plain I2C devices with > > SMBus block read request. > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > Thanks for your patch! > > > --- a/drivers/i2c/busses/i2c-rcar.c > > +++ b/drivers/i2c/busses/i2c-rcar.c > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > > /* > > * The last two bytes needs to be fetched using PIO in > > * order for the STOP phase to work. > > + * > > + * For SMBus block read the first byte was received using PIO. > > So it might be easier to read, and more maintainable, to keep the > old assignments: > > buf = priv->msg->buf; > len = priv->msg->len - 2; > > and adjust them for SMBus afterwards: > > if (block_data) { > /* For SMBus block read the first byte was received using PIO */ > buf++; > len--; > } > > ? > > > */ > > - buf = priv->msg->buf; > > - len = priv->msg->len - 2; > > + if (block_data) { > > + buf = priv->msg->buf + 1; > > + len = priv->msg->len - 3; > > + } else { > > + buf = priv->msg->buf; > > + len = priv->msg->len - 2; > > + } > > } else { > > /* > > * First byte in message was sent using PIO. > > And below we have another case handling buf and len :-( > > So perhaps: > > buf = priv->msg->buf; > len = priv->msg->len; > > if (read) { > /* > * The last two bytes needs to be fetched using PIO in > * order for the STOP phase to work. > */ > len -= 2; > } > if (!read || block_data) { > /* First byte in message was sent using PIO * > buf++; > len--; > } Probably I was trying to minimize the changes ;-) However, I agree with you that the whole code fragment can be simplified and your variant indeed looks more clean and understandable. Thank you for your suggestion, I'll submit version 2 of the patch with this fragment changed. Thanks! Best regards, Andrew
Hello Geert, Wolfram, Do you have any feedback on version 2 of this patch, that was submitted after your review comments below? https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ Thanks! Best regards, Andrew > -----Original Message----- > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > Sent: Wednesday, October 06, 2021 9:12 PM > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > Hi Geert, > > Thank you for your review! > > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: Tuesday, October 05, 2021 4:32 PM > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support > > > > Hi Andrew, > > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov > > <andrew_gabbasov@mentor.com> wrote: > > > The smbus block read is not currently supported for rcar i2c devices. > > > This patchset adds the support to rcar i2c bus so that blocks of data > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > > > function from the i2c-core-smbus.c). > > > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > > > > > This patch (adapted) was tested with v4.14, but due to lack of real > > > hardware with SMBus block read operations support, using "simulation", > > > that is manual analysis of data, read from plain I2C devices with > > > SMBus block read request. > > > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > Thanks for your patch! > > > > > --- a/drivers/i2c/busses/i2c-rcar.c > > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > > > /* > > > * The last two bytes needs to be fetched using PIO in > > > * order for the STOP phase to work. > > > + * > > > + * For SMBus block read the first byte was received using PIO. > > > > So it might be easier to read, and more maintainable, to keep the > > old assignments: > > > > buf = priv->msg->buf; > > len = priv->msg->len - 2; > > > > and adjust them for SMBus afterwards: > > > > if (block_data) { > > /* For SMBus block read the first byte was received using PIO */ > > buf++; > > len--; > > } > > > > ? > > > > > */ > > > - buf = priv->msg->buf; > > > - len = priv->msg->len - 2; > > > + if (block_data) { > > > + buf = priv->msg->buf + 1; > > > + len = priv->msg->len - 3; > > > + } else { > > > + buf = priv->msg->buf; > > > + len = priv->msg->len - 2; > > > + } > > > } else { > > > /* > > > * First byte in message was sent using PIO. > > > > And below we have another case handling buf and len :-( > > > > So perhaps: > > > > buf = priv->msg->buf; > > len = priv->msg->len; > > > > if (read) { > > /* > > * The last two bytes needs to be fetched using PIO in > > * order for the STOP phase to work. > > */ > > len -= 2; > > } > > if (!read || block_data) { > > /* First byte in message was sent using PIO * > > buf++; > > len--; > > } > > Probably I was trying to minimize the changes ;-) > > However, I agree with you that the whole code fragment can be simplified > and your variant indeed looks more clean and understandable. > Thank you for your suggestion, I'll submit version 2 of the patch > with this fragment changed. > > Thanks! > > Best regards, > Andrew
Hello Geert, Wolfram, Could you please let me know your opinion on version 2 of this patch, that addressed your earlier review comments? https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ Does it still need any further modifications or are you going to promote it further upstream? Thanks. Best regards, Andrew > -----Original Message----- > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > Sent: Thursday, November 18, 2021 1:35 PM > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > Hello Geert, Wolfram, > > Do you have any feedback on version 2 of this patch, that was submitted > after your review comments below? > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ > > Thanks! > > Best regards, > Andrew > > > -----Original Message----- > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > Sent: Wednesday, October 06, 2021 9:12 PM > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > Hi Geert, > > > > Thank you for your review! > > > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Sent: Tuesday, October 05, 2021 4:32 PM > > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support > > > > > > Hi Andrew, > > > > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov > > > <andrew_gabbasov@mentor.com> wrote: > > > > The smbus block read is not currently supported for rcar i2c devices. > > > > This patchset adds the support to rcar i2c bus so that blocks of data > > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > > > > function from the i2c-core-smbus.c). > > > > > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > > > > > > > This patch (adapted) was tested with v4.14, but due to lack of real > > > > hardware with SMBus block read operations support, using "simulation", > > > > that is manual analysis of data, read from plain I2C devices with > > > > SMBus block read request. > > > > > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > > > Thanks for your patch! > > > > > > > --- a/drivers/i2c/busses/i2c-rcar.c > > > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > > > > /* > > > > * The last two bytes needs to be fetched using PIO in > > > > * order for the STOP phase to work. > > > > + * > > > > + * For SMBus block read the first byte was received using PIO. > > > > > > So it might be easier to read, and more maintainable, to keep the > > > old assignments: > > > > > > buf = priv->msg->buf; > > > len = priv->msg->len - 2; > > > > > > and adjust them for SMBus afterwards: > > > > > > if (block_data) { > > > /* For SMBus block read the first byte was received using PIO */ > > > buf++; > > > len--; > > > } > > > > > > ? > > > > > > > */ > > > > - buf = priv->msg->buf; > > > > - len = priv->msg->len - 2; > > > > + if (block_data) { > > > > + buf = priv->msg->buf + 1; > > > > + len = priv->msg->len - 3; > > > > + } else { > > > > + buf = priv->msg->buf; > > > > + len = priv->msg->len - 2; > > > > + } > > > > } else { > > > > /* > > > > * First byte in message was sent using PIO. > > > > > > And below we have another case handling buf and len :-( > > > > > > So perhaps: > > > > > > buf = priv->msg->buf; > > > len = priv->msg->len; > > > > > > if (read) { > > > /* > > > * The last two bytes needs to be fetched using PIO in > > > * order for the STOP phase to work. > > > */ > > > len -= 2; > > > } > > > if (!read || block_data) { > > > /* First byte in message was sent using PIO * > > > buf++; > > > len--; > > > } > > > > Probably I was trying to minimize the changes ;-) > > > > However, I agree with you that the whole code fragment can be simplified > > and your variant indeed looks more clean and understandable. > > Thank you for your suggestion, I'll submit version 2 of the patch > > with this fragment changed. > > > > Thanks! > > > > Best regards, > > Andrew
Hello Geert, Wolfram, Any feedback on the patch, please? Thanks. Best regards, Andrew > -----Original Message----- > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > Sent: Sunday, January 09, 2022 10:20 PM > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > Hello Geert, Wolfram, > > Could you please let me know your opinion on version 2 of this patch, > that addressed your earlier review comments? > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ > > Does it still need any further modifications or are you going to promote it further upstream? > > Thanks. > > Best regards, > Andrew > > > -----Original Message----- > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > Sent: Thursday, November 18, 2021 1:35 PM > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel > > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > Hello Geert, Wolfram, > > > > Do you have any feedback on version 2 of this patch, that was submitted > > after your review comments below? > > > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ > > > > Thanks! > > > > Best regards, > > Andrew > > > > > -----Original Message----- > > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > Sent: Wednesday, October 06, 2021 9:12 PM > > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > > > Hi Geert, > > > > > > Thank you for your review! > > > > > > > -----Original Message----- > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Sent: Tuesday, October 05, 2021 4:32 PM > > > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support > > > > > > > > Hi Andrew, > > > > > > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov > > > > <andrew_gabbasov@mentor.com> wrote: > > > > > The smbus block read is not currently supported for rcar i2c devices. > > > > > This patchset adds the support to rcar i2c bus so that blocks of data > > > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > > > > > function from the i2c-core-smbus.c). > > > > > > > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > > > > > > > > > This patch (adapted) was tested with v4.14, but due to lack of real > > > > > hardware with SMBus block read operations support, using "simulation", > > > > > that is manual analysis of data, read from plain I2C devices with > > > > > SMBus block read request. > > > > > > > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > > > > > Thanks for your patch! > > > > > > > > > --- a/drivers/i2c/busses/i2c-rcar.c > > > > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > > > > > /* > > > > > * The last two bytes needs to be fetched using PIO in > > > > > * order for the STOP phase to work. > > > > > + * > > > > > + * For SMBus block read the first byte was received using PIO. > > > > > > > > So it might be easier to read, and more maintainable, to keep the > > > > old assignments: > > > > > > > > buf = priv->msg->buf; > > > > len = priv->msg->len - 2; > > > > > > > > and adjust them for SMBus afterwards: > > > > > > > > if (block_data) { > > > > /* For SMBus block read the first byte was received using PIO */ > > > > buf++; > > > > len--; > > > > } > > > > > > > > ? > > > > > > > > > */ > > > > > - buf = priv->msg->buf; > > > > > - len = priv->msg->len - 2; > > > > > + if (block_data) { > > > > > + buf = priv->msg->buf + 1; > > > > > + len = priv->msg->len - 3; > > > > > + } else { > > > > > + buf = priv->msg->buf; > > > > > + len = priv->msg->len - 2; > > > > > + } > > > > > } else { > > > > > /* > > > > > * First byte in message was sent using PIO. > > > > > > > > And below we have another case handling buf and len :-( > > > > > > > > So perhaps: > > > > > > > > buf = priv->msg->buf; > > > > len = priv->msg->len; > > > > > > > > if (read) { > > > > /* > > > > * The last two bytes needs to be fetched using PIO in > > > > * order for the STOP phase to work. > > > > */ > > > > len -= 2; > > > > } > > > > if (!read || block_data) { > > > > /* First byte in message was sent using PIO * > > > > buf++; > > > > len--; > > > > } > > > > > > Probably I was trying to minimize the changes ;-) > > > > > > However, I agree with you that the whole code fragment can be simplified > > > and your variant indeed looks more clean and understandable. > > > Thank you for your suggestion, I'll submit version 2 of the patch > > > with this fragment changed. > > > > > > Thanks! > > > > > > Best regards, > > > Andrew
Hello Geert, Wolfram, Could you please let us know your opinion on this patch and further requirements, if any. Thanks! Best regards, Andrew > -----Original Message----- > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > Sent: Tuesday, January 25, 2022 9:46 AM > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > Hello Geert, Wolfram, > > Any feedback on the patch, please? > > Thanks. > > Best regards, > Andrew > > > -----Original Message----- > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > Sent: Sunday, January 09, 2022 10:20 PM > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux Kernel > > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > Hello Geert, Wolfram, > > > > Could you please let me know your opinion on version 2 of this patch, > > that addressed your earlier review comments? > > > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ > > > > Does it still need any further modifications or are you going to promote it further upstream? > > > > Thanks. > > > > Best regards, > > Andrew > > > > > -----Original Message----- > > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > Sent: Thursday, November 18, 2021 1:35 PM > > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > > Cc: 'Linux-Renesas' <linux-renesas-soc@vger.kernel.org>; 'Linux I2C' <linux-i2c@vger.kernel.org>; 'Linux > Kernel > > > Mailing List' <linux-kernel@vger.kernel.org>; 'Wolfram Sang' <wsa+renesas@sang-engineering.com>; Surachari, > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > > > Hello Geert, Wolfram, > > > > > > Do you have any feedback on version 2 of this patch, that was submitted > > > after your review comments below? > > > > > > https://lore.kernel.org/all/20211006182314.10585-1-andrew_gabbasov@mentor.com/ > > > > > > Thanks! > > > > > > Best regards, > > > Andrew > > > > > > > -----Original Message----- > > > > From: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > Sent: Wednesday, October 06, 2021 9:12 PM > > > > To: 'Geert Uytterhoeven' <geert@linux-m68k.org> > > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux Kernel > > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > > Subject: RE: [PATCH] i2c: rcar: add SMBus block read support > > > > > > > > Hi Geert, > > > > > > > > Thank you for your review! > > > > > > > > > -----Original Message----- > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Sent: Tuesday, October 05, 2021 4:32 PM > > > > > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > > > > > Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>; Linux I2C <linux-i2c@vger.kernel.org>; Linux > Kernel > > > > > Mailing List <linux-kernel@vger.kernel.org>; Wolfram Sang <wsa+renesas@sang-engineering.com>; Surachari, > > > > > Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > > > > > Subject: Re: [PATCH] i2c: rcar: add SMBus block read support > > > > > > > > > > Hi Andrew, > > > > > > > > > > On Wed, Sep 22, 2021 at 6:14 PM Andrew Gabbasov > > > > > <andrew_gabbasov@mentor.com> wrote: > > > > > > The smbus block read is not currently supported for rcar i2c devices. > > > > > > This patchset adds the support to rcar i2c bus so that blocks of data > > > > > > can be read using SMbus block reads.(using i2c_smbus_read_block_data() > > > > > > function from the i2c-core-smbus.c). > > > > > > > > > > > > Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") > > > > > > > > > > > > This patch (adapted) was tested with v4.14, but due to lack of real > > > > > > hardware with SMBus block read operations support, using "simulation", > > > > > > that is manual analysis of data, read from plain I2C devices with > > > > > > SMBus block read request. > > > > > > > > > > > > Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> > > > > > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > --- a/drivers/i2c/busses/i2c-rcar.c > > > > > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > > > > @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) > > > > > > /* > > > > > > * The last two bytes needs to be fetched using PIO in > > > > > > * order for the STOP phase to work. > > > > > > + * > > > > > > + * For SMBus block read the first byte was received using PIO. > > > > > > > > > > So it might be easier to read, and more maintainable, to keep the > > > > > old assignments: > > > > > > > > > > buf = priv->msg->buf; > > > > > len = priv->msg->len - 2; > > > > > > > > > > and adjust them for SMBus afterwards: > > > > > > > > > > if (block_data) { > > > > > /* For SMBus block read the first byte was received using PIO */ > > > > > buf++; > > > > > len--; > > > > > } > > > > > > > > > > ? > > > > > > > > > > > */ > > > > > > - buf = priv->msg->buf; > > > > > > - len = priv->msg->len - 2; > > > > > > + if (block_data) { > > > > > > + buf = priv->msg->buf + 1; > > > > > > + len = priv->msg->len - 3; > > > > > > + } else { > > > > > > + buf = priv->msg->buf; > > > > > > + len = priv->msg->len - 2; > > > > > > + } > > > > > > } else { > > > > > > /* > > > > > > * First byte in message was sent using PIO. > > > > > > > > > > And below we have another case handling buf and len :-( > > > > > > > > > > So perhaps: > > > > > > > > > > buf = priv->msg->buf; > > > > > len = priv->msg->len; > > > > > > > > > > if (read) { > > > > > /* > > > > > * The last two bytes needs to be fetched using PIO in > > > > > * order for the STOP phase to work. > > > > > */ > > > > > len -= 2; > > > > > } > > > > > if (!read || block_data) { > > > > > /* First byte in message was sent using PIO * > > > > > buf++; > > > > > len--; > > > > > } > > > > > > > > Probably I was trying to minimize the changes ;-) > > > > > > > > However, I agree with you that the whole code fragment can be simplified > > > > and your variant indeed looks more clean and understandable. > > > > Thank you for your suggestion, I'll submit version 2 of the patch > > > > with this fragment changed. > > > > > > > > Thanks! > > > > > > > > Best regards, > > > > Andrew
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index bff9913c37b8..a9fc2b3b6392 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -105,6 +105,7 @@ #define ID_DONE (1 << 2) #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) +#define ID_EPROTO (1 << 5) /* persistent flags */ #define ID_P_HOST_NOTIFY BIT(28) #define ID_P_REP_AFTER_RD BIT(29) @@ -412,6 +413,7 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) struct device *dev = rcar_i2c_priv_to_dev(priv); struct i2c_msg *msg = priv->msg; bool read = msg->flags & I2C_M_RD; + bool block_data = msg->flags & I2C_M_RECV_LEN; enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE; struct dma_chan *chan = read ? priv->dma_rx : priv->dma_tx; struct dma_async_tx_descriptor *txdesc; @@ -429,9 +431,16 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) /* * The last two bytes needs to be fetched using PIO in * order for the STOP phase to work. + * + * For SMBus block read the first byte was received using PIO. */ - buf = priv->msg->buf; - len = priv->msg->len - 2; + if (block_data) { + buf = priv->msg->buf + 1; + len = priv->msg->len - 3; + } else { + buf = priv->msg->buf; + len = priv->msg->len - 2; + } } else { /* * First byte in message was sent using PIO. @@ -530,6 +539,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) { struct i2c_msg *msg = priv->msg; + bool block_data = msg->flags & I2C_M_RECV_LEN; /* FIXME: sometimes, unknown interrupt happened. Do nothing */ if (!(msr & MDR)) @@ -538,8 +548,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) if (msr & MAT) { /* * Address transfer phase finished, but no data at this point. - * Try to use DMA to receive data. + * Try to use DMA to receive data if it is not SMBus block + * data read. */ + if (block_data) + goto next_txn; + + rcar_i2c_dma(priv); + } else if (priv->pos == 0 && block_data) { + /* + * First byte is the length of remaining packet + * in the SMBus block data read. Add it to + * msg->len. + */ + u8 data = rcar_i2c_read(priv, ICRXTX); + + if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) { + priv->flags |= ID_DONE | ID_EPROTO; + return; + } + msg->len += data; + msg->buf[priv->pos] = data; + priv->pos++; + /* Still try to use DMA to receive the rest of data */ rcar_i2c_dma(priv); } else if (priv->pos < msg->len) { /* get received data */ @@ -557,6 +588,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) } } +next_txn: if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) rcar_i2c_next_msg(priv); else @@ -855,6 +887,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -917,6 +951,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -983,7 +1019,8 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap) * I2C_M_IGNORE_NAK (automatically sends STOP after NAK) */ u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE | - (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); + (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) | + I2C_FUNC_SMBUS_READ_BLOCK_DATA; if (priv->flags & ID_P_HOST_NOTIFY) func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
The smbus block read is not currently supported for rcar i2c devices. This patchset adds the support to rcar i2c bus so that blocks of data can be read using SMbus block reads.(using i2c_smbus_read_block_data() function from the i2c-core-smbus.c). Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") This patch (adapted) was tested with v4.14, but due to lack of real hardware with SMBus block read operations support, using "simulation", that is manual analysis of data, read from plain I2C devices with SMBus block read request. Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> --- drivers/i2c/busses/i2c-rcar.c | 45 +++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-)