Hi Jarkko, thanks for the review! > On 12.09.2023, at 19:32, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote: > > On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote: [...] >> - /* Payload contains the key. */ >> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY; >> + if (key_referenced) { >> + /* Set OTP key bit to select the key via KEY_SELECT. */ >> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY; >> + } else { >> + /* Payload contains the key. */ >> + desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY; >> + } > > Remove curly braces (coding style). Will fix that and all similar cases for v3. >> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key, >> + unsigned int len) >> +{ >> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm); >> + >> + if (len != DCP_PAES_KEYSIZE) >> + return -EINVAL; >> + >> + switch (key[0]) { >> + case DCP_PAES_KEY_SLOT0: >> + case DCP_PAES_KEY_SLOT1: >> + case DCP_PAES_KEY_SLOT2: >> + case DCP_PAES_KEY_SLOT3: >> + case DCP_PAES_KEY_UNIQUE: >> + case DCP_PAES_KEY_OTP: >> + memcpy(actx->key, key, len); >> + break; > > I don't understand why the "commit" is split into two parts > (memcpy and assignments in different code blocks). You should > probably rather: > > switch (key[0]) { > case DCP_PAES_KEY_SLOT0: > case DCP_PAES_KEY_SLOT1: > case DCP_PAES_KEY_SLOT2: > case DCP_PAES_KEY_SLOT3: > case DCP_PAES_KEY_UNIQUE: > case DCP_PAES_KEY_OTP: > memcpy(actx->key, key, len); > actx->key_len = len; > actx->refkey = true; > return 0; > > default: > return -EINVAL; > } > } > > Or alternatively you can move all operations after the switch-case > statement. IMHO, any state change is better to put into a singular > location. Makes sense. I’ll change that. >> diff --git a/include/soc/fsl/dcp.h b/include/soc/fsl/dcp.h >> new file mode 100644 >> index 000000000000..df6678ee10a1 >> --- /dev/null >> +++ b/include/soc/fsl/dcp.h >> @@ -0,0 +1,19 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2021 sigma star gmbh >> + * Authors: David Gstir <david@xxxxxxxxxxxxx> >> + * Richard Weinberger <richard@xxxxxxxxxxxxx> > > Git already has author-field and commit can have co-developed-by so > this is totally obsolete. Also noted for v3. Thanks, - David -- sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, Austria UID/VAT Nr: ATU 66964118 | FN: 374287y