Hello,
checkpatch.pl --strict complains about multiple blank lines and alignment.
On 31/10/2019 22:38:35+0100, Andreas Kemnade wrote:
> +static int rc5t619_rtc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
> + struct rc5t619_rtc *rtc;
> + uint8_t alarm_flag;
> + unsigned int ctrl2;
> + int err;
> +
> + rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> + if (IS_ERR(rtc)) {
> + err = PTR_ERR(rtc);
> + return -ENOMEM;
> + }
> +
> + rtc->rn5t618 = rn5t618;
> +
> + dev_set_drvdata(dev, rtc);
> + rtc->irq = -1;
> +
> + if (rn5t618->irq_data)
> + rtc->irq = regmap_irq_get_virq(rn5t618->irq_data,
> + RN5T618_IRQ_RTC);
> +
> + if (rtc->irq < 0) {
> + dev_err(dev, "no irq specified, wakeup is disabled\n");
I don't think it is worth having an error message here, especially since
you have a second one later.
> + rtc->irq = -1;
> + }
> +
> + err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
> + if (err < 0)
> + return err;
> +
> + /* get interrupt flag */
> + err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> + if (err)
> + return err;
> +
> + /* disable rtc periodic function */
> + err = rc5t619_rtc_periodic_disable(&pdev->dev);
> + if (err)
> + return err;
> +
> + /* disable interrupt */
> + err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> + if (err)
> + return err;
Is it really useful to disable the alarm to reenable them later?
> +
> + if (ctrl2 & CTRL2_PON) {
> + alarm_flag = 0;
> + err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> + if (err)
> + return err;
> + }
> +
> + rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> +
Please remove this blank line.
> + if (IS_ERR(rtc->rtc)) {
> + err = PTR_ERR(rtc->rtc);
> + dev_err(dev, "RTC device register: err %d\n", err);
> + return err;
> + }
> +
> + rtc->rtc->ops = &rc5t619_rtc_ops;
> + rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_1900;
> + rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> + /* set interrupt and enable it */
> + if (rtc->irq != -1) {
> + device_init_wakeup(&pdev->dev, 1);
> +
> + err = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> + rc5t619_rtc_irq,
> + IRQF_ONESHOT,
> + "rtc-rc5t619",
> + &pdev->dev);
> + if (err < 0) {
> + dev_err(&pdev->dev, "request IRQ:%d fail\n", rtc->irq);
> + rtc->irq = -1;
> +
> + err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> + if (err)
> + return err;
> +
> + } else {
> + /* enable wake */
I think you should move device_init_wakeup() here, unless your parse the
wakeup-source property.
> + enable_irq_wake(rtc->irq);
> + /* enable alarm_d */
> + err = rc5t619_rtc_alarm_enable(&pdev->dev, alarm_flag);
> + if (err) {
> + dev_err(&pdev->dev, "failed rtc setup\n");
> + return -EBUSY;
> + }
> + }
> + } else {
> + /* system don't want to using alarm interrupt, so close it */
> + err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> + if (err) {
> + dev_err(&pdev->dev, "disable rtc alarm error\n");
I don't think this message is necessary.
> + return err;
> + }
> +
> + dev_err(&pdev->dev, "ricoh61x interrupt is disabled\n");
Maybe dev_warn as the driver just continues on.
> + }
> +
> + return rtc_register_device(rtc->rtc);
> +}
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
