linux-kernel - Re: [PATCH 4/8] rtc: max77686: Add an indirection level to access RTC registers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <56A02EEF.2060604@samsung.com>
Date:	Thu, 21 Jan 2016 10:05:51 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Javier Martinez Canillas <javier@....samsung.com>,
	linux-kernel@...r.kernel.org
Cc:	Kukjin Kim <kgene@...nel.org>, rtc-linux@...glegroups.com,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Laxman Dewangan <ldewangan@...dia.com>,
	linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH 4/8] rtc: max77686: Add an indirection level to access RTC
 registers

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The max77686 driver is generic enough that can be used for other
> Maxim RTC IP blocks but these might not have the same registers
> layout so instead of accessing the registers directly, add a map
> to translate offsets to the real registers addresses for each IP.
> 
> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 75 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 441d163dcbeb..7316e41820c7 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -41,6 +41,8 @@
>  #define ALARM_ENABLE_SHIFT		7
>  #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>  
> +#define REG_RTC_NONE			0xdeadbeef
> +
>  enum {
>  	RTC_SEC = 0,
>  	RTC_MIN,
> @@ -55,6 +57,7 @@ enum {
>  struct rtc_driver_data {
>  	unsigned long		delay;
>  	int			mask;
> +	const unsigned int	*map;
>  };
>  
>  struct max77686_rtc_info {
> @@ -77,9 +80,53 @@ enum MAX77686_RTC_OP {
>  	MAX77686_RTC_READ,
>  };
>  
> +/* These are not registers but just offsets that are mapped to addresses */
> +enum rtc_reg {

enum max77686_rtc_reg_offset?

> +	REG_RTC_CONTROLM = 0,
> +	REG_RTC_CONTROL,
> +	REG_RTC_UPDATE0,
> +	REG_RTC_UPDATE1,
> +	REG_WTSR_SMPL_CNTL,
> +	REG_RTC_SEC,
> +	REG_RTC_MIN,
> +	REG_RTC_HOUR,
> +	REG_RTC_WEEKDAY,
> +	REG_RTC_MONTH,
> +	REG_RTC_YEAR,
> +	REG_RTC_DATE,
> +	REG_ALARM1_SEC,
> +	REG_ALARM1_MIN,
> +	REG_ALARM1_HOUR,
> +	REG_ALARM1_WEEKDAY,
> +	REG_ALARM1_MONTH,
> +	REG_ALARM1_YEAR,
> +	REG_ALARM1_DATE,
> +	REG_ALARM2_SEC,
> +	REG_ALARM2_MIN,
> +	REG_ALARM2_HOUR,
> +	REG_ALARM2_WEEKDAY,
> +	REG_ALARM2_MONTH,
> +	REG_ALARM2_YEAR,
> +	REG_ALARM2_DATE,
> +	REG_RTC_END,
> +};
> +

A short comment what is mapped into what would be appreciated.

> +static const unsigned int max77686_map[REG_RTC_END] = {
> +	MAX77686_RTC_CONTROLM, MAX77686_RTC_CONTROL, MAX77686_RTC_UPDATE0,
> +	REG_RTC_NONE, MAX77686_WTSR_SMPL_CNTL, MAX77686_RTC_SEC,
> +	MAX77686_RTC_MIN, MAX77686_RTC_HOUR, MAX77686_RTC_WEEKDAY,
> +	MAX77686_RTC_MONTH, MAX77686_RTC_YEAR, MAX77686_RTC_DATE,
> +	MAX77686_ALARM1_SEC, MAX77686_ALARM1_MIN, MAX77686_ALARM1_HOUR,
> +	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,
> +};

It is difficult to check for mistakes here. I would prefer direct mapping:
	[REG_RTC_CONTROLM] = MAX77686_RTC_CONTROLM,
	....


Rest looks good but I did not check the correctness of mapping above.

BR,
Krzysztof