Patchwork [RESEND] RTC: MAX77686: Add Maxim 77686 driver

login
register
mail settings
Submitter Andrew Morton
Date Dec. 19, 2012, 12:30 a.m.
Message ID <20121218163009.fb21fec8.akpm@linux-foundation.org>
Download mbox | patch
Permalink /patch/207233/
State New
Headers show

Comments

Andrew Morton - Dec. 19, 2012, 12:30 a.m.
On Wed, 28 Nov 2012 15:50:57 +0900
Jonghwa Lee <jonghwa3.lee@samsung.com> wrote:

> Add driver for support max77686 rtc.
> MAX77686 rtc support smpl and wtsr mode. It has two alarm register
> which can be used for alarming to wake system up. This drvier uses regmap
> to access its register.
> 
> ...
>
> +static inline int max77686_rtc_calculate_wday(u8 shifted)
> +{
> +	int counter = -1;
> +	while (shifted) {
> +		shifted >>= 1;
> +		counter++;
> +	}
> +	return counter;
> +}

We should be able to use log2() in here?

>
> ...
>
> +static inline int max77686_rtc_update(struct max77686_rtc_info *info,
> +	enum MAX77686_RTC_OP op)
> +{
> +	int ret;
> +	unsigned int data;
> +
> +	switch (op) {
> +	case MAX77686_RTC_WRITE:
> +		data = 1 << RTC_UDR_SHIFT;
> +		break;
> +	case MAX77686_RTC_READ:
> +		data = 1 << RTC_RBUDR_SHIFT;
> +		break;
> +	}
> +
> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
> +				 MAX77686_RTC_UPDATE0, data, data);
> +	if (ret < 0)
> +		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. */
> +		msleep(MAX77686_RTC_UPDATE_DELAY);
> +	}
> +
> +	return ret;
> +}

