rtc-2123: access the clock offset feature
cover rtc-2123: access the clock offset feature
Greetings,
This series was prompted by a need to adjust the clock rate of the rtc The existing code performs a soft reset during probe, which wipes out several registers including the offset register, which performs adjustments to the clock rate.
The first several patches are cleanup, with patch 5 and 6 avoiding the reset, and patch 9 adding a nice sysfs interface to the clock offset.
I know that this is not the only rtc to provide a programmable clock offset I wonder if this interface would make a good addition to the rtc api?
The rtc chips I have seen list their clock adjustments in parts per million. I went with parts per billion, since the ppm listed was listed with a fractional component.
Joshua Clayton (9):
rtc-pcf2123: Document all registers and useful bits
rtc-pcf2123: clean up reads from the chip
rtc-pcf2123: clean up writes to the rtc chip
rtc-pcf2123: replace magic numbers with defines
rtc-pcf2123: put the chip reset into a function
rtc-pcf2123: avoid resetting the clock if possible
rtc-pcf2123: allow sysfs to accept hexidecimal
rtc-pcf2123: use sysfs groups
rtc-pcf2123: adjust the clock rate via sysfs
drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++---------------- 1 file changed, 257 insertions(+), 134 deletions(-)
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 1/9 rtc-pcf2123: Document all registers and useful bits
Document all 16 registers in the pcf2123, as well as useful bit masks from the Control1 and seconds registers
--- drivers/rtc/rtc-pcf2123.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/rtc/rtc-pcf2123.c +22 -1
@@ -47,6 +47,7 @@#define DRV_VERSION "0.6" +/* REGISTERS */ #define PCF2123_REG_CTRL1 (0x00)/* Control Register 1 */ #define PCF2123_REG_CTRL2 (0x01)/* Control Register 2 */ #define PCF2123_REG_SC (0x02)/* datetime */
@@ -56,7 +57,27 @@#define PCF2123_REG_DW (0x06)#define PCF2123_REG_MO (0x07)#define PCF2123_REG_YR (0x08) - +#define PCF2123_REG_ALRM_MN (0x09)/* Alarm Registers */ +#define PCF2123_REG_ALRM_HR (0x0a) +#define PCF2123_REG_ALRM_DM (0x0b) +#define PCF2123_REG_ALRM_DW (0x0c) +#define PCF2123_REG_OFFSET (0x0d)/* Clock Rate Offset Register */ +#define PCF2123_REG_TMR_CLKOUT (0x0e)/* Timer Registers */ +#define PCF2123_REG_CTDWN_TMR (0x0f) +#define PCF2123_REG_MAX (PCF2123_REG_CTDWN_TMR) + +/* PCF2123_REG_CTRL1 BITS */ +#define CTRL1_CLEAR (0x00)/* Clear */ +#define CTRL1_CORRECTION_INT (0x02)/* Correction Interrupt */ +#define CTRL1_12_HOUR (0x04)/* 12 hour time */ +#define CTRL1_STOP (0x20)/* Stop the clock */ +#define CTRL1_SW_RESET (0x58)/* Software reset */ +#define CTRL1_EXT_TEST (0x80)/* External Clock Test mode */ + +/* PCF2123_REG_SC BITS */ +#define OSC_HAS_STOPPED (0x80)/* Clock has been stopped */ + +/* READ/WRITE ADDRESS BITS */ #define PCF2123_SUBADDR (1 << 4)#define PCF2123_WRITE ((0 << 7) | PCF2123_SUBADDR)#define PCF2123_READ ((1 << 7) | PCF2123_SUBADDR)
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 1/9] rtc-pcf2123: Document all registers and useful bits
Hi,
Quoted 35 lines · Joshua Clayton
Document all 16 registers in the pcf2123, as well as useful bit masks from the Control1 and seconds registers
Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)drivers/rtc/rtc-pcf2123.c +11
-1@@ -47,6 +47,7 @@#define DRV_VERSION "0.6" +/* REGISTERS */ #define PCF2123_REG_CTRL1 (0x00)/* Control Register 1 */ #define PCF2123_REG_CTRL2 (0x01)/* Control Register 2 */ #define PCF2123_REG_SC (0x02)/* datetime */ @@ -56,7 +57,27 @@#define PCF2123_REG_DW (0x06)#define PCF2123_REG_MO (0x07)#define PCF2123_REG_YR (0x08) - +#define PCF2123_REG_ALRM_MN (0x09)/* Alarm Registers */ +#define PCF2123_REG_ALRM_HR (0x0a) +#define PCF2123_REG_ALRM_DM (0x0b) +#define PCF2123_REG_ALRM_DW (0x0c) +#define PCF2123_REG_OFFSET (0x0d)/* Clock Rate Offset Register */ +#define PCF2123_REG_TMR_CLKOUT (0x0e)/* Timer Registers */ +#define PCF2123_REG_CTDWN_TMR (0x0f) +#define PCF2123_REG_MAX (PCF2123_REG_CTDWN_TMR) + +/* PCF2123_REG_CTRL1 BITS */
drivers/rtc/rtc-pcf2123.c line 79
Quoted 10 lines
drivers/rtc/rtc-pcf2123.c +10
-0+#define CTRL1_CLEAR (0x00)/* Clear */ +#define CTRL1_CORRECTION_INT (0x02)/* Correction Interrupt */ +#define CTRL1_12_HOUR (0x04)/* 12 hour time */ +#define CTRL1_STOP (0x20)/* Stop the clock */ +#define CTRL1_SW_RESET (0x58)/* Software reset */ +#define CTRL1_EXT_TEST (0x80)/* External Clock Test mode */ + +/* PCF2123_REG_SC BITS */ +#define OSC_HAS_STOPPED (0x80)/* Clock has been stopped */ +
Can you use the BIT() macro?
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 1/9] rtc-pcf2123: Document all registers and useful bits
Quoted 42 lines · On Tue, 24 Nov 2015 22:51:36 +0100 Alexandre Belloni…
Hi,
Quoted 39 lines · Joshua Clayton
Document all 16 registers in the pcf2123, as well as useful bit masks from the Control1 and seconds registers
Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)drivers/rtc/rtc-pcf2123.c +1
-0@@ -47,6 +47,7 @@#define DRV_VERSION "0.6" +/* REGISTERS */ #define PCF2123_REG_CTRL1 (0x00)/* Control Register 1 */ #define
PCF2123_REG_CTRL2(0x01) /* Control
Register 2 */ #definePCF2123_REG_SC
(0x02) /* datetime */ @@ -56,7 +57,27 @@
#definePCF2123_REG_DW(0x06)
#definePCF2123_REG_MO(0x07)
#definePCF2123_REG_YR(0x08)drivers/rtc/rtc-pcf2123.c +1
-1- +#define PCF2123_REG_ALRM_MN (0x09)/* Alarm Registers */ +#define
PCF2123_REG_ALRM_HR(0x0a)drivers/rtc/rtc-pcf2123.c +3
-0+#define PCF2123_REG_ALRM_DM (0x0b) +#define PCF2123_REG_ALRM_DW (0x0c) +#define PCF2123_REG_OFFSET (0x0d)/* Clock Rate Offset Register */ +#define
PCF2123_REG_TMR_CLKOUT
(0x0e) /* Timer Registers */ +#definePCF2123_REG_CTDWN_TMR(0x0f) +#definePCF2123_REG_MAX(PCF2123_REG_CTDWN_TMR) +drivers/rtc/rtc-pcf2123.c +3
-0+/* PCF2123_REG_CTRL1 BITS */ +#define CTRL1_CLEAR (0x00)/* Clear */ +#define CTRL1_CORRECTION_INT (0x02)/* Correction Interrupt */ +#define
CTRL1_12_HOUR(0x04) /*
12 hour time */ +#defineCTRL1_STOP(0x20) /*
Quoted 10 lines
Quoted 6 lines
Stop the clock */ +#define
CTRL1_SW_RESET
(0x58) /* Software reset */ +#defineCTRL1_EXT_TEST(0x80) /* External Clock Test
mode */ + +/*PCF2123_REG_SCBITS */
+#defineOSC_HAS_STOPPED(0x80) /* Clock has
been stopped */ +Can you use the
BIT()macro?
Sure. I Will change that in v2
Signature · 9 lines
-- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 2/9 rtc-pcf2123: clean up reads from the chip
In reply to Joshua Claytonput read operations into a function.
This makes the starting register more prominent and hides the delay.
--- drivers/rtc/rtc-pcf2123.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-)
drivers/rtc/rtc-pcf2123.c +22 -17
@@ -104,12 +104,26 @@ static inline void pcf2123_delay_trec(void)ndelay (30 ); } +static int pcf2123_read (struct device *dev,u8 reg,u8 *rxbuf,size_t size) +{ +struct spi_device *spi =to_spi_device (dev); +int ret; + +if (reg >PCF2123_REG_MAX ) +return -EFAULT ; + + reg |=PCF2123_READ ; + ret =spi_write_then_read (spi, ®,1 , rxbuf, size); +pcf2123_delay_trec (); + +return ret; +} +static ssize_t pcf2123_show (struct device *dev,struct device_attribute *attr,char *buffer) { -struct spi_device *spi =to_spi_device (dev);struct pcf2123_sysfs_reg *r; -u8 txbuf[1 ], rxbuf[1 ]; +u8 rxbuf[1 ];unsigned long reg;int ret;
@@ -119,11 +133,10 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,if (ret)return ret; - txbuf[0 ] =PCF2123_READ | reg; - ret =spi_write_then_read (spi, txbuf,1 , rxbuf,1 ); + ret =pcf2123_read (dev, reg, rxbuf,1 );if (ret <0 )return -EIO ; -pcf2123_delay_trec (); +return sprintf (buffer,"0x%x\n" , rxbuf[0 ]); }
@@ -158,16 +171,12 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,static int pcf2123_rtc_read_time (struct device *dev,struct rtc_time *tm) { -struct spi_device *spi =to_spi_device (dev); -u8 txbuf[1 ], rxbuf[7 ]; +u8 rxbuf[7 ];int ret; - txbuf[0 ] =PCF2123_READ |PCF2123_REG_SC ; - ret =spi_write_then_read (spi, txbuf,sizeof (txbuf), - rxbuf,sizeof (rxbuf)); + ret =pcf2123_read (dev,PCF2123_REG_SC , rxbuf,sizeof (rxbuf));if (ret <0 )return ret; -pcf2123_delay_trec (); tm->tm_sec =bcd2bin (rxbuf[0 ] &0x7F ); tm->tm_min =bcd2bin (rxbuf[1 ] &0x7F );
@@ -279,16 +288,12 @@ static int pcf2123_probe(struct spi_device *spi)pcf2123_delay_trec ();/* See if the counter was actually stopped */ - txbuf[0 ] =PCF2123_READ |PCF2123_REG_CTRL1 ; -dev_dbg (&spi->dev,"checking for presence of RTC (0x%02X)\n" , - txbuf[0 ]); - ret =spi_write_then_read (spi, txbuf,1 *sizeof (u8), - rxbuf,2 *sizeof (u8)); +dev_dbg (&spi->dev,"checking for presence of RTC\n" ); + ret =pcf2123_read (&spi->dev,PCF2123_REG_CTRL1 , rxbuf,sizeof (rxbuf));dev_dbg (&spi->dev,"received data from RTC (0x%02X 0x%02X)\n" , rxbuf[0 ], rxbuf[1 ]);if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();if (!(rxbuf[0 ] &0x20 )) {dev_err (&spi->dev,"chip not found\n" );
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 3/9 rtc-pcf2123: clean up writes to the rtc chip
In reply to Joshua ClaytonA new function pcf2123_write() hides the delay and the spi device, while pcf2123_write_reg() further simplifies the most common case:
writing a single register.
--- drivers/rtc/rtc-pcf2123.c | 70 +++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 35 deletions(-)
drivers/rtc/rtc-pcf2123.c +35 -35
@@ -119,6 +119,30 @@ static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size)return ret; } +static int pcf2123_write (struct device *dev,u8 *txbuf,size_t size) +{ +struct spi_device *spi =to_spi_device (dev); +int ret; + +if (txbuf[0 ] >PCF2123_REG_MAX ) +return -EFAULT ; + + txbuf[0 ] |=PCF2123_WRITE ; + ret =spi_write (spi, txbuf, size); +pcf2123_delay_trec (); + +return ret; +} + +static int pcf2123_write_reg (struct device *dev,u8 reg,u8 val) +{ +u8 txbuf[2 ]; + + txbuf[0 ] = reg; + txbuf[1 ] = val; +return pcf2123_write (dev, txbuf,sizeof (txbuf)); +} +static ssize_t pcf2123_show (struct device *dev,struct device_attribute *attr,char *buffer) {
@@ -142,9 +166,7 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,static ssize_t pcf2123_store (struct device *dev,struct device_attribute *attr,const char *buffer,size_t count) { -struct spi_device *spi =to_spi_device (dev);struct pcf2123_sysfs_reg *r; -u8 txbuf[2 ];unsigned long reg;unsigned long val;
@@ -160,12 +182,9 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,if (ret)return ret; - txbuf[0 ] =PCF2123_WRITE | reg; - txbuf[1 ] = val; - ret =spi_write (spi, txbuf,sizeof (txbuf)); +pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; -pcf2123_delay_trec ();return count; }
@@ -205,7 +224,6 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)static int pcf2123_rtc_set_time (struct device *dev,struct rtc_time *tm) { -struct spi_device *spi =to_spi_device (dev);u8 txbuf[8 ];int ret;
@@ -216,15 +234,12 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);/* Stop the counter first */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x20 ; - ret =spi_write (spi, txbuf,2 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x20 );if (ret <0 )return ret; -pcf2123_delay_trec ();/* Set the new time */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_SC ; + txbuf[0 ] =PCF2123_REG_SC ; txbuf[1 ] =bin2bcd (tm->tm_sec &0x7F ); txbuf[2 ] =bin2bcd (tm->tm_min &0x7F ); txbuf[3 ] =bin2bcd (tm->tm_hour &0x3F );
@@ -233,18 +248,14 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) txbuf[6 ] =bin2bcd ((tm->tm_mon +1 ) &0x1F );/* rtc mn 1-12 */ txbuf[7 ] =bin2bcd (tm->tm_year <100 ? tm->tm_year : tm->tm_year -100 ); - ret =spi_write (spi, txbuf,sizeof (txbuf)); + ret =pcf2123_write (dev, txbuf,sizeof (txbuf));if (ret <0 )return ret; -pcf2123_delay_trec ();/* Start the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x00 ; - ret =spi_write (spi, txbuf,2 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x00 );if (ret <0 )return ret; -pcf2123_delay_trec ();return 0 ; }
@@ -258,7 +269,7 @@ static int pcf2123_probe(struct spi_device *spi) {struct rtc_device *rtc;struct pcf2123_plat_data *pdata; -u8 txbuf[2 ], rxbuf[2 ]; +u8 rxbuf[2 ];int ret, i; pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
@@ -268,24 +279,16 @@ static int pcf2123_probe(struct spi_device *spi) spi->dev.platform_data = pdata;/* Send a software reset command */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x58 ; -dev_dbg (&spi->dev,"resetting RTC (0x%02X 0x%02X)\n" , - txbuf[0 ], txbuf[1 ]); - ret =spi_write (spi, txbuf,2 *sizeof (u8)); +dev_dbg (&spi->dev,"resetting RTC\n" ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x58 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* Stop the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x20 ; -dev_dbg (&spi->dev,"stopping RTC (0x%02X 0x%02X)\n" , - txbuf[0 ], txbuf[1 ]); - ret =spi_write (spi, txbuf,2 *sizeof (u8)); +dev_dbg (&spi->dev,"stopping RTC\n" ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x20 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* See if the counter was actually stopped */ dev_dbg (&spi->dev,"checking for presence of RTC\n" );
@@ -306,12 +309,9 @@ static int pcf2123_probe(struct spi_device *spi) (spi->max_speed_hz + 500) / 1000);/* Start the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x00 ; - ret =spi_write (spi, txbuf,sizeof (txbuf)); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x00 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* Finalize the initialization */ rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 3/9] rtc-pcf2123: clean up writes to the rtc chip
drivers/rtc/rtc-pcf2123.c static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size) · line 129
Quoted 8 lines · Joshua Clayton
drivers/rtc/rtc-pcf2123.c +8
-0+static int pcf2123_write (struct device *dev,u8 *txbuf,size_t size) +{ +struct spi_device *spi =to_spi_device (dev); +int ret; + +if (txbuf[0 ] >PCF2123_REG_MAX ) +return -EFAULT ; +
Is that test really necessary? From what I understand the driver always controls which register is written.
Quoted 145 lines
drivers/rtc/rtc-pcf2123.c +27
-35+ txbuf[0 ] |=PCF2123_WRITE ; + ret =spi_write (spi, txbuf, size); +pcf2123_delay_trec (); + +return ret; +} + +static int pcf2123_write_reg (struct device *dev,u8 reg,u8 val) +{ +u8 txbuf[2 ]; + + txbuf[0 ] = reg; + txbuf[1 ] = val; +return pcf2123_write (dev, txbuf,sizeof (txbuf)); +} +static ssize_t pcf2123_show (struct device *dev,struct device_attribute *attr,char *buffer) {@@ -142,9 +166,7 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,static ssize_t pcf2123_store (struct device *dev,struct device_attribute *attr,const char *buffer,size_t count) { -struct spi_device *spi =to_spi_device (dev);struct pcf2123_sysfs_reg *r; -u8 txbuf[2 ];unsigned long reg;unsigned long val;@@ -160,12 +182,9 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,if (ret)return ret; - txbuf[0 ] =PCF2123_WRITE | reg; - txbuf[1 ] = val; - ret =spi_write (spi, txbuf,sizeof (txbuf)); +pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; -pcf2123_delay_trec ();return count; }@@ -205,7 +224,6 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)static int pcf2123_rtc_set_time (struct device *dev,struct rtc_time *tm) { -struct spi_device *spi =to_spi_device (dev);u8 txbuf[8 ];int ret;@@ -216,15 +234,12 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);/* Stop the counter first */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x20 ; - ret =spi_write (spi, txbuf,2 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x20 );if (ret <0 )return ret; -pcf2123_delay_trec ();/* Set the new time */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_SC ; + txbuf[0 ] =PCF2123_REG_SC ; txbuf[1 ] =bin2bcd (tm->tm_sec &0x7F ); txbuf[2 ] =bin2bcd (tm->tm_min &0x7F ); txbuf[3 ] =bin2bcd (tm->tm_hour &0x3F );@@ -233,18 +248,14 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) txbuf[6 ] =bin2bcd ((tm->tm_mon +1 ) &0x1F );/* rtc mn 1-12 */ txbuf[7 ] =bin2bcd (tm->tm_year <100 ? tm->tm_year : tm->tm_year -100 ); - ret =spi_write (spi, txbuf,sizeof (txbuf)); + ret =pcf2123_write (dev, txbuf,sizeof (txbuf));if (ret <0 )return ret; -pcf2123_delay_trec ();/* Start the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x00 ; - ret =spi_write (spi, txbuf,2 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x00 );if (ret <0 )return ret; -pcf2123_delay_trec ();return 0 ; }@@ -258,7 +269,7 @@ static int pcf2123_probe(struct spi_device *spi) {struct rtc_device *rtc;struct pcf2123_plat_data *pdata; -u8 txbuf[2 ], rxbuf[2 ]; +u8 rxbuf[2 ];int ret, i; pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),@@ -268,24 +279,16 @@ static int pcf2123_probe(struct spi_device *spi) spi->dev.platform_data = pdata;/* Send a software reset command */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x58 ; -dev_dbg (&spi->dev,"resetting RTC (0x%02X 0x%02X)\n" , - txbuf[0 ], txbuf[1 ]); - ret =spi_write (spi, txbuf,2 *sizeof (u8)); +dev_dbg (&spi->dev,"resetting RTC\n" ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x58 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* Stop the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x20 ; -dev_dbg (&spi->dev,"stopping RTC (0x%02X 0x%02X)\n" , - txbuf[0 ], txbuf[1 ]); - ret =spi_write (spi, txbuf,2 *sizeof (u8)); +dev_dbg (&spi->dev,"stopping RTC\n" ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x20 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* See if the counter was actually stopped */ dev_dbg (&spi->dev,"checking for presence of RTC\n" );@@ -306,12 +309,9 @@ static int pcf2123_probe(struct spi_device *spi) (spi->max_speed_hz + 500) / 1000);/* Start the counter */ - txbuf[0 ] =PCF2123_WRITE |PCF2123_REG_CTRL1 ; - txbuf[1 ] =0x00 ; - ret =spi_write (spi, txbuf,sizeof (txbuf)); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x00 );if (ret <0 ) goto kfree_exit; -pcf2123_delay_trec ();/* Finalize the initialization */ rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,Signature · 1 line
2.5.0
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 3/9] rtc-pcf2123: clean up writes to the rtc chip
Quoted 13 lines · On Tue, 24 Nov 2015 23:16:26 +0100 Alexandre Belloni…
drivers/rtc/rtc-pcf2123.c static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size) · line 129
Quoted 8 lines · Joshua Clayton
+static int
pcf2123_write(struct device *dev, u8 *txbuf, size_t size)+{drivers/rtc/rtc-pcf2123.c +6
-0+struct spi_device *spi =to_spi_device (dev); +int ret; + +if (txbuf[0 ] >PCF2123_REG_MAX ) +return -EFAULT ; +Is that test really necessary? From what I understand the driver always controls which register is written.
In the larger context of the driver, you are correct, there is no way for an out of range request unless someone were to add new code. I can remove it.
Signature · 9 lines
-- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 4/9 rtc-pcf2123: replace magic numbers with defines
In reply to Joshua ClaytonAll of the magic numbers pertain to register CTRL1. document all the values in CTRL1 with defines from the datasheet
--- drivers/rtc/rtc-pcf2123.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/rtc/rtc-pcf2123.c +6 -6
@@ -234,7 +234,7 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);/* Stop the counter first */ - ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x20 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_STOP );if (ret <0 )return ret;
@@ -253,7 +253,7 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)return ret;/* Start the counter */ - ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,0x00 ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_CLEAR );if (ret <0 )return ret;
@@ -280,13 +280,13 @@ static int pcf2123_probe(struct spi_device *spi)/* Send a software reset command */ dev_dbg (&spi->dev,"resetting RTC\n" ); - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x58 ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_SW_RESET );if (ret <0 ) goto kfree_exit;/* Stop the counter */ dev_dbg (&spi->dev,"stopping RTC\n" ); - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x20 ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_STOP );if (ret <0 ) goto kfree_exit;
@@ -298,7 +298,7 @@ static int pcf2123_probe(struct spi_device *spi)if (ret <0 ) goto kfree_exit; -if (!(rxbuf[0 ] &0x20 )) { +if (!(rxbuf[0 ] &CTRL1_STOP )) {dev_err (&spi->dev,"chip not found\n" ); ret = -ENODEV ; goto kfree_exit;
@@ -309,7 +309,7 @@ static int pcf2123_probe(struct spi_device *spi) (spi->max_speed_hz + 500) / 1000);/* Start the counter */ - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,0x00 ); + ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_CLEAR );if (ret <0 ) goto kfree_exit;
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 5/9 rtc-pcf2123: put the chip reset into a function
In reply to Joshua Clayton--- drivers/rtc/rtc-pcf2123.c | 64 ++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 28 deletions(-)
drivers/rtc/rtc-pcf2123.c +36 -28
@@ -260,6 +260,40 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)return 0 ; } +static int pcf2123_reset (struct device *dev) +{ +int ret; +u8 rxbuf[2 ]; + + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_SW_RESET ); +if (ret <0 ) +return ret; + +/* Stop the counter */ +dev_dbg (dev,"stopping RTC\n" ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_STOP ); +if (ret <0 ) +return ret; + +/* See if the counter was actually stopped */ +dev_dbg (dev,"checking for presence of RTC\n" ); + ret =pcf2123_read (dev,PCF2123_REG_CTRL1 , rxbuf,sizeof (rxbuf)); +if (ret <0 ) +return ret; + +dev_dbg (dev,"received data from RTC (0x%02X 0x%02X)\n" , + rxbuf[0 ], rxbuf[1 ]); +if (!(rxbuf[0 ] &CTRL1_STOP )) +return -ENODEV ; + +/* Start the counter */ + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_CLEAR ); +if (ret <0 ) +return ret; + +return 0 ; +} +static const struct rtc_class_ops pcf2123_rtc_ops = { .read_time = pcf2123_rtc_read_time, .set_time = pcf2123_rtc_set_time,
@@ -269,7 +303,6 @@ static int pcf2123_probe(struct spi_device *spi) {struct rtc_device *rtc;struct pcf2123_plat_data *pdata; -u8 rxbuf[2 ];int ret, i; pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
@@ -278,29 +311,9 @@ static int pcf2123_probe(struct spi_device *spi)return -ENOMEM ; spi->dev.platform_data = pdata; -/* Send a software reset command */ -dev_dbg (&spi->dev,"resetting RTC\n" ); - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_SW_RESET ); -if (ret <0 ) - goto kfree_exit; - -/* Stop the counter */ -dev_dbg (&spi->dev,"stopping RTC\n" ); - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_STOP ); -if (ret <0 ) - goto kfree_exit; - -/* See if the counter was actually stopped */ -dev_dbg (&spi->dev,"checking for presence of RTC\n" ); - ret =pcf2123_read (&spi->dev,PCF2123_REG_CTRL1 , rxbuf,sizeof (rxbuf)); -dev_dbg (&spi->dev,"received data from RTC (0x%02X 0x%02X)\n" , - rxbuf[0 ], rxbuf[1 ]); -if (ret <0 ) - goto kfree_exit; - -if (!(rxbuf[0 ] &CTRL1_STOP )) { + ret =pcf2123_reset (&spi->dev); +if (ret <0 ) {dev_err (&spi->dev,"chip not found\n" ); - ret = -ENODEV ; goto kfree_exit; }
@@ -308,11 +321,6 @@ static int pcf2123_probe(struct spi_device *spi)dev_info (&spi->dev,"spiclk %u KHz.\n" , (spi->max_speed_hz +500 ) /1000 ); -/* Start the counter */ - ret =pcf2123_write_reg (&spi->dev,PCF2123_REG_CTRL1 ,CTRL1_CLEAR ); -if (ret <0 ) - goto kfree_exit; -/* Finalize the initialization */ rtc =devm_rtc_device_register (&spi->dev, pcf2123_driver.driver.name, &pcf2123_rtc_ops,THIS_MODULE );
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 5/9] rtc-pcf2123: put the chip reset into a function
On 04/11/2015 at 07:36:36 -0800, Joshua Clayton wrote :
A commit message, even when simple is mandatory.
Quoted 26 lines
Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 64 ++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 28 deletions(-)drivers/rtc/rtc-pcf2123.c +13
-0@@ -260,6 +260,40 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)return 0 ; } +static int pcf2123_reset (struct device *dev) +{ +int ret; +u8 rxbuf[2 ]; + + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_SW_RESET ); +if (ret <0 ) +return ret; + +/* Stop the counter */ +dev_dbg (dev,"stopping RTC\n" ); + ret =pcf2123_write_reg (dev,PCF2123_REG_CTRL1 ,CTRL1_STOP ); +if (ret <0 )
drivers/rtc/rtc-pcf2123.c static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 285
Quoted 10 lines
drivers/rtc/rtc-pcf2123.c +10
-0+return ret; + +/* See if the counter was actually stopped */ +dev_dbg (dev,"checking for presence of RTC\n" ); + ret =pcf2123_read (dev,PCF2123_REG_CTRL1 , rxbuf,sizeof (rxbuf)); +if (ret <0 ) +return ret; + +dev_dbg (dev,"received data from RTC (0x%02X 0x%02X)\n" , + rxbuf[0 ], rxbuf[1 ]);
This is not properly aligned (use checkpatch --strict)
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 5/9] rtc-pcf2123: put the chip reset into a function
Quoted 4 lines · On Wed, 25 Nov 2015 00:17:36 +0100 Alexandre Belloni…
On 04/11/2015 at 07:36:36 -0800, Joshua Clayton wrote :
A commit message, even when simple is mandatory.
Sorry about that. I know better. will fix for v2
Quoted 13 lines
drivers/rtc/rtc-pcf2123.c static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 285
Quoted 9 lines
drivers/rtc/rtc-pcf2123.c +3
-0+/* See if the counter was actually stopped */ +dev_dbg (dev,"checking for presence of RTC\n" ); + ret = pcf2123_read(dev, PCF2123_REG_CTRL1, rxbuf,sizeof(rxbuf));
drivers/rtc/rtc-pcf2123.c +5
-0+if (ret <0 ) +return ret; + +dev_dbg (dev,"received data from RTC (0x%02X 0x%02X)\n" , + rxbuf[0 ], rxbuf[1 ]);This is not properly aligned (use checkpatch --strict)
Will fix.
Signature · 9 lines
-- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 6/9 rtc-pcf2123: avoid resetting the clock if possible
In reply to Joshua Claytonpcf2123 data sheet recommends a software reset when the chip is first powered on. This change avoids resetting the chip every time the driver is loaded, which has some negative effects.
There are several registers including a clock rate adjustment that really should survive a reload of the driver (or reboot).
In addition, stopping and restarting the clock to verify the chip is there is not a good thing once the time is set.
According to the data sheet, the seconds register has a 1 in the high bit when the voltage has gotten low. We check for this condition, as well as whether the time retrieved from the chip is valid. We reset the rtc only if the time is not reliable and valid. This is sufficient for checking for the presence of the chip, as either all zeros or all 0xff will result in an invalid time/date
--- drivers/rtc/rtc-pcf2123.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-)
drivers/rtc/rtc-pcf2123.c +33 -4
@@ -260,6 +260,33 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)return 0 ; } +static bool pcf2123_time_valid (struct device *dev) +{ +u8 seconds; +struct rtc_time tm; +int ret; + + ret =pcf2123_read (dev,PCF2123_REG_SC , &seconds,1 ); +if (ret <0 ) +return false; + +if (seconds &OSC_HAS_STOPPED ) { +dev_info (dev,"clock was stopped. seconds: 0x%02X\n" , seconds); +return false; + } + + ret =pcf2123_rtc_read_time (dev, &tm); +if (ret <0 ) +return false; + +if (rtc_valid_tm (&tm) <0 ) { +dev_err (dev,"retrieved date/time is not valid.\n" ); +return false; + } + +return true; +} +static int pcf2123_reset (struct device *dev) {int ret;
@@ -311,10 +338,12 @@ static int pcf2123_probe(struct spi_device *spi)return -ENOMEM ; spi->dev.platform_data = pdata; - ret =pcf2123_reset (&spi->dev); -if (ret <0 ) { -dev_err (&spi->dev,"chip not found\n" ); - goto kfree_exit; +if (!pcf2123_time_valid (&spi->dev)) { + ret =pcf2123_reset (&spi->dev); +if (ret <0 ) { +dev_err (&spi->dev,"chip not found\n" ); + goto kfree_exit; + } }dev_info (&spi->dev,"chip found, driver version " DRV_VERSION "\n" );
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible
drivers/rtc/rtc-pcf2123.c static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 286
Quoted 9 lines · Joshua Clayton
drivers/rtc/rtc-pcf2123.c +9
-0+ ret =pcf2123_rtc_read_time (dev, &tm); +if (ret <0 ) +return false; + +if (rtc_valid_tm (&tm) <0 ) { +dev_err (dev,"retrieved date/time is not valid.\n" ); +return false; + } +
I would remove that test as basically, the date/time will only be valid when OSC_HAS_STOPPED is not set.
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible
Quoted 15 lines · On Wed, 25 Nov 2015 00:25:12 +0100 Alexandre Belloni…
drivers/rtc/rtc-pcf2123.c static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 277
Quoted 10 lines · Joshua Clayton
drivers/rtc/rtc-pcf2123.c +6
-0+ ret =pcf2123_rtc_read_time (dev, &tm); +if (ret <0 ) +return false; + +if (rtc_valid_tm (&tm) <0 ) { + dev_err(dev, "retrieved date/time is notvalid.\n");
drivers/rtc/rtc-pcf2123.c +3
-0+return false; + } +I would remove that test as basically, the date/time will only be valid when
OSC_HAS_STOPPEDis not set.
OSC_HAS_STOPPED really only protects us in case everything else looks good, but we've lost power.
There are other reasons to check for a valid time. Specifically, if there is no communication with the device, the spi operation will succeed and return all zeros or all ones. Since either of these results in an invalid time, it is a nice way to probe whether we really have a pcf2123 compatible device.
I don't think I should remove this test, but I can add a comment
Signature · 9 lines
-- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible
Quoted 26 lines · Joshua Clayton
Quoted 15 lines · On Wed, 25 Nov 2015 00:25:12 +0100 Alexandre Belloni…
drivers/rtc/rtc-pcf2123.c static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 277
Quoted 10 lines · Joshua Clayton
drivers/rtc/rtc-pcf2123.c +6
-0+ ret =pcf2123_rtc_read_time (dev, &tm); +if (ret <0 ) +return false; + +if (rtc_valid_tm (&tm) <0 ) { + dev_err(dev, "retrieved date/time is notvalid.\n");
drivers/rtc/rtc-pcf2123.c +3
-0+return false; + } +I would remove that test as basically, the date/time will only be valid when
OSC_HAS_STOPPEDis not set.
OSC_HAS_STOPPEDreally only protects us in case everything else looks good, but we've lost power.
There are other reasons to check for a valid time. Specifically, if there is no communication with the device, the spi operation will succeed and return all zeros or all ones. Since either of these results in an invalid time, it is a nice way to probe whether we really have a pcf2123 compatible device.
Quoted 1 line
I don't think I should remove this test, but I can add a comment
OK but then pcf2123_rtc_read_time actually returns rtc_valid_tm(tm) ;)
The proper course of action is probably to do the OSC_HAS_STOPPED check in pcf2123_rtc_read_time then you don't even need pcf2123_time_valid().
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 7/9 rtc-pcf2123: allow sysfs to accept hexidecimal
In reply to Joshua Claytonpcf2123 registers store their values in bcd.
sysfs sensibly displays them in hexidecimal.
Up to now, the sysfs store functions only accept base 10, which makes no sense.
Add support for hexidecimal without removing base10 support.
--- drivers/rtc/rtc-pcf2123.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rtc/rtc-pcf2123.c +1 -1
@@ -178,7 +178,7 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,if (ret)return ret; - ret =kstrtoul (buffer,10 , &val); + ret =kstrtoul (buffer,0 , &val);if (ret)return ret;
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 8/9 rtc-pcf2123: use sysfs groups
In reply to Joshua ClaytonSwitch from manually creating all the sysfs attributes to defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove the group manually, but everythig else is done by The Book(tm) .
--- drivers/rtc/rtc-pcf2123.c | 118 +++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 63 deletions(-)
drivers/rtc/rtc-pcf2123.c +55 -63
@@ -84,16 +84,6 @@static struct spi_driver pcf2123_driver; -struct pcf2123_sysfs_reg { -struct device_attribute attr; -char name[2 ]; -}; - -struct pcf2123_plat_data { -struct rtc_device *rtc; -struct pcf2123_sysfs_reg regs[16 ]; -}; -/* * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select * is released properly after an SPI write. This function should be
@@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)static ssize_t pcf2123_show (struct device *dev,struct device_attribute *attr,char *buffer) { -struct pcf2123_sysfs_reg *r;u8 rxbuf[1 ];unsigned long reg;int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - + reg =hex_to_bin (attr->attr.name[0 ]); ret =pcf2123_read (dev, reg, rxbuf,1 );if (ret <0 )return -EIO ;
@@ -166,25 +150,19 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,static ssize_t pcf2123_store (struct device *dev,struct device_attribute *attr,const char *buffer,size_t count) { -struct pcf2123_sysfs_reg *r;unsigned long reg;unsigned long val; -int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - ret =kstrtoul (buffer,0 , &val);if (ret)return ret; + reg =hex_to_bin (attr->attr.name[0 ]);pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; +return count; }
@@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = { .set_time = pcf2123_rtc_set_time, }; +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); + +static struct attribute *pcf2123_attrs[] = { + &dev_attr_0.attr, + &dev_attr_1.attr, + &dev_attr_2.attr, + &dev_attr_3.attr, + &dev_attr_4.attr, + &dev_attr_5.attr, + &dev_attr_6.attr, + &dev_attr_7.attr, + &dev_attr_8.attr, + &dev_attr_9.attr, + &dev_attr_a.attr, + &dev_attr_b.attr, + &dev_attr_c.attr, + &dev_attr_d.attr, + &dev_attr_e.attr, + &dev_attr_f.attr, +NULL +}; + +static const struct attribute_group pcf2123_group = { + .attrs = pcf2123_attrs, +}; + +static const struct attribute_group *pcf2123_groups[] = { + &pcf2123_group, +NULL +}; +static int pcf2123_probe (struct spi_device *spi) {struct rtc_device *rtc; -struct pcf2123_plat_data *pdata; -int ret, i; - - pdata =devm_kzalloc (&spi->dev,sizeof (struct pcf2123_plat_data ), -GFP_KERNEL ); -if (!pdata) -return -ENOMEM ; - spi->dev.platform_data = pdata; +int ret;if (!pcf2123_time_valid (&spi->dev)) { ret =pcf2123_reset (&spi->dev);
@@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi) goto kfree_exit; } - pdata->rtc = rtc; - -for (i =0 ; i <16 ; i++) { -sysfs_attr_init (&pdata->regs[i].attr.attr); -sprintf (pdata->regs[i].name,"%1x" , i); - pdata->regs[i].attr.attr.mode =S_IRUGO |S_IWUSR ; - pdata->regs[i].attr.attr.name = pdata->regs[i].name; - pdata->regs[i].attr.show = pcf2123_show; - pdata->regs[i].attr.store = pcf2123_store; - ret =device_create_file (&spi->dev, &pdata->regs[i].attr); -if (ret) { -dev_err (&spi->dev,"Unable to create sysfs %s\n" , - pdata->regs[i].name); - goto sysfs_exit; - } - } + ret =sysfs_create_groups (&spi->dev.kobj, pcf2123_groups); +if (ret) + goto sysfs_exit;return 0 ; - sysfs_exit: -for (i--; i >=0 ; i--) -device_remove_file (&spi->dev, &pdata->regs[i].attr); - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups); kfree_exit: spi->dev.platform_data =NULL ;return ret;
@@ -390,16 +391,7 @@ kfree_exit:static int pcf2123_remove (struct spi_device *spi) { -struct pcf2123_plat_data *pdata =dev_get_platdata (&spi->dev); -int i; - -if (pdata) { -for (i =0 ; i <16 ; i++) -if (pdata->regs[i].name[0 ]) -device_remove_file (&spi->dev, - &pdata->regs[i].attr); - } - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups);return 0 ; }
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups
Quoted 4 lines · Joshua Clayton
Switch from manually creating all the sysfs attributes to defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove the group manually, but everythig else is done by The Book(tm) .
s/everythig/everything/
Signature · 12 lines
~Joshua Clayton -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups
In reply to Joshua ClaytonHi,
This is okay but I'm not actually sure about the usefulness of that interface. The driver is exactly here to abstract access to the registers. Limit it to registers that are not used by the driver? Also, It would probably better live in debugfs rather than in sysfs.
Quoted 198 lines · Joshua Clayton
Switch from manually creating all the sysfs attributes to defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove the group manually, but everythig else is done by The Book(tm) .Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 118 +++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 63 deletions(-)drivers/rtc/rtc-pcf2123.c +55
-63@@ -84,16 +84,6 @@static struct spi_driver pcf2123_driver; -struct pcf2123_sysfs_reg { -struct device_attribute attr; -char name[2 ]; -}; - -struct pcf2123_plat_data { -struct rtc_device *rtc; -struct pcf2123_sysfs_reg regs[16 ]; -}; -/* * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select * is released properly after an SPI write. This function should be @@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)static ssize_t pcf2123_show (struct device *dev,struct device_attribute *attr,char *buffer) { -struct pcf2123_sysfs_reg *r;u8 rxbuf[1 ];unsigned long reg;int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - + reg =hex_to_bin (attr->attr.name[0 ]); ret =pcf2123_read (dev, reg, rxbuf,1 );if (ret <0 )return -EIO ;@@ -166,25 +150,19 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,static ssize_t pcf2123_store (struct device *dev,struct device_attribute *attr,const char *buffer,size_t count) { -struct pcf2123_sysfs_reg *r;unsigned long reg;unsigned long val; -int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - ret =kstrtoul (buffer,0 , &val);if (ret)return ret; + reg =hex_to_bin (attr->attr.name[0 ]);pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; +return count; }@@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = { .set_time = pcf2123_rtc_set_time, }; +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); + +static struct attribute *pcf2123_attrs[] = { + &dev_attr_0.attr, + &dev_attr_1.attr, + &dev_attr_2.attr, + &dev_attr_3.attr, + &dev_attr_4.attr, + &dev_attr_5.attr, + &dev_attr_6.attr, + &dev_attr_7.attr, + &dev_attr_8.attr, + &dev_attr_9.attr, + &dev_attr_a.attr, + &dev_attr_b.attr, + &dev_attr_c.attr, + &dev_attr_d.attr, + &dev_attr_e.attr, + &dev_attr_f.attr, +NULL +}; + +static const struct attribute_group pcf2123_group = { + .attrs = pcf2123_attrs, +}; + +static const struct attribute_group *pcf2123_groups[] = { + &pcf2123_group, +NULL +}; +static int pcf2123_probe (struct spi_device *spi) {struct rtc_device *rtc; -struct pcf2123_plat_data *pdata; -int ret, i; - - pdata =devm_kzalloc (&spi->dev,sizeof (struct pcf2123_plat_data ), -GFP_KERNEL ); -if (!pdata) -return -ENOMEM ; - spi->dev.platform_data = pdata; +int ret;if (!pcf2123_time_valid (&spi->dev)) { ret =pcf2123_reset (&spi->dev);@@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi) goto kfree_exit; } - pdata->rtc = rtc; - -for (i =0 ; i <16 ; i++) { -sysfs_attr_init (&pdata->regs[i].attr.attr); -sprintf (pdata->regs[i].name,"%1x" , i); - pdata->regs[i].attr.attr.mode =S_IRUGO |S_IWUSR ; - pdata->regs[i].attr.attr.name = pdata->regs[i].name; - pdata->regs[i].attr.show = pcf2123_show; - pdata->regs[i].attr.store = pcf2123_store; - ret =device_create_file (&spi->dev, &pdata->regs[i].attr); -if (ret) { -dev_err (&spi->dev,"Unable to create sysfs %s\n" , - pdata->regs[i].name); - goto sysfs_exit; - } - } + ret =sysfs_create_groups (&spi->dev.kobj, pcf2123_groups); +if (ret) + goto sysfs_exit;return 0 ; - sysfs_exit: -for (i--; i >=0 ; i--) -device_remove_file (&spi->dev, &pdata->regs[i].attr); - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups); kfree_exit: spi->dev.platform_data =NULL ;return ret;@@ -390,16 +391,7 @@ kfree_exit:static int pcf2123_remove (struct spi_device *spi) { -struct pcf2123_plat_data *pdata =dev_get_platdata (&spi->dev); -int i; - -if (pdata) { -for (i =0 ; i <16 ; i++) -if (pdata->regs[i].name[0 ]) -device_remove_file (&spi->dev, - &pdata->regs[i].attr); - } - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups);return 0 ; }Signature · 1 line
2.5.0
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups
Quoted 7 lines · On Wed, 25 Nov 2015 00:31:01 +0100 Alexandre Belloni…
Hi,
This is okay but I'm not actually sure about the usefulness of that interface. The driver is exactly here to abstract access to the registers. Limit it to registers that are not used by the driver? Also, It would probably better live in debugfs rather than in sysfs.
I agree completely... can I do that? Can I just move them to debugfs or remove them?
I ask because these sysfs register files are in mainline already, and I know removing user facing kernel features is usually frowned on.
Quoted 210 lines · Joshua Clayton
Quoted 208 lines
Switch from manually creating all the sysfs attributes to defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove the group manually, but everythig else is done by The Book(tm) .Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 118+++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 63 deletions(-)
drivers/rtc/rtc-pcf2123.c +0
-10@@ -84,16 +84,6 @@static struct spi_driver pcf2123_driver; -struct pcf2123_sysfs_reg { -struct device_attribute attr; -char name[2 ]; -}; - -struct pcf2123_plat_data { -struct rtc_device *rtc; -struct pcf2123_sysfs_reg regs[16 ]; -}; -/* * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
* is released properly after an SPI write. This function should
be @@ -146,17 +136,11 @@ static intpcf2123_write_reg(struct device *dev, u8 reg, u8 val)staticssize_tpcf2123_show(struct device *dev, struct device_attribute *attr, char *buffer)
{drivers/rtc/rtc-pcf2123.c +4
-15-struct pcf2123_sysfs_reg *r;u8 rxbuf[1 ];unsigned long reg;int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - + reg =hex_to_bin (attr->attr.name[0 ]); ret =pcf2123_read (dev, reg, rxbuf,1 );if (ret <0 )return -EIO ;@@ -166,25 +150,19 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr, static ssize_t pcf2123_store(struct device *dev, structdevice_attribute *attr,const char *buffer,size_t count) { -struct pcf2123_sysfs_reg *r;unsigned long reg;unsigned long val; -int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - ret =kstrtoul (buffer,0 , &val);if (ret)return ret; + reg =hex_to_bin (attr->attr.name[0 ]);pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; +return count; }@@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = { .set_time = pcf2123_rtc_set_time, }; +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR,
pcf2123_show,pcf2123_store); +staticDEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); + +static struct attribute *pcf2123_attrs[] = {drivers/rtc/rtc-pcf2123.c +28
-4+ &dev_attr_0.attr, + &dev_attr_1.attr, + &dev_attr_2.attr, + &dev_attr_3.attr, + &dev_attr_4.attr, + &dev_attr_5.attr, + &dev_attr_6.attr, + &dev_attr_7.attr, + &dev_attr_8.attr, + &dev_attr_9.attr, + &dev_attr_a.attr, + &dev_attr_b.attr, + &dev_attr_c.attr, + &dev_attr_d.attr, + &dev_attr_e.attr, + &dev_attr_f.attr, +NULL +}; + +static const struct attribute_group pcf2123_group = { + .attrs = pcf2123_attrs, +}; + +static const struct attribute_group *pcf2123_groups[] = { + &pcf2123_group, +NULL +}; +static int pcf2123_probe (struct spi_device *spi) {struct rtc_device *rtc; -struct pcf2123_plat_data *pdata; -int ret, i; - - pdata = devm_kzalloc(&spi->dev, sizeof(struct
pcf2123_plat_data),drivers/rtc/rtc-pcf2123.c +4
-23- GFP_KERNEL); -if (!pdata) -return -ENOMEM ; - spi->dev.platform_data = pdata; +int ret;if (!pcf2123_time_valid (&spi->dev)) { ret =pcf2123_reset (&spi->dev);@@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi) goto kfree_exit; } - pdata->rtc = rtc; - -for (i =0 ; i <16 ; i++) { -sysfs_attr_init (&pdata->regs[i].attr.attr); -sprintf (pdata->regs[i].name,"%1x" , i); - pdata->regs[i].attr.attr.mode =S_IRUGO |S_IWUSR ; - pdata->regs[i].attr.attr.name = pdata->regs[i].name; - pdata->regs[i].attr.show = pcf2123_show; - pdata->regs[i].attr.store = pcf2123_store; - ret =device_create_file (&spi->dev, &pdata->regs[i].attr); -if (ret) { - dev_err(&spi->dev, "Unable to create sysfs %s\n", - pdata->regs[i].name); - goto sysfs_exit; - } - } + ret =sysfs_create_groups (&spi->dev.kobj, pcf2123_groups); +if (ret) + goto sysfs_exit;return 0 ; - sysfs_exit: -for (i--; i >=0 ; i--) - device_remove_file(&spi->dev,&pdata->regs[i].attr); -
drivers/rtc/rtc-pcf2123.c +2
-10+sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups); kfree_exit: spi->dev.platform_data =NULL ;return ret;@@ -390,16 +391,7 @@ kfree_exit:static int pcf2123_remove (struct spi_device *spi) { -struct pcf2123_plat_data *pdata =dev_get_platdata (&spi->dev); -int i; - -if (pdata) { -for (i =0 ; i <16 ; i++) -if (pdata->regs[i].name[0 ]) -device_remove_file (&spi->dev, - &pdata->regs[i].attr); - } - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups);return 0 ;}
Signature · 1 line
2.5.0
Signature · 9 lines
-- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups
Quoted 12 lines · Joshua Clayton
Quoted 7 lines · On Wed, 25 Nov 2015 00:31:01 +0100 Alexandre Belloni…
Hi,
This is okay but I'm not actually sure about the usefulness of that interface. The driver is exactly here to abstract access to the registers. Limit it to registers that are not used by the driver? Also, It would probably better live in debugfs rather than in sysfs.
Quoted 5 lines
I agree completely... can I do that? Can I just move them to debugfs or remove them?
I ask because these sysfs register files are in mainline already, and I know removing user facing kernel features is usually frowned on.
My guess is that they haven't been documented anyway. If that is the case, I wouldn't mind removing them. My thinking is that using them in a production system is totally insane.
Quoted 211 lines
Quoted 210 lines · Joshua Clayton
Quoted 208 lines
Switch from manually creating all the sysfs attributes to defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove the group manually, but everythig else is done by The Book(tm) .Signed-off-by Joshua Clayton <[email protected]>--- drivers/rtc/rtc-pcf2123.c | 118+++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 63 deletions(-)
drivers/rtc/rtc-pcf2123.c +0
-10@@ -84,16 +84,6 @@static struct spi_driver pcf2123_driver; -struct pcf2123_sysfs_reg { -struct device_attribute attr; -char name[2 ]; -}; - -struct pcf2123_plat_data { -struct rtc_device *rtc; -struct pcf2123_sysfs_reg regs[16 ]; -}; -/* * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
* is released properly after an SPI write. This function should
be @@ -146,17 +136,11 @@ static intpcf2123_write_reg(struct device *dev, u8 reg, u8 val)staticssize_tpcf2123_show(struct device *dev, struct device_attribute *attr, char *buffer)
{drivers/rtc/rtc-pcf2123.c +4
-15-struct pcf2123_sysfs_reg *r;u8 rxbuf[1 ];unsigned long reg;int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - + reg =hex_to_bin (attr->attr.name[0 ]); ret =pcf2123_read (dev, reg, rxbuf,1 );if (ret <0 )return -EIO ;@@ -166,25 +150,19 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr, static ssize_t pcf2123_store(struct device *dev, structdevice_attribute *attr,const char *buffer,size_t count) { -struct pcf2123_sysfs_reg *r;unsigned long reg;unsigned long val; -int ret; - r = container_of(attr, struct pcf2123_sysfs_reg, attr); - - ret =kstrtoul (r->name,16 , ®); -if (ret) -return ret; - ret =kstrtoul (buffer,0 , &val);if (ret)return ret; + reg =hex_to_bin (attr->attr.name[0 ]);pcf2123_write_reg (dev, reg, val);if (ret <0 )return -EIO ; +return count; }@@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = { .set_time = pcf2123_rtc_set_time, }; +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR,
pcf2123_show,pcf2123_store); +staticDEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +staticDEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); + +static struct attribute *pcf2123_attrs[] = {drivers/rtc/rtc-pcf2123.c +28
-4+ &dev_attr_0.attr, + &dev_attr_1.attr, + &dev_attr_2.attr, + &dev_attr_3.attr, + &dev_attr_4.attr, + &dev_attr_5.attr, + &dev_attr_6.attr, + &dev_attr_7.attr, + &dev_attr_8.attr, + &dev_attr_9.attr, + &dev_attr_a.attr, + &dev_attr_b.attr, + &dev_attr_c.attr, + &dev_attr_d.attr, + &dev_attr_e.attr, + &dev_attr_f.attr, +NULL +}; + +static const struct attribute_group pcf2123_group = { + .attrs = pcf2123_attrs, +}; + +static const struct attribute_group *pcf2123_groups[] = { + &pcf2123_group, +NULL +}; +static int pcf2123_probe (struct spi_device *spi) {struct rtc_device *rtc; -struct pcf2123_plat_data *pdata; -int ret, i; - - pdata = devm_kzalloc(&spi->dev, sizeof(struct
pcf2123_plat_data),drivers/rtc/rtc-pcf2123.c +4
-23- GFP_KERNEL); -if (!pdata) -return -ENOMEM ; - spi->dev.platform_data = pdata; +int ret;if (!pcf2123_time_valid (&spi->dev)) { ret =pcf2123_reset (&spi->dev);@@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi) goto kfree_exit; } - pdata->rtc = rtc; - -for (i =0 ; i <16 ; i++) { -sysfs_attr_init (&pdata->regs[i].attr.attr); -sprintf (pdata->regs[i].name,"%1x" , i); - pdata->regs[i].attr.attr.mode =S_IRUGO |S_IWUSR ; - pdata->regs[i].attr.attr.name = pdata->regs[i].name; - pdata->regs[i].attr.show = pcf2123_show; - pdata->regs[i].attr.store = pcf2123_store; - ret =device_create_file (&spi->dev, &pdata->regs[i].attr); -if (ret) { - dev_err(&spi->dev, "Unable to create sysfs %s\n", - pdata->regs[i].name); - goto sysfs_exit; - } - } + ret =sysfs_create_groups (&spi->dev.kobj, pcf2123_groups); +if (ret) + goto sysfs_exit;return 0 ; - sysfs_exit: -for (i--; i >=0 ; i--) - device_remove_file(&spi->dev,&pdata->regs[i].attr); -
drivers/rtc/rtc-pcf2123.c +2
-10+sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups); kfree_exit: spi->dev.platform_data =NULL ;return ret;@@ -390,16 +391,7 @@ kfree_exit:static int pcf2123_remove (struct spi_device *spi) { -struct pcf2123_plat_data *pdata =dev_get_platdata (&spi->dev); -int i; - -if (pdata) { -for (i =0 ; i <16 ; i++) -if (pdata->regs[i].name[0 ]) -device_remove_file (&spi->dev, - &pdata->regs[i].attr); - } - +sysfs_remove_groups (&spi->dev.kobj, pcf2123_groups);return 0 ;}
Signature · 1 line
2.5.0
Signature · 14 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
PATCH 9/9 rtc-pcf2123: adjust the clock rate via sysfs
In reply to Joshua Claytonpcf2123 has an offset register, which can be used to make minor adjustments to the clock rate to compensate for temperature or a crystal that is not exactly right.
The adjustment is calculated in parts per billion. The data sheet uses parts per million (as do some others), but with 2 digits of precision. Since floating point is forbidden, parts per billion is a better fit.
Add a pair of sysfs files to seetand retrieve the offset in ppm.
--- drivers/rtc/rtc-pcf2123.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
drivers/rtc/rtc-pcf2123.c +68 -0
@@ -77,11 +77,17 @@/* PCF2123_REG_SC BITS */ #define OSC_HAS_STOPPED (0x80)/* Clock has been stopped */ +/* PCF2123_REG_OFFSET BITS */ +#define OFFSET_COARSE (0x80)/* Coarse Mode Offset */ +/* READ/WRITE ADDRESS BITS */ #define PCF2123_SUBADDR (1 << 4)#define PCF2123_WRITE ((0 << 7) | PCF2123_SUBADDR)#define PCF2123_READ ((1 << 7) | PCF2123_SUBADDR) +/* offset granularity in parts per billion in fine mode */ +#define OFFSET_STEP (2170) +static struct spi_driver pcf2123_driver;/*
@@ -166,6 +172,65 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,return count; } +static ssize_t pcf2123_adjust_show (struct device *dev, +struct device_attribute *attr,char *buffer) +{ +ssize_t ret; +s8 reg; + + ret =pcf2123_read (dev,PCF2123_REG_OFFSET , ®,1 ); +if (ret <0 ) +return -EIO ; + +if (reg &OFFSET_COARSE ) { + reg <<=1 ; + }else { + reg &= ~OFFSET_COARSE ; + reg |= (reg &0x40 ) <<1 ;/* sign extend */ + } + +return sprintf (buffer,"%ld\n" , ((long )reg) *OFFSET_STEP ); +} + +static ssize_t pcf2123_adjust_store (struct device *dev, +struct device_attribute *attr, +const char *buffer,size_t count) +{ +ssize_t ret; +long val; +s8 reg; + + ret =kstrtol (buffer,10 , &val); +if (ret) +return ret; + +if (val >OFFSET_STEP *127 ) + reg =127 ; +else if (val <OFFSET_STEP *-128 ) + reg =-128 ; +else + reg = (s8)((val + (OFFSET_STEP >>1 )) /OFFSET_STEP ); + +/* +* Each even value of the fine adjust overlaps with a value of the coarse +* adjustment, and since the coarse adjsutment will spread the adjustments +* over both hours, we use coarse for all even values, as well as values +* that are beyond the range of fine adjustment +*/ +if (reg <=63 && reg >=-64 && reg &1 ) { + reg &= ~OFFSET_COARSE ; + }else { + reg >>=1 ; + reg |=OFFSET_COARSE ; + } + +pcf2123_write_reg (dev,PCF2123_REG_OFFSET , reg); +if (ret <0 ) +return -EIO ; + +return count; +} +static int pcf2123_rtc_read_time (struct device *dev,struct rtc_time *tm) {u8 rxbuf[7 ];
@@ -320,6 +385,8 @@ static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(adjust, S_IRUGO | S_IWUSR, pcf2123_adjust_show, + pcf2123_adjust_store);static struct attribute *pcf2123_attrs[] = { &dev_attr_0.attr,
@@ -338,6 +405,7 @@ static struct attribute *pcf2123_attrs[] = { &dev_attr_d.attr, &dev_attr_e.attr, &dev_attr_f.attr, + &dev_attr_adjust.attr, NULL };
Signature · 12 lines
2.5.0 -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 9/9] rtc-pcf2123: adjust the clock rate via sysfs
Quoted 10 lines · Joshua Clayton
pcf2123 has an offset register, which can be used to make minor adjustments to the clock rate to compensate for temperature or a crystal that is not exactly right.
The adjustment is calculated in parts per billion. The data sheet uses parts per million (as do some others), but with 2 digits of precision. Since floating point is forbidden, parts per billion is a better fit.
Add a pair of sysfs files to seetand retrieve the offset in ppm.
Bah.
I noticed a couple of typos in the last line of the commit message. Will get rid of "in ppm" since it is not in ppm. s/seetand/set and/
Signature · 12 lines
~Joshua Clayton -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Quoted 27 lines · Joshua Clayton
Greetings,
This series was prompted by a need to adjust the clock rate of the rtc The existing code performs a soft reset during probe, which wipes out several registers including the offset register, which performs adjustments to the clock rate.The first several patches are cleanup, with patch 5 and 6 avoiding the reset, and patch 9 adding a nice sysfs interface to the clock offset.
I know that this is not the only rtc to provide a programmable clock offset I wonder if this interface would make a good addition to the rtc api?
The rtc chips I have seen list their clock adjustments in parts per million. I went with parts per billion, since the ppm listed was listed with a fractional component.
Joshua Clayton (9):
rtc-pcf2123: Document all registers and useful bits
rtc-pcf2123: clean up reads from the chip
rtc-pcf2123: clean up writes to the rtc chip
rtc-pcf2123: replace magic numbers with defines
rtc-pcf2123: put the chip reset into a function
rtc-pcf2123: avoid resetting the clock if possible
rtc-pcf2123: allow sysfs to accept hexidecimal
rtc-pcf2123: use sysfs groups
rtc-pcf2123: adjust the clock rate via sysfs
Quoted 4 lines
drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++---------------- 1 file changed, 257 insertions(+), 134 deletions(-)
Any comments on this series?
I realize now that I submitted it during the merge window, so it may have been overlooked.
Signature · 1 line
~Joshua Clayton
Quoted 28 lines · Joshua Clayton
Quoted 27 lines · Joshua Clayton
Greetings,
This series was prompted by a need to adjust the clock rate of the rtc The existing code performs a soft reset during probe, which wipes out several registers including the offset register, which performs adjustments to the clock rate.The first several patches are cleanup, with patch 5 and 6 avoiding the reset, and patch 9 adding a nice sysfs interface to the clock offset.
I know that this is not the only rtc to provide a programmable clock offset I wonder if this interface would make a good addition to the rtc api?
The rtc chips I have seen list their clock adjustments in parts per million. I went with parts per billion, since the ppm listed was listed with a fractional component.
Joshua Clayton (9):
rtc-pcf2123: Document all registers and useful bits
rtc-pcf2123: clean up reads from the chip
rtc-pcf2123: clean up writes to the rtc chip
rtc-pcf2123: replace magic numbers with defines
rtc-pcf2123: put the chip reset into a function
rtc-pcf2123: avoid resetting the clock if possible
rtc-pcf2123: allow sysfs to accept hexidecimal
rtc-pcf2123: use sysfs groups
rtc-pcf2123: adjust the clock rate via sysfs
Quoted 7 lines
Quoted 4 lines
drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++---------------- 1 file changed, 257 insertions(+), 134 deletions(-)Any comments on this series?
I realize now that I submitted it during the merge window, so it may have been overlooked.
I will have a few comments but I didn't review everything thoroughly yet. As you mentioned, you submitted during the merge window so this was not going to be in 4.4 anyway.
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Quoted 11 lines · Alexandre Belloni
Quoted 4 lines · Joshua Clayton
On Wednesday, November 04, 2015 07:36:31 AM Joshua Clayton wrote:
Any comments on this series?
I realize now that I submitted it during the merge window, so it may have been overlooked.I will have a few comments but I didn't review everything thoroughly yet. As you mentioned, you submitted during the merge window so this was not going to be in 4.4 anyway.
Thanks!
I can relax since I know it is on your radar.
I have responded my self to a couple of fat fingers in the commit messages. I am also in the process of testing the actual effects of the feature.
One thing I'm not sure I have adequately described (and didn't really understand until I started testing) is that a positive value in the adjust register increases the average value of a second, so it makes the clock slower, while a negative adjustment makes it faster.
This was counterintuitive to me.
Not having deeply examined other rtc's with similar capabilities, I can't say whether this is the norm.
Signature · 12 lines
~Joshua Clayton -- -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.