Re: [PATCH] rtc: rv3028: new driver

[Marek Vasut]

On 1/30/19 4:00 PM, Alexandre Belloni wrote:
> Add a driver for the MicroCrystal RV-3028.

Some additional information in the commit message won't hurt.

> Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/rtc/rtc.txt |   1 +
>  drivers/rtc/Kconfig                           |   9 +
>  drivers/rtc/Makefile                          |   1 +
>  drivers/rtc/rtc-rv3028.c                      | 732 ++++++++++++++++++
>  4 files changed, 743 insertions(+)
>  create mode 100644 drivers/rtc/rtc-rv3028.c
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc.txt b/Documentation/devicetree/bindings/rtc/rtc.txt
> index d86e2850fe1c..3e6a215e7304 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc.txt
> @@ -52,6 +52,7 @@ emmicro,em3027		EM Microelectronic EM3027 Real-time Clock
>  isil,isl1208		Intersil ISL1208 Low Power RTC with Battery Backed SRAM
>  isil,isl1218		Intersil ISL1218 Low Power RTC with Battery Backed SRAM
>  isil,isl12022		Intersil ISL12022 Real-time Clock
> +microcrystal,rv3028	Real Time Clock Module with I2C-Bus

Shouldn't this be a separate patch ?

>  microcrystal,rv3029	Real Time Clock Module with I2C-Bus
>  microcrystal,rv8523	Real Time Clock
>  nxp,pcf2127		Real-time clock
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index faa9ae1a3062..845c7eef548a 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -626,6 +626,15 @@ config RTC_DRV_EM3027
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-em3027.
>  
> +config RTC_DRV_RV3028
> +	tristate "Micro Crystal RV3028"
> +	help
> +	  If you say yes here you get support for the Micro Crystal
> +	  RV3028.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-rv3028.
> +
>  config RTC_DRV_RV8803
>  	tristate "Micro Crystal RV8803, Epson RX8900"
>  	help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index faca02109aaa..a701d68bb5ca 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -138,6 +138,7 @@ obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
>  obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
>  obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
>  obj-$(CONFIG_RTC_DRV_RTD119X)	+= rtc-rtd119x.o
> +obj-$(CONFIG_RTC_DRV_RV3028)	+= rtc-rv3028.o
>  obj-$(CONFIG_RTC_DRV_RV3029C2)	+= rtc-rv3029c2.o
>  obj-$(CONFIG_RTC_DRV_RV8803)	+= rtc-rv8803.o
>  obj-$(CONFIG_RTC_DRV_RX4581)	+= rtc-rx4581.o
> diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> new file mode 100644
> index 000000000000..cd1600feaed1
> --- /dev/null
> +++ b/drivers/rtc/rtc-rv3028.c
> @@ -0,0 +1,732 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RTC driver for the Micro Crystal RV3028
> + *
> + * Copyright (C) 2018 Micro Crystal SA

2019 ?

> + * Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> + *
> + */
> +
> +#include <linux/bcd.h>
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define RV3028_SEC			0x00
> +#define RV3028_MIN			0x01
> +#define RV3028_HOUR			0x02
> +#define RV3028_WDAY			0x03
> +#define RV3028_DAY			0x04
> +#define RV3028_MONTH			0x05
> +#define RV3028_YEAR			0x06
> +#define RV3028_ALARM_MIN		0x07
> +#define RV3028_ALARM_HOUR		0x08
> +#define RV3028_ALARM_DAY		0x09
> +#define RV3028_STATUS			0x0E
> +#define RV3028_CTRL1			0x0F
> +#define RV3028_CTRL2			0x10
> +#define RV3028_EVT_CTRL			0x13
> +#define RV3028_TS_COUNT			0x14
> +#define RV3028_TS_SEC			0x15
> +#define RV3028_RAM1			0x1F
> +#define RV3028_EEPROM_ADDR		0x25
> +#define RV3028_EEPROM_DATA		0x26
> +#define RV3028_EEPROM_CMD		0x27
> +#define RV3028_CLKOUT			0x35
> +#define RV3028_OFFSET			0x36
> +#define RV3028_BACKUP			0x37
> +
> +#define RV3028_STATUS_PORF		BIT(0)
> +#define RV3028_STATUS_EVF		BIT(1)
> +#define RV3028_STATUS_AF		BIT(2)
> +#define RV3028_STATUS_TF		BIT(3)
> +#define RV3028_STATUS_UF		BIT(4)
> +#define RV3028_STATUS_BSF		BIT(5)
> +#define RV3028_STATUS_CLKF		BIT(6)
> +#define RV3028_STATUS_EEBUSY		BIT(7)
> +
> +#define RV3028_CTRL1_EERD		BIT(3)
> +#define RV3028_CTRL1_WADA		BIT(5)
> +
> +#define RV3028_CTRL2_RESET		BIT(0)
> +#define RV3028_CTRL2_12_24		BIT(1)
> +#define RV3028_CTRL2_EIE		BIT(2)
> +#define RV3028_CTRL2_AIE		BIT(3)
> +#define RV3028_CTRL2_TIE		BIT(4)
> +#define RV3028_CTRL2_UIE		BIT(5)
> +#define RV3028_CTRL2_TSE		BIT(7)
> +
> +#define RV3028_EVT_CTRL_TSR		BIT(2)
> +
> +#define RV3028_EEPROM_CMD_WRITE		0x21
> +#define RV3028_EEPROM_CMD_READ		0x22
> +
> +#define RV3028_EEBUSY_POLL		10000
> +#define RV3028_EEBUSY_TIMEOUT		100000
> +
> +#define RV3028_BACKUP_TCE		BIT(5)
> +#define RV3028_BACKUP_TCR_MASK		GENMASK(1,0)
> +
> +#define OFFSET_STEP_PPT			953674
> +
> +enum rv3028_type {
> +	rv_3028,
> +};
> +
> +struct rv3028_data {
> +	struct regmap *regmap;
> +	struct rtc_device *rtc;
> +	enum rv3028_type type;
> +};
> +
> +static u32 rv3028_trickle_resistors[] = {1000, 3000, 6000, 11000};

u16 ?

The rest looks good to me.

-- 
Best regards,
Marek Vasut