Microchip MCP795xx RTC driver patch

  RTC Microchip mcp795 patch for 5.19-rc5

Here is the mcp795 patch for the 5.19-rc5 merge window.
The main modification are the following:
        - fixed wrong driver naming: RTC is a Microchip
          mcp795 and not Maxim mcp795
        - added date/time write sequences: added date/time      
          write sequences accordingly to official silicon 
          errata document to fix silicon bugs.
        - added support to date/time permanent storage:
          the driver was missing some register bits in order
          to enable the storage of date/time whenever the       
          device looses main power.
        - added a check after starting oscillator in order
          to be sure that it is properly working as specified
          in the datasheet
        - renamed OSCON bit to OSCRUN as in the datasheet
        - add debug messages when setting and reading the
          alarm
        - removed wrong offset when retrieving the current
          year from the device

The patch has been successfully tested in the latest
linux-next releases, and the original problems that I found have
all been resolved.

Signed-off-by: tollsimy <simonetollardo@gmail.com>

---
 .../devicetree/bindings/rtc/maxim,mcp795.txt  |  11 -
 .../bindings/rtc/microchip,mcp795.txt         |  11 +
 drivers/rtc/rtc-mcp795.c                      | 359 ++++++++++++++----
 3 files changed, 296 insertions(+), 85 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
  

Hi tollsimy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on linus/master v6.1-rc1 next-20221019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/tollsimy/Microchip-MCP795xx-RTC-driver-patch/20221017-123625
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20221016162913.38661-1-simonetollardo%40gmail.com
patch subject: [PATCH] Microchip MCP795xx RTC driver patch
config: arc-randconfig-r043-20221017
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cb64c8f9d220eaeef864af84eb164fa9440fe6da
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review tollsimy/Microchip-MCP795xx-RTC-driver-patch/20221017-123625
        git checkout cb64c8f9d220eaeef864af84eb164fa9440fe6da
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/gpio/ drivers/nvmem/ drivers/rtc/ kernel/events/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/rtc/rtc-mcp795.c: In function 'mcp795_read_time':
>> drivers/rtc/rtc-mcp795.c:405:13: warning: variable 'tm_c_sec' set but not used [-Wunused-but-set-variable]
     405 |         int tm_c_sec;
         |             ^~~~~~~~


vim +/tm_c_sec +405 drivers/rtc/rtc-mcp795.c

   400	
   401	static int mcp795_read_time(struct device *dev, struct rtc_time *tim)
   402	{
   403		int ret;
   404		u8 data[8];
 > 405		int tm_c_sec;
   406	
   407		ret = mcp795_rtcc_read(dev, MCP795_REG_TMSECONDS, data, sizeof(data));
   408	
   409		if (ret)
   410			return ret;
   411	
   412		tm_c_sec	= bcd2bin(data[0] & 0xFF);
   413		tim->tm_sec	= bcd2bin(data[1] & 0x7F);
   414		tim->tm_min	= bcd2bin(data[2] & 0x7F);
   415		/* This is only for 24h mode, see datasheet for 12h mode */
   416		tim->tm_hour	= bcd2bin(data[3] & 0x3F);
   417		tim->tm_wday	= bcd2bin(data[4] & 0x07) - 1;
   418		tim->tm_mday	= bcd2bin(data[5] & 0x3F);
   419		tim->tm_mon	= bcd2bin(data[6] & 0x1F) - 1;
   420		tim->tm_year	= bcd2bin(data[7]);
   421	
   422		dev_dbg(dev, "Read from mcp795: %ptR\n", tim);
   423	
   424		return 0;
   425	}
   426
  

Hello,

On 16/10/2022 18:29:13+0200, tollsimy wrote:
> RTC Microchip mcp795 patch for 5.19-rc5
> 
> Here is the mcp795 patch for the 5.19-rc5 merge window.
> The main modification are the following:
>         - fixed wrong driver naming: RTC is a Microchip
>           mcp795 and not Maxim mcp795
>         - added date/time write sequences: added date/time      
>           write sequences accordingly to official silicon 
>           errata document to fix silicon bugs.
>         - added support to date/time permanent storage:
>           the driver was missing some register bits in order
>           to enable the storage of date/time whenever the       
>           device looses main power.
>         - added a check after starting oscillator in order
>           to be sure that it is properly working as specified
>           in the datasheet
>         - renamed OSCON bit to OSCRUN as in the datasheet
>         - add debug messages when setting and reading the
>           alarm
>         - removed wrong offset when retrieving the current
>           year from the device
> 
> The patch has been successfully tested in the latest
> linux-next releases, and the original problems that I found have
> all been resolved.
> 
> Signed-off-by: tollsimy <simonetollardo@gmail.com>
> 
> ---
>  .../devicetree/bindings/rtc/maxim,mcp795.txt  |  11 -
>  .../bindings/rtc/microchip,mcp795.txt         |  11 +
>  drivers/rtc/rtc-mcp795.c                      | 359 ++++++++++++++----
>  3 files changed, 296 insertions(+), 85 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
>  create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mcp795.txt

