Hi Pascal, On Fri, Jul 26, 2019 at 02:43:24PM +0200, Pascal van Leeuwen wrote: > + if (priv->version == EIP197D_MRVL) { I see you renamed EIP197D to EIP197D_MRVL in the v2. Such a rename should not be part of this patch, as it has nothing to do with the engine you're adding support for. Is there a reason to have this one linked to Marvell? Aren't there other EIP197 (or EIP97) engines not on Marvell SoCs? (I'm pretty sure I know at least one). > - switch (priv->version) { > - case EIP197B: > - dir = "eip197b"; > - break; > - case EIP197D: > - dir = "eip197d"; > - break; > - default: > + if (priv->version == EIP97IES_MRVL) > /* No firmware is required */ > return 0; > - } > + else if (priv->version == EIP197D_MRVL) > + dir = "eip197d"; > + else > + /* Default to minimum EIP197 config */ > + dir = "eip197b"; You're moving the default choice from "no firmware" to being a specific one. > - if (priv->version != EIP197B) > + if (!(priv->version == EIP197B_MRVL)) '!=' ? > - /* Fallback to the old firmware location for the > + /* > + * Fallback to the old firmware location for the This is actually the expected comment style in net/ and crypto/. (There are other examples). > /* For EIP197 set maximum number of TX commands to 2^5 = 32 */ > - if (priv->version == EIP197B || priv->version == EIP197D) > + if (priv->version != EIP97IES_MRVL) I would really prefer having explicit checks here. More engines will be supported in the future and doing will help. (There are others). > @@ -869,9 +898,6 @@ static int safexcel_register_algorithms(struct safexcel_crypto_priv *priv) > for (i = 0; i < ARRAY_SIZE(safexcel_algs); i++) { > safexcel_algs[i]->priv = priv; > > - if (!(safexcel_algs[i]->engines & priv->version)) > - continue; You should remove the 'engines' flag in a separate patch. I'm really not sure about this. I don't think all the IS EIP engines support the same sets of algorithms? > @@ -925,22 +945,18 @@ static void safexcel_configure(struct safexcel_crypto_priv *priv) > val = readl(EIP197_HIA_AIC_G(priv) + EIP197_HIA_OPTIONS); > > /* Read number of PEs from the engine */ > - switch (priv->version) { > - case EIP197B: > - case EIP197D: > - mask = EIP197_N_PES_MASK; > - break; > - default: > + if (priv->version == EIP97IES_MRVL) > mask = EIP97_N_PES_MASK; > - } > + else > + mask = EIP197_N_PES_MASK; > + You also lose readability here. > + if (IS_ENABLED(CONFIG_PCI) && (priv->version == EIP197_DEVBRD)) { You have extra parenthesis here. > - platform_set_drvdata(pdev, priv); Side note: this is why you had to send the patch fixing rmmod. > static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv) > @@ -1160,6 +1139,78 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv) > } > } > > +#if (IS_ENABLED(CONFIG_OF)) No need for the extra parenthesis here. > + if (priv->version == EIP197_DEVBRD) { It seems to me this is mixing an engine version information and a board were the engine is integrated. Are there differences in the engine itself, or only in the way it's wired? We had this discussion on the v1. Your point was that you wanted this information to be in .data. One solution I proposed then was to use a struct (with both a 'version' and a 'flag' variable inside) instead of a single 'version' variable, so that we still can make checks on the version itself and not on something too specific. > +static int __init crypto_is_init(void) > +{ > + int rc; > + > + #if (IS_ENABLED(CONFIG_OF)) > + /* Register platform driver */ > + platform_driver_register(&crypto_safexcel); > + #endif When used in the code directly, you should use: if (IS_ENABLED(CONFIG_OF)) Thanks! Antoine -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com