Re: [PATCH v0 5/6] ACPI: TAD: Add RTC class device interface

[Rafael J. Wysocki]

Hi,

On Fri, Feb 27, 2026 at 11:37 AM Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxx> wrote:
>
> Hello,
>
> On 22/02/2026 15:18:29+0100, Rafael J. Wysocki wrote:
> > +static int acpi_tad_rtc_procfs(struct device *dev, struct seq_file *seq)
> > +{
> > +     struct acpi_tad_rt rt;
> > +     int ret;
> > +
> > +     ret = acpi_tad_get_real_time(dev, &rt);
> > +     if (ret)
> > +             return ret;
> > +
> > +     seq_printf(seq,
> > +                "Time\t\t: %u:%u:%u\n"
> > +                "Date\t\t: %u-%u-%u\n"
> > +                "Daylight\t: %s\n",
> > +                rt.hour, rt.minute, rt.second,
> > +                rt.year, rt.month, rt.day,
> > +                str_yes_no(rt.daylight == ACPI_TAD_TIME_ISDST));
> > +
> > +     if (rt.tz == ACPI_TAD_TZ_UNSPEC)
> > +             seq_puts(seq, "Timezone\t: unspecified\n");
> > +     else
> > +             seq_printf(seq, "Timezone\t: %d\n", rt.tz);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct rtc_class_ops acpi_tad_rtc_ops = {
> > +     .read_time = acpi_tad_rtc_read_time,
> > +     .set_time = acpi_tad_rtc_set_time,
> > +     .proc = acpi_tad_rtc_procfs,
>
> I would avoid implementing .proc, it has been deprecated for a while and
> doesn't bring much.

OK

> > +};
> > +
> > +/* Platform driver interface */
> > +
> >  static int acpi_tad_disable_timer(struct device *dev, u32 timer_id)
> >  {
> >       return acpi_tad_wake_set(dev, "_STV", timer_id, ACPI_TAD_WAKE_DISABLED);
> > @@ -655,10 +730,16 @@ static int acpi_tad_probe(struct platfor
> >       pm_runtime_suspend(dev);
> >
> >       ret = sysfs_create_group(&dev->kobj, &acpi_tad_attr_group);
> > -     if (ret)
> > +     if (ret) {
> >               acpi_tad_remove(pdev);
> > +             return ret;
> > +     }
> >
> > -     return ret;
> > +     if (caps & ACPI_TAD_RT)
> > +             devm_rtc_device_register(dev, "acpi-tad-rtc", &acpi_tad_rtc_ops,
> > +                                      THIS_MODULE);
> > +
>
> Please use devm_rtc_allocate_device() and devm_rtc_register_device() so
> you can set range_min and range_max. I get you don't need the rtc_device
> later so we could have a helper that takes the range and registers the
> rtc.

I'll need to use them anyway later when I add read/set_alarm to this
device (because the alarm may not be supported in which case I'll need
to set a flag to indicate that before registering the RTC device,
IIUC).