--------------------------------------------
On Thu, 1/5/17, bret.ng via rtc-linux <rtc-linux@googlegroups.com> wrote:
Subject: Re: [rtc-linux] [PATCH] rtc: Add support for RX4035
To: rtc-linux@googlegroups.com
Date: Thursday, January 5, 2017, 12:40 AM
--------------------------------------------
On Wed, 1/4/17, Alexandre Belloni <alexandre.bell...@free-electrons.com>
wrote:
Subject: Re: [rtc-linux] [PATCH] rtc: Add support for
RX4035
To: "Ryan Jones" <r...@rjones.io>
Cc: rtc-linux@googlegroups.com
Date: Wednesday, January 4, 2017, 11:57 PM
Hi,
Sorry for the late review!
On 05/08/2016 at 11:54:36 -0600, Ryan Jones wrote :
> + * Based on:
> + * drivers/rtc/rtc-rx4581.c
I'd argue that it doesn't really matter ;)
> +#define RX4035_REG_SC
0x00 /* Second in BCD */
> +#define RX4035_REG_MN
0x01 /* Minute in BCD */
> +#define RX4035_REG_HR
0x02 /* Hour in BCD */
> +#define RX4035_REG_DW
0x03 /* Day of Week */
> +#define RX4035_REG_DM
0x04 /* Day of Month in BCD */
> +#define RX4035_REG_MO
0x05 /* Month in BCD */
> +#define RX4035_REG_YR
0x06 /* Year in BCD */
> +#define RX4035_REG_RAM
0x0D /* RAM */
> +#define RX4035_REG_CTL1
0x0E /* Control Register 1*/
> +#define RX4035_REG_CTL2
0x0F /* Control Register 2*/
> +
> +/* Flag Register bit definitions */
> +#define RX4035_FLAG_VDET 0x40 /* Low
Voltage Detection */
> +#define RX4035_FLAG_PON
0x10 /* Indicates a Power On condition
*/
Please use BIT().
> +
> +static int rx4035_set_reg(struct spi_device *spi,
unsigned char address,
> + unsigned char data)
> +{
> + unsigned char buf[2];
> +
> + /*Set MSB to indicate a write
transfer*/
> + buf[0] = (address<<4) |
0x08;
checkpatch --strict is complaining about spaces here and
also a few
alignements later. Can you fix those?
> + buf[1] = data;
> +
> + return spi_write_then_read(spi,
buf, 2, NULL, 0);
> +}
> +
> +static int rx4035_get_reg(struct spi_device *spi,
unsigned char address,
> + unsigned char *data)
> +{
> + /*Set MSB to indicate a write
transfer*/
This comment is probably wrong...
> + *data = (address<<4) | 0x0c;
> +
> + return spi_write_then_read(spi,
data, 1, data, 1);
> +}
> +
> +/*
> + * In the routines that deal directly with the
rx4035
hardware, we use
> + * rtc_time -- month 0-11, hour 0-23, yr = calendar
year-epoch.
> + */
This comment is not useful
> +static int rx4035_get_datetime(struct device *dev,
struct rtc_time *tm)
> +{
> + struct spi_device *spi =
to_spi_device(dev);
> + unsigned char date[7];
> + unsigned char data;
> + int err;
> +
> + /*Check that data is reliable*/
> + err = rx4035_get_reg(spi,
RX4035_REG_CTL1, &data);
> + if (err != 0) {
> + dev_err(dev,
"Unable to read control register\n");
> + return -EIO;
> + }
> + if (data & (RX4035_FLAG_VDET |
RX4035_FLAG_PON))
> + dev_err(dev,
"RTC DATA UNRELIABLE - battery voltage fell below minimum
value\n");
It prints an error but the function continues as if
nothing
happend so
userspace doesn't actually know something is wrong. You
probably need to
return -EINVAL here.
> +
> + /* Read time and date */
> + date[0] = 0x04;
This is a magic value. I understand what it does but maybe
you could have
a define for each read and write mode and then remove the
(inaccurate)
comments.
> + err = spi_write_then_read(spi,
date, 1, date, 7);
> + if (err < 0) {
> + dev_err(dev,
"Unable to read date\n");
> + return -EIO;
> + }
> +
> + dev_dbg(dev,
> + "%s: raw data is
sec=%02x, min=%02x, hr=%02x, wday=%02x, mday=%02x,
mon=%02x,
year=%02x\n",
> + __func__,
> + date[0],
date[1], date[2], date[3], date[4], date[5], date[6]);
> +
> + tm->tm_sec =
bcd2bin(date[RX4035_REG_SC] & 0x7F);
> + tm->tm_min =
bcd2bin(date[RX4035_REG_MN] & 0x7F);
> + tm->tm_hour =
bcd2bin(date[RX4035_REG_HR] & 0x3F); /* rtc hr 0-23 */
> + tm->tm_wday =
date[RX4035_REG_DW] & 0x7F;
> + tm->tm_mday =
bcd2bin(date[RX4035_REG_DM] & 0x3F);
> + tm->tm_mon =
bcd2bin(date[RX4035_REG_MO] & 0x1F) - 1; /* rtc mn
1-12
*/
> + tm->tm_year =
bcd2bin(date[RX4035_REG_YR]);
> + if (tm->tm_year < 70)
> + tm->tm_year
+= 100; /* assume we are in 1970...2069 */
This is bad and usually doesn't work properly.
The datasheet explicitly state:
"The auto calendar function updates all dates, months, and
years from
January 1, 2001 to December 31, 2099."
It will not work outside this range. The leap year
handling
will
certainly fail at the edges.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
You received this message because you are subscribed to
"rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux
.
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to
the
Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails
from it, send an email to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit
https://groups.google.com/d/optout.
entre de presa politica germana Centre de presa politica
maghiara | Aparitia unor organe de presa muncitoresti
Asociatii stiintifice culturale si
academice0 W Periodice de cultura
stiintifice
--
You received this message because you are subscribed to
"rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux
.
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to the
Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails
from it, send an email to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit
https://groups.google.com/d/optout.ntre timp in tara se inregistreaza primul
semn de sange al veacului. tarani rasculati la 1907 sunt ucisi sau raniti de
armata din ordinul autoritatilor presate si de un factor extrem de periculos
armatele Austro-Ungariei ocupa pozitii de atac pe linia Carpatilor iar cele
ale Rusiei la Prut . Dumnezeu sa-i ierte scrie N. lorga in ziarul Neamul
Romanesc indemnand la o reconciliere din care rezulta prezenta eroica a
fiilor satelor in Razboiul pentru Reintregirea Nationala 1916-1919 .
Mobilizati de un guvern condus de Ion I.C. Bratianu si comandati intre alti
generali de Alexandru Averescu peste 800 000 de ostasi participa la cele trei
campanii de pe frontul romanesc 300 000 dintre ei jertfindu-si viata pentru
supremul ideal national.
--
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to the Google Groups
"rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rtc-linux+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
