[PATCH v3 0/4] Add NXP pcf85263 real-time clock support

[PATCH v3 0/4] Add NXP pcf85263 real-time clock support

[Biju Das]

This patch set aims to add support for NXP pcf85263 real-time clock.
pcf85263 rtc is compatible with pcf85363 rtc except that pcf85363 has
64 bytes additional RAM.

1 byte of nvmem is supported in pcf85263 and is exposed through sysfs.

The details of pcf85363 and pcf85263 can be found in the below data sheets.

https://www.nxp.com/docs/en/data-sheet/PCF85363A.pdf

https://www.nxp.com/docs/en/data-sheet/PCF85263A.pdf

This patch is tested against linux-next.

V1-->V2
   * Incorporated simon's review comment for binding patch.
   * Incorporated Geert and Alexandre's review comments.
V2-->V3
   * Incorporated Geert's review comments.

Biju Das (4):
  dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock
  rtc: pcf85363: Add support for NXP pcf85263 rtc
  ARM: shmobile: Enable NXP pcf85363 rtc in shmobile_defconfig
  ARM: dts: iwg23s-sbc: Enable RTC

 Documentation/devicetree/bindings/rtc/pcf85363.txt |  4 +-
 arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts          | 18 +++++
 arch/arm/configs/shmobile_defconfig                |  1 +
 drivers/rtc/rtc-pcf85363.c                         | 87 +++++++++++++++++-----
 4 files changed, 90 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock

[Biju Das]

The pcf85263 RTC is compatible with the pcf85363 RTC.

The difference between the pcf85263 and pcf85363 RTC is that the latter has
64 bytes more RAM. This renders them incompatible from a DT point of view.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V1-->V2
	* Incorporated Simon's review comment.
V2-->V3
	* No Change
---
 Documentation/devicetree/bindings/rtc/pcf85363.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/pcf85363.txt b/Documentation/devicetree/bindings/rtc/pcf85363.txt
index 76fdabc..94adc1c 100644
--- a/Documentation/devicetree/bindings/rtc/pcf85363.txt
+++ b/Documentation/devicetree/bindings/rtc/pcf85363.txt
@@ -1,8 +1,8 @@
-NXP PCF85363 Real Time Clock
+NXP PCF85263/PCF85363 Real Time Clock
 ============================
 
 Required properties:
-- compatible: Should contain "nxp,pcf85363".
+- compatible: Should contain "nxp,pcf85263" or "nxp,pcf85363".
 - reg: I2C address for chip.
 
 Optional properties:
-- 
2.7.4

Re: [PATCH v3 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock

[Geert Uytterhoeven]

On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> The pcf85263 RTC is compatible with the pcf85363 RTC.
>
> The difference between the pcf85263 and pcf85363 RTC is that the latter has
> 64 bytes more RAM. This renders them incompatible from a DT point of view.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Biju Das]

Add support for NXP pcf85263 real-time clock. pcf85263 rtc is compatible
with pcf85363,except that pcf85363 has additional 64 bytes of RAM.

1 byte of nvmem is supported and exposed in sysfs (# is the instance
number,starting with 0): /sys/bus/nvmem/devices/pcf85x63-#/nvmem

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V1-->V2
	* Incorporated Alexandre and Geert's review comment.
V2-->V3
	* Incorporated Geert's review comment.
---
 drivers/rtc/rtc-pcf85363.c | 87 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c04a1ed..6a0a994 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -120,6 +120,11 @@ struct pcf85363 {
 	struct regmap		*regmap;
 };
 
+struct pcf85x63_config {
+	struct regmap_config regmap;
+	unsigned int num_nvram;
+};
+
 static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
@@ -311,25 +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned int offset, void *val,
 				 val, bytes);
 }
 
-static const struct regmap_config regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-	.max_register = 0x7f,
+static int pcf85x63_nvram_read(void *priv, unsigned int offset, void *val,
+			       size_t bytes)
+{
+	struct pcf85363 *pcf85363 = priv;
+
+	return regmap_read(pcf85363->regmap, CTRL_RAMBYTE, val);
+}
+
+static int pcf85x63_nvram_write(void *priv, unsigned int offset, void *val,
+				size_t bytes)
+{
+	struct pcf85363 *pcf85363 = priv;
+
+	return regmap_write(pcf85363->regmap, CTRL_RAMBYTE,
+				*((unsigned int *)val));
+}
+
+static const struct pcf85x63_config pcf_85263_config = {
+	{
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = 0x2f,
+	},
+	1
+};
+
+static const struct pcf85x63_config pcf_85363_config = {
+	{
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = 0x7f,
+	},
+	2
 };
 
 static int pcf85363_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
 	struct pcf85363 *pcf85363;
