On 00:00-20220518, Alexandre Belloni wrote: > Hello Nishanth, > > I have some very minor comments: Thanks for the review. > > On 13/05/2022 14:44:57-0500, Nishanth Menon wrote: > > diff --git a/drivers/rtc/rtc-ti-k3.c b/drivers/rtc/rtc-ti-k3.c > > new file mode 100644 > > index 000000000000..21a64051fd42 > > --- /dev/null > > +++ b/drivers/rtc/rtc-ti-k3.c > > @@ -0,0 +1,695 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Texas Instruments K3 RTC driver > > + * > > + * Copyright (C) 2021-2022 Texas Instruments Incorporated - https://www.ti.com/ > > + */ > > + > > +#define dev_fmt(fmt) "%s: " fmt, __func__ > > Are you sure you want to keep this line? Saves me the headache of trying to find which function reported it from the logs, but I can drop it. > > > +static int ti_k3_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm) > > +{ > > + struct ti_k3_rtc *priv = dev_get_drvdata(dev); > > + time64_t seconds; > > + int ret; > > + > > + seconds = rtc_tm_to_time64(&alarm->time); > > + > > + k3rtc_field_write(priv, K3RTC_ALM_S_CNT_LSW, seconds); > > + k3rtc_field_write(priv, K3RTC_ALM_S_CNT_MSW, (seconds >> 32)); > > + > > + /* Make sure the alarm time is synced in */ > > + ret = k3rtc_fence(priv); > > + if (ret) { > > + dev_err(dev, "Failed to fence(%d)!\n", ret); > > I'm not sure this message is useful because the only thing the user may > do would be trying to set the time again. I should probably indicate a potential s/w config problem in driver here - but the fail here is crucial for me to understand if there is some other problem that I have'nt un-covered in the testing (hoping none, but everytime I have had a configuration error, this shows up as a symptom allowing me to drill down to the problem). Let me know if you feel strongly about this, will drop. > > > + return ret; > > + } > > + > > + /* Alarm irq enable will do a sync */ > > + return ti_k3_rtc_alarm_irq_enable(dev, alarm->enabled); > > +} > > + > > > > + > > +static int k3rtc_get_vbusclk(struct device *dev, struct ti_k3_rtc *priv) > > +{ > > + int ret; > > + struct clk *clk; > > + > > + /* Note: VBUS is'nt a context clock, it is needed for hardware operation */ > typo ---------------^ yup, will replace with isn't > > > + clk = devm_clk_get(dev, "vbus"); > > + if (IS_ERR(clk)) { > > + dev_err(dev, "No input vbus clock\n"); > > + return PTR_ERR(clk); > > + } > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > + dev_err(dev, "Failed to enable the vbus clock(%d)\n", ret); > > I would also remove those two dev_err OK. > > > + return ret; > > + } > > + > > + ret = devm_add_action_or_reset(dev, (void (*)(void *))clk_disable_unprepare, clk); > > + return ret; And will drop the ret usage here.. just return the result of devm_add_action_or_reset > > +} > > + > > +static int ti_k3_rtc_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct ti_k3_rtc *priv; > > + void __iomem *rtc_base; > > + int ret; > > + [...] > > + ret = k3rtc_configure(dev); > > + if (ret) > > + return ret; > > + > > + if (device_property_present(dev, "wakeup-source")) > > + device_init_wakeup(dev, true); > > + else > > + device_set_wakeup_capable(dev, true); > > + > > + ret = devm_rtc_register_device(priv->rtc_dev); > > + if (ret) > > + return ret; > > + > > + ret = devm_rtc_nvmem_register(priv->rtc_dev, &ti_k3_rtc_nvmem_config); > > + return ret; > > You don't need ret here and if I take that, I'll soon get an > automatically generated patch. Yup. will do. [...] -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
