Messages in this thread | | | Date | Fri, 22 Apr 2016 01:44:00 +0200 | From | Alexandre Belloni <> | Subject | Re: [PATCH 3/3] rtc: add Cypress FM33256B RTC driver |
| |
Hi,
Looks mostly good, a few comments below
On 20/04/2016 at 13:07:51 +0200, Jeppe Ledet-Pedersen wrote :
Please always include a commit message.
> Signed-off-by: Jeppe Ledet-Pedersen <jlp@gomspace.com> > ---
[...]
> +static int fm33256b_rtc_readtime(struct device *dev, struct rtc_time *tm) > +{ > + int ret; > + struct fm33256b_rtc *rtc = dev_get_drvdata(dev); > + uint8_t time[7];
Use u8 here
> + > + /* Lock time update */ > + ret = regmap_update_bits(rtc->fm33256b->regmap_pc, > + FM33256B_RTC_ALARM_CONTROL_REG, > + FM33256B_R, FM33256B_R); > + if (ret < 0) > + return ret; > + > + ret = regmap_bulk_read(rtc->fm33256b->regmap_pc, > + FM33256B_SECONDS_REG, time, sizeof(time)); > + if (ret < 0) > + return ret; > + > + /* Unlock time update */ > + ret = regmap_update_bits(rtc->fm33256b->regmap_pc, > + FM33256B_RTC_ALARM_CONTROL_REG, > + FM33256B_R, 0); > + if (ret < 0) > + return ret; > + > + tm->tm_sec = bcd2bin(time[0]); > + tm->tm_min = bcd2bin(time[1]); > + tm->tm_hour = bcd2bin(time[2]); > + tm->tm_wday = bcd2bin(time[3]) - 1; > + tm->tm_mday = bcd2bin(time[4]); > + tm->tm_mon = bcd2bin(time[5]) - 1; > + tm->tm_year = bcd2bin(time[6]); > + > + if (tm->tm_year < 70) > + tm->tm_year += 100; > +
I would advise against using that construct. It doesn't work well with leap years and you probably don't care about dates before 2000.
> + return rtc_valid_tm(tm); > +} > + > +static int fm33256b_rtc_settime(struct device *dev, struct rtc_time *tm) > +{ > + int ret; > + struct fm33256b_rtc *rtc = dev_get_drvdata(dev); > + uint8_t time[7]; > +
u8
> + time[0] = bin2bcd(tm->tm_sec); > + time[1] = bin2bcd(tm->tm_min); > + time[2] = bin2bcd(tm->tm_hour); > + time[3] = bin2bcd(tm->tm_wday + 1); > + time[4] = bin2bcd(tm->tm_mday); > + time[5] = bin2bcd(tm->tm_mon + 1); > + time[6] = bin2bcd(tm->tm_year % 100); > +
I would also prefer that you explicitly enforce the range of supported years and return -EINVAL for out of range values.
> + /* Unlock time update */ > + ret = regmap_update_bits(rtc->fm33256b->regmap_pc, > + FM33256B_RTC_ALARM_CONTROL_REG, > + FM33256B_W, FM33256B_W); > + if (ret < 0) > + return ret; > + > + ret = regmap_bulk_write(rtc->fm33256b->regmap_pc, > + FM33256B_SECONDS_REG, time, sizeof(time)); > + if (ret < 0) > + return ret; > + > + /* Lock time update */ > + ret = regmap_update_bits(rtc->fm33256b->regmap_pc, > + FM33256B_RTC_ALARM_CONTROL_REG, > + FM33256B_W, 0); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static const struct rtc_class_ops fm33256b_rtc_ops = { > + .read_time = fm33256b_rtc_readtime, > + .set_time = fm33256b_rtc_settime, > +}; > + > +static int fm33256b_rtc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct fm33256b *fm33256b; > + struct fm33256b_rtc *rtc; > + > + fm33256b = dev_get_drvdata(dev->parent); > + > + rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, rtc); > + > + rtc->fm33256b = fm33256b; > + rtc->rtcdev = devm_rtc_device_register(&pdev->dev, KBUILD_MODNAME, > + &fm33256b_rtc_ops, THIS_MODULE); > + if (IS_ERR(rtc->rtcdev)) > + return PTR_ERR(rtc->rtcdev); > + > + return 0;
You can use PTR_ERR_OR_ZERO
> +} > + > +static int fm33256b_rtc_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct fm33256b *fm33256b; > + struct fm33256b_rtc *rtc = platform_get_drvdata(pdev); > + > + fm33256b = dev_get_drvdata(dev->parent); > + > + devm_rtc_device_unregister(&pdev->dev, rtc->rtcdev); > + > + return 0; > +} > +
The whole fm33256b_rtc_remove() is unnecessary.
> +static const struct of_device_id fm33256b_rtc_dt_ids[] = { > + { .compatible = "cypress,fm33256b-rtc" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, fm33256b_rtc_dt_ids); > + > +static struct platform_driver fm33256b_rtc_driver = { > + .driver = { > + .name = "fm33256b-rtc", > + .of_match_table = fm33256b_rtc_dt_ids, > + }, > + .probe = fm33256b_rtc_probe, > + .remove = fm33256b_rtc_remove, > +}; > +module_platform_driver(fm33256b_rtc_driver); > + > +MODULE_ALIAS("platform:fm33256b-rtc"); > +MODULE_AUTHOR("Jeppe Ledet-Pedersen <jlp@gomspace.com>"); > +MODULE_DESCRIPTION("Cypress FM33256B Processor Companion RTC Driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.1.4 >
-- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
| |