This function has about ten callers.  No way in the world do we want it
to be inlined!  Probably the compiler will just ignore your `inline',
but we should remove it to be sure.

>
> ...
>
> +static const struct rtc_class_ops max77686_rtc_ops = {
> +	.read_time = max77686_rtc_read_time,
> +	.set_time = max77686_rtc_set_time,
> +	.read_alarm = max77686_rtc_read_alarm,
> +	.set_alarm = max77686_rtc_set_alarm,
> +	.alarm_irq_enable = max77686_rtc_alarm_irq_enable,
> +};
> +
> +#ifdef MAX77686_RTC_WTSR_SMPL

hm, this is always undefined.  Why is this code here?

> +static void max77686_rtc_enable_wtsr(struct max77686_rtc_info *info, bool enable)
> +{
> +	int ret;
> +	unsigned int val, mask;
> +
> +	if (enable)
> +		val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
> +	else
> +		val = 0;
> +
> +	mask = WTSR_EN_MASK | WTSRT_MASK;
> +
> +	dev_info(info->dev, "%s: %s WTSR\n", __func__,
> +			enable ? "enable" : "disable");
> +
> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
> +				 MAX77686_WTSR_SMPL_CNTL, mask, val);
> +	if (ret < 0) {
> +		dev_err(info->dev, "%s: fail to update WTSR reg(%d)\n",
> +				__func__, ret);
> +		return;
> +	}
> +
> +	max77686_rtc_update(info, MAX77686_RTC_WRITE);
> +}
> +
> +static void max77686_rtc_enable_smpl(struct max77686_rtc_info *info, bool enable)
> +{
> +	int ret;
> +	unsigned int val, mask;
> +
> +	if (enable)
> +		val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
> +	else
> +		val = 0;
> +
> +	mask = SMPL_EN_MASK | SMPLT_MASK;
> +
> +	dev_info(info->dev, "%s: %s SMPL\n", __func__,
> +			enable ? "enable" : "disable");
> +
> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
> +				 MAX77686_WTSR_SMPL_CNTL, mask, val);
> +	if (ret < 0) {
> +		dev_err(info->dev, "%s: fail to update SMPL reg(%d)\n",
> +				__func__, ret);
> +		return;
> +	}
> +
> +	max77686_rtc_update(info, MAX77686_RTC_WRITE);
> +
> +	val = 0;
> +	regmap_read(info->max77686->rtc_regmap, MAX77686_WTSR_SMPL_CNTL, &val);
> +	pr_info("%s: WTSR_SMPL(0x%02x)\n", __func__, val);
> +}
> +#endif /* MAX77686_RTC_WTSR_SMPL */
>
> ...
>
> +static int __devinit max77686_rtc_probe(struct platform_device *pdev)

CONFIG_HOTPLUG is removed, so we should stop using devinit, devexit,
devinit_p, etc.

>
> ...
>
Jonghwa Lee - Dec. 20, 2012, 1:40 a.m.
Hi, Andrew,
On 2012년 12월 19일 09:30, Andrew Morton wrote:
> On Wed, 28 Nov 2012 15:50:57 +0900
> Jonghwa Lee <jonghwa3.lee@samsung.com> wrote:
> 
>> Add driver for support max77686 rtc.
>> MAX77686 rtc support smpl and wtsr mode. It has two alarm register
>> which can be used for alarming to wake system up. This drvier uses regmap
>> to access its register.
>>
>> ...
>>
>> +static inline int max77686_rtc_calculate_wday(u8 shifted)
>> +{
>> +	int counter = -1;
>> +	while (shifted) {
>> +		shifted >>= 1;
>> +		counter++;
>> +	}
>> +	return counter;
>> +}
> 
> We should be able to use log2() in here?
> 
Okay, and I'd like to use Joe's recommendation. fls(shitfted) - 1

>>
>> ...
>>
>> +static inline int max77686_rtc_update(struct max77686_rtc_info *info,
>> +	enum MAX77686_RTC_OP op)
>> +{
>> +	int ret;
>> +	unsigned int data;
>> +
>> +	switch (op) {
>> +	case MAX77686_RTC_WRITE:
>> +		data = 1 << RTC_UDR_SHIFT;
>> +		break;
>> +	case MAX77686_RTC_READ:
>> +		data = 1 << RTC_RBUDR_SHIFT;
>> +		break;
>> +	}
>> +
>> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
>> +				 MAX77686_RTC_UPDATE0, data, data);
>> +	if (ret < 0)
>> +		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. */
>> +		msleep(MAX77686_RTC_UPDATE_DELAY);
>> +	}
>> +
>> +	return ret;
>> +}
> 
> This function has about ten callers.  No way in the world do we want it
> to be inlined!  Probably the compiler will just ignore your `inline',
> but we should remove it to be sure.
> 
Okay, I'll remove it.
>>
>> ...
>>
>> +static const struct rtc_class_ops max77686_rtc_ops = {
>> +	.read_time = max77686_rtc_read_time,
>> +	.set_time = max77686_rtc_set_time,
>> +	.read_alarm = max77686_rtc_read_alarm,
>> +	.set_alarm = max77686_rtc_set_alarm,
>> +	.alarm_irq_enable = max77686_rtc_alarm_irq_enable,
>> +};
>> +
>> +#ifdef MAX77686_RTC_WTSR_SMPL
> 
> hm, this is always undefined.  Why is this code here?
>
Yes, you're right, It isn't needed. I'll fix it right.

