Re: [PATCH v6 2/4] crypto: starfive - Add crypto engine support

[Jia Jie Ho <jiajie.ho@starfivetech.com>]

On 26/4/2023 3:56 pm, Christophe JAILLET wrote:
> Le 26/04/2023 à 08:58, Jia Jie Ho a écrit :
>> Adding device probe and DMA init for StarFive cryptographic module.
>>
>> Co-developed-by: Huan Feng <huan.feng-bONrM45KWFOXmMXjJBpWqg@public.gmane.org>
>> Signed-off-by: Huan Feng <huan.feng-bONrM45KWFOXmMXjJBpWqg@public.gmane.org>
>> Signed-off-by: Jia Jie Ho <jiajie.ho-bONrM45KWFOXmMXjJBpWqg@public.gmane.org>
>> ---
>>   MAINTAINERS                           |   7 +
>>   drivers/crypto/Kconfig                |   1 +
>>   drivers/crypto/Makefile               |   1 +
>>   drivers/crypto/starfive/Kconfig       |  17 +++
>>   drivers/crypto/starfive/Makefile      |   4 +
>>   drivers/crypto/starfive/jh7110-cryp.c | 199 ++++++++++++++++++++++++++
>>   drivers/crypto/starfive/jh7110-cryp.h |  63 ++++++++
>>   7 files changed, 292 insertions(+)
>>   create mode 100644 drivers/crypto/starfive/Kconfig
>>   create mode 100644 drivers/crypto/starfive/Makefile
>>   create mode 100644 drivers/crypto/starfive/jh7110-cryp.c
>>   create mode 100644 drivers/crypto/starfive/jh7110-cryp.h
>>

[...]

>> +
>> +static int starfive_dma_init(struct starfive_cryp_dev *cryp)
>> +{
>> +    dma_cap_mask_t mask;
>> +
>> +    cryp->tx = NULL;
>> +    cryp->rx = NULL;
> 
> Harmless, but 'crypt' is kzalloc()'ed, so these fields are already NULL.
>

Hi Christophe,
I'll remove these in the next version.
 
>> +
>> +    dma_cap_zero(mask);
>> +    dma_cap_set(DMA_SLAVE, mask);
>> +
>> +    cryp->tx = dma_request_chan(cryp->dev, "tx");
>> +    if (IS_ERR(cryp->tx))
>> +        return dev_err_probe(cryp->dev, PTR_ERR(cryp->tx),
>> +                     "Error requesting tx dma channel.\n");
>> +
>> +    cryp->rx = dma_request_chan(cryp->dev, "rx");
>> +    if (IS_ERR(cryp->rx)) {
>> +        dma_release_channel(cryp->tx);
>> +        return dev_err_probe(cryp->dev, PTR_ERR(cryp->rx),
>> +                     "Error requesting rx dma channel.\n");
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void starfive_dma_cleanup(struct starfive_cryp_dev *cryp)
>> +{
>> +    dma_release_channel(cryp->tx);
>> +    dma_release_channel(cryp->rx);
>> +}
>> +
>> +static int starfive_cryp_probe(struct platform_device *pdev)
>> +{

[...]

>> +
>> +    ret = crypto_engine_start(cryp->engine);
>> +    if (ret)
>> +        goto err_engine_start;
>> +
>> +    return 0;
>> +
>> +err_engine_start:
>> +    crypto_engine_exit(cryp->engine);
>> +err_engine:
>> +    starfive_dma_cleanup(cryp);
>> +err_dma_init:
>> +    spin_lock(&dev_list.lock);
>> +    list_del(&cryp->list);
>> +    spin_unlock(&dev_list.lock);
> 
> I think that there should be:
>     clk_disable_unprepare(cryp->hclk);
>     clk_disable_unprepare(cryp->ahb);
>     reset_control_assert(cryp->rst);
> 
> as in the remove function.
> 

Will add these in next version.

>> +
>> +    return ret;
>> +}
>> +
>> +static int starfive_cryp_remove(struct platform_device *pdev)
>> +{
>> +    struct starfive_cryp_dev *cryp = platform_get_drvdata(pdev);
>> +
>> +    if (!cryp)
>> +        return -ENODEV;
> 
> I don't think that this can happen.
> 

I'll update this too along with your other comments.
Thanks for taking time reviewing this patch series.

Best regards,
Jia Jie