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 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, ); >> +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); >
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@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 @@
Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Tue, 2012-12-18 at 16:30 -0800, Andrew Morton wrote: > On Wed, 28 Nov 2012 15:50:57 +0900 > Jonghwa Lee 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? fls(shifted) - 1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Wed, 28 Nov 2012 15:50:57 +0900 Jonghwa Lee 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, ); > + 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
Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Wed, 28 Nov 2012 15:50:57 +0900 Jonghwa Lee jonghwa3@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. ... --- 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 =
Re: [rtc-linux] [RESEND PATCH] RTC: MAX77686: Add Maxim 77686 driver
On Tue, 2012-12-18 at 16:30 -0800, Andrew Morton wrote: On Wed, 28 Nov 2012 15:50:57 +0900 Jonghwa Lee jonghwa3@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? fls(shifted) - 1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/