Re: [PATCH v8 09/12] rtc: bd70528: add BD71828 support

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Mon, 30 Dec 2019, Matti Vaittinen wrote:

> ROHM BD71828 PMIC RTC block is from many parts similar to one
> on BD70528. Support BD71828 RTC using BD70528 RTC driver and
> avoid re-inventing the wheel.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> Acked-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> ---
> 
> Changes from v7 - no changes
> 
>  drivers/rtc/Kconfig              |   3 +-
>  drivers/rtc/rtc-bd70528.c        | 168 ++++++++++++++++++++++++++++---
>  include/linux/mfd/rohm-bd70528.h |  13 +--
>  include/linux/mfd/rohm-bd71828.h |   4 +-
>  include/linux/mfd/rohm-shared.h  |  27 +++++
>  5 files changed, 186 insertions(+), 29 deletions(-)
>  create mode 100644 include/linux/mfd/rohm-shared.h
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index d77515d8382c..df7a3843069d 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -498,12 +498,13 @@ config RTC_DRV_M41T80_WDT
>  	help
>  	  If you say Y here you will get support for the
>  	  watchdog timer in the ST M41T60 and M41T80 RTC chips series.
> +
>  config RTC_DRV_BD70528
>  	tristate "ROHM BD70528 PMIC RTC"
>  	depends on MFD_ROHM_BD70528 && (BD70528_WATCHDOG || !BD70528_WATCHDOG)
>  	help
>  	  If you say Y here you will get support for the RTC
> -	  on ROHM BD70528 Power Management IC.
> +	  block on ROHM BD70528 and BD71828 Power Management IC.
>  
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-bd70528.
> diff --git a/drivers/rtc/rtc-bd70528.c b/drivers/rtc/rtc-bd70528.c
> index 627037aa66a8..2ce202040556 100644
> --- a/drivers/rtc/rtc-bd70528.c
> +++ b/drivers/rtc/rtc-bd70528.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/bcd.h>
>  #include <linux/mfd/rohm-bd70528.h>
> +#include <linux/mfd/rohm-bd71828.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -15,7 +16,7 @@
>  /*
>   * We read regs RTC_SEC => RTC_YEAR
>   * this struct is ordered according to chip registers.
> - * Keep it u8 only to avoid padding issues.
> + * Keep it u8 only (or packed) to avoid padding issues.
>   */
>  struct bd70528_rtc_day {
>  	u8 sec;
> @@ -36,6 +37,13 @@ struct bd70528_rtc_wake {
>  	u8 ctrl;
>  } __packed;
>  
> +struct bd71828_rtc_alm {
> +	struct bd70528_rtc_data alm0;
> +	struct bd70528_rtc_data alm1;
> +	u8 alm_mask;
> +	u8 alm1_mask;
> +} __packed;
> +
>  struct bd70528_rtc_alm {
>  	struct bd70528_rtc_data data;
>  	u8 alm_mask;
> @@ -45,6 +53,8 @@ struct bd70528_rtc_alm {
>  struct bd70528_rtc {
>  	struct rohm_regmap_dev *mfd;

I think it would be better if you fixed this up be more forthcoming.
It took some grepping to find out what this actually meant.  An MFD
isn't really a thing, we made it up.  Here you are referring to this
platform device's parent's device data.

With that in mind I offer some suggestions:

  'struct rohm_parent_ddata pddata'
  'struct rohm_parent_ddata parent'

>  	struct device *dev;
> +	u8 reg_time_start;
> +	bool has_rtc_timers;
>  };

[...]

> +static int bd71828_set_alarm(struct device *dev, struct rtc_wkalrm *a)
> +{
> +	int ret;
> +	struct bd71828_rtc_alm alm;
> +	struct bd70528_rtc *r = dev_get_drvdata(dev);
> +	struct rohm_regmap_dev *bd71828 = r->mfd;

Then ...

	struct rohm_parent_ddata *parent = r->parent;

	ret = regmap_bulk_read(parent->regmap, BD71828_REG_RTC_ALM_START,
			       &alm, sizeof(alm));
	if (ret) {
		dev_err(dev, "Failed to read alarm regs\n");
		return ret;
	}

[...]

> diff --git a/include/linux/mfd/rohm-bd70528.h b/include/linux/mfd/rohm-bd70528.h
> index 2ad2320d0a96..a57af878fd0c 100644
> --- a/include/linux/mfd/rohm-bd70528.h
> +++ b/include/linux/mfd/rohm-bd70528.h
> @@ -7,6 +7,7 @@
>  #include <linux/bits.h>
>  #include <linux/device.h>
>  #include <linux/mfd/rohm-generic.h>
> +#include <linux/mfd/rohm-shared.h>
>  #include <linux/regmap.h>
>  
>  enum {
> @@ -307,17 +308,6 @@ enum {
>  
>  /* RTC masks to mask out reserved bits */
>  
> -#define BD70528_MASK_RTC_SEC		0x7f
> -#define BD70528_MASK_RTC_MINUTE		0x7f
> -#define BD70528_MASK_RTC_HOUR_24H	0x80
> -#define BD70528_MASK_RTC_HOUR_PM	0x20
> -#define BD70528_MASK_RTC_HOUR		0x1f
> -#define BD70528_MASK_RTC_DAY		0x3f
> -#define BD70528_MASK_RTC_WEEK		0x07
> -#define BD70528_MASK_RTC_MONTH		0x1f
> -#define BD70528_MASK_RTC_YEAR		0xff
> -#define BD70528_MASK_RTC_COUNT_L	0x7f
> -
>  #define BD70528_MASK_ELAPSED_TIMER_EN	0x1
>  /* Mask second, min and hour fields
>   * HW would support ALM irq for over 24h
> @@ -326,7 +316,6 @@ enum {
>   * wake-up we limit ALM to 24H and only
>   * unmask sec, min and hour
>   */
> -#define BD70528_MASK_ALM_EN		0x7
>  #define BD70528_MASK_WAKE_EN		0x1
>  
>  /* WDT masks */
> diff --git a/include/linux/mfd/rohm-bd71828.h b/include/linux/mfd/rohm-bd71828.h
> index d013e03f742d..017a4c01cb31 100644
> --- a/include/linux/mfd/rohm-bd71828.h
> +++ b/include/linux/mfd/rohm-bd71828.h
> @@ -5,6 +5,7 @@
>  #define __LINUX_MFD_BD71828_H__
>  
>  #include <linux/mfd/rohm-generic.h>
> +#include <linux/mfd/rohm-shared.h>

Isn't generic shared?

>  /* Regulator IDs */
>  enum {
> @@ -160,6 +161,7 @@ enum {
>  #define BD71828_REG_RTC_YEAR		0x52
>  
>  #define BD71828_REG_RTC_ALM0_SEC	0x53
> +#define BD71828_REG_RTC_ALM_START	BD71828_REG_RTC_ALM0_SEC
>  #define BD71828_REG_RTC_ALM0_MINUTE	0x54
>  #define BD71828_REG_RTC_ALM0_HOUR	0x55
>  #define BD71828_REG_RTC_ALM0_WEEK	0x56
> @@ -178,6 +180,7 @@ enum {
>  #define BD71828_REG_RTC_ALM1_MASK	0x62
>  
>  #define BD71828_REG_RTC_ALM2		0x63
> +#define BD71828_REG_RTC_START		BD71828_REG_RTC_SEC
>  
>  /* Charger/Battey */
>  #define BD71828_REG_CHG_STATE		0x65
> @@ -204,7 +207,6 @@ enum {
>  #define BD71828_REG_INT_MASK_TEMP	0xdd
>  #define BD71828_REG_INT_MASK_RTC	0xde
>  
> -
>  #define BD71828_REG_INT_MAIN		0xdf
>  #define BD71828_REG_INT_BUCK		0xe0
>  #define BD71828_REG_INT_DCIN1		0xe1
> diff --git a/include/linux/mfd/rohm-shared.h b/include/linux/mfd/rohm-shared.h
> new file mode 100644
> index 000000000000..f16fc3b5000e
> --- /dev/null
> +++ b/include/linux/mfd/rohm-shared.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2018 ROHM Semiconductors */

This is very out of data now!

> +
> +#ifndef __LINUX_MFD_ROHM_SHARED_H__
> +#define __LINUX_MFD_ROHM_SHARED_H__
> +
> +/*
> + * RTC definitions shared between
> + *
> + * BD70528
> + * and BD71828

This reads poorly.

Either form a bullet pointed list, or just write it out.

> + */
> +
> +
> +#define BD70528_MASK_RTC_SEC		0x7f
> +#define BD70528_MASK_RTC_MINUTE	0x7f
> +#define BD70528_MASK_RTC_HOUR_24H	0x80
> +#define BD70528_MASK_RTC_HOUR_PM	0x20
> +#define BD70528_MASK_RTC_HOUR		0x3f
> +#define BD70528_MASK_RTC_DAY		0x3f
> +#define BD70528_MASK_RTC_WEEK		0x07
> +#define BD70528_MASK_RTC_MONTH		0x1f
> +#define BD70528_MASK_RTC_YEAR		0xff
> +#define BD70528_MASK_ALM_EN		0x7
> +
> +#endif /* __LINUX_MFD_ROHM_SHARED_H__ */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog