Re: [PATCH 3/8] rtc: max77686: Use a driver data struct instead hard-coded values

[Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>]

Hello Krzysztof,

Thanks a lot for your review.

On 01/20/2016 09:45 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, 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@xxxxxxxxxxxxxxx>
> > ---
> >
> >   drivers/rtc/rtc-max77686.c | 47 +++++++++++++++++++++++++++++-----------------
> >   1 file changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> > index ae4d61e7ce4b..441d163dcbeb 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,11 @@ enum {
> >   	RTC_NR_TIME
> >   };
> >
> > +struct rtc_driver_data {
>
> 1. This is only local structure so maybe: max77686_rtc_driver_data?

Agreed.

> 2. Can you put here a comment explaining the purpose of this delay
> (quite generic name) and used units?
>
> > +	unsigned long		delay;
>
> A comment here - what 'mask'? Generic name so explanation would be welcomed.
>
> > +	int			mask;
>
> I think this should be u8 to avoid early promotions to int. It is used
> directly with other u8 values. At the end effect of operation is
> promoted to int anyway but using only u8 for operations looks more
> consistent to me.

Ok, I added comments for these two fields and used u8 for mask instead.
 
> > +};
> > +
> >   struct max77686_rtc_info {
> >   	struct device		*dev;
> >   	struct max77686_dev	*max77686;
> > @@ -63,6 +66,8 @@ struct max77686_rtc_info {
> >
> >   	struct regmap		*regmap;
> >
> > +	struct rtc_driver_data	*drv_data;
>
> That may be pointer to const data. Actually not "may be" but
> even "should be" because you allocate later really a const data and you
> are discarding const-ness with cast.

Agreed, I missed that before.
 
> > +
> >   	int virq;
> >   	int rtc_24hr_mode;
> >   };
> > @@ -72,12 +77,19 @@ enum MAX77686_RTC_OP {
> >   	MAX77686_RTC_READ,
> >   };
> >
> > +static const struct rtc_driver_data max77686_drv_data = {
> > +	.delay = 1600,
> > +	.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;
> > +
> > +	tm->tm_sec = data[RTC_SEC] & mask;
> > +	tm->tm_min = data[RTC_MIN] & mask;
> > +	if (info->rtc_24hr_mode)
> >   		tm->tm_hour = data[RTC_HOUR] & 0x1f;
> >   	else {
> >   		tm->tm_hour = data[RTC_HOUR] & 0x0f;
> > @@ -86,10 +98,10 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> >   	}
> >
> >   	/* Only a single bit is set in data[], so fls() would be equivalent */
> > -	tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1;
> > +	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] & 0x7f) + 100;
> > +	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
> >   	tm->tm_yday = 0;
> >   	tm->tm_isdst = 0;
> >   }
> > @@ -117,6 +129,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
> >   {
> >   	int ret;
> >   	unsigned int data;
> > +	unsigned long delay = info->drv_data->delay;
> >
> >   	if (op == MAX77686_RTC_WRITE)
> >   		data = 1 << RTC_UDR_SHIFT;
> > @@ -129,9 +142,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
> >   		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
> >   				__func__, ret, data);
> >   	else {
> > -		/* Minimum 16ms delay required before RTC update. */
> > -		usleep_range(MAX77686_RTC_UPDATE_DELAY,
> > -			     MAX77686_RTC_UPDATE_DELAY * 2);
> > +		/* Minimum delay required before RTC update. */
> > +		usleep_range(delay, delay * 2);
> >   	}
> >
> >   	return ret;
> > @@ -156,7 +168,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >   		goto out;
> >   	}
> >
> > -	max77686_rtc_data_to_tm(data, tm, info->rtc_24hr_mode);
> > +	max77686_rtc_data_to_tm(data, tm, info);
> >
> >   	ret = rtc_valid_tm(tm);
> >
> > @@ -213,7 +225,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >   		goto out;
> >   	}
> >
> > -	max77686_rtc_data_to_tm(data, &alrm->time, info->rtc_24hr_mode);
> > +	max77686_rtc_data_to_tm(data, &alrm->time, info);
> >
> >   	alrm->enabled = 0;
> >   	for (i = 0; i < RTC_NR_TIME; i++) {
> > @@ -260,7 +272,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
> >   		goto out;
> >   	}
> >
> > -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
> > +	max77686_rtc_data_to_tm(data, &tm, info);
> >
> >   	for (i = 0; i < RTC_NR_TIME; i++)
> >   		data[i] &= ~ALARM_ENABLE_MASK;
> > @@ -299,7 +311,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
> >   		goto out;
> >   	}
> >
> > -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
> > +	max77686_rtc_data_to_tm(data, &tm, info);
> >
> >   	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> >   	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> > @@ -307,7 +319,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
> >   	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> >   	if (data[RTC_MONTH] & 0xf)
> >   		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> > -	if (data[RTC_YEAR] & 0x7f)
> > +	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);
> > @@ -436,6 +448,7 @@ 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;
>
> That cast dropping constness is bad.

Agreed, fixed.
 
> Best regards,
> Krzysztof

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html