From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20180228151701.GM1479@piout.net> References: <20180222200432.22003-1-david.daney@cavium.com> <20180228151701.GM1479@piout.net> From: David Daney Date: Fri, 2 Mar 2018 08:22:22 -0800 Message-ID: Subject: Re: [PATCH v6] rtc: isl12026: Add driver. Content-Type: multipart/alternative; boundary="001a114b04a89b6f5d056670615e" To: Alexandre Belloni Cc: David Daney , Alessandro Zummo , Rob Herring , Mark Rutland , linux-rtc@vger.kernel.org, devicetree@vger.kernel.org, Linux Kernel Mailing List , Andy Shevchenko List-ID: --001a114b04a89b6f5d056670615e Content-Type: text/plain; charset="UTF-8" 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 > --001a114b04a89b6f5d056670615e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I am no longer able to test this patch as I lost access to= the hardware.

If someone else wants to take charge of t= he 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.c= om> wrote:
Hi,

That's mostly good

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

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

> +=C2=A0 =C2=A0 =C2=A0ret =3D rtc_register_device(priv->rtc); > +=C2=A0 =C2=A0 =C2=A0if (ret)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.name =3D "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.

> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.read_only =3D false;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.root_only =3D true;

any reason to have it root only?

> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.base_dev =3D &client->dev;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.priv =3D priv;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.stride =3D 1;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.word_size =3D 1;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.size =3D 512;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.reg_read =3D isl12026_nvm_read;
> +=C2=A0 =C2=A0 =C2=A0nvm_cfg.reg_write =3D isl12026_nvm_write;
> +
> +=C2=A0 =C2=A0 =C2=A0return rtc_nvmem_register(priv->rtc, &nvm_= cfg);

The probe function must not fail after rtc_register_device has been<= br> 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,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!nvmem_config)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENODEV;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0nvmem_config->dev =3D &rtc->dev;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0nvmem_config->dev =3D rtc->dev.parent; =C2=A0 =C2=A0 =C2=A0 =C2=A0 nvmem_config->owner =3D rtc->owner;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 rtc->nvmem =3D nvmem_register(nvmem_config);=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR_OR_NULL(rtc->nvmem))

I'll submit this patch for this cycle before there are many users of th= e
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&q= uot; in
the body of a message to major= domo@vger.kernel.org
More majordomo info at=C2=A0 http://vger.kernel.org/m= ajordomo-info.html

--001a114b04a89b6f5d056670615e--