lkml.org 
[lkml]   [2018]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v6] rtc: isl12026: Add driver.
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

\
 
 \ /
  Last update: 2018-02-28 16:17    [W:0.038 / U:0.996 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site