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