Re: [PATCH V5 1/3] rtc: atcrtc100: Add ATCRTC100 RTC driver

[CL Wang]

Hi Alexandre,

Thank you very much for your feedback on the patch, and sorry for the delayed response.
Below are my replies to your comments and questions. I will prepare and send the next
version of the patch as soon as possible.

> +#define RTC_MINUTE(x)        ((x >> MIN_OFF) & MIN_MSK)      /* RTC min */
> +#define RTC_HOUR(x)  ((x >> HOUR_OFF) & HOUR_MSK)    /* RTC hour */
> +#define RTC_DAYS(x)  ((x >> DAY_OFF) & DAY_MSK)      /* RTC day */

FIELD_PREP can probably replace those.
=> That's a good suggestion. I will update this to use bitfield-related macros instead.


> +struct atcrtc_dev {
> +     struct rtc_device       *rtc_dev;
> +     struct regmap           *regmap;
> +     struct delayed_work     rtc_work;
> +     struct mutex            lock;

This mutex is not necessary, simply use rtc_lock() in you interrupt handler, the rtc core is already locking before calling the rtc_ops.
=> You're absolutely right. I will remove the mutex and clean up this
   part accordingly.

> +             usleep_range(ATCRTC_TIMEOUT_USLEEP_MIN,
> +                          ATCRTC_TIMEOUT_USLEEP_MAX);
> +     }
> +     dev_err(&rtc->rtc_dev->dev, "Device is busy too long\n");

Is this error message useful, what would be the user action after seeing this?
==> This message indicates that the RTC hardware might be stuck in a busy state.
    If this occurs, it suggests a potential hardware issue. During development, it
    can serve as a hint to review the RTC module's design. In production, a system
    reset might be required to recover. Based on that, I would prefer to keep this
    error message for diagnostic purposes.


> +static time64_t atcrtc_read_rtc_time(struct atcrtc_dev *rtc)

Does this have to be in a separate function?
=> Not necessarily. It can be merged into atcrtc_read_time(). I will
   make this adjustment.


> +     rtc_time64_to_tm(time, tm);
> +     if (rtc_valid_tm(tm) < 0) {

This is not necessary, the core always checks whether the tm is valid.
=> Thanks for pointing that out. I’ll remove this check.


> +     rem -= hour * 3600;
> +     min = rem / 60;
> +     sec = rem - min * 60;

You already had the broken down hour, min and sec, it is not necessary to compute that again here, just fold this function in atcrtc_set_time
=> You're right, I will simplify this part by integrating it directly
   into atcrtc_set_time().

> +     ret = atcrtc_check_write_done(rtc);
> +     if (ret)
> +             return ret;
> +     regmap_update_bits(rtc->regmap, RTC_CR, RTC_EN, RTC_EN);

This is losing some important information, the RTC must only be enabled once the time has been correctly set, then you can check RTC_EN in
atcrtc_read_time() to know whether the time is actually valid or not.
=> I will move the RTC_EN setting to atcrtc_set_time() and add a check for
   this bit in atcrtc_read_time() to ensure the time from RTC is valid.

> +     if (IS_ERR(atcrtc_dev->rtc_dev)) {
> +             dev_err(&pdev->dev,
> +                     "Failed to allocate RTC device: %ld\n",
> +                     PTR_ERR(atcrtc_dev->rtc_dev));
> +             return PTR_ERR(atcrtc_dev->rtc_dev);
> +     }
> +
> +     ret = atcrtc_alarm_enable(&pdev->dev, true);

Can't atcrtc_alarm_enable be part of atcrtc_hw_init so you don't have to wait twice?
=> After reviewing your comment, I agree. I think atcrtc_alarm_enable()
   should instead be integrated into atcrtc_set_alarm() and removed from
   here.

Thanks again for your detailed feedback. I'll revise the patch accordingly
and send out the updated version soon.

Best regards,
CL