Re: [PATCH v6 1/6] nvmem: add driver for JZ4780 efuse

Hi Paul,

Am 27.02.2020 um 15:47 schrieb Paul Cercueil [off-list ref]:

Hi Nikolaus,


Le mer., févr. 26, 2020 at 12:15, H. Nikolaus Schaller [off-list ref] a écrit :

quoted

From: PrasannaKumar Muralidharan <redacted>
This patch brings support for the JZ4780 efuse. Currently it only exposes
a read only access to the entire 8K bits efuse memory and nvmem cells.
To fetch for example the MAC address:
dd if=/sys/devices/platform/134100d0.efuse/jz4780-efuse0/nvmem bs=1 skip=34 count=6 status=none | xxd
Tested-by: Mathieu Malaterre <redacted>
Signed-off-by: PrasannaKumar Muralidharan <redacted>
Signed-off-by: Mathieu Malaterre <redacted>
Signed-off-by: H. Nikolaus Schaller <redacted>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/nvmem/Kconfig        |  12 ++
drivers/nvmem/Makefile       |   2 +
drivers/nvmem/jz4780-efuse.c | 239 +++++++++++++++++++++++++++++++++++
3 files changed, 253 insertions(+)
create mode 100644 drivers/nvmem/jz4780-efuse.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 35efab1ba8d9..d7b7f6d688e7 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig

@@ -55,6 +55,18 @@ config NVMEM_IMX_OCOTP_SCU

quoted

+	clk_rate = clk_get_rate(efuse->clk);
+
+	efuse->dev = dev;
+
+	/*
+	 * rd_adj and rd_strobe are 4 bit values
+	 * conditions:
+	 *   bus clk_period * (rd_adj + 1) > 6.5ns
+	 *   bus clk_period * (rd_adj + 5 + rd_strobe) > 35ns
+	 *   i.e. rd_adj >= 6.5ns / clk_period
+	 *   i.e. rd_strobe >= 35 ns / clk_period - 5 - rd_adj + 1
+	 * constants:
+	 *   1 / 6.5ns == 153846154 Hz
+	 *   1 / 35ns == 28571429 Hz
+	 */
+
+	rd_adj = clk_rate / 153846154;
+	rd_strobe = clk_rate / 28571429 - 5 - rd_adj + 1;
+
+	if (rd_adj > BIT(4) - 1 || rd_strobe > BIT(4) - 1) {

Just use 0xF or GENMASK(3, 0) instead of BIT(4) - 1

Or would rd_adj >= BIT(4) be better since this is not used as a mask?
This would also correspond to that the register is 4 bits wide.

quoted

+		dev_err(&pdev->dev, "Cannot set clock configuration\n");
+		return -EINVAL;

You need to clk_disable_unprepare() before you return now, since the clock has been enabled.

Ah, ok! Forgot about that.

I'd suggest registering a cleanup function (with devm_add_action_or_reset) after clk_prepare_enable() is called, then you don't need extra handling in the rest of the probe, and the .remove function (and platform_set_drvdata) can be dropped too.

Ok, this will indeed be simpler that the remove function if we take the error path into account.

Noted for v7.

The rest looks pretty good.

Cheers,
-Paul

BR,
Nikolaus