Received-SPF: pass (google.com: domain of linux-kernel+bounces-95242-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <8e288c24-72a9-4f9a-8aa1-ecd4c9b1652b@quicinc.com>
Date: Thu, 7 Mar 2024 15:13:20 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI
 mode
Content-Language: en-US
To: Vinod Koul <vkoul@kernel.org>, Bjorn Andersson <andersson@kernel.org>
CC: <konrad.dybcio@linaro.org>, <bjorn.andersson@linaro.org>,
        <andi.shyti@kernel.org>, <wsa@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <dmaengine@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-i2c@vger.kernel.org>,
        <quic_vdadhani@quicinc.com>
References: <20240301112638.990045-1-quic_msavaliy@quicinc.com>
 <fuggv2kghhxijcljavzsus5uagjiknj5mrzwmqbxbhkyov5t75@smpxbhpdz7cv>
 <ZeSFVEcA9g2WTyJz@matsya>
From: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
In-Reply-To: <ZeSFVEcA9g2WTyJz@matsya>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 3/3/2024 7:42 PM, Vinod Koul wrote:
> On 01-03-24, 10:37, Bjorn Andersson wrote:
>> On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
>>> we are seeing protocol errors like NACK as transfer failure but
>>> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>>>
>>> Hence we are adding such error support in GSI mode and reporting it
>>> accordingly by adding respective error logs.
>>>
>>> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
>>> stack memory to handle and store the geni_i2c_dev handle.
>>>
>>> Copy event status from GSI driver to the i2c device status and parse
>>> error when callback comes from gsi driver to the i2c driver. In the
>>> gpi.c, we need to store callback param into i2c config data structure
>>> so that inside the i2c driver, we can check what exactly the error is
>>> and parse it accordingly.
>>>
>>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>>> Co-developed-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>>   drivers/dma/qcom/gpi.c             | 12 +++++++-
>>>   drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
>>>   include/linux/dma/qcom-gpi-dma.h   |  4 +++
>>>   3 files changed, 44 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>>> index 1c93864e0e4d..6d718916fba4 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_config *i2c;
>>> +
>>> +		dmaengine_desc_get_callback(&vd->tx, &cb);
>>> +		i2c = cb.callback_param;
>>> +		i2c->status = compl_event->status;
>>
>> What would the DMA maintainer say about extending struct
>> dmaengine_tx_result with some protocol-specific status field?
> 
> That would be sane thing to do if we can get protocol status. Most of
> the times DMA txn would tell you success or fail, here we have firmware
> which tells us this and I see no reason why this can't be propagated.

Thanks Vinod for reviewing the change. I have uploaded latest patch 
considering other review comments.
>