Re: [PATCH v6] rtc: isl12026: Add driver.

[Alexandre Belloni]

Hi,

That's mostly good

On 22/02/2018 at 12:04:32 -0800, David Daney wrote:
> +	priv->rtc->ops = &isl12026_rtc_ops;
> +	priv->rtc->nvram_old_abi = false;

This allocation is not necessary and I would refer not having t so when
the ABI goes away, it is not necessary to change this driver.

> +	ret = rtc_register_device(priv->rtc);
> +	if (ret)
> +		return ret;
> +
> +	memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> +	nvm_cfg.name = "eeprom";

You probably need something more descriptive, usually the rtc model name
else, it is difficult (but not impossible) to find where this nvmem is
actually located when using /sys/bus/nvmem/devices
isl12026- would be a good choice.

> +	nvm_cfg.read_only = false;
> +	nvm_cfg.root_only = true;

any reason to have it root only?

> +	nvm_cfg.base_dev = &client->dev;
> +	nvm_cfg.priv = priv;
> +	nvm_cfg.stride = 1;
> +	nvm_cfg.word_size = 1;
> +	nvm_cfg.size = 512;
> +	nvm_cfg.reg_read = isl12026_nvm_read;
> +	nvm_cfg.reg_write = isl12026_nvm_write;
> +
> +	return rtc_nvmem_register(priv->rtc, &nvm_cfg);

The probe function must not fail after rtc_register_device has been
called so you must return 0 here.

It is not currently possible to call rtc_nvmem_register before
rtc_register_device unless we move the nvmem to the parent device:

diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c
index 3a02357eb783..17ec4c8d0fad 100644
--- a/drivers/rtc/nvmem.c
+++ b/drivers/rtc/nvmem.c
@@ -91,7 +91,7 @@ int rtc_nvmem_register(struct rtc_device *rtc,
        if (!nvmem_config)
                return -ENODEV;
 
-       nvmem_config->dev = &rtc->dev;
+       nvmem_config->dev = rtc->dev.parent;
        nvmem_config->owner = rtc->owner;
        rtc->nvmem = nvmem_register(nvmem_config);
        if (IS_ERR_OR_NULL(rtc->nvmem))

I'll submit this patch for this cycle before there are many users of the
interface (converted drivers are still exposing the old ABI).

If you prefer, you can depend on it.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com