| 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
| ||
|
Message-id: <50D26C7D.8010500@samsung.com>
Date: Thu, 20 Dec 2012 10:40:13 +0900
From: jonghwa3.lee@...sung.com
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org,
Alessandro Zummo <a.zummo@...ertech.it>,
Abhilash Kesavan <a.kesavan@...sung.com>,
Chiwoong Byun <woong.byun@...sung.com>,
Myugnjoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
Hi, Andrew,
On 2012년 12월 19일 09:30, Andrew Morton wrote:
> On Wed, 28 Nov 2012 15:50:57 +0900
> Jonghwa Lee <jonghwa3.lee@...sung.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists