2023-11-03 12:18:45

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

On 03/11/2023 13:13, Md Sadre Alam wrote:
>
>
> On 10/31/2023 10:43 PM, Krzysztof Kozlowski wrote:
>> On 31/10/2023 13:03, Md Sadre Alam wrote:
>>> Add qpic spi nand driver support for qcom soc.
>>
>> What is "qcom soc"? Did you mean Qualcomm and SoC?
>
> Yes Qualcomm SoC
>

Then please use full sentences and full names with proper capitalization
of letters for acronyms.

Best regards,
Krzysztof

2023-11-03 12:25:10

by Md Sadre Alam

[permalink] [raw]

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

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
>
>> Signed-off-by: Md Sadre Alam <[email protected]>
>> Signed-off-by: Sricharan R <[email protected]>
>> ---
>> 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
>

2023-11-03 12:33:54

by Dmitry Baryshkov

[permalink] [raw]

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

On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <[email protected]> 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.

> >
> >> Signed-off-by: Md Sadre Alam <[email protected]>
> >> Signed-off-by: Sricharan R <[email protected]>
> >> ---
> >> 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

2023-11-03 12:47:45

On 11/3/2023 6:17 PM, Mark Brown wrote:
> On Fri, Nov 03, 2023 at 04:50:54PM +0530, Md Sadre Alam wrote:
>> On 10/31/2023 7:53 PM, Mark Brown wrote:
>>> On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:
>
>>>> + ret = submit_descs(snandc);
>>>> + if (ret)
>>>> + dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
>>>> +
>>>> + free_descs(snandc);
>
>>> This seems to be doing a bit more than I would expect an init function
>>> to, and it's very surprising to see the descriptors freed immediately
>>> after something called a submit (which suggests that the descriptors are
>>> still in flight).
>
>> Our controller supports only bam mode , that means for writing/reading even
>> single register we have to use bam.
>> submit_descs() is synchronous so I/O is complete when it returns.
>> Hence freeing the descriptor after it.
>
> That seems like the BAM API is very confusing and error prone.

Sorry for late reply
Yeah, By HW itself its complex due to security reason. We are trying
to write a BAM common API interface for all the driver which will use BAM.