Re: [PATCH 5/8] rtc: max77686: Add max77802 support

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The MAX77686 and MAX77802 RTC IP blocks are very similar with only
> these differences:
> 
> 0) The RTC registers layout and addresses are different.
> 
> 1) The MAX77686 use 1 bit of the sec/min/hour/etc registers as the
>    alarm enable while MAX77802 has a separate register for that.
> 
> 2) The MAX77686 RTCYEAR register valid values range is 0..99 while
>    for MAX77802 is 0..199.
> 
> 3) The MAX77686 has a separate I2C address for the RTC registers
>    while the MAX77802 uses the same I2C address as the PMIC regs.
> 
> 5) They minium delay before a RTC update (16ms vs 200 usecs).
> 
> There are separate drivers for MAX77686 and MAX77802 RTC IP blocks
> but the differences are not that big so the driver can be extended
> to support both instead of duplicating a lot of code in 2 drivers.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 176 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 128 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 7316e41820c7..7a144e7ecd27 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -1,5 +1,5 @@
>  /*
> - * RTC driver for Maxim MAX77686
> + * RTC driver for Maxim MAX77686 and MAX77802
>   *
>   * Copyright (C) 2012 Samsung Electronics Co.Ltd
>   *
> @@ -43,6 +43,13 @@
>  
>  #define REG_RTC_NONE			0xdeadbeef
>  
> +/*
> + * MAX77802 has separate register (RTCAE1) for alarm enable instead
> + * using 1 bit from registers RTC{SEC,MIN,HOUR,DAY,MONTH,YEAR,DATE}
> + * as in done in MAX77686.
> + */
> +#define ALARM_ENABLE_VALUE		0x77

MAX77802_ALARM_ENABLE_VALUE
(it is specific to 77802, right?)