This needs to be a separate change, also, please convert to yaml.

> 
> diff --git a/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt b/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
> deleted file mode 100644
> index a59fdd8c236d..000000000000
> --- a/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -* Maxim MCP795		SPI Serial Real-Time Clock
> -
> -Required properties:
> -- compatible: Should contain "maxim,mcp795".
> -- reg: SPI address for chip
> -
> -Example:
> -	mcp795: rtc@0 {
> -		compatible = "maxim,mcp795";
> -		reg = <0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt b/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
> new file mode 100644
> index 000000000000..854364c3a173
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
> @@ -0,0 +1,11 @@
> +* Microchip MCP795		SPI Serial Real-Time Clock
> +
> +Required properties:
> +- compatible: Should contain "microchip,mcp795".
> +- reg: SPI address for chip
> +
> +Example:
> +	mcp795: rtc@0 {
> +		compatible = "microchip,mcp795";
> +		reg = <0>;
> +	};
> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
> index 0d515b3df571..292a440b62dd 100644
> --- a/drivers/rtc/rtc-mcp795.c
> +++ b/drivers/rtc/rtc-mcp795.c
> @@ -3,11 +3,15 @@
>   * SPI Driver for Microchip MCP795 RTC
>   *
>   * Copyright (C) Josef Gajdusek <atx@atx.name>
> + * Copyright (C) 2022 Simone Tollardo <simone.tollardo@gmail.com>
>   *
>   * based on other Linux RTC drivers
>   *
>   * Device datasheet:
> - * https://ww1.microchip.com/downloads/en/DeviceDoc/22280A.pdf
> + * https://ww1.microchip.com/downloads/en/DeviceDoc/MCP7951X-MCP7952X-Battery-Backed-SPI-RTCC-20002300C.pdf
> + *
> + * Device errata:
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/MCP795XX-Family-Silicon-Errata-80000680D.pdf
>   */
>  
>  #include <linux/module.h>
> @@ -32,28 +36,148 @@
>  #define MCP795_UNLOCK	0x14
>  #define MCP795_IDWRITE	0x32
>  #define MCP795_IDREAD	0x33
> -#define MCP795_CLRWDT	0x44
>  #define MCP795_CLRRAM	0x54
>  
>  /* MCP795 RTCC registers, see datasheet table 4-1 */
> +#define MCP795_REG_TMSECONDS	0x00	/* Tenths of ms (=centi-seconds) */
>  #define MCP795_REG_SECONDS	0x01
> +#define MCP795_REG_MINUTES	0x02
> +#define MCP795_REG_HOURS	0x03
>  #define MCP795_REG_DAY		0x04
> +#define MCP795_REG_DATE		0x05
>  #define MCP795_REG_MONTH	0x06
> +#define MCP795_REG_YEAR		0x07
>  #define MCP795_REG_CONTROL	0x08
> +#define MCP795_REG_OSC_TRIM	0x09
> +/* Alarm 0 */
>  #define MCP795_REG_ALM0_SECONDS	0x0C
> +#define MCP795_REG_ALM0_MINUTES	0x0D
> +#define MCP795_REG_ALM0_HOURS	0x0E
>  #define MCP795_REG_ALM0_DAY	0x0F
> -
> -#define MCP795_ST_BIT		BIT(7)
> -#define MCP795_24_BIT		BIT(6)
> -#define MCP795_LP_BIT		BIT(5)
> -#define MCP795_EXTOSC_BIT	BIT(3)
> -#define MCP795_OSCON_BIT	BIT(5)
> -#define MCP795_ALM0_BIT		BIT(4)
> -#define MCP795_ALM1_BIT		BIT(5)
> -#define MCP795_ALM0IF_BIT	BIT(3)
> -#define MCP795_ALM0C0_BIT	BIT(4)
> -#define MCP795_ALM0C1_BIT	BIT(5)
> -#define MCP795_ALM0C2_BIT	BIT(6)
> +#define MCP795_REG_ALM0_DATE	0x10
> +#define MCP795_REG_ALM0_MONTH	0x11	/* Month only for alarm 0 */
> +/* Alarm 1 */
> +#define MCP795_REG_ALM1_TMSECONDS	0x12	/* Centi-seconds only for alarm 1 */
> +#define MCP795_REG_ALM1_SECONDS	0x13
> +#define MCP795_REG_ALM1_MINUTES	0x14
> +#define MCP795_REG_ALM1_HOURS	0x15
> +#define MCP795_REG_ALM1_DAY	0x16
> +#define MCP795_REG_ALM1_DATE	0x17
> +
> +/* Power-Fail Timestamp */
> +/* Power-Down Timetsamp */
> +#define MCP795_REG_PWD_MINUTES	0x18
> +#define MCP795_REG_PWD_HOURS	0x19
> +#define MCP795_REG_PWD_DATE	0x1A
> +#define MCP795_REG_PWD_MONTH	0x1B
> +/* Power-Up Timestamp */
> +#define MCP795_REG_PUP_MINUTES	0x1C
> +#define MCP795_REG_PUP_HOURS	0x1D
> +#define MCP795_REG_PUP_DATE	0x1E
> +#define MCP795_REG_PUP_MONTH	0x1F
> +
> +/* MCP795_REG_CONTROL bits */
> +
> +/* MISC */
> +#define MCP795_ST_BIT		BIT(7)	/* Status bit */
> +#define MCP795_24_BIT		BIT(6)	/* 12/24 hour mode */
> +#define MCP795_LP_BIT		BIT(5)	/* Leap Year */
> +#define MCP795_OSCRUN_BIT	BIT(5)  /* Oscillator Run */
> +#define MCP795_PWRFAIL_BIT	BIT(4)  /* Power Fail */
> +#define MCP795_VBATEN_BIT	BIT(3)  /* VBAT Enable */
> +
> +/* RTCC 0x08 register bits */
> +#define MCP795_SQWEN_BIT	BIT(6)  /* Square Wave Enable (MFP pin) */
> +#define MCP795_ALM1_EN_BIT	BIT(5)  /* Alarm 1 Enable */
> +#define MCP795_ALM0_EN_BIT	BIT(4)  /* Alarm 0 Enable */
> +#define MCP795_EXTOSC_BIT	BIT(3)	/* External Oscillator */
> +#define MCP795_CRSTRIM_BIT	BIT(2)  /* Crystal Trim */
> +#define MCP795_SQWFS_BIT_H	BIT(1)  /* Square Wave Frequency Select H */
> +#define MCP795_SQWFS_BIT_L	BIT(0)  /* Square Wave Frequency Select L */
> +
> +/* Alarms (0 and 1) */
> +
> +/* Centi-seconds - ALARM 1 ONLY */
> +/* Alarms BCD Decimal Values of Centi-Second's Tens Digit (=deci-seconds) */
> +#define MCP795_ALM1_CSEC_T_C0_BIT	BIT(4)
> +#define MCP795_ALM1_CSEC_T_C1_BIT	BIT(5)
> +#define MCP795_ALM1_CSEC_T_C2_BIT	BIT(6)
> +#define MCP795_ALM1_CSEC_T_C3_BIT	BIT(6)
> +/* Alarms BCD Decimal Values of Centi-Second's Ones Digit (=centi-seconds) */
> +#define MCP795_ALM1_CSEC_O_C0_BIT	BIT(0)
> +#define MCP795_ALM1_CSEC_O_C1_BIT	BIT(1)
> +#define MCP795_ALM1_CSEC_O_C2_BIT	BIT(2)
> +#define MCP795_ALM1_CSEC_O_C3_BIT	BIT(3)
> +
> +/* Seconds */
> +/* Alarms BCD Decimal Values of Second's Tens Digit */
> +#define MCP795_ALM_SEC_T_C0_BIT	BIT(4)
> +#define MCP795_ALM_SEC_T_C1_BIT	BIT(5)
> +#define MCP795_ALM_SEC_T_C2_BIT	BIT(6)
> +/* Alarms BCD Decimal Values of Second's Ones Digit */
> +#define MCP795_ALM_SEC_O_C0_BIT	BIT(0)
> +#define MCP795_ALM_SEC_O_C1_BIT	BIT(1)
> +#define MCP795_ALM_SEC_O_C2_BIT	BIT(2)
> +#define MCP795_ALM_SEC_O_C3_BIT	BIT(3)
> +
> +/* Minutes */
> +/* Alarms BCD Decimal Values of Minute's Tens Digit */
> +#define MCP795_ALM_MIN_T_C0_BIT	BIT(4)
> +#define MCP795_ALM_MIN_T_C1_BIT	BIT(5)
> +#define MCP795_ALM_MIN_T_C2_BIT	BIT(6)
> +/* Alarms BCD Decimal Values of Minute's Ones Digit */
> +#define MCP795_ALM_MIN_O_C0_BIT	BIT(0)
> +#define MCP795_ALM_MIN_O_C1_BIT	BIT(1)
> +#define MCP795_ALM_MIN_O_C2_BIT	BIT(2)
> +#define MCP795_ALM_MIN_O_C3_BIT	BIT(3)
> +
> +/* Hours */
> +/* Alarms BCD Decimal Values of Hour's Tens Digit */
> +#define MCP795_ALM_HOUR_T_C0_BIT	BIT(4)
> +/* Alarms BCD Decimal Values of Hour's Ones Digit */
> +#define MCP795_ALM_HOUR_O_C0_BIT	BIT(0)
> +#define MCP795_ALM_HOUR_O_C1_BIT	BIT(1)
> +#define MCP795_ALM_HOUR_O_C2_BIT	BIT(2)
> +#define MCP795_ALM_HOUR_O_C3_BIT	BIT(3)
> +
> +/* Days */
> +/* Alarms Interrupt Flag */
> +#define MCP795_ALM_IF_BIT	BIT(3)
> +/* Alarms BCD Decimal Values of Day's Ones Digit */
> +#define MCP795_ALM_DAY_O_C0_BIT	BIT(0)
> +#define MCP795_ALM_DAY_O_C1_BIT	BIT(1)
> +#define MCP795_ALM_DAY_O_C2_BIT	BIT(2)
> +
> +/* Date */
> +/* Alarms BCD Decimal Values of Date's Tens Digit */
> +#define MCP795_ALM_DATE_T_C0_BIT	BIT(4)
> +#define MCP795_ALM_DATE_T_C1_BIT	BIT(5)
> +/* Alarms BCD Decimal Values of Date's Ones Digit */
> +#define MCP795_ALM_DATE_O_C0_BIT	BIT(0)
> +#define MCP795_ALM_DATE_O_C1_BIT	BIT(1)
> +#define MCP795_ALM_DATE_O_C2_BIT	BIT(2)
> +#define MCP795_ALM_DATE_O_C3_BIT	BIT(3)
> +
> +/* Months */
> +/* Alarms BCD Decimal Values of Month's Tens Digit */
> +#define MCP795_ALM0_MONTH_T_C0_BIT	BIT(3)
> +/* Alarms BCD Decimal Values of Month's Ones Digit */
> +#define MCP795_ALM0_MONTH_O_C0_BIT	BIT(0)
> +#define MCP795_ALM0_MONTH_O_C1_BIT	BIT(1)
> +#define MCP795_ALM0_MONTH_O_C2_BIT	BIT(2)
> +#define MCP795_ALM0_MONTH_O_C3_BIT	BIT(3)
> +
> +/* Alarms Masks */
> +#define MCP795_ALM_MASKS_BIT	BIT(4)
> +
> +/* BCD mask values for alarm matching - need BCD conversion */
> +#define MCP795_ALM_MASK_SEC	0x00
> +#define MCP795_ALM_MASK_MIN	0x01
> +#define MCP795_ALM_MASK_HOUR	0x02
> +#define MCP795_ALM_MASK_DAY	0x03
> +#define MCP795_ALM_MASK_DATE	0x04
> +#define MCP795_ALM_MASK_CSEC	0x05
> +#define MCP795_ALM_MASK_ALL	0x07

You have a bunch of unused defines, please remove them

>  
>  #define SEC_PER_DAY		(24 * 60 * 60)
>  
> @@ -69,7 +193,7 @@ static int mcp795_rtcc_read(struct device *dev, u8 addr, u8 *buf, u8 count)
>  
>  	if (ret)
>  		dev_err(dev, "Failed reading %d bytes from address %x.\n",
> -					count, addr);
> +			count, addr);

