lkml.org 
[lkml]   [2022]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V3 2/2] rtc: Introduce ti-k3-rtc
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

\
 
 \ /
  Last update: 2022-05-18 01:19    [W:0.063 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site