linux-kernel - Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <CR556BV2M4I4.2L3LLJ8V1I352@burritosblues>
Date:   Mon, 13 Mar 2023 10:18:45 +0100
From:   "Esteban Blanc" <eblanc@...libre.com>
To:     "Alexandre Belloni" <alexandre.belloni@...tlin.com>
Cc:     <linus.walleij@...aro.org>, <lgirdwood@...il.com>,
        <broonie@...nel.org>, <a.zummo@...ertech.it>,
        <linux-kernel@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <linux-rtc@...r.kernel.org>, <jpanis@...libre.com>,
        <jneanne@...libre.com>
Subject: Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594
 PMIC RTC

On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > +struct tps6594_rtc {
> > +   struct rtc_device *rtc;
> > +};
>
> Is the struct actually useful?

Good catch, it's not. I will remove it for V2.

(...)

> > +/*
> > + * Gets current tps6594 RTC time and date parameters.
> > + *
> > + * The RTC's time/alarm representation is not what gmtime(3) requires
> > + * Linux to use:
> > + *
> > + *  - Months are 1..12 vs Linux 0-11
> > + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> > + */
>
> I don't find this comment to be particularly useful.

Ok. I propose that I add 2 constants for the -1 and +100 in the month and year
calculation. This way, without the comment the computation would be a
bit more self explanatory.
What do you think?

(...)

> > +static int tps6594_rtc_probe(struct platform_device *pdev)
> > +{
> > +   struct tps6594 *tps6594;
> > +   struct tps6594_rtc *tps_rtc;
> > +   int irq;
> > +   int ret;
> > +
> > +   tps6594 = dev_get_drvdata(pdev->dev.parent);
> > +
> > +   tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> > +                          GFP_KERNEL);
> > +   if (!tps_rtc)
> > +           return -ENOMEM;
> > +
> > +   tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > +   if (IS_ERR(tps_rtc->rtc))
> > +           return PTR_ERR(tps_rtc->rtc);
> > +
> > +   /* Enable crystal oscillator */
> > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> > +                         TPS6594_BIT_XTAL_EN);
> > +   if (ret < 0)
> > +           return ret;
> > +
> > +   /* Start rtc */
> > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> > +                         TPS6594_BIT_STOP_RTC);
> > +   if (ret < 0)
> > +           return ret;
>
> Do that (XTAL_EN and clearing STOP) only once the time is known to be
> set to a correct value so read_time doesn't have a chance to return a
> bogus value.
>

(...)

I understand your point, however I'm not sure of the canonical way to do
this. Simply calling `tps6594_rtc_set_time` is enough?

> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks for your time. Best regards,
-- 
Esteban Blanc
BayLibre