| Message ID | 1544086559-47141-3-git-send-email-biju.das@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add NXP pcf85263 real-time clock support | expand |
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
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.
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
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.
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.
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.
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
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.
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
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, ®map_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");
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(-)