Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <4b911907-44b9-c164-9648-3d399e557672@quicinc.com>
Date:   Fri, 3 Nov 2023 18:53:46 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine
 Driver
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <conor+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>,
        <broonie@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>,
        <quic_srichara@quicinc.com>, <qpic_varada@quicinc.com>
References: <20231031120307.1600689-1-quic_mdalam@quicinc.com>
 <20231031120307.1600689-2-quic_mdalam@quicinc.com>
 <b9af01d2-1a86-4b41-9bd6-3bf7a0dde1c0@linaro.org>
 <553c1373-c9a0-b2af-2286-058824e31bad@quicinc.com>
 <CAA8EJpp-xsP1x==a5DH8pKpy7XH75UF-L8ewKWmeL8ePtxUq-A@mail.gmail.com>
From:   Md Sadre Alam <quic_mdalam@quicinc.com>
In-Reply-To: <CAA8EJpp-xsP1x==a5DH8pKpy7XH75UF-L8ewKWmeL8ePtxUq-A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/3/2023 6:03 PM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
>>
>>
>>
>> On 10/31/2023 10:41 PM, Krzysztof Kozlowski wrote:
>>> On 31/10/2023 13:03, Md Sadre Alam wrote:
>>>
>>> Eh? Empty?
>>
>> QPIC controller has the ecc pipelined so will keep the ecc support
>> inlined in both raw nand and serial nand driver.
>>
>> Droping this driver since device node was NAK-ed
>> https://www.spinics.net/lists/linux-arm-msm/msg177596.html
> 
> It seems, we have to repeat the same thing again and again:
> 
> It was not the device node that was NAKed. It was the patch that was
> NAKed. Please read the emails carefully.
> 
> And next time please perform dtbs_check, dt_binding_check and
> checkpatch before sending the patch.
> 
> It is perfectly fine to ask questions 'like we are getting we are
> getting this and that issues with the bindings, please advise'. It is
> not fine to skip that step completely.

Sorry in V1 will run all basic checks.

Based on below feedback [1] and NAK on the device node patch
got idea of having separate device node for ECC is not acceptable.
Could you please help to clarify that.

Since ECC block is inlined with QPIC controller so is the below
device node acceptable ?

    bch: qpic_ecc {
                           compatible = "qcom,ipq9574-ecc";
                           status = "ok";
                   };

  [1] https://www.spinics.net/lists/linux-arm-msm/msg177525.html

> 
>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>>>> ---
>>>>    drivers/mtd/nand/Kconfig    |   7 ++
>>>>    drivers/mtd/nand/Makefile   |   1 +
>>>>    drivers/mtd/nand/ecc-qcom.c | 198 ++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 206 insertions(+)
>>>>    create mode 100644 drivers/mtd/nand/ecc-qcom.c
>>>>
>>>
>>> ...
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_config);
>>>> +
>>>> +void qcom_ecc_enable(struct qcom_ecc *ecc)
>>>> +{
>>>> +    ecc->use_ecc = true;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_enable);
>>>
>>> Drop this and all other exports. Nothing here explains the need for them.
>>>
>>>> +
>>>> +void qcom_ecc_disable(struct qcom_ecc *ecc)
>>>> +{
>>>> +    ecc->use_ecc = false;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_disable);
>>>> +
>>>> +static const struct of_device_id qpic_ecc_dt_match[] = {
>>>> +    {
>>>> +            .compatible = "qcom,ipq9574-ecc",
>>>
>>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>>> warnings can be ignored, but the code here looks like it needs a fix.
>>> Feel free to get in touch if the warning is not clear.
>>>
>>> Checkpatch is preerquisite. Don't send patches which have obvious issues
>>> pointed out by checkpatch. It's a waste of reviewers time.
>>>
>>>> +    },
>>>> +    {},
>>>> +};
>>>> +
>>>> +static int qpic_ecc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct qpic_ecc *ecc;
>>>> +    u32 max_eccdata_size;
>>>> +
>>>> +    ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
>>>> +    if (!ecc)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    ecc->caps = of_device_get_match_data(dev);
>>>> +
>>>> +    ecc->dev = dev;
>>>> +    platform_set_drvdata(pdev, ecc);
>>>> +    dev_info(dev, "probed\n");
>>>
>>> No, no such messages.
>>>
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
> 
> 
>