lkml.org 
[lkml]   [2023]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine Driver
    On Fri, 3 Nov 2023 at 15:24, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
    >
    >
    >
    > 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 ?

    No, the node below is not acceptable. And you have already got two
    reasons for that. Let me repeat them for you:

    - it is "okay" not "ok"
    - no underscores in node names.

    If you want to have a more meaningful discussion, please provide full
    ECC + NAND + SPI DT bindings, only then we can discuss them.

    > 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
    > >>>
    > >
    > >
    > >



    --
    With best wishes
    Dmitry

    \
     
     \ /
      Last update: 2023-11-20 13:47    [W:3.094 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site