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

[David Daney <ddaney.cavm@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

I am no longer able to test this patch as I lost access to the hardware.

If someone else wants to take charge of the patch, that would be great.

Sorry I couldn't drive this to completion,
David Daney

On Wed, Feb 28, 2018 at 7:17 AM, Alexandre Belloni <
alexandre.belloni@free-electrons.com> wrote:

> 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
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: text/html, Size: 4067 bytes --]