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: <56A0F1BD.3010203@osg.samsung.com>
Date:	Thu, 21 Jan 2016 11:57:01 -0300
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.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

Hello Krzysztof,

Thanks a lot for your review.

On 01/20/2016 10:05 PM, Krzysztof Kozlowski wrote:
> 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?
>

Agreed.
  
>> +	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.
>

Ok, added such comment for v2.
  
>> +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,
> 	....
>

Right, that's a very good idea.
  
>
> Rest looks good but I did not check the correctness of mapping above.
>

Great, thanks.
  
> BR,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America