Re: [PATCH v5 3/4] rtc: rtc-tps6594: Add support for TPS6594 PMIC RTC

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Matt Ranostay <mranostay@ti.com>
Cc: michael@walle.cc, vigneshr@ti.com, robh@kernel.org,
	krzysztof.kozlowski@linaro.org, a.zummo@towertech.it,
	linus.walleij@linaro.org, lee@kernel.org, brgl@bgdev.pl,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-gpio@vger.kernel.org, linux-rtc@vger.kernel.org,
	Keerthy <j-keerthy@ti.com>
Subject: Re: [PATCH v5 3/4] rtc: rtc-tps6594: Add support for TPS6594 PMIC RTC
Date: Sun, 11 Dec 2022 21:05:46 +0100	[thread overview]
Message-ID: <Y5Y4GlHc0aI6GG2g@mail.local> (raw)
In-Reply-To: <20221123053512.1195309-4-mranostay@ti.com>

Hello,

On 22/11/2022 21:35:11-0800, Matt Ranostay wrote:
> +static int tps6594_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned char rtc_data[TPS6594_NUM_TIME_REGS];
> +	struct tps6594 *tps6594 = dev_get_drvdata(dev->parent);
> +	unsigned int val;
> +	int ret;
> +
> +	rtc_data[0] = bin2bcd(tm->tm_sec);
> +	rtc_data[1] = bin2bcd(tm->tm_min);
> +	rtc_data[2] = bin2bcd(tm->tm_hour);
> +	rtc_data[3] = bin2bcd(tm->tm_mday);
> +	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
> +	rtc_data[5] = bin2bcd(tm->tm_year - 100);
> +
> +	/* Stop RTC while updating the RTC time registers */
> +	ret = regmap_update_bits(tps6594->regmap, TPS6594_RTC_CTRL_1,
> +				 TPS6594_RTC_CTRL_REG_STOP_RTC, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC stop failed, err = %d\n", ret);

This function is way too verbose. There is no other action for the user
than retrying to set the time. You should probably remove all the
dev_err calls.

> +		return ret;
> +	}
> +
> +	/* Waiting till RTC isn't running */
> +	ret = regmap_read_poll_timeout(tps6594->regmap, TPS6594_RTC_STATUS,
> +				       val, !(val & TPS6594_RTC_STATUS_RUN),
> +				       TPS6594_RTC_POLL, TPS6594_RTC_TIMEOUT);
> +	if (ret) {
> +		dev_err(dev, "RTC_STATUS is still RUNNING\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_bulk_write(tps6594->regmap, TPS6594_RTC_SECONDS,
> +		rtc_data, TPS6594_NUM_TIME_REGS);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC_SECONDS reg write failed, err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Start back RTC */
> +	ret = regmap_update_bits(tps6594->regmap, TPS6594_RTC_CTRL_1,
> +				 TPS6594_RTC_CTRL_REG_STOP_RTC,
> +				 TPS6594_RTC_CTRL_REG_STOP_RTC);
> +	if (ret < 0)
> +		dev_err(dev, "RTC start failed, err = %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static const struct rtc_class_ops tps6594_rtc_ops = {
> +	.read_time	= tps6594_rtc_read_time,
> +	.set_time	= tps6594_rtc_set_time,
> +};
> +
> +static int tps6594_rtc_probe(struct platform_device *pdev)
> +{
> +	struct tps6594_rtc *tps6594_rtc;
> +
> +	tps6594_rtc = devm_kzalloc(&pdev->dev, sizeof(*tps6594_rtc), GFP_KERNEL);
> +	if (!tps6594_rtc)
> +		return -ENOMEM;
> +
> +	tps6594_rtc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, tps6594_rtc);
> +
> +	tps6594_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(tps6594_rtc->rtc))
> +		return PTR_ERR(tps6594_rtc->rtc);
> +
> +	tps6594_rtc->rtc->ops = &tps6594_rtc_ops;
> +
> +	return devm_rtc_register_device(tps6594_rtc->rtc);
> +}
> +
> +static const struct of_device_id of_tps6594_rtc_match[] = {
> +	{ .compatible = "ti,tps6594-rtc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_tps6594_rtc_match);
> +
> +static struct platform_driver tps6594_rtc_driver = {
> +	.probe		= tps6594_rtc_probe,
> +	.driver		= {
> +		.name	= "tps6594-rtc",
> +		.of_match_table = of_tps6594_rtc_match,
> +	},
> +};
> +
> +module_platform_driver(tps6594_rtc_driver);
> +
> +MODULE_ALIAS("platform:tps6594-rtc");
> +MODULE_DESCRIPTION("TI TPS6594 series RTC driver");
> +MODULE_AUTHOR("Keerthy J <j-keerthy@ti.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.38.GIT
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com