linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Add NXP pcf85263 real-time clock support
@ 2018-11-29 16:54 Biju Das
  2018-11-29 16:54 ` [PATCH v2 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock Biju Das
  2018-11-29 16:54 ` [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc Biju Das
  0 siblings, 2 replies; 8+ messages in thread
From: Biju Das @ 2018-11-29 16:54 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland
  Cc: Biju Das, linux-rtc, devicetree, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

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.

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                         | 72 +++++++++++++++++-----
 4 files changed, 78 insertions(+), 17 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock
  2018-11-29 16:54 [PATCH v2 0/4] Add NXP pcf85263 real-time clock support Biju Das
@ 2018-11-29 16:54 ` Biju Das
  2018-11-29 16:54 ` [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc Biju Das
  1 sibling, 0 replies; 8+ messages in thread
From: Biju Das @ 2018-11-29 16:54 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Simon Horman,
	Mark Rutland
  Cc: Biju Das, linux-rtc, devicetree, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

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.
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-29 16:54 [PATCH v2 0/4] Add NXP pcf85263 real-time clock support Biju Das
  2018-11-29 16:54 ` [PATCH v2 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock Biju Das
@ 2018-11-29 16:54 ` Biju Das
  2018-11-30 11:05   ` Geert Uytterhoeven
  1 sibling, 1 reply; 8+ messages in thread
From: Biju Das @ 2018-11-29 16:54 UTC (permalink / raw)
  To: Alessandro Zummo, Geert Uytterhoeven, Alexandre Belloni
  Cc: Biju Das, linux-rtc, Simon Horman, Chris Paterson,
	Fabrizio Castro, linux-renesas-soc

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.
---
 drivers/rtc/rtc-pcf85363.c | 72 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c04a1ed..9c567c8 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -311,7 +311,30 @@ static int pcf85363_nvram_write(void *priv, unsigned int offset, void *val,
 				 val, bytes);
 }
 
-static const struct regmap_config regmap_config = {
+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 regmap_config pcf_85263_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x2f,
+};
+
+static const struct regmap_config pcf_85363_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.max_register = 0x7f,
@@ -321,15 +344,25 @@ 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 regmap_config *regmap_config = &pcf_85363_regmap_config;
+	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, num_nvmem = 2;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -339,7 +372,13 @@ static int pcf85363_probe(struct i2c_client *client,
 	if (!pcf85363)
 		return -ENOMEM;
 
-	pcf85363->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (of_device_get_match_data(&client->dev) ==
+					&pcf_85263_regmap_config) {
+		regmap_config = &pcf_85263_regmap_config;
+		num_nvmem = 1;
+	}
+
+	pcf85363->regmap = devm_regmap_init_i2c(client, regmap_config);
 	if (IS_ERR(pcf85363->regmap)) {
 		dev_err(&client->dev, "regmap allocation failed\n");
 		return PTR_ERR(pcf85363->regmap);
@@ -370,15 +409,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 < num_nvmem ; 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_regmap_config },
+	{ .compatible = "nxp,pcf85363", .data = &pcf_85363_regmap_config },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dev_ids);
 
@@ -393,5 +435,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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-29 16:54 ` [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc Biju Das
@ 2018-11-30 11:05   ` Geert Uytterhoeven
  2018-11-30 12:32     ` Alexandre Belloni
  2018-12-05 13:44     ` Biju Das
  0 siblings, 2 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2018-11-30 11:05 UTC (permalink / raw)
  To: Biju Das
  Cc: Alessandro Zummo, Geert Uytterhoeven, Alexandre Belloni,
	linux-rtc, Simon Horman, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

Hi Biju,

On Thu, Nov 29, 2018 at 6:03 PM 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.

Thanks for the update!

> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c

> @@ -321,15 +344,25 @@ 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 regmap_config *regmap_config = &pcf_85363_regmap_config;
> +       struct nvmem_config nvmem_cfg[] = {

static?

Although the nvmem_config is copied, and thus static is not needed, I
guess using static will decrease kernel size.

> +               {
> +                       .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, num_nvmem = 2;
>
>         if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>                 return -ENODEV;
> @@ -339,7 +372,13 @@ static int pcf85363_probe(struct i2c_client *client,
>         if (!pcf85363)
>                 return -ENOMEM;
>
> -       pcf85363->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +       if (of_device_get_match_data(&client->dev) ==
> +                                       &pcf_85263_regmap_config) {
> +               regmap_config = &pcf_85263_regmap_config;
> +               num_nvmem = 1;

I think it's cleaner if you store the full config (regmap_config + num_nvmem)
in of_device_id.data, instead of just the regmap_config, using

    struct pcf85x63_config {
            struct regmap_config regmap;
            unsigned int num_nvram;
    };

    static const struct pcf85x63_config pcf85263_config = { ... };
    static const struct pcf85x63_config pcf85363_config = { ... };

    static const struct of_device_id dev_ids[] = {
            { .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
            { .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
            { /* sentinel */ }
    };

Then you can just do

        struct pcf85x63_config *config = &pcf85363_config; /* default
for non-DT */
        void *data = of_device_get_match_data(&client->dev);
        if (data)
                config = data;

> +       }
> +
> +       pcf85363->regmap = devm_regmap_init_i2c(client, regmap_config);
>         if (IS_ERR(pcf85363->regmap)) {
>                 dev_err(&client->dev, "regmap allocation failed\n");
>                 return PTR_ERR(pcf85363->regmap);


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-30 11:05   ` Geert Uytterhoeven
@ 2018-11-30 12:32     ` Alexandre Belloni
  2018-11-30 12:52       ` Geert Uytterhoeven
  2018-12-05 13:44     ` Biju Das
  1 sibling, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2018-11-30 12:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Biju Das, Alessandro Zummo, Geert Uytterhoeven, linux-rtc,
	Simon Horman, Chris Paterson, Fabrizio Castro, Linux-Renesas

On 30/11/2018 12:05:16+0100, Geert Uytterhoeven wrote:
> Hi Biju,
> 
> On Thu, Nov 29, 2018 at 6:03 PM 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.
> 
> Thanks for the update!
> 
> > --- a/drivers/rtc/rtc-pcf85363.c
> > +++ b/drivers/rtc/rtc-pcf85363.c
> 
> > @@ -321,15 +344,25 @@ 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 regmap_config *regmap_config = &pcf_85363_regmap_config;
> > +       struct nvmem_config nvmem_cfg[] = {
> 
> static?
> 
> Although the nvmem_config is copied, and thus static is not needed, I
> guess using static will decrease kernel size.
> 

Hum, I don't think, this is on the stack anyway.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-30 12:32     ` Alexandre Belloni
@ 2018-11-30 12:52       ` Geert Uytterhoeven
  2018-12-05 13:42         ` Biju Das
  0 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2018-11-30 12:52 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Biju Das, Alessandro Zummo, Geert Uytterhoeven, linux-rtc,
	Simon Horman, Chris Paterson, Fabrizio Castro, Linux-Renesas

Hi Alexandre,

On Fri, Nov 30, 2018 at 1:32 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 30/11/2018 12:05:16+0100, Geert Uytterhoeven wrote:
> > On Thu, Nov 29, 2018 at 6:03 PM 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.
> >
> > Thanks for the update!
> >
> > > --- a/drivers/rtc/rtc-pcf85363.c
> > > +++ b/drivers/rtc/rtc-pcf85363.c
> >
> > > @@ -321,15 +344,25 @@ 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 regmap_config *regmap_config = &pcf_85363_regmap_config;
> > > +       struct nvmem_config nvmem_cfg[] = {
> >
> > static?
> >
> > Although the nvmem_config is copied, and thus static is not needed, I
> > guess using static will decrease kernel size.
> >
>
> Hum, I don't think, this is on the stack anyway.

If you make it static, it's no longer allocated on the stack, and gcc has no
longer to emit code to initialize all members.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-30 12:52       ` Geert Uytterhoeven
@ 2018-12-05 13:42         ` Biju Das
  0 siblings, 0 replies; 8+ messages in thread
From: Biju Das @ 2018-12-05 13:42 UTC (permalink / raw)
  To: Geert Uytterhoeven, Alexandre Belloni
  Cc: Alessandro Zummo, Geert Uytterhoeven, linux-rtc, Simon Horman,
	Chris Paterson, Fabrizio Castro, Linux-Renesas

Hi Geert and Alexandre,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> Hi Alexandre,
>
> On Fri, Nov 30, 2018 at 1:32 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 30/11/2018 12:05:16+0100, Geert Uytterhoeven wrote:
> > > On Thu, Nov 29, 2018 at 6:03 PM 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.
> > >
> > > Thanks for the update!
> > >
> > > > --- a/drivers/rtc/rtc-pcf85363.c
> > > > +++ b/drivers/rtc/rtc-pcf85363.c
> > >
> > > > @@ -321,15 +344,25 @@ 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 regmap_config *regmap_config =
> &pcf_85363_regmap_config;
> > > > +       struct nvmem_config nvmem_cfg[] = {
> > >
> > > static?
> > >
> > > Although the nvmem_config is copied, and thus static is not needed,
> > > I guess using static will decrease kernel size.
> > >
> >
> > Hum, I don't think, this is on the stack anyway.
>
> If you make it static, it's no longer allocated on the stack, and gcc has no
> longer to emit code to initialize all members.
>

I have used "size" command to check the size of vmlinux with and without static. Please find the results.

Without static
===========
$ size vmlinux
   text   data    bss    dec    hexfilename
71473602625982 29260810065950 99981evmlinux

With static variable
==================
$ size vmlinux
   text   data    bss    dec    hexfilename
71472002626110 29260810065918 9997fevmlinux

So overall with static, there is a reduction in kernel size. I will send V3 with declaring it as static.

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
  2018-11-30 11:05   ` Geert Uytterhoeven
  2018-11-30 12:32     ` Alexandre Belloni
@ 2018-12-05 13:44     ` Biju Das
  1 sibling, 0 replies; 8+ messages in thread
From: Biju Das @ 2018-12-05 13:44 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Alessandro Zummo, Geert Uytterhoeven, Alexandre Belloni,
	linux-rtc, Simon Horman, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc
>
> Hi Biju,
>
> On Thu, Nov 29, 2018 at 6:03 PM 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.
>
> Thanks for the update!
>
> > --- a/drivers/rtc/rtc-pcf85363.c
> > +++ b/drivers/rtc/rtc-pcf85363.c
>
> > @@ -321,15 +344,25 @@ 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 regmap_config *regmap_config =
> &pcf_85363_regmap_config;
> > +       struct nvmem_config nvmem_cfg[] = {
>
> static?
>
> Although the nvmem_config is copied, and thus static is not needed, I guess
> using static will decrease kernel size.
>
> > +               {
> > +                       .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, num_nvmem = 2;
> >
> >         if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> >                 return -ENODEV;
> > @@ -339,7 +372,13 @@ static int pcf85363_probe(struct i2c_client *client,
> >         if (!pcf85363)
> >                 return -ENOMEM;
> >
> > -       pcf85363->regmap = devm_regmap_init_i2c(client, &regmap_config);
> > +       if (of_device_get_match_data(&client->dev) ==
> > +                                       &pcf_85263_regmap_config) {
> > +               regmap_config = &pcf_85263_regmap_config;
> > +               num_nvmem = 1;
>
> I think it's cleaner if you store the full config (regmap_config + num_nvmem)
> in of_device_id.data, instead of just the regmap_config, using
>
>     struct pcf85x63_config {
>             struct regmap_config regmap;
>             unsigned int num_nvram;
>     };
>
>     static const struct pcf85x63_config pcf85263_config = { ... };
>     static const struct pcf85x63_config pcf85363_config = { ... };
>
>     static const struct of_device_id dev_ids[] = {
>             { .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
>             { .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
>             { /* sentinel */ }
>     };
>
> Then you can just do
>
>         struct pcf85x63_config *config = &pcf85363_config; /* default for non-DT
> */
>         void *data = of_device_get_match_data(&client->dev);
>         if (data)
>                 config = data;
>
> > +       }

Will send V3 with above changes.

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-12-05 13:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29 16:54 [PATCH v2 0/4] Add NXP pcf85263 real-time clock support Biju Das
2018-11-29 16:54 ` [PATCH v2 1/4] dt-bindings: rtc: pcf85363: Document pcf85263 real-time clock Biju Das
2018-11-29 16:54 ` [PATCH v2 2/4] rtc: pcf85363: Add support for NXP pcf85263 rtc Biju Das
2018-11-30 11:05   ` Geert Uytterhoeven
2018-11-30 12:32     ` Alexandre Belloni
2018-11-30 12:52       ` Geert Uytterhoeven
2018-12-05 13:42         ` Biju Das
2018-12-05 13:44     ` Biju Das

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).