Re: [PATCH v8 5/5] crypto: aspeed: add HACE crypto driver

[Dhananjay Phadke]

On 7/26/2022 10:31 PM, Neal Liu wrote:
> > Why are separate options required for hash and crypto algorithms, if hace is
> > only hw crypto on the SoCs?
> >
> > Looks like that's requiring unnecessary __weak register / unregister functions
> > [see below].
> >
> > Couldn't just two options CONFIG_CRYPTO_DEV_ASPEED and
> > CONFIG_CRYPTO_DEV_ASPEED_DEBUG be simpler to set for downstream
> > defconfigs?
> I would like to separate different algorithms by different options for more convenient for further use and debug.
> We also have RSA engine named ACRY, and would upstream once hash & crypto being accepted.
> So combined them into one option seems not a good choice for multiple hw crypto, do you agree?

Not sure what's the use case of just enabling crypto or hash separately
out of same HW engine and esp. when there's no alternative accel 
available, but that's fine.

If ARCY is different HW engine (interface) then having separate config
sounds logical.

Multiplying DEBUG configs seems unnecessary though. With dynamic debug
any of the dev_dbg could be turned on. Suggest using single one for
the module, if not drop it altogether. Following code is still not
covered by Kconfig, it in common code.

> +#ifdef ASPEED_HACE_DEBUG
> +#define HACE_DBG(d, fmt, ...)	\
> +	dev_info((d)->dev, "%s() " fmt, __func__, ##__VA_ARGS__)
> +#else
> +#define HACE_DBG(d, fmt, ...)	\
> +	dev_dbg((d)->dev, "%s() " fmt, __func__, ##__VA_ARGS__)
> +#endif

Regards,
Dhananjay