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

[H. Nikolaus Schaller]

Hi Paul,

> Am 27.02.2020 um 15:47 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
> 
> Hi Nikolaus,
> 
> 
> Le mer., févr. 26, 2020 at 12:15, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit :
>> From: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx>
>> 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 <malat@xxxxxxxxxx>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx>
>> Signed-off-by: Mathieu Malaterre <malat@xxxxxxxxxx>
>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
>> ---
>> 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


>> +	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.

> 
>> +		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