Unnecessary change

>  
>  	return ret;
>  }
> @@ -88,7 +212,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
>  
>  	if (ret)
>  		dev_err(dev, "Failed to write %d bytes to address %x.\n",
> -					count, addr);
> +			count, addr);
>  

Unnecessary change

>  	return ret;
>  }
> @@ -123,17 +247,16 @@ static int mcp795_stop_oscillator(struct device *dev, bool *extosc)
>  	if (ret)
>  		return ret;
>  	*extosc = !!(data & MCP795_EXTOSC_BIT);
> -	ret = mcp795_rtcc_set_bits(
> -				dev, MCP795_REG_CONTROL, MCP795_EXTOSC_BIT, 0);
> +	ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL, MCP795_EXTOSC_BIT, 0);

ditto

>  	if (ret)
>  		return ret;
> -	/* wait for the OSCON bit to clear */
> +	/* wait for the OSCRUN bit to clear */
>  	do {
>  		usleep_range(700, 800);
>  		ret = mcp795_rtcc_read(dev, MCP795_REG_DAY, &data, 1);
>  		if (ret)
>  			break;
> -		if (!(data & MCP795_OSCON_BIT))
> +		if (!(data & MCP795_OSCRUN_BIT))

I'm not really in favour of changing a define name but do so in a
separate patch.

>  			break;
>  
>  	} while (--retries);
> @@ -143,17 +266,34 @@ static int mcp795_stop_oscillator(struct device *dev, bool *extosc)
>  
>  static int mcp795_start_oscillator(struct device *dev, bool *extosc)
>  {
> +	int ret;
> +	int retries = 5;
> +
>  	if (extosc) {
>  		u8 data = *extosc ? MCP795_EXTOSC_BIT : 0;
> -		int ret;
>  
> -		ret = mcp795_rtcc_set_bits(
> -			dev, MCP795_REG_CONTROL, MCP795_EXTOSC_BIT, data);
> +		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL, MCP795_EXTOSC_BIT, data);

Unnecessary change

>  		if (ret)
>  			return ret;
>  	}
> -	return mcp795_rtcc_set_bits(
> -			dev, MCP795_REG_SECONDS, MCP795_ST_BIT, MCP795_ST_BIT);
> +	ret = mcp795_rtcc_set_bits(dev, MCP795_REG_SECONDS, MCP795_ST_BIT, MCP795_ST_BIT);
> +	if (ret)
> +		return ret;
> +
> +	/* wait for the OSCRUN bit to set */

