Re: [PATCH v2 04/10] rtc: max77686: Use a driver data struct instead hard-coded values

[Javier Martinez Canillas <javier@osg.samsung.com>]

Hello Krzysztof,

On 01/21/2016 10:00 PM, Krzysztof Kozlowski wrote:
> On 22.01.2016 05:23, Javier Martinez Canillas wrote:
>> The driver has some hard-coded values such as the minimum delay needed
>> before a RTC update or the mask used for the sec/min/hour/etc registers.
>>
>> Use a data structure that contains these values and pass as driver data
>> using the platform device ID table for each device.
>>
>> This allows to make the driver's ops callbacks more generic so other RTC
>> that are similar but don't have the same values can also be supported.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>>
>> Changes in v2:
>> - Add a max77686 prefix to rtc_driver_data. Suggested by Krzysztof Kozlowski.
>> - Comment about the .delay and .mask fields. Suggested by Krzysztof Kozlowski.
>> - Change .mask type to u8. Suggested by Krzysztof Kozlowski.
>> - Make .drv_data field const. Suggested by Krzysztof Kozlowski.
>> - Don't cast to drop const on .drv_data asign. Suggested by Krzysztof Kozlowski.
>> - Use platform_get_device_id() macro. Suggested by Krzysztof Kozlowski.
>>
>>   drivers/rtc/rtc-max77686.c | 51 ++++++++++++++++++++++++++++++----------------
>>   1 file changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index 71ef2240b3fc..16033a2c2756 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -41,8 +41,6 @@
>>   #define ALARM_ENABLE_SHIFT		7
>>   #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>>
>> -#define MAX77686_RTC_UPDATE_DELAY	16000
>> -
>>   enum {
>>   	RTC_SEC = 0,
>>   	RTC_MIN,
>> @@ -54,6 +52,13 @@ enum {
>>   	RTC_NR_TIME
>>   };
>>
>> +struct max77686_rtc_driver_data {
>> +	/* Minimum delay needed for a RTC update */
>> +	unsigned long		delay;
>> +	/* Mask used to read RTC registers value */
>> +	u8			mask;
>> +};
>> +
>>   struct max77686_rtc_info {
>>   	struct device		*dev;
>>   	struct max77686_dev	*max77686;
>> @@ -63,6 +68,8 @@ struct max77686_rtc_info {
>>
>>   	struct regmap		*regmap;
>>
>> +	const struct max77686_rtc_driver_data *drv_data;
>> +
>>   	int virq;
>>   	int rtc_24hr_mode;
>>   };
>> @@ -72,12 +79,19 @@ enum MAX77686_RTC_OP {
>>   	MAX77686_RTC_READ,
>>   };
>>
>> +static const struct max77686_rtc_driver_data max77686_drv_data = {
>> +	.delay = 1600,
>
> 16000.
> wakealarm does not work with 1600.
>
>

Sigh, sorry for the silly typo error...
  
>> +	.mask  = 0x7f,
>> +};
>> +
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> -				   int rtc_24hr_mode)
>> +				    struct max77686_rtc_info *info)
>>   {
>> -	tm->tm_sec = data[RTC_SEC] & 0x7f;
>> -	tm->tm_min = data[RTC_MIN] & 0x7f;
>> -	if (rtc_24hr_mode)
>> +	int mask = info->drv_data->mask;
>
> u8 mask
>

Right, I forgot to change this when used u8 for the mask field.
  
> Best regards,
> Krzysztof
>

If you don't mind I'll wait a couple of days before posting a v3
otherwise I may be re-spining too fast and spamming other people
that didn't have time to look at the series yet.

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