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:
> > > +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?
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
>
> (...)
>
> > > +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.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