This comment is not really useful

> +	do {
> +		u8 data = MCP795_OSCRUN_BIT;
> +
> +		usleep_range(700, 800);

but this needs to be documented


> +		ret = mcp795_rtcc_read(dev, MCP795_REG_DAY, &data, 1);
> +		if (ret)
> +			break;
> +		if ((data & MCP795_OSCRUN_BIT))
> +			break;
> +
> +	} while (--retries);

Then number of retires too. Maybe it would better to have a timeout
instead?

> +
> +	return !retries ? -EIO : ret;
>  }
>  
>  /* Enable or disable Alarm 0 in RTC */
> @@ -164,18 +304,24 @@ static int mcp795_update_alarm(struct device *dev, bool enable)
>  	dev_dbg(dev, "%s alarm\n", enable ? "Enable" : "Disable");
>  
>  	if (enable) {
> +		/* Disable Square Wave Function to enable Alarm output */
> +		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL,
> +					   MCP795_SQWEN_BIT, 0);

ret is not checked here?

Also, the SQW may be useful to calibrate the oscillator, this
unconditionally breaks that feature

>  		/* clear ALM0IF (Alarm 0 Interrupt Flag) bit */
>  		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_ALM0_DAY,
> -					MCP795_ALM0IF_BIT, 0);
> +					   MCP795_ALM_IF_BIT, 0);