-	struct nvmem_config nvmem_cfg = {
-		.name = "pcf85363-",
-		.word_size = 1,
-		.stride = 1,
-		.size = NVRAM_SIZE,
-		.reg_read = pcf85363_nvram_read,
-		.reg_write = pcf85363_nvram_write,
+	const struct pcf85x63_config *config = &pcf_85363_config;
+	const void *data = of_device_get_match_data(&client->dev);
+	static struct nvmem_config nvmem_cfg[] = {
+		{
+			.name = "pcf85x63-",
+			.word_size = 1,
+			.stride = 1,
+			.size = 1,
+			.reg_read = pcf85x63_nvram_read,
+			.reg_write = pcf85x63_nvram_write,
+		}, {
+			.name = "pcf85363-",
+			.word_size = 1,
+			.stride = 1,
+			.size = NVRAM_SIZE,
+			.reg_read = pcf85363_nvram_read,
+			.reg_write = pcf85363_nvram_write,
+		},
 	};
-	int ret;
+	int ret, i;
+
+	if (data)
+		config = data;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -339,7 +387,7 @@ static int pcf85363_probe(struct i2c_client *client,
 	if (!pcf85363)
 		return -ENOMEM;
 
-	pcf85363->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
 	if (IS_ERR(pcf85363->regmap)) {
 		dev_err(&client->dev, "regmap allocation failed\n");
 		return PTR_ERR(pcf85363->regmap);
@@ -370,15 +418,18 @@ static int pcf85363_probe(struct i2c_client *client,
 
 	ret = rtc_register_device(pcf85363->rtc);
 
-	nvmem_cfg.priv = pcf85363;
-	rtc_nvmem_register(pcf85363->rtc, &nvmem_cfg);
+	for (i = 0; i < config->num_nvram; i++) {
+		nvmem_cfg[i].priv = pcf85363;
+		rtc_nvmem_register(pcf85363->rtc, &nvmem_cfg[i]);
+	}
 
 	return ret;
 }
 
 static const struct of_device_id dev_ids[] = {
-	{ .compatible = "nxp,pcf85363" },
-	{}
+	{ .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
+	{ .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dev_ids);
 
@@ -393,5 +444,5 @@ static struct i2c_driver pcf85363_driver = {
 module_i2c_driver(pcf85363_driver);
 
 MODULE_AUTHOR("Eric Nelson");
-MODULE_DESCRIPTION("pcf85363 I2C RTC driver");
+MODULE_DESCRIPTION("pcf85263/pcf85363 I2C RTC driver");
 MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Geert Uytterhoeven]

Hi Biju,

CC nvmem maintainer

On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> Add support for NXP pcf85263 real-time clock. pcf85263 rtc is compatible
> with pcf85363,except that pcf85363 has additional 64 bytes of RAM.
>
> 1 byte of nvmem is supported and exposed in sysfs (# is the instance
> number,starting with 0): /sys/bus/nvmem/devices/pcf85x63-#/nvmem
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
> V1-->V2
>         * Incorporated Alexandre and Geert's review comment.
> V2-->V3
>         * Incorporated Geert's review comment.

Thanks for the update!

> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -120,6 +120,11 @@ struct pcf85363 {
>         struct regmap           *regmap;
>  };
>
> +struct pcf85x63_config {
> +       struct regmap_config regmap;
> +       unsigned int num_nvram;
> +};
> +
>  static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>         struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> @@ -311,25 +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned int offset, void *val,
>                                  val, bytes);
>  }
>
> -static const struct regmap_config regmap_config = {
> -       .reg_bits = 8,
> -       .val_bits = 8,
> -       .max_register = 0x7f,
> +static int pcf85x63_nvram_read(void *priv, unsigned int offset, void *val,
> +                              size_t bytes)

Given bytes should be 1, val should be a pointer to a single byte...
What if bytes == 0?

> +{> +       struct pcf85363 *pcf85363 = priv;
> +
> +       return regmap_read(pcf85363->regmap, CTRL_RAMBYTE, val);

However, regmap_read() has an unsigned int output parameter!
So it's writing too many bytes, and only writing the actual data byte to the
correct address on little-endian systems.
Hence you need to use an intermediate variable to convert from unsigned
int to byte.

> +}
> +
> +static int pcf85x63_nvram_write(void *priv, unsigned int offset, void *val,
> +                               size_t bytes)
> +{
> +       struct pcf85363 *pcf85363 = priv;
> +
> +       return regmap_write(pcf85363->regmap, CTRL_RAMBYTE,
> +                               *((unsigned int *)val));

Likewise for writing.

> +}

BTW, while the nvmem_device_{read,write}() public API is documented, the
nvmem_device.reg_{read,write}() driver API isn't.
And the behavior might be confusing.

E.g.
 * Return: length of successful bytes read on success and negative
 * error code on error.

The public API seems to assume the driver API returns zero on success,
and replaces the zero by the number of bytes requested.
If the requested number of bytes is too large, a zero success would be
converted to a value that's larger than the actual number of bytes
transferred!
However, the driver API can return a smaller (positive) number, which matches
"standard" read/write() APIs.

> +static const struct pcf85x63_config pcf_85263_config = {
> +       {
> +               .reg_bits = 8,
> +               .val_bits = 8,
> +               .max_register = 0x2f,
> +       },
> +       1

The "1" looks funny. Please use C99 initializers for all struct members.

> +};
> +
> +static const struct pcf85x63_config pcf_85363_config = {
> +       {
> +               .reg_bits = 8,
> +               .val_bits = 8,
> +               .max_register = 0x7f,
> +       },
> +       2

Likewise.

The rest looks good to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Biju Das]

Hi Geert,

Thanks for feedback.

> Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> Hi Biju,
>
> CC nvmem maintainer
>
> On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > Add support for NXP pcf85263 real-time clock. pcf85263 rtc is
> > compatible with pcf85363,except that pcf85363 has additional 64 bytes of
> RAM.
> >
> > 1 byte of nvmem is supported and exposed in sysfs (# is the instance
> > number,starting with 0): /sys/bus/nvmem/devices/pcf85x63-#/nvmem
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> > V1-->V2
> >         * Incorporated Alexandre and Geert's review comment.
> > V2-->V3
> >         * Incorporated Geert's review comment.
>
> Thanks for the update!
>
> > --- a/drivers/rtc/rtc-pcf85363.c
> > +++ b/drivers/rtc/rtc-pcf85363.c
> > @@ -120,6 +120,11 @@ struct pcf85363 {
> >         struct regmap           *regmap;
> >  };
> >
> > +struct pcf85x63_config {
> > +       struct regmap_config regmap;
> > +       unsigned int num_nvram;
> > +};
> > +
> >  static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time
> > *tm)  {
> >         struct pcf85363 *pcf85363 = dev_get_drvdata(dev); @@ -311,25
> > +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned int
> offset, void *val,
> >                                  val, bytes);  }
> >
> > -static const struct regmap_config regmap_config = {
> > -       .reg_bits = 8,
> > -       .val_bits = 8,
> > -       .max_register = 0x7f,
> > +static int pcf85x63_nvram_read(void *priv, unsigned int offset, void *val,
> > +                              size_t bytes)
>
> Given bytes should be 1, val should be a pointer to a single byte...
> What if bytes == 0?

I doubt we get "bytes==0" because of the checks in " drivers/nvmem/core.c"
Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".


> > +{> +       struct pcf85363 *pcf85363 = priv;
> > +
> > +       return regmap_read(pcf85363->regmap, CTRL_RAMBYTE, val);
>
> However, regmap_read() has an unsigned int output parameter!
> So it's writing too many bytes, and only writing the actual data byte to the
> correct address on little-endian systems.
> Hence you need to use an intermediate variable to convert from unsigned int
> to byte.

OK. Will use an intermediate integer variable.

> > +}
> > +
> > +static int pcf85x63_nvram_write(void *priv, unsigned int offset, void *val,
> > +                               size_t bytes) {
> > +       struct pcf85363 *pcf85363 = priv;
> > +
> > +       return regmap_write(pcf85363->regmap, CTRL_RAMBYTE,
> > +                               *((unsigned int *)val));
>
> Likewise for writing.
>
> > +}
>
> BTW, while the nvmem_device_{read,write}() public API is documented, the
> nvmem_device.reg_{read,write}() driver API isn't.
> And the behavior might be confusing.
>
> E.g.
>  * Return: length of successful bytes read on success and negative
>  * error code on error.
>
> The public API seems to assume the driver API returns zero on success, and
> replaces the zero by the number of bytes requested.
> If the requested number of bytes is too large, a zero success would be
> converted to a value that's larger than the actual number of bytes
> transferred!
> However, the driver API can return a smaller (positive) number, which
> matches "standard" read/write() APIs.
>
> > +static const struct pcf85x63_config pcf_85263_config = {
> > +       {
> > +               .reg_bits = 8,
> > +               .val_bits = 8,
> > +               .max_register = 0x2f,
> > +       },
> > +       1
>
> The "1" looks funny. Please use C99 initializers for all struct members.

OK will fix this.
> > +};
> > +
> > +static const struct pcf85x63_config pcf_85363_config = {
> > +       {
> > +               .reg_bits = 8,
> > +               .val_bits = 8,
> > +               .max_register = 0x7f,
> > +       },
> > +       2
>
> Likewise.

 OK will fix this.

Regards,
Biju

> The rest looks good to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds


[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Geert Uytterhoeven]

Hi Biju,

On Thu, Dec 6, 2018 at 4:24 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
> > CC nvmem maintainer
> >
> > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Add support for NXP pcf85263 real-time clock. pcf85263 rtc is
> > > compatible with pcf85363,except that pcf85363 has additional 64 bytes of
> > RAM.

> > > --- a/drivers/rtc/rtc-pcf85363.c
> > > +++ b/drivers/rtc/rtc-pcf85363.c
> > > @@ -120,6 +120,11 @@ struct pcf85363 {
> > >         struct regmap           *regmap;
> > >  };
> > >
> > > +struct pcf85x63_config {
> > > +       struct regmap_config regmap;
> > > +       unsigned int num_nvram;
> > > +};
> > > +
> > >  static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time
> > > *tm)  {
> > >         struct pcf85363 *pcf85363 = dev_get_drvdata(dev); @@ -311,25
> > > +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned int
> > offset, void *val,
> > >                                  val, bytes);  }
> > >
> > > -static const struct regmap_config regmap_config = {
> > > -       .reg_bits = 8,
> > > -       .val_bits = 8,
> > > -       .max_register = 0x7f,
> > > +static int pcf85x63_nvram_read(void *priv, unsigned int offset, void *val,
> > > +                              size_t bytes)
> >
> > Given bytes should be 1, val should be a pointer to a single byte...
> > What if bytes == 0?
>
> I doubt we get "bytes==0" because of the checks in " drivers/nvmem/core.c"
> Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".

Depends. There are other functions calling nvmem_reg_{read,write}(),
e.g. nvmem_device_{read,write}().

>
> > > +{> +       struct pcf85363 *pcf85363 = priv;
> > > +
> > > +       return regmap_read(pcf85363->regmap, CTRL_RAMBYTE, val);
> >
> > However, regmap_read() has an unsigned int output parameter!
> > So it's writing too many bytes, and only writing the actual data byte to the
> > correct address on little-endian systems.
> > Hence you need to use an intermediate variable to convert from unsigned int
> > to byte.
>
> OK. Will use an intermediate integer variable.
>
> > > +}
> > > +
> > > +static int pcf85x63_nvram_write(void *priv, unsigned int offset, void *val,
> > > +                               size_t bytes) {
> > > +       struct pcf85363 *pcf85363 = priv;
> > > +
> > > +       return regmap_write(pcf85363->regmap, CTRL_RAMBYTE,
> > > +                               *((unsigned int *)val));
> >
> > Likewise for writing.
> >
> > > +}
> >
> > BTW, while the nvmem_device_{read,write}() public API is documented, the
> > nvmem_device.reg_{read,write}() driver API isn't.
> > And the behavior might be confusing.
> >
> > E.g.
> >  * Return: length of successful bytes read on success and negative
> >  * error code on error.
> >
> > The public API seems to assume the driver API returns zero on success, and
> > replaces the zero by the number of bytes requested.
> > If the requested number of bytes is too large, a zero success would be
> > converted to a value that's larger than the actual number of bytes
> > transferred!
> > However, the driver API can return a smaller (positive) number, which
> > matches "standard" read/write() APIs.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> Hi Biju,
>
> On Thu, Dec 6, 2018 at 4:24 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > pcf85263 rtc CC nvmem maintainer
> > >
> > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com>
> wrote:
> > > > Add support for NXP pcf85263 real-time clock. pcf85263 rtc is
> > > > compatible with pcf85363,except that pcf85363 has additional 64
> > > > bytes of
> > > RAM.
>
> > > > --- a/drivers/rtc/rtc-pcf85363.c
> > > > +++ b/drivers/rtc/rtc-pcf85363.c
> > > > @@ -120,6 +120,11 @@ struct pcf85363 {
> > > >         struct regmap           *regmap;
> > > >  };
> > > >
> > > > +struct pcf85x63_config {
> > > > +       struct regmap_config regmap;
> > > > +       unsigned int num_nvram;
> > > > +};
> > > > +
> > > >  static int pcf85363_rtc_read_time(struct device *dev, struct
> > > > rtc_time
> > > > *tm)  {
> > > >         struct pcf85363 *pcf85363 = dev_get_drvdata(dev); @@
> > > > -311,25
> > > > +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned
> > > > +int
> > > offset, void *val,
> > > >                                  val, bytes);  }
> > > >
> > > > -static const struct regmap_config regmap_config = {
> > > > -       .reg_bits = 8,
> > > > -       .val_bits = 8,
> > > > -       .max_register = 0x7f,
> > > > +static int pcf85x63_nvram_read(void *priv, unsigned int offset, void
> *val,
> > > > +                              size_t bytes)
> > >
> > > Given bytes should be 1, val should be a pointer to a single byte...
> > > What if bytes == 0?
> >
> > I doubt we get "bytes==0" because of the checks in "
> drivers/nvmem/core.c"
> > Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".
>
> Depends. There are other functions calling nvmem_reg_{read,write}(), e.g.
> nvmem_device_{read,write}().

OK. In that case, I will return (-EINVAL)  for "bytes !=1"

> >
> > > > +{> +       struct pcf85363 *pcf85363 = priv;
> > > > +
> > > > +       return regmap_read(pcf85363->regmap, CTRL_RAMBYTE, val);
> > >
> > > However, regmap_read() has an unsigned int output parameter!
> > > So it's writing too many bytes, and only writing the actual data
> > > byte to the correct address on little-endian systems.
> > > Hence you need to use an intermediate variable to convert from
> > > unsigned int to byte.
> >
> > OK. Will use an intermediate integer variable.
> >
> > > > +}
> > > > +
> > > > +static int pcf85x63_nvram_write(void *priv, unsigned int offset, void
> *val,
> > > > +                               size_t bytes) {
> > > > +       struct pcf85363 *pcf85363 = priv;
> > > > +
> > > > +       return regmap_write(pcf85363->regmap, CTRL_RAMBYTE,
> > > > +                               *((unsigned int *)val));
> > >
> > > Likewise for writing.
> > >
> > > > +}
> > >
> > > BTW, while the nvmem_device_{read,write}() public API is documented,
> > > the
> > > nvmem_device.reg_{read,write}() driver API isn't.
> > > And the behavior might be confusing.
> > >
> > > E.g.
> > >  * Return: length of successful bytes read on success and negative
> > >  * error code on error.
> > >
> > > The public API seems to assume the driver API returns zero on
> > > success, and replaces the zero by the number of bytes requested.
> > > If the requested number of bytes is too large, a zero success would
> > > be converted to a value that's larger than the actual number of
> > > bytes transferred!
> > > However, the driver API can return a smaller (positive) number,
> > > which matches "standard" read/write() APIs.

Regards,
Biju


[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Alexandre Belloni]

On 06/12/2018 15:49:57+0000, Biju Das wrote:
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
> >
> > Hi Biju,
> >
> > On Thu, Dec 6, 2018 at 4:24 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > pcf85263 rtc CC nvmem maintainer
> > > >
> > > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com>
> > wrote:
> > > > > Add support for NXP pcf85263 real-time clock. pcf85263 rtc is
> > > > > compatible with pcf85363,except that pcf85363 has additional 64
> > > > > bytes of
> > > > RAM.
> >
> > > > > --- a/drivers/rtc/rtc-pcf85363.c
> > > > > +++ b/drivers/rtc/rtc-pcf85363.c
> > > > > @@ -120,6 +120,11 @@ struct pcf85363 {
> > > > >         struct regmap           *regmap;
> > > > >  };
> > > > >
> > > > > +struct pcf85x63_config {
> > > > > +       struct regmap_config regmap;
> > > > > +       unsigned int num_nvram;
> > > > > +};
> > > > > +
> > > > >  static int pcf85363_rtc_read_time(struct device *dev, struct
> > > > > rtc_time
> > > > > *tm)  {
> > > > >         struct pcf85363 *pcf85363 = dev_get_drvdata(dev); @@
> > > > > -311,25
> > > > > +316,68 @@ static int pcf85363_nvram_write(void *priv, unsigned
> > > > > +int
> > > > offset, void *val,
> > > > >                                  val, bytes);  }
> > > > >
> > > > > -static const struct regmap_config regmap_config = {
> > > > > -       .reg_bits = 8,
> > > > > -       .val_bits = 8,
> > > > > -       .max_register = 0x7f,
> > > > > +static int pcf85x63_nvram_read(void *priv, unsigned int offset, void
> > *val,
> > > > > +                              size_t bytes)
> > > >
> > > > Given bytes should be 1, val should be a pointer to a single byte...
> > > > What if bytes == 0?
> > >
> > > I doubt we get "bytes==0" because of the checks in "
> > drivers/nvmem/core.c"
> > > Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".
> >
> > Depends. There are other functions calling nvmem_reg_{read,write}(), e.g.
> > nvmem_device_{read,write}().
> 
> OK. In that case, I will return (-EINVAL)  for "bytes !=1"
> 

I think it is probably better to ensure the nvmem core never passes an
invalid number of bytes. All the ther RTC drivers make that assumption.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Biju Das]

Hi Alexandre,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> On 06/12/2018 15:49:57+0000, Biju Das wrote:
> > Hi Geert,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > pcf85263 rtc
> > >
> > > Hi Biju,
> > >
> > > On Thu, Dec 6, 2018 at 4:24 PM Biju Das <biju.das@bp.renesas.com>
> wrote:
> > > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > > pcf85263 rtc CC nvmem maintainer
> > > > >
> > > > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das
> > > > > <biju.das@bp.renesas.com>
> > > wrote:
> > > > > > Add support for NXP pcf85263 real-time clock. pcf85263 rtc is
> > > > > > compatible with pcf85363,except that pcf85363 has additional
> > > > > > 64 bytes of
> > > > > RAM.
> > >
> > > > > > --- a/drivers/rtc/rtc-pcf85363.c
> > > > > > +++ b/drivers/rtc/rtc-pcf85363.c
> > > > > > @@ -120,6 +120,11 @@ struct pcf85363 {
> > > > > >         struct regmap           *regmap;
> > > > > >  };
> > > > > >
> > > > > > +struct pcf85x63_config {
> > > > > > +       struct regmap_config regmap;
> > > > > > +       unsigned int num_nvram; };
> > > > > > +
> > > > > >  static int pcf85363_rtc_read_time(struct device *dev, struct
> > > > > > rtc_time
> > > > > > *tm)  {
> > > > > >         struct pcf85363 *pcf85363 = dev_get_drvdata(dev); @@
> > > > > > -311,25
> > > > > > +316,68 @@ static int pcf85363_nvram_write(void *priv,
> > > > > > +unsigned int
> > > > > offset, void *val,
> > > > > >                                  val, bytes);  }
> > > > > >
> > > > > > -static const struct regmap_config regmap_config = {
> > > > > > -       .reg_bits = 8,
> > > > > > -       .val_bits = 8,
> > > > > > -       .max_register = 0x7f,
> > > > > > +static int pcf85x63_nvram_read(void *priv, unsigned int
> > > > > > +offset, void
> > > *val,
> > > > > > +                              size_t bytes)
> > > > >
> > > > > Given bytes should be 1, val should be a pointer to a single byte...
> > > > > What if bytes == 0?
> > > >
> > > > I doubt we get "bytes==0" because of the checks in "
> > > drivers/nvmem/core.c"
> > > > Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".
> > >
> > > Depends. There are other functions calling nvmem_reg_{read,write}(),
> e.g.
> > > nvmem_device_{read,write}().
> >
> > OK. In that case, I will return (-EINVAL)  for "bytes !=1"
> >
>
> I think it is probably better to ensure the nvmem core never passes an invalid
> number of bytes. All the ther RTC drivers make that assumption.

In that case, I will do following checks in nvmem_device_{read,write}() before calling nvmem_reg_{read,write}(),

nvmem_device_read

/* Stop the user from reading */
if (offset  >= nvmem->size)
return 0;

if (bytes == 0)
return -EINVAL;

if (offset + bytes > nvmem->size)
bytes = nvmem->size - offset;

nvmem_device_write

/* Stop the user from writing */
if (offset  >= nvmem->size)
return -EFBIG;

if (bytes == 0)
return -EINVAL;

if (offset + bytes > nvmem->size)
bytes = nvmem->size - offset;

regards,
Biju





[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Geert Uytterhoeven]

Hi Biju,

On Fri, Dec 7, 2018 at 10:34 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
> >
> > On 06/12/2018 15:49:57+0000, Biju Das wrote:

> > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > pcf85263 rtc

> > > > On Thu, Dec 6, 2018 at 4:24 PM Biju Das <biju.das@bp.renesas.com>
> > wrote:
> > > > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > > > pcf85263 rtc CC nvmem maintainer
> > > > > > Given bytes should be 1, val should be a pointer to a single byte...
> > > > > > What if bytes == 0?
> > > > >
> > > > > I doubt we get "bytes==0" because of the checks in "
> > > > drivers/nvmem/core.c"
> > > > > Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".
> > > >
> > > > Depends. There are other functions calling nvmem_reg_{read,write}(),
> > e.g.
> > > > nvmem_device_{read,write}().
> > >
> > > OK. In that case, I will return (-EINVAL)  for "bytes !=1"
> >
> > I think it is probably better to ensure the nvmem core never passes an invalid
> > number of bytes. All the ther RTC drivers make that assumption.
>
> In that case, I will do following checks in nvmem_device_{read,write}() before calling nvmem_reg_{read,write}(),
>
> nvmem_device_read
>
> /* Stop the user from reading */
> if (offset  >= nvmem->size)
> return 0;
>
> if (bytes == 0)
> return -EINVAL;

Why not 0?

>
> if (offset + bytes > nvmem->size)

This might overflow, please use check_add_overflow().

> bytes = nvmem->size - offset;
>
> nvmem_device_write
>
> /* Stop the user from writing */
> if (offset  >= nvmem->size)
> return -EFBIG;

ENOSPC?

+ same comments as for read.

Oh, and offset is unsigned int instead of loff_t.
Nobody's envisioning nvmem devices larger than 4 GiB?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> Hi Biju,
>
> On Fri, Dec 7, 2018 at 10:34 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > pcf85263 rtc
> > >
> > > On 06/12/2018 15:49:57+0000, Biju Das wrote:
>
> > > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > > pcf85263 rtc
>
> > > > > On Thu, Dec 6, 2018 at 4:24 PM Biju Das
> > > > > <biju.das@bp.renesas.com>
> > > wrote:
> > > > > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for
> > > > > > > NXP
> > > > > > > pcf85263 rtc CC nvmem maintainer Given bytes should be 1,
> > > > > > > val should be a pointer to a single byte...
> > > > > > > What if bytes == 0?
> > > > > >
> > > > > > I doubt we get "bytes==0" because of the checks in "
> > > > > drivers/nvmem/core.c"
> > > > > > Function " bin_attr_nvmem_read/ bin_attr_nvmem_write".
> > > > >
> > > > > Depends. There are other functions calling
> > > > > nvmem_reg_{read,write}(),
> > > e.g.
> > > > > nvmem_device_{read,write}().
> > > >
> > > > OK. In that case, I will return (-EINVAL)  for "bytes !=1"
> > >
> > > I think it is probably better to ensure the nvmem core never passes
> > > an invalid number of bytes. All the ther RTC drivers make that
> assumption.
> >
> > In that case, I will do following checks in
> > nvmem_device_{read,write}() before calling nvmem_reg_{read,write}(),
> >
> > nvmem_device_read
> >
> > /* Stop the user from reading */
> > if (offset  >= nvmem->size)
> > return 0;
> >
> > if (bytes == 0)
> > return -EINVAL;
>
> Why not 0?

Ok. Will merge with above check.

if ((offset  >= nvmem->size)  && (bytes == 0))
        return 0;

> >
> > if (offset + bytes > nvmem->size)
>
> This might overflow, please use check_add_overflow().

Will use check_add_overflow() and then the  result is compared with nvmem->size, if the check operation is successful.

> > bytes = nvmem->size - offset;
> >
> > nvmem_device_write
> >
> > /* Stop the user from writing */
> > if (offset  >= nvmem->size)
> > return -EFBIG;
>
> ENOSPC?

OK, Will change it to ENOSPC.

> + same comments as for read.
>
> Oh, and offset is unsigned int instead of loff_t.
> Nobody's envisioning nvmem devices larger than 4 GiB?

Regards,
Biju


[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc

[Geert Uytterhoeven]

Hi Biju,

On Fri, Dec 7, 2018 at 11:16 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
> > On Fri, Dec 7, 2018 at 10:34 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH v3 2/4] rtc: pcf85363: Add support for NXP
> > > > pcf85263 rtc
> > > > I think it is probably better to ensure the nvmem core never passes
> > > > an invalid number of bytes. All the ther RTC drivers make that
> > assumption.
> > >
> > > In that case, I will do following checks in
> > > nvmem_device_{read,write}() before calling nvmem_reg_{read,write}(),
> > >
> > > nvmem_device_read
> > >
> > > /* Stop the user from reading */
> > > if (offset  >= nvmem->size)
> > > return 0;
> > >
> > > if (bytes == 0)
> > > return -EINVAL;
> >
> > Why not 0?
>
> Ok. Will merge with above check.
>
> if ((offset  >= nvmem->size)  && (bytes == 0))

"||" ;-)

>         return 0;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 3/4] ARM: shmobile: Enable NXP pcf85363 rtc in shmobile_defconfig

[Biju Das]

The iWave RZ/G1C SBC supports RTC (NXP pcf85263). To increase hardware
support enable the driver in the shmobile_defconfig multiplatform
configuration.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V1-->V2
	* No change.
V2-->V3
	* No change.
---
 arch/arm/configs/shmobile_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig
index 9e5a5ad..fdac4e4 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -177,6 +177,7 @@ CONFIG_LEDS_CLASS=y
 CONFIG_LEDS_GPIO=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_RS5C372=y
+CONFIG_RTC_DRV_PCF85363=y
 CONFIG_RTC_DRV_BQ32K=y
 CONFIG_RTC_DRV_S35390A=y
 CONFIG_RTC_DRV_RX8581=y
-- 
2.7.4

Re: [PATCH v3 3/4] ARM: shmobile: Enable NXP pcf85363 rtc in shmobile_defconfig

[Geert Uytterhoeven]

On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
>
> The iWave RZ/G1C SBC supports RTC (NXP pcf85263). To increase hardware
> support enable the driver in the shmobile_defconfig multiplatform
> configuration.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Biju Das]

Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
V1-->V2
	* No change
V2-->V3
	* No change
---
 arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
index 40b7f98..77d1824 100644
--- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
+++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
@@ -84,12 +84,30 @@
 	clock-frequency = <20000000>;
 };
 