> +
>  enum {
>  	RTC_SEC = 0,
>  	RTC_MIN,
> @@ -58,6 +65,10 @@ struct rtc_driver_data {
>  	unsigned long		delay;
>  	int			mask;
>  	const unsigned int	*map;
> +	/* Has a separate alarm enable register? */
> +	bool			rtcae;
> +	/* Has a separate I2C regmap for the RTC? */
> +	bool			rtcrm;

Both members are a tongue twisters. :)

'rtcae' you are mostly using in an inverted way (!rtcae) so how about:
'alarm_enable_bit'?

'rtcrm' - 'separate_i2c_addr'?

By the way, I was thinking that you would do decoupling of i2c and
regmap here. It is not required (more useful for Laxman's patch) but it
might by a part of these series.

>  };
>  
>  struct max77686_rtc_info {
> @@ -108,6 +119,8 @@ enum rtc_reg {
>  	REG_ALARM2_MONTH,
>  	REG_ALARM2_YEAR,
>  	REG_ALARM2_DATE,
> +	REG_RTC_AE1,
> +	REG_RTC_AE2,
>  	REG_RTC_END,
>  };
>  
> @@ -120,13 +133,36 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>  	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
>  	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
>  	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
> -	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
> +	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE, REG_RTC_NONE, REG_RTC_NONE,
>  };
>  
>  static const struct rtc_driver_data max77686_drv_data = {
>  	.delay = 1600,
>  	.mask  = 0x7f,
>  	.map   = max77686_map,
> +	.rtcae = false,
> +	.rtcrm = true,
> +};
> +
> +static const unsigned int max77802_map[REG_RTC_END] = {
> +	MAX77802_RTC_CONTROLM, MAX77802_RTC_CONTROL, MAX77802_RTC_UPDATE0,
> +	REG_RTC_NONE, MAX77802_WTSR_SMPL_CNTL, MAX77802_RTC_SEC,
> +	MAX77802_RTC_MIN, MAX77802_RTC_HOUR, MAX77802_RTC_WEEKDAY,
> +	MAX77802_RTC_MONTH, MAX77802_RTC_YEAR, MAX77802_RTC_DATE,
> +	MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN, MAX77802_ALARM1_HOUR,
> +	MAX77686_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH, MAX77802_ALARM1_YEAR,
> +	MAX77802_ALARM1_DATE, MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN,
> +	MAX77802_ALARM1_HOUR, MAX77802_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH,
> +	MAX77802_ALARM1_YEAR, MAX77802_ALARM1_DATE, MAX77802_RTC_AE1,
> +	MAX77802_RTC_AE2,
> +};
> +
> +static const struct rtc_driver_data max77802_drv_data = {
> +	.delay = 200,
> +	.mask  = 0xff,
> +	.map   = max77802_map,
> +	.rtcae = true,
> +	.rtcrm = false,
>  };
>  
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -148,12 +184,20 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>  	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
>  	tm->tm_mday = data[RTC_DATE] & 0x1f;
>  	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
> -	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
> +	tm->tm_year = data[RTC_YEAR] & mask;
>  	tm->tm_yday = 0;
>  	tm->tm_isdst = 0;
> +
> +	/*
> +	 * MAX77686 uses 1 bit from sec/min/hour/etc RTC registers and the
> +	 * year values are just 0..99 so add 100 to support up to 2099.
> +	 */
> +	if (!info->drv_data->rtcae)
> +		tm->tm_year += 100;
>  }
>  
> -static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
> +static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
> +				   struct max77686_rtc_info *info)
>  {
>  	data[RTC_SEC] = tm->tm_sec;
>  	data[RTC_MIN] = tm->tm_min;
> @@ -161,13 +205,19 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
>  	data[RTC_WEEKDAY] = 1 << tm->tm_wday;
>  	data[RTC_DATE] = tm->tm_mday;
>  	data[RTC_MONTH] = tm->tm_mon + 1;
> -	data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
>  
> -	if (tm->tm_year < 100) {
> -		pr_warn("RTC cannot handle the year %d.  Assume it's 2000.\n",
> -			1900 + tm->tm_year);
> -		return -EINVAL;
> +	if (!info->drv_data->rtcae) {
> +		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
> +
> +		if (tm->tm_year < 100) {
> +			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> +				1900 + tm->tm_year);

Maybe in a separate patch use dev_warn()? It wasn't possible before
because you need 'info' argument but it is possible.

> +			return -EINVAL;
> +		}
> +	} else {
> +		data[RTC_YEAR] = tm->tm_year;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -232,7 +282,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	u8 data[RTC_NR_TIME];
>  	int ret;
>  
> -	ret = max77686_rtc_tm_to_data(tm, data);
> +	ret = max77686_rtc_tm_to_data(tm, data, info);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -279,11 +329,24 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	max77686_rtc_data_to_tm(data, &alrm->time, info);
>  
>  	alrm->enabled = 0;
> -	for (i = 0; i < RTC_NR_TIME; i++) {
> -		if (data[i] & ALARM_ENABLE_MASK) {
> -			alrm->enabled = 1;
> -			break;
> +
> +	if (!info->drv_data->rtcae) {
> +		for (i = 0; i < RTC_NR_TIME; i++) {
> +			if (data[i] & ALARM_ENABLE_MASK) {
> +				alrm->enabled = 1;
> +				break;
> +			}
>  		}
> +	} else {
> +		ret = regmap_read(info->max77686->regmap,
> +				  map[REG_RTC_AE1], &val);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
> +				__func__, __LINE__, ret);

I don't like the func/LINE. I know that driver is using them already but
I think it is better not to add new usages of it.

> +			goto out;

Actually I think there is a bug here already. The function will always
return '0'. Instead the 'out' label should return 'ret'. Can you fix it
in separate patch (with reported-by :) )?

> +		}
> +		if (val)
> +			alrm->enabled = 1;
>  	}
>  
>  	alrm->pending = 0;
> @@ -316,21 +379,27 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> -	if (ret < 0) {
> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> +	if (!info->drv_data->rtcae) {
> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>  				__func__, ret);
> -		goto out;
> -	}
> +			goto out;
> +		}
>  
> -	max77686_rtc_data_to_tm(data, &tm, info);
> +		max77686_rtc_data_to_tm(data, &tm, info);
>  
> -	for (i = 0; i < RTC_NR_TIME; i++)
> -		data[i] &= ~ALARM_ENABLE_MASK;
> +		for (i = 0; i < RTC_NR_TIME; i++)
> +			data[i] &= ~ALARM_ENABLE_MASK;
> +
> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +	} else {
> +		ret = regmap_write(info->max77686->regmap,
> +				   map[REG_RTC_AE1], 0);
> +	}
>  
> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>  				__func__, ret);
> @@ -356,29 +425,35 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> -	if (ret < 0) {
> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> +	if (!info->drv_data->rtcae) {
> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>  				__func__, ret);
> -		goto out;
> -	}
> -
> -	max77686_rtc_data_to_tm(data, &tm, info);
> +			goto out;
> +		}
>  
> -	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> -	if (data[RTC_MONTH] & 0xf)
> -		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> -	if (data[RTC_YEAR] & info->drv_data->mask)
> -		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> -	if (data[RTC_DATE] & 0x1f)
> -		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> +		max77686_rtc_data_to_tm(data, &tm, info);
> +
> +		data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> +		if (data[RTC_MONTH] & 0xf)
> +			data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> +		if (data[RTC_YEAR] & info->drv_data->mask)
> +			data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> +		if (data[RTC_DATE] & 0x1f)
> +			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> +
> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +	} else {
> +		ret = regmap_write(info->max77686->regmap,
> +				   map[REG_RTC_AE1], ALARM_ENABLE_VALUE);
> +	}
>  
> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>  				__func__, ret);
> @@ -396,7 +471,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	u8 data[RTC_NR_TIME];
>  	int ret;
>  
> -	ret = max77686_rtc_tm_to_data(&alrm->time, data);
> +	ret = max77686_rtc_tm_to_data(&alrm->time, data, info);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -490,6 +565,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  {
>  	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>  	struct max77686_rtc_info *info;
> +	const struct platform_device_id *id = pdev->id_entry;
>  	int ret;
>  
>  	dev_info(&pdev->dev, "%s\n", __func__);
> @@ -503,7 +579,10 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	info->dev = &pdev->dev;
>  	info->max77686 = max77686;
>  	info->rtc = max77686->rtc;
> -	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;

Comment for previous patch: use platform_get_device_id(pdev).

Best regards,
Krzysztof