This renaming also seems unnecessary and makes reading the patch more
complicated

>  		if (ret)
>  			return ret;
>  		/* enable alarm 0 */
>  		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL,
> -					MCP795_ALM0_BIT, MCP795_ALM0_BIT);
> +					   MCP795_ALM0_EN_BIT, MCP795_ALM0_EN_BIT);
>  	} else {
>  		/* disable alarm 0 and alarm 1 */
>  		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL,
> -					MCP795_ALM0_BIT | MCP795_ALM1_BIT, 0);
> +					   MCP795_ALM0_EN_BIT | MCP795_ALM1_EN_BIT, 0);
> +		/* Disable Alarm output to let enable Square Wave Function */
> +		ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL,
> +					   MCP795_SQWEN_BIT, 0);

The code doesn't do what the comment says and so the comment is useless

>  	}
>  	return ret;
>  }
> @@ -183,7 +329,7 @@ static int mcp795_update_alarm(struct device *dev, bool enable)
>  static int mcp795_set_time(struct device *dev, struct rtc_time *tim)
>  {
>  	int ret;
> -	u8 data[7];
> +	u8 data[8];
>  	bool extosc;
>  
>  	/* Stop RTC and store current value of EXTOSC bit */
> @@ -192,32 +338,42 @@ static int mcp795_set_time(struct device *dev, struct rtc_time *tim)
>  		return ret;
>  
>  	/* Read first, so we can leave config bits untouched */
> -	ret = mcp795_rtcc_read(dev, MCP795_REG_SECONDS, data, sizeof(data));
> +	ret = mcp795_rtcc_read(dev, MCP795_REG_TMSECONDS, data, sizeof(data));

Why do you read centiseconds here?

>  
>  	if (ret)
>  		return ret;
>  
> -	data[0] = (data[0] & 0x80) | bin2bcd(tim->tm_sec);
> -	data[1] = (data[1] & 0x80) | bin2bcd(tim->tm_min);
> -	data[2] = bin2bcd(tim->tm_hour);
> -	data[3] = (data[3] & 0xF8) | bin2bcd(tim->tm_wday + 1);
> -	data[4] = bin2bcd(tim->tm_mday);
> -	data[5] = (data[5] & MCP795_LP_BIT) | bin2bcd(tim->tm_mon + 1);
> -
> -	if (tim->tm_year > 100)
> -		tim->tm_year -= 100;
> -
> -	data[6] = bin2bcd(tim->tm_year);
> -
> -	/* Always write the date and month using a separate Write command.
> -	 * This is a workaround for a know silicon issue that some combinations
> -	 * of date and month values may result in the date being reset to 1.
> -	 */
> -	ret = mcp795_rtcc_write(dev, MCP795_REG_SECONDS, data, 5);
> +	data[0] = 0x00;	//centiseconds not implemented
> +	data[1] = (data[0] & 0x80) | bin2bcd(tim->tm_sec);
I think one of the index is not correct
> +	data[2] = (data[1] & 0x80) | bin2bcd(tim->tm_min);
ditto
> +	data[3] = (data[3] & 0xC0) | bin2bcd(tim->tm_hour);
> +	/* Add one beacause Linux weekday is from 0 to 6 */

I don't think this comment is useful

> +	data[4] = (data[3] & 0xF8) | bin2bcd(tim->tm_wday + 1);
> +	data[5] = (data[5] & 0xC0) | bin2bcd(tim->tm_mday);
> +	/* Add one because Linux month is from 0 to 11 */

ditto

> +	data[6] = (data[6] & 0xC0) | ((data[6] & MCP795_LP_BIT)
> +				| bin2bcd(tim->tm_mon + 1));
> +	data[7] = bin2bcd(tim->tm_year);
> +
> +	/* Set time sequence needed to avoid common device problems, see errata */
> +	/* Write month */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_MONTH, &data[6], 1);
>  	if (ret)
>  		return ret;
> -
> -	ret = mcp795_rtcc_write(dev, MCP795_REG_MONTH, &data[5], 2);
> +	/* Write data (month day) */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_DATE, &data[5], 1);
> +	if (ret)
> +		return ret;
> +	/* Write month again */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_MONTH, &data[6], 1);
> +	if (ret)
> +		return ret;
> +	/* Send the rest apart from year and dayweek */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_TMSECONDS, data, 4);
> +	if (ret)
> +		return ret;
> +	/* Send the year */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_YEAR, &data[7], 1);
>  	if (ret)
>  		return ret;
>  
> @@ -229,6 +385,14 @@ static int mcp795_set_time(struct device *dev, struct rtc_time *tim)
>  	if (ret)
>  		return ret;
>  
> +	/* Always set the weekday using a separate Write command and when
> +	 * oscillator is running.
> +	 * This is a workaround for a know silicon issue.
> +	 */
> +	ret = mcp795_rtcc_write(dev, MCP795_REG_DAY, &data[4], 1);
> +	if (ret)
> +		return ret;
> +
>  	dev_dbg(dev, "Set mcp795: %ptR\n", tim);
>  
>  	return 0;
> @@ -237,20 +401,23 @@ static int mcp795_set_time(struct device *dev, struct rtc_time *tim)
>  static int mcp795_read_time(struct device *dev, struct rtc_time *tim)
>  {
>  	int ret;
> -	u8 data[7];
> +	u8 data[8];
> +	int tm_c_sec;
>  
> -	ret = mcp795_rtcc_read(dev, MCP795_REG_SECONDS, data, sizeof(data));
> +	ret = mcp795_rtcc_read(dev, MCP795_REG_TMSECONDS, data, sizeof(data));
>  
>  	if (ret)
>  		return ret;
>  
> -	tim->tm_sec	= bcd2bin(data[0] & 0x7F);
> -	tim->tm_min	= bcd2bin(data[1] & 0x7F);
> -	tim->tm_hour	= bcd2bin(data[2] & 0x3F);
> -	tim->tm_wday	= bcd2bin(data[3] & 0x07) - 1;
> -	tim->tm_mday	= bcd2bin(data[4] & 0x3F);
> -	tim->tm_mon	= bcd2bin(data[5] & 0x1F) - 1;
> -	tim->tm_year	= bcd2bin(data[6]) + 100; /* Assume we are in 20xx */
> +	tm_c_sec	= bcd2bin(data[0] & 0xFF);
> +	tim->tm_sec	= bcd2bin(data[1] & 0x7F);
> +	tim->tm_min	= bcd2bin(data[2] & 0x7F);
> +	/* This is only for 24h mode, see datasheet for 12h mode */
> +	tim->tm_hour	= bcd2bin(data[3] & 0x3F);
> +	tim->tm_wday	= bcd2bin(data[4] & 0x07) - 1;
> +	tim->tm_mday	= bcd2bin(data[5] & 0x3F);
> +	tim->tm_mon	= bcd2bin(data[6] & 0x1F) - 1;
> +	tim->tm_year	= bcd2bin(data[7]);
>  
>  	dev_dbg(dev, "Read from mcp795: %ptR\n", tim);
>  
> @@ -265,6 +432,17 @@ static int mcp795_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	u8 tmp[6];
>  	int ret;
>  
> +	/* Set Alarm 0 Mask to match all */
> +	/* Read first, so we can leave other bits untouched */
> +	ret = mcp795_rtcc_read(dev, MCP795_REG_ALM0_DAY, tmp, 1);
> +	if (ret)
> +		return ret;
> +	/* Set Alarm 0 Mask to match all */
> +	tmp[0] = (tmp[0] & 0x8F) | (bin2bcd(MCP795_ALM_MASK_ALL) << 4);

If possible, I wouldn't match all and simply drop weekday, there is no
guarantee that userspace will provide a valid weekday and you could then
drop the previous workaround while setting weekday.

> +	ret = mcp795_rtcc_write(dev, MCP795_REG_ALM0_DAY, tmp, 1);
> +	if (ret)
> +		return ret;
> +
>  	/* Read current time from RTC hardware */
>  	ret = mcp795_read_time(dev, &now_tm);
>  	if (ret)
> @@ -291,12 +469,14 @@ static int mcp795_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	alm->time.tm_isdst	= -1;
>  	alm->time.tm_yday	= -1;
>  
> +	/* set alarm match: seconds, minutes, hour, day, date and month */
>  	tmp[0] = (tmp[0] & 0x80) | bin2bcd(alm->time.tm_sec);
>  	tmp[1] = (tmp[1] & 0x80) | bin2bcd(alm->time.tm_min);
>  	tmp[2] = (tmp[2] & 0xE0) | bin2bcd(alm->time.tm_hour);
>  	tmp[3] = (tmp[3] & 0x80) | bin2bcd(alm->time.tm_wday + 1);
> -	/* set alarm match: seconds, minutes, hour, day, date and month */
> -	tmp[3] |= (MCP795_ALM0C2_BIT | MCP795_ALM0C1_BIT | MCP795_ALM0C0_BIT);
> +	/* ALMxIF cleared in the update function */
> +	tmp[3] |= (MCP795_ALM_SEC_T_C2_BIT | MCP795_ALM_SEC_T_C1_BIT
> +				| MCP795_ALM_SEC_T_C0_BIT);
>  	tmp[4] = (tmp[4] & 0xC0) | bin2bcd(alm->time.tm_mday);
>  	tmp[5] = (tmp[5] & 0xE0) | bin2bcd(alm->time.tm_mon + 1);
>  
> @@ -309,10 +489,14 @@ static int mcp795_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  		ret = mcp795_update_alarm(dev, true);
>  		if (ret)
>  			return ret;
> +		alm->pending = 1;

Why is that needed?

>  		dev_dbg(dev, "Alarm IRQ armed\n");
>  	}
> -	dev_dbg(dev, "Set alarm: %ptRdr(%d) %ptRt\n",
> -		&alm->time, alm->time.tm_wday, &alm->time);
> +
> +	/* do not print year, it's not implemented in the RTC hardware */
> +	dev_dbg(dev, "Set alarm: %02d-%02dT%02d:%02d:%02d\n", alm->time.tm_mon + 1,
> +		alm->time.tm_mday, alm->time.tm_hour, alm->time.tm_min, alm->time.tm_sec);
> +

