I2C driver currently reports "DMA txn failed" error even though it's
NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
on the bus instead of generic transfer failure which doesn't give any
specific clue.
Make Changes inside i2c driver callback handler function
i2c_gpi_cb_result() to parse these errors and make sure GSI driver
stores the error status during error interrupt.
Co-developed-by: Viken Dadhaniya <[email protected]>
Signed-off-by: Viken Dadhaniya <[email protected]>
Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
---
v3 -> v4:
- Included bitfield.h to fix compilation issue for x86 arch.
- Removed Fixes tag as this is not fixing any crash.
- Added Reviewed-by tag.
v2 -> v3:
- Modifed commit log reflecting an imperative mood.
v1 -> v2:
- Commit log changed we->We.
- Explained the problem that we are not detecing NACK error.
- Removed Heap based memory allocation and hence memory leakage issue.
- Used FIELD_GET and removed shiting and masking every time as suggested by Bjorn.
- Changed commit log to reflect the code changes done.
- Removed adding anything into struct gpi_i2c_config and created new structure
for error status as suggested by Bjorn.
---
drivers/dma/qcom/gpi.c | 12 +++++++++++-
drivers/i2c/busses/i2c-qcom-geni.c | 20 ++++++++++++++++----
include/linux/dma/qcom-gpi-dma.h | 10 ++++++++++
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index 1c93864e0e4d..e3508d51fdc9 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -1076,7 +1076,17 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
dma_cookie_complete(&vd->tx);
- dmaengine_desc_get_callback_invoke(&vd->tx, &result);
+ if (gchan->protocol == QCOM_GPI_I2C) {
+ struct dmaengine_desc_callback cb;
+ struct gpi_i2c_result *i2c;
+
+ dmaengine_desc_get_callback(&vd->tx, &cb);
+ i2c = cb.callback_param;
+ i2c->status = compl_event->status;
+ dmaengine_desc_callback_invoke(&cb, &result);
+ } else {
+ dmaengine_desc_get_callback_invoke(&vd->tx, &result);
+ }
gpi_free_desc:
spin_lock_irqsave(&gchan->vc.lock, flags);
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index da94df466e83..11dcfcf13d8b 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -2,6 +2,7 @@
// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
#include <linux/acpi.h>
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/dmaengine.h>
#include <linux/dma-mapping.h>
@@ -66,6 +67,7 @@ enum geni_i2c_err_code {
GENI_TIMEOUT,
};
+#define I2C_DMA_TX_IRQ_MASK GENMASK(12, 5)
#define DM_I2C_CB_ERR ((BIT(NACK) | BIT(BUS_PROTO) | BIT(ARB_LOST)) \
<< 5)
@@ -99,6 +101,7 @@ struct geni_i2c_dev {
struct dma_chan *rx_c;
bool gpi_mode;
bool abort_done;
+ struct gpi_i2c_result i2c_result;
};
struct geni_i2c_desc {
@@ -484,9 +487,18 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
{
- struct geni_i2c_dev *gi2c = cb;
-
- if (result->result != DMA_TRANS_NOERROR) {
+ struct gpi_i2c_result *i2c_res = cb;
+ struct geni_i2c_dev *gi2c = container_of(i2c_res, struct geni_i2c_dev, i2c_result);
+ u32 status;
+
+ status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
+ if (status == BIT(NACK)) {
+ geni_i2c_err(gi2c, NACK);
+ } else if (status == BIT(BUS_PROTO)) {
+ geni_i2c_err(gi2c, BUS_PROTO);
+ } else if (status == BIT(ARB_LOST)) {
+ geni_i2c_err(gi2c, ARB_LOST);
+ } else if (result->result != DMA_TRANS_NOERROR) {
dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
gi2c->err = -EIO;
} else if (result->residue) {
@@ -568,7 +580,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
}
desc->callback_result = i2c_gpi_cb_result;
- desc->callback_param = gi2c;
+ desc->callback_param = &gi2c->i2c_result;
dmaengine_submit(desc);
*buf = dma_buf;
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 6680dd1a43c6..f585c6a35e51 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -80,4 +80,14 @@ struct gpi_i2c_config {
bool multi_msg;
};
+/**
+ * struct gpi_i2c_result - i2c transfer status result in GSI mode
+ *
+ * @status: store txfer status value as part of callback
+ *
+ */
+struct gpi_i2c_result {
+ u32 status;
+};
+
#endif /* QCOM_GPI_DMA_H */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 13-03-24, 10:56, Mukesh Kumar Savaliya wrote:
> I2C driver currently reports "DMA txn failed" error even though it's
> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> on the bus instead of generic transfer failure which doesn't give any
> specific clue.
>
> Make Changes inside i2c driver callback handler function
> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> stores the error status during error interrupt.
>
> Co-developed-by: Viken Dadhaniya <[email protected]>
> Signed-off-by: Viken Dadhaniya <[email protected]>
> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
> v3 -> v4:
> - Included bitfield.h to fix compilation issue for x86 arch.
> - Removed Fixes tag as this is not fixing any crash.
> - Added Reviewed-by tag.
>
> v2 -> v3:
> - Modifed commit log reflecting an imperative mood.
>
> v1 -> v2:
> - Commit log changed we->We.
> - Explained the problem that we are not detecing NACK error.
> - Removed Heap based memory allocation and hence memory leakage issue.
> - Used FIELD_GET and removed shiting and masking every time as suggested by Bjorn.
> - Changed commit log to reflect the code changes done.
> - Removed adding anything into struct gpi_i2c_config and created new structure
> for error status as suggested by Bjorn.
> ---
> drivers/dma/qcom/gpi.c | 12 +++++++++++-
> drivers/i2c/busses/i2c-qcom-geni.c | 20 ++++++++++++++++----
> include/linux/dma/qcom-gpi-dma.h | 10 ++++++++++
> 3 files changed, 37 insertions(+), 5 deletions(-)
Urgh, why do we have i2c and dma changes in single patch? I dont think
they would be dependent
>
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 1c93864e0e4d..e3508d51fdc9 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1076,7 +1076,17 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
> dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
>
> dma_cookie_complete(&vd->tx);
> - dmaengine_desc_get_callback_invoke(&vd->tx, &result);
> + if (gchan->protocol == QCOM_GPI_I2C) {
> + struct dmaengine_desc_callback cb;
> + struct gpi_i2c_result *i2c;
> +
> + dmaengine_desc_get_callback(&vd->tx, &cb);
> + i2c = cb.callback_param;
> + i2c->status = compl_event->status;
> + dmaengine_desc_callback_invoke(&cb, &result);
This is generic, why should this be i2c specific... we should set
generic status value
> + } else {
> + dmaengine_desc_get_callback_invoke(&vd->tx, &result);
> + }
>
> gpi_free_desc:
> spin_lock_irqsave(&gchan->vc.lock, flags);
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index da94df466e83..11dcfcf13d8b 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -2,6 +2,7 @@
> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>
> #include <linux/acpi.h>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> @@ -66,6 +67,7 @@ enum geni_i2c_err_code {
> GENI_TIMEOUT,
> };
>
> +#define I2C_DMA_TX_IRQ_MASK GENMASK(12, 5)
> #define DM_I2C_CB_ERR ((BIT(NACK) | BIT(BUS_PROTO) | BIT(ARB_LOST)) \
> << 5)
>
> @@ -99,6 +101,7 @@ struct geni_i2c_dev {
> struct dma_chan *rx_c;
> bool gpi_mode;
> bool abort_done;
> + struct gpi_i2c_result i2c_result;
> };
>
> struct geni_i2c_desc {
> @@ -484,9 +487,18 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>
> static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
> {
> - struct geni_i2c_dev *gi2c = cb;
> -
> - if (result->result != DMA_TRANS_NOERROR) {
> + struct gpi_i2c_result *i2c_res = cb;
> + struct geni_i2c_dev *gi2c = container_of(i2c_res, struct geni_i2c_dev, i2c_result);
> + u32 status;
> +
> + status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
> + if (status == BIT(NACK)) {
> + geni_i2c_err(gi2c, NACK);
> + } else if (status == BIT(BUS_PROTO)) {
> + geni_i2c_err(gi2c, BUS_PROTO);
> + } else if (status == BIT(ARB_LOST)) {
> + geni_i2c_err(gi2c, ARB_LOST);
> + } else if (result->result != DMA_TRANS_NOERROR) {
> dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> gi2c->err = -EIO;
> } else if (result->residue) {
> @@ -568,7 +580,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> }
>
> desc->callback_result = i2c_gpi_cb_result;
> - desc->callback_param = gi2c;
> + desc->callback_param = &gi2c->i2c_result;
>
> dmaengine_submit(desc);
> *buf = dma_buf;
> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
> index 6680dd1a43c6..f585c6a35e51 100644
> --- a/include/linux/dma/qcom-gpi-dma.h
> +++ b/include/linux/dma/qcom-gpi-dma.h
> @@ -80,4 +80,14 @@ struct gpi_i2c_config {
> bool multi_msg;
> };
>
> +/**
> + * struct gpi_i2c_result - i2c transfer status result in GSI mode
> + *
> + * @status: store txfer status value as part of callback
> + *
> + */
> +struct gpi_i2c_result {
> + u32 status;
> +};
> +
> #endif /* QCOM_GPI_DMA_H */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
~Vinod
Hi
On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> I2C driver currently reports "DMA txn failed" error even though it's
> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> on the bus instead of generic transfer failure which doesn't give any
> specific clue.
>
> Make Changes inside i2c driver callback handler function
> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> stores the error status during error interrupt.
>
> [...]
Applied to i2c/i2c-host-next on
git://git.kernel.org/pub/scm/linux/kernel/git/local tree
Thank you,
Andi
Patches applied
===============
[1/1] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode
commit: 394b3e3ead0d9fdcc1ef53bb893fdbe7bf1db3ac
On 28-03-24, 08:36, Andi Shyti wrote:
> Hi
>
> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> > I2C driver currently reports "DMA txn failed" error even though it's
> > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > on the bus instead of generic transfer failure which doesn't give any
> > specific clue.
> >
> > Make Changes inside i2c driver callback handler function
> > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > stores the error status during error interrupt.
> >
> > [...]
>
> Applied to i2c/i2c-host-next on
>
> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
You applied changes to dmaengine driver without my ack! I dont agree to
the approach here, we could do better
--
~Vinod
Hi Vinod,
On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
> On 28-03-24, 08:36, Andi Shyti wrote:
> > On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> > > I2C driver currently reports "DMA txn failed" error even though it's
> > > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > > on the bus instead of generic transfer failure which doesn't give any
> > > specific clue.
> > >
> > > Make Changes inside i2c driver callback handler function
> > > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > > stores the error status during error interrupt.
> > >
> > > [...]
> >
> > Applied to i2c/i2c-host-next on
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>
> You applied changes to dmaengine driver without my ack! I dont agree to
> the approach here, we could do better
This patch has been around for quite some time and there has been
time to review it. Altrady two people have approved it.
This patch has already been merged once via the i2c with the
agreement of everyone, but reverted for a trivial failure.
Your review come after I have merged the patch (I did merge it
even earlier, but forgot to send the notification, which was
anyway sent before your review).
Above all, I appreciate your review, but it wouldn't be fair to
revert it now. If Mukesh is OK, I can do it, otherwise we can
send subsequent patches.
Mukesh, please let me know what's your preference.
Andi
Hi Vinod,
On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
> On 28-03-24, 08:36, Andi Shyti wrote:
> > Hi
> >
> > On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> > > I2C driver currently reports "DMA txn failed" error even though it's
> > > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > > on the bus instead of generic transfer failure which doesn't give any
> > > specific clue.
> > >
> > > Make Changes inside i2c driver callback handler function
> > > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > > stores the error status during error interrupt.
> > >
> > > [...]
> >
> > Applied to i2c/i2c-host-next on
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>
> You applied changes to dmaengine driver without my ack! I dont agree to
> the approach here, we could do better
this must be an error from b4 ty. The changes have been added to
pub/scm/linux/kernel/git/andi.shyti/linux.git
branch i2c/i2c-host, As it has been agreed from very long.
Anyway, the changes are in -next. What do we do now? Do I revert
it? Mukesh, can you please agree with Vinod?
Andi
Thanks Vinod and Andi !
It had time and also there was a comment to get sign off from DMA
maintainers, we have had review and discussion on DMA part too.
Hi Vinod, Since this is already merged, do you have preference to revert
OR making a new change if any BUG OR design issue ? I can also fix the
changes you suggest and raise a new patch in case of any real bug OR
design expectations.
On 4/2/2024 10:14 PM, Andi Shyti wrote:
> Hi Vinod,
>
> On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
>> On 28-03-24, 08:36, Andi Shyti wrote:
>>> Hi
>>>
>>> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
>>>> I2C driver currently reports "DMA txn failed" error even though it's
>>>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
>>>> on the bus instead of generic transfer failure which doesn't give any
>>>> specific clue.
>>>>
>>>> Make Changes inside i2c driver callback handler function
>>>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
>>>> stores the error status during error interrupt.
>>>>
>>>> [...]
>>>
>>> Applied to i2c/i2c-host-next on
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>>
>> You applied changes to dmaengine driver without my ack! I dont agree to
>> the approach here, we could do better
>
> this must be an error from b4 ty. The changes have been added to
>
> pub/scm/linux/kernel/git/andi.shyti/linux.git
>
> branch i2c/i2c-host, As it has been agreed from very long.
>
> Anyway, the changes are in -next. What do we do now? Do I revert
> it? Mukesh, can you please agree with Vinod?
>
> Andi
On 30/03/2024 00:54, Andi Shyti wrote:
> Hi Vinod,
>
> On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
>> On 28-03-24, 08:36, Andi Shyti wrote:
>>> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
>>>> I2C driver currently reports "DMA txn failed" error even though it's
>>>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
>>>> on the bus instead of generic transfer failure which doesn't give any
>>>> specific clue.
>>>>
>>>> Make Changes inside i2c driver callback handler function
>>>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
>>>> stores the error status during error interrupt.
>>>>
>>>> [...]
>>>
>>> Applied to i2c/i2c-host-next on
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>>
>> You applied changes to dmaengine driver without my ack! I dont agree to
>> the approach here, we could do better
>
> This patch has been around for quite some time and there has been
> time to review it. Altrady two people have approved it.
That's not true. The patch was sent during merge window, so that time
obviously does not count. Anything not being a fix sent during merge
window is postponed by many maintainers. Therefore this patch *must be*
considered as sent on 24th of March. You applied it 4 days later, giving
Vinod exactly only four days to react.
>
> This patch has already been merged once via the i2c with the
> agreement of everyone, but reverted for a trivial failure.
You need agreement of other maintainers. Or at least ping them. Did it
happen here?
>
> Your review come after I have merged the patch (I did merge it
> even earlier, but forgot to send the notification, which was
> anyway sent before your review).
So you applied it during merge window? How anyone can react to this?
Best regards,
Krzysztof
On 03/04/2024 08:09, Mukesh Kumar Savaliya wrote:
> Thanks Vinod and Andi !
>
> It had time and also there was a comment to get sign off from DMA
> maintainers, we have had review and discussion on DMA part too.
>
> Hi Vinod, Since this is already merged, do you have preference to revert
> OR making a new change if any BUG OR design issue ? I can also fix the
> changes you suggest and raise a new patch in case of any real bug OR
> design expectations.
Can you address Vinod's comments?
Best regards,
Krzysztof
Hi Vinod,
On 3/29/2024 10:15 PM, Vinod Koul wrote:
> On 28-03-24, 08:36, Andi Shyti wrote:
>> Hi
>>
>> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
>>> I2C driver currently reports "DMA txn failed" error even though it's
>>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
>>> on the bus instead of generic transfer failure which doesn't give any
>>> specific clue.
>>>
>>> Make Changes inside i2c driver callback handler function
>>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
>>> stores the error status during error interrupt.
>>>
>>> [...]
>>
>> Applied to i2c/i2c-host-next on
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>
> You applied changes to dmaengine driver without my ack! I dont agree to
> the approach here, we could do better
>
Could you please suggest changes OR approach related comments ? So i can
make a new change which can clean the merged code ? Hope that can
address the concerns.
On 03/04/2024 08:46, Mukesh Kumar Savaliya wrote:
> Hi Vinod,
>
> On 3/29/2024 10:15 PM, Vinod Koul wrote:
>> On 28-03-24, 08:36, Andi Shyti wrote:
>>> Hi
>>>
>>> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
>>>> I2C driver currently reports "DMA txn failed" error even though it's
>>>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
>>>> on the bus instead of generic transfer failure which doesn't give any
>>>> specific clue.
>>>>
>>>> Make Changes inside i2c driver callback handler function
>>>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
>>>> stores the error status during error interrupt.
>>>>
>>>> [...]
>>>
>>> Applied to i2c/i2c-host-next on
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
>>
>> You applied changes to dmaengine driver without my ack! I dont agree to
>> the approach here, we could do better
>>
> Could you please suggest changes OR approach related comments ? So i can
> make a new change which can clean the merged code ? Hope that can
> address the concerns.
Can you address original comments?
Best regards,
Krzysztof
On 03-04-24, 11:14, Krzysztof Kozlowski wrote:
> On 03/04/2024 08:46, Mukesh Kumar Savaliya wrote:
> > Hi Vinod,
> >
> > On 3/29/2024 10:15 PM, Vinod Koul wrote:
> >> On 28-03-24, 08:36, Andi Shyti wrote:
> >>> Hi
> >>>
> >>> On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> >>>> I2C driver currently reports "DMA txn failed" error even though it's
> >>>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> >>>> on the bus instead of generic transfer failure which doesn't give any
> >>>> specific clue.
> >>>>
> >>>> Make Changes inside i2c driver callback handler function
> >>>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> >>>> stores the error status during error interrupt.
> >>>>
> >>>> [...]
> >>>
> >>> Applied to i2c/i2c-host-next on
> >>>
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/local tree
> >>
> >> You applied changes to dmaengine driver without my ack! I dont agree to
> >> the approach here, we could do better
> >>
> > Could you please suggest changes OR approach related comments ? So i can
> > make a new change which can clean the merged code ? Hope that can
> > address the concerns.
>
> Can you address original comments?
That is the best advice!
--
~Vinod
On 02-04-24, 18:44, Andi Shyti wrote:
> Hi Vinod,
>
> On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
> > On 28-03-24, 08:36, Andi Shyti wrote:
> > > Hi
> > >
> > > On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> > > > I2C driver currently reports "DMA txn failed" error even though it's
> > > > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > > > on the bus instead of generic transfer failure which doesn't give any
> > > > specific clue.
> > > >
> > > > Make Changes inside i2c driver callback handler function
> > > > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > > > stores the error status during error interrupt.
> > > >
> > > > [...]
> > >
> > > Applied to i2c/i2c-host-next on
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/local tree
> >
> > You applied changes to dmaengine driver without my ack! I dont agree to
> > the approach here, we could do better
>
> this must be an error from b4 ty. The changes have been added to
>
> pub/scm/linux/kernel/git/andi.shyti/linux.git
>
> branch i2c/i2c-host, As it has been agreed from very long.
>
> Anyway, the changes are in -next. What do we do now? Do I revert
> it? Mukesh, can you please agree with Vinod?
I dont apply patches to other subsystem without the ack. Either way you
can ask always!
I will leave it upto you...
--
~Vinod
Hi Vinod, Mukesh,
On Sun, Apr 07, 2024 at 01:42:48PM +0530, Vinod Koul wrote:
> On 02-04-24, 18:44, Andi Shyti wrote:
> > On Fri, Mar 29, 2024 at 10:15:24PM +0530, Vinod Koul wrote:
> > > On 28-03-24, 08:36, Andi Shyti wrote:
> > > > On Wed, 13 Mar 2024 10:56:39 +0530, Mukesh Kumar Savaliya wrote:
> > > > > I2C driver currently reports "DMA txn failed" error even though it's
> > > > > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > > > > on the bus instead of generic transfer failure which doesn't give any
> > > > > specific clue.
> > > > >
> > > > > Make Changes inside i2c driver callback handler function
> > > > > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > > > > stores the error status during error interrupt.
> > > > >
> > > > > [...]
> > > >
> > > > Applied to i2c/i2c-host-next on
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/local tree
> > >
> > > You applied changes to dmaengine driver without my ack! I dont agree to
> > > the approach here, we could do better
> >
> > this must be an error from b4 ty. The changes have been added to
> >
> > pub/scm/linux/kernel/git/andi.shyti/linux.git
> >
> > branch i2c/i2c-host, As it has been agreed from very long.
> >
> > Anyway, the changes are in -next. What do we do now? Do I revert
> > it? Mukesh, can you please agree with Vinod?
>
> I dont apply patches to other subsystem without the ack. Either way you
> can ask always!
Yes, you are totally right; but please, keep in mind that this
patch has some history and I would have loved to hear from you
earlier. Anyway...
> I will leave it upto you...
.. Mukesh, I'm sorry, but I'm going to revert this patch again
until we address all the last minute issues from Vinod. The
silence on this thread is worrying me more than reverting it.
I hope this will be the last time I revert this patch.
Moreover, in order to avoid maintainers' rumble (:)), please
let's try to split patches that are touching more than one
subsystems keeping the logical meainings intact.
I hope this is fine with you, Vinod.
Andi
On 16-04-24, 17:05, Andi Shyti wrote:
> > > Anyway, the changes are in -next. What do we do now? Do I revert
> > > it? Mukesh, can you please agree with Vinod?
> >
> > I dont apply patches to other subsystem without the ack. Either way you
> > can ask always!
>
> Yes, you are totally right; but please, keep in mind that this
> patch has some history and I would have loved to hear from you
> earlier. Anyway...
There was merge window, I dont look up during that. Then I had some
family stuff and travel to take care... Things happen.
When in doubt pls ask, a gentle reminder goes long way!
>
> > I will leave it upto you...
>
> ... Mukesh, I'm sorry, but I'm going to revert this patch again
> until we address all the last minute issues from Vinod. The
> silence on this thread is worrying me more than reverting it.
>
> I hope this will be the last time I revert this patch.
>
> Moreover, in order to avoid maintainers' rumble (:)), please
> let's try to split patches that are touching more than one
> subsystems keeping the logical meainings intact.
That is best. Very rarely we have a situation where we add
changes which break bisect and it has to be clubbed together. But for
other cases, it should always be split!
> I hope this is fine with you, Vinod.
Thank you for understanding
--
~Vinod
Hi,
On Wed, Apr 17, 2024 at 10:27:52PM +0530, Vinod Koul wrote:
> On 16-04-24, 17:05, Andi Shyti wrote:
> > > > Anyway, the changes are in -next. What do we do now? Do I revert
> > > > it? Mukesh, can you please agree with Vinod?
> > >
> > > I dont apply patches to other subsystem without the ack. Either way you
> > > can ask always!
> >
> > Yes, you are totally right; but please, keep in mind that this
> > patch has some history and I would have loved to hear from you
> > earlier. Anyway...
>
> There was merge window, I dont look up during that. Then I had some
> family stuff and travel to take care... Things happen.
>
> When in doubt pls ask, a gentle reminder goes long way!
sure... I'll be more patient... thanks!
> > > I will leave it upto you...
> >
> > ... Mukesh, I'm sorry, but I'm going to revert this patch again
> > until we address all the last minute issues from Vinod. The
> > silence on this thread is worrying me more than reverting it.
> >
> > I hope this will be the last time I revert this patch.
> >
> > Moreover, in order to avoid maintainers' rumble (:)), please
> > let's try to split patches that are touching more than one
> > subsystems keeping the logical meainings intact.
>
> That is best. Very rarely we have a situation where we add
> changes which break bisect and it has to be clubbed together. But for
> other cases, it should always be split!
Please Mukesh, address Vinod's comments and let's get this patch
in.
Thanks,
Andi