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

[Neal Liu]

> 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

Okay, I'll leverage your suggestion with different crypto algos options and 1 debug option for all modules.
Thanks !