On Wed, Jan 12, 2022 at 02:45:13PM +0800, Adrian Hunter wrote:
>> Because of the possible failure of the dma_supported(), the
>> dma_set_mask_and_coherent() may return error num.
>> Therefore, it should be better to check it and return the error if
>> fails.
>> Also, the caller, esdhc_of_resume(), should deal with the return
>> value.
>> Moreover, as the sdhci_esdhc_driver has not been used, it does not
>> need to
>> be considered.
>
> Apologies, but that last sentence I don't understand. Can you clarify
> it a bit.
> What doesn't need to be considered and why?
Thanks, because the original esdhc_of_enable_dma() only returns 0, the
caller may not consider to check the return value.
I also notice that the esdhc_of_enable_dma() is assigned to
sdhci_esdhc_le_pdata and sdhci_esdhc_be_pdata, which is only used by
sdhci_esdhc_driver.
And now the sdhci_esdhc_driver only have 'probe' and 'remove', without
other action.
So we should not consider to check whether there is a caller for
esdhc_of_enable_dma() in sdhci_esdhc_driver.
>> if (ret == 0) {
>> /* Isn't this already done by sdhci_resume_host() ?
>> --rmk */
>> - esdhc_of_enable_dma(host);
>> + ret = esdhc_of_enable_dma(host);
>> + if (ret)
>> + return ret;
>> +
>
> This is already done by sdhci_resume_host(), which assumes there can be
> no
> error if DMA has been enabled previously i.e. -> enable_dma() is called
> at setup and the return value checked then. If it is possible that DMA
> support can disappear later, then it would be better to address that in
> SDHCI so that all SDHCI drivers get the benefit.
Fine, since it is already checked in setup, I think it is no need to
check later.
I will send a v2 without the change of esdhc_of_resume().
Sincerely thanks,
Jiang