>> +static void max77686_rtc_enable_wtsr(struct max77686_rtc_info *info, bool enable)
>> +{
>> +	int ret;
>> +	unsigned int val, mask;
>> +
>> +	if (enable)
>> +		val = (1 << WTSR_EN_SHIFT) | (3 << WTSRT_SHIFT);
>> +	else
>> +		val = 0;
>> +
>> +	mask = WTSR_EN_MASK | WTSRT_MASK;
>> +
>> +	dev_info(info->dev, "%s: %s WTSR\n", __func__,
>> +			enable ? "enable" : "disable");
>> +
>> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
>> +				 MAX77686_WTSR_SMPL_CNTL, mask, val);
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "%s: fail to update WTSR reg(%d)\n",
>> +				__func__, ret);
>> +		return;
>> +	}
>> +
>> +	max77686_rtc_update(info, MAX77686_RTC_WRITE);
>> +}
>> +
>> +static void max77686_rtc_enable_smpl(struct max77686_rtc_info *info, bool enable)
>> +{
>> +	int ret;
>> +	unsigned int val, mask;
>> +
>> +	if (enable)
>> +		val = (1 << SMPL_EN_SHIFT) | (0 << SMPLT_SHIFT);
>> +	else
>> +		val = 0;
>> +
>> +	mask = SMPL_EN_MASK | SMPLT_MASK;
>> +
>> +	dev_info(info->dev, "%s: %s SMPL\n", __func__,
>> +			enable ? "enable" : "disable");
>> +
>> +	ret = regmap_update_bits(info->max77686->rtc_regmap,
>> +				 MAX77686_WTSR_SMPL_CNTL, mask, val);
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "%s: fail to update SMPL reg(%d)\n",
>> +				__func__, ret);
>> +		return;
>> +	}
>> +
>> +	max77686_rtc_update(info, MAX77686_RTC_WRITE);
>> +
>> +	val = 0;
>> +	regmap_read(info->max77686->rtc_regmap, MAX77686_WTSR_SMPL_CNTL, &val);
>> +	pr_info("%s: WTSR_SMPL(0x%02x)\n", __func__, val);
>> +}
>> +#endif /* MAX77686_RTC_WTSR_SMPL */
>>
>> ...
>>
>> +static int __devinit max77686_rtc_probe(struct platform_device *pdev)
> 
> CONFIG_HOTPLUG is removed, so we should stop using devinit, devexit,
> devinit_p, etc.
> 
>>
>> ...
>>
> 
> --- a/drivers/rtc/rtc-max77686.c~rtc-max77686-add-maxim-77686-driver-fix
> +++ a/drivers/rtc/rtc-max77686.c
> @@ -129,7 +129,7 @@ static int max77686_rtc_tm_to_data(struc
>  	return 0;
>  }
>  
> -static inline int max77686_rtc_update(struct max77686_rtc_info *info,
> +static int max77686_rtc_update(struct max77686_rtc_info *info,
>  	enum MAX77686_RTC_OP op)
>  {
>  	int ret;
> @@ -501,7 +501,7 @@ static struct regmap_config max77686_rtc
>  	.val_bits = 8,
>  };
>  
> -static int __devinit max77686_rtc_probe(struct platform_device *pdev)
> +static int max77686_rtc_probe(struct platform_device *pdev)
>  {
>  	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>  	struct max77686_rtc_info *info;
> @@ -575,7 +575,7 @@ out:
>  	return ret;
>  }
>  
> -static int __devexit max77686_rtc_remove(struct platform_device *pdev)
> +static int max77686_rtc_remove(struct platform_device *pdev)
>  {
>  	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
>  
> @@ -623,7 +623,7 @@ static struct platform_driver max77686_r
>  		.owner	= THIS_MODULE,
>  	},
>  	.probe		= max77686_rtc_probe,
> -	.remove		= __devexit_p(max77686_rtc_remove),
> +	.remove		= max77686_rtc_remove,
>  	.shutdown	= max77686_rtc_shutdown,
>  	.id_table	= rtc_id,
>  };
> _
> 
> 
Best regards
Jonghwa.

Patch

--- a/drivers/rtc/rtc-max77686.c~rtc-max77686-add-maxim-77686-driver-fix
+++ a/drivers/rtc/rtc-max77686.c
@@ -129,7 +129,7 @@  static int max77686_rtc_tm_to_data(struc
 	return 0;
 }
 
-static inline int max77686_rtc_update(struct max77686_rtc_info *info,
+static int max77686_rtc_update(struct max77686_rtc_info *info,
 	enum MAX77686_RTC_OP op)
 {
 	int ret;
@@ -501,7 +501,7 @@  static struct regmap_config max77686_rtc
 	.val_bits = 8,
 };
 
-static int __devinit max77686_rtc_probe(struct platform_device *pdev)
+static int max77686_rtc_probe(struct platform_device *pdev)
 {
 	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
 	struct max77686_rtc_info *info;
@@ -575,7 +575,7 @@  out:
 	return ret;
 }
 
-static int __devexit max77686_rtc_remove(struct platform_device *pdev)
+static int max77686_rtc_remove(struct platform_device *pdev)
 {
 	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
 
@@ -623,7 +623,7 @@  static struct platform_driver max77686_r
 		.owner	= THIS_MODULE,
 	},
 	.probe		= max77686_rtc_probe,
-	.remove		= __devexit_p(max77686_rtc_remove),
+	.remove		= max77686_rtc_remove,
 	.shutdown	= max77686_rtc_shutdown,
 	.id_table	= rtc_id,
 };