+&i2c3 {
+	pinctrl-0 = <&i2c3_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+	clock-frequency = <400000>;
+
+	rtc@51 {
+		compatible = "nxp,pcf85263";
+		reg = <0x51>;
+	};
+};
+
 &pfc {
 	avb_pins: avb {
 		groups = "avb_mdio", "avb_gmii_tx_rx";
 		function = "avb";
 	};
 
+	i2c3_pins: i2c3 {
+		groups = "i2c3_c";
+		function = "i2c3";
+	};
+
 	mmc_pins_uhs: mmc_uhs {
 		groups = "mmc_data8", "mmc_ctrl";
 		function = "mmc";
-- 
2.7.4

Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Geert Uytterhoeven]

Hi Biju,

On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> @@ -84,12 +84,30 @@
>         clock-frequency = <20000000>;
>  };
>
> +&i2c3 {
> +       pinctrl-0 = <&i2c3_pins>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +       clock-frequency = <400000>;
> +
> +       rtc@51 {
> +               compatible = "nxp,pcf85263";
> +               reg = <0x51>;

You might want to enable the optional interrupt:

    interrupt-parent = <&gpio0>;
    interrupts = <2 IRQ_TYPE_LEVEL_LOW>;

> +       };


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC
>
> Hi Biju,
>
> On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > @@ -84,12 +84,30 @@
> >         clock-frequency = <20000000>;
> >  };
> >
> > +&i2c3 {
> > +       pinctrl-0 = <&i2c3_pins>;
> > +       pinctrl-names = "default";
> > +
> > +       status = "okay";
> > +       clock-frequency = <400000>;
> > +
> > +       rtc@51 {
> > +               compatible = "nxp,pcf85263";
> > +               reg = <0x51>;
>
> You might want to enable the optional interrupt:

I have enabled this but unfortunately it is generating 100000 of gpio interrupts during boot.

The reason is, by default this pin is configured as function(Power on reset/at u-boot).
Currently there is no function available in kernel to convert a pin from function to gpio (Similar to the issue Fab is facing for display hot plug interrupt)

May be we can add optional interrupt at a later stage, once we have a solution for converting  pin from function to gpio.

Please share your opinion on this.

Regards,
Biju





[https://www2.renesas.eu/media/email/unicef.jpg]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Geert Uytterhoeven]

Hi Biju,

On Thu, Dec 6, 2018 at 1:41 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

> > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
> > >
> > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > @@ -84,12 +84,30 @@
> > >         clock-frequency = <20000000>;
> > >  };
> > >
> > > +&i2c3 {
> > > +       pinctrl-0 = <&i2c3_pins>;
> > > +       pinctrl-names = "default";
> > > +
> > > +       status = "okay";
> > > +       clock-frequency = <400000>;
> > > +
> > > +       rtc@51 {
> > > +               compatible = "nxp,pcf85263";
> > > +               reg = <0x51>;
> >
> > You might want to enable the optional interrupt:
>
> I have enabled this but unfortunately it is generating 100000 of gpio interrupts during boot.

Oh, the DT bindings claim interrupt support hasn't been implement yet ;-)

> The reason is, by default this pin is configured as function(Power on reset/at u-boot).
> Currently there is no function available in kernel to convert a pin from function to gpio (Similar to the issue Fab is facing for display hot plug interrupt)
>
> May be we can add optional interrupt at a later stage, once we have a solution for converting  pin from function to gpio.
>
> Please share your opinion on this.

IC. In that case, please postpone describing the interrupt until the issue is
fixed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Simon Horman]

On Thu, Dec 06, 2018 at 01:59:58PM +0100, Geert Uytterhoeven wrote:
> Hi Biju,
> 
> On Thu, Dec 6, 2018 at 1:41 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC
> 
> > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
> > > >
> > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > > --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > @@ -84,12 +84,30 @@
> > > >         clock-frequency = <20000000>;
> > > >  };
> > > >
> > > > +&i2c3 {
> > > > +       pinctrl-0 = <&i2c3_pins>;
> > > > +       pinctrl-names = "default";
> > > > +
> > > > +       status = "okay";
> > > > +       clock-frequency = <400000>;
> > > > +
> > > > +       rtc@51 {
> > > > +               compatible = "nxp,pcf85263";
> > > > +               reg = <0x51>;
> > >
> > > You might want to enable the optional interrupt:
> >
> > I have enabled this but unfortunately it is generating 100000 of gpio interrupts during boot.
> 
> Oh, the DT bindings claim interrupt support hasn't been implement yet ;-)
> 
> > The reason is, by default this pin is configured as function(Power on reset/at u-boot).
> > Currently there is no function available in kernel to convert a pin from function to gpio (Similar to the issue Fab is facing for display hot plug interrupt)
> >
> > May be we can add optional interrupt at a later stage, once we have a solution for converting  pin from function to gpio.
> >
> > Please share your opinion on this.
> 
> IC. In that case, please postpone describing the interrupt until the issue is
> fixed.

It feels like this patch is ready to be accepted.
Geert, do you concur?

Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Geert Uytterhoeven]

Hi Simon,

On Mon, Dec 10, 2018 at 12:52 PM Simon Horman <horms@verge.net.au> wrote:
> On Thu, Dec 06, 2018 at 01:59:58PM +0100, Geert Uytterhoeven wrote:
> > On Thu, Dec 6, 2018 at 1:41 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC
> >
> > > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > > Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > > --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > > +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > > @@ -84,12 +84,30 @@
> > > > >         clock-frequency = <20000000>;
> > > > >  };
> > > > >
> > > > > +&i2c3 {
> > > > > +       pinctrl-0 = <&i2c3_pins>;
> > > > > +       pinctrl-names = "default";
> > > > > +
> > > > > +       status = "okay";
> > > > > +       clock-frequency = <400000>;
> > > > > +
> > > > > +       rtc@51 {
> > > > > +               compatible = "nxp,pcf85263";
> > > > > +               reg = <0x51>;
> > > >
> > > > You might want to enable the optional interrupt:
> > >
> > > I have enabled this but unfortunately it is generating 100000 of gpio interrupts during boot.
> >
> > Oh, the DT bindings claim interrupt support hasn't been implement yet ;-)
> >
> > > The reason is, by default this pin is configured as function(Power on reset/at u-boot).
> > > Currently there is no function available in kernel to convert a pin from function to gpio (Similar to the issue Fab is facing for display hot plug interrupt)
> > >
> > > May be we can add optional interrupt at a later stage, once we have a solution for converting  pin from function to gpio.
> > >
> > > Please share your opinion on this.
> >
> > IC. In that case, please postpone describing the interrupt until the issue is
> > fixed.
>
> It feels like this patch is ready to be accepted.
> Geert, do you concur?

Yes, I agree.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC

[Simon Horman]

On Mon, Dec 10, 2018 at 12:56:11PM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Mon, Dec 10, 2018 at 12:52 PM Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Dec 06, 2018 at 01:59:58PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Dec 6, 2018 at 1:41 PM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH v3 4/4] ARM: dts: iwg23s-sbc: Enable RTC
> > >
> > > > > On Thu, Dec 6, 2018 at 10:04 AM Biju Das <biju.das@bp.renesas.com> wrote:
> > > > > > Enable NXP pcf85263 real time clock for the iWave SBC based on RZ/G1C.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > > >
> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > >
> > > > > > --- a/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > > > +++ b/arch/arm/boot/dts/r8a77470-iwg23s-sbc.dts
> > > > > > @@ -84,12 +84,30 @@
> > > > > >         clock-frequency = <20000000>;
> > > > > >  };
> > > > > >
> > > > > > +&i2c3 {
> > > > > > +       pinctrl-0 = <&i2c3_pins>;
> > > > > > +       pinctrl-names = "default";
> > > > > > +
> > > > > > +       status = "okay";
> > > > > > +       clock-frequency = <400000>;
> > > > > > +
> > > > > > +       rtc@51 {
> > > > > > +               compatible = "nxp,pcf85263";
> > > > > > +               reg = <0x51>;
> > > > >
> > > > > You might want to enable the optional interrupt:
> > > >
> > > > I have enabled this but unfortunately it is generating 100000 of gpio interrupts during boot.
> > >
> > > Oh, the DT bindings claim interrupt support hasn't been implement yet ;-)
> > >
> > > > The reason is, by default this pin is configured as function(Power on reset/at u-boot).
> > > > Currently there is no function available in kernel to convert a pin from function to gpio (Similar to the issue Fab is facing for display hot plug interrupt)
> > > >
> > > > May be we can add optional interrupt at a later stage, once we have a solution for converting  pin from function to gpio.
> > > >
> > > > Please share your opinion on this.
> > >
> > > IC. In that case, please postpone describing the interrupt until the issue is
> > > fixed.
> >
> > It feels like this patch is ready to be accepted.
> > Geert, do you concur?
> 
> Yes, I agree.

Thanks, I have applied v4 (which is the same as v3).