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
