Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 17 May 2022 18:17:45 -0500
From:   Nishanth Menon <nm@ti.com>
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Alessandro Zummo <a.zummo@towertech.it>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-rtc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Andrew Davis <afd@ti.com>
Subject: Re: [PATCH V3 2/2] rtc: Introduce ti-k3-rtc
Message-ID: <20220517231745.j364tz7djodgsh6p@gallon>
References: <20220513194457.25942-1-nm@ti.com>
 <20220513194457.25942-3-nm@ti.com>
 <YoQa6FoJV5eoxloa@mail.local>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YoQa6FoJV5eoxloa@mail.local>
User-Agent: NeoMutt/20171215
Precedence: bulk

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