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

[Esteban Blanc]

On Mon Mar 13, 2023 at 12:01 PM CET, Alexandre Belloni wrote:
> On 13/03/2023 10:18:45+0100, Esteban Blanc wrote:
> > On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> > > On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > > > +/*
> > > > + * 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?
>
> I don't think this is necessary, keep -1 for the month and +100 for the
> year, those are very common operations in the subsystem and don't really
> need any explanation

Ok. I will just remove the comment then.

> > > > +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?
>
> Yeah, let userspace set the time and start the rtc at that point.

The problem with that is we might have some RTCs that will just not be
usable. We have boards with multiple TP6594 PMICs where only one of them
has a crystal oscillator. The way to detect this is to start the RTC
then checked if the STOP_RTC bit is still 0. By doing this in the probe,
I'm able to not register an RTC device that doesn't work.

If I just start the RTC on the first call to `tps6594_rtc_set_time`, it
will work for the RTC with the crystal and fails for all the others 

I can stop the RTC at the end of the probe, after the check to rule out
unusable devices. If I add the check you proposed in
`tps6594_rtc_read_time` it will fail until a successful call to
`tps6594_rtc_set_time`. Would that be a suitable solution?

Best regards,
-- 
Esteban Blanc
BayLibre