This is a debug print, I don't think it is an issue to print year
anyway.

>  	return 0;
>  }
>  
> @@ -321,8 +505,7 @@ static int mcp795_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	u8 data[6];
>  	int ret;
>  
> -	ret = mcp795_rtcc_read(
> -			dev, MCP795_REG_ALM0_SECONDS, data, sizeof(data));
> +	ret = mcp795_rtcc_read(dev, MCP795_REG_ALM0_SECONDS, data, sizeof(data));

Unnecessary change

>  	if (ret)
>  		return ret;
>  
> @@ -336,8 +519,17 @@ static int mcp795_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	alm->time.tm_isdst	= -1;
>  	alm->time.tm_yday	= -1;
>  
> -	dev_dbg(dev, "Read alarm: %ptRdr(%d) %ptRt\n",
> -		&alm->time, alm->time.tm_wday, &alm->time);
> +	alm->pending = !!(data[3] & MCP795_ALM_IF_BIT);
> +
> +	ret = mcp795_rtcc_read(dev, MCP795_REG_CONTROL, data, 1);
> +	if (ret)
> +		return ret;
> +	alm->enabled = !!(data[0] & MCP795_ALM0_EN_BIT);
> +
> +	/* Do not print year, it's not implemented in the RTC hardware */
> +	dev_dbg(dev, "Read alarm: %02d-%02dT%02d:%02d:%02d\n", alm->time.tm_mon + 1,
> +		alm->time.tm_mday, alm->time.tm_hour, alm->time.tm_min, alm->time.tm_sec);
> +
>  	return 0;
>  }
>  
> @@ -381,6 +573,7 @@ static int mcp795_probe(struct spi_device *spi)
>  {
>  	struct rtc_device *rtc;
>  	int ret;
> +	u8 data;
>  
>  	spi->mode = SPI_MODE_0;
>  	spi->bits_per_word = 8;
> @@ -393,10 +586,10 @@ static int mcp795_probe(struct spi_device *spi)
>  	/* Start the oscillator but don't set the value of EXTOSC bit */
>  	mcp795_start_oscillator(&spi->dev, NULL);
>  	/* Clear the 12 hour mode flag*/
> -	mcp795_rtcc_set_bits(&spi->dev, 0x03, MCP795_24_BIT, 0);
> +	mcp795_rtcc_set_bits(&spi->dev, MCP795_REG_HOURS, MCP795_24_BIT, 0);
>  
>  	rtc = devm_rtc_device_register(&spi->dev, "rtc-mcp795",
> -					&mcp795_rtc_ops, THIS_MODULE);
> +				       &mcp795_rtc_ops, THIS_MODULE);

