[RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine Driver

[Md Sadre Alam]

On 11/3/2023 6:03 PM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <quic_mdalam at 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 at quicinc.com>
>>>> Signed-off-by: Sricharan R <quic_srichara at 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
>>>
> 
> 
>