ditto

>  	if (IS_ERR(rtc))
>  		return PTR_ERR(rtc);
>  
> @@ -409,22 +602,40 @@ static int mcp795_probe(struct spi_device *spi)
>  		 * the interrupt.
>  		 */
>  		mcp795_rtcc_set_bits(&spi->dev, MCP795_REG_ALM0_DAY,
> -					MCP795_ALM0IF_BIT, 0);
> -		ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
> -				mcp795_irq, IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				dev_name(&rtc->dev), spi);
> +				     MCP795_ALM_IF_BIT, 0);
> +		ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL, mcp795_irq,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						dev_name(&rtc->dev), spi);

ditto

>  		if (ret)
>  			dev_err(&spi->dev, "Failed to request IRQ: %d: %d\n",
> -						spi->irq, ret);
> +				spi->irq, ret);

ditto

>  		else
>  			device_init_wakeup(&spi->dev, true);
>  	}
> +
> +	/* If PWRFAIL BIT is set, it must be cleared */
> +	ret = mcp795_rtcc_read(&spi->dev, MCP795_REG_DAY, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (!!(data & MCP795_PWRFAIL_BIT)) {
> +		ret = mcp795_rtcc_set_bits(&spi->dev, MCP795_REG_DAY,
> +					   MCP795_PWRFAIL_BIT, 0);
> +		if (ret)
> +			return ret;

If this indicates that the time/date is invalid, then it must not be
cleared in the probe and instead in set_time also this information has
to be used in read_time

> +	}
> +	/* Set VBAT flag to 1 */
> +	ret = mcp795_rtcc_set_bits(&spi->dev, MCP795_REG_DAY, MCP795_VBATEN_BIT,
> +				   MCP795_VBATEN_BIT);

There is a proper interface to set the backup mode, please use it

> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id mcp795_of_match[] = {
> -	{ .compatible = "maxim,mcp795" },
> +	{ .compatible = "microchip,mcp795" },
>  	{ }


You should really slice this up in separate changes to make reviewing
easier.
  

On 16/10/2022 12:29, tollsimy wrote:
> RTC Microchip mcp795 patch for 5.19-rc5
> 
> Here is the mcp795 patch for the 5.19-rc5 merge window.
> The main modification are the following:
>         - fixed wrong driver naming: RTC is a Microchip
>           mcp795 and not Maxim mcp795
>         - added date/time write sequences: added date/time      
>           write sequences accordingly to official silicon 
>           errata document to fix silicon bugs.
>         - added support to date/time permanent storage:
>           the driver was missing some register bits in order
>           to enable the storage of date/time whenever the       
>           device looses main power.
>         - added a check after starting oscillator in order
>           to be sure that it is properly working as specified
>           in the datasheet
>         - renamed OSCON bit to OSCRUN as in the datasheet
>         - add debug messages when setting and reading the
>           alarm
>         - removed wrong offset when retrieving the current
>           year from the device
> 
> The patch has been successfully tested in the latest
> linux-next releases, and the original problems that I found have
> all been resolved.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed several people so it seems you did not follow the process.

> 
> Signed-off-by: tollsimy <simonetollardo@gmail.com>

Full name is needed.

> 
> ---
>  .../devicetree/bindings/rtc/maxim,mcp795.txt  |  11 -
>  .../bindings/rtc/microchip,mcp795.txt         |  11 +

Bindings are always separate patch.

>  drivers/rtc/rtc-mcp795.c                      | 359 ++++++++++++++----
>  3 files changed, 296 insertions(+), 85 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
>  create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt b/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
> deleted file mode 100644
> index a59fdd8c236d..000000000000
> --- a/Documentation/devicetree/bindings/rtc/maxim,mcp795.txt
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -* Maxim MCP795		SPI Serial Real-Time Clock
> -
> -Required properties:
> -- compatible: Should contain "maxim,mcp795".
> -- reg: SPI address for chip
> -
> -Example:
> -	mcp795: rtc@0 {
> -		compatible = "maxim,mcp795";
> -		reg = <0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt b/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
> new file mode 100644
> index 000000000000..854364c3a173
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/microchip,mcp795.txt
> @@ -0,0 +1,11 @@
> +* Microchip MCP795		SPI Serial Real-Time Clock
> +
> +Required properties:
> +- compatible: Should contain "microchip,mcp795".
> +- reg: SPI address for chip
> +
> +Example:
> +	mcp795: rtc@0 {
> +		compatible = "microchip,mcp795";
> +		reg = <0>;
> +	};

This part is not erally explained/justified.

Best regards,
Krzysztof