All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
Cc: a.zummo@towertech.it, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor@kernel.org,
	conor+dt@kernel.org, chao.wei@sophgo.com,
	unicorn_wang@outlook.com, paul.walmsley@sifive.com,
	palmer@dabbelt.com, aou@eecs.berkeley.edu,
	linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, dlan@gentoo.org,
	inochiama@outlook.com
Subject: Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC
Date: Tue, 26 Dec 2023 13:37:43 +0100	[thread overview]
Message-ID: <20231226123743bd2d3f0d@mail.local> (raw)
In-Reply-To: <20231226100431.331616-4-qiujingbao.dlmu@gmail.com>

Hello,

please run checkpatch.pl --strict, there are a few issues.

On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote:
> +struct cv1800_rtc_priv {
> +	struct rtc_device *rtc_dev;
> +	struct device *dev;
> +	struct regmap *rtc_map;
> +	struct clk *clk;
> +	spinlock_t rtc_lock;

This lock seems unnecessary, please check

> +	int irq;
> +};
> +
> +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +
> +	if (enabled)
> +		regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);
> +	else
> +		regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +

This could be:
	regmap_write(info->rtc_map, ALARM_ENABLE, enabled);

> +	return 0;
> +}
> +
> +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned long alarm_time;
> +
> +	alarm_time = rtc_tm_to_time64(&alrm->time);
> +
> +	if (alarm_time > SEC_MAX_VAL)
> +		return -EINVAL;

The core is already checking fr this.

> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +
> +	udelay(DEALY_TIME_PREPARE);

Why is this needed?

> +
> +	regmap_write(info->rtc_map, ALARM_TIME, alarm_time);
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN);

You must follow alrm->enabled instead of unconditionally enabling the
alarm.

> +
> +	return 0;
> +}
> +


> +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info)

Please explain those two calibration functions. I don't think you can
achieve what you want to do.

> +{
> +	uint32_t calib_val = 0;
> +	uint32_t coarse_val = 0;
> +	uint32_t time_now = 0;
> +	uint32_t time_next = 0;
> +	uint32_t offset = CALIB_OFFSET_INIT;
> +	uint32_t coarse_timeout = REG_INIT_TIMEOUT;
> +	uint32_t get_val_timeout = 0;
> +
> +	regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL);
> +
> +	udelay(DEALY_TIME_PREPARE);
> +
> +	/* Select 32K OSC tuning val source from rtc_sys */
> +	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> +			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> +			   (unsigned int)(~SEC_PULSE_SEL_INNER));
> +
> +	regmap_read(info->rtc_map, ANA_CALIB, &calib_val);
> +
> +	regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN);
> +
> +	while (--coarse_timeout) {
> +		regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now);
> +		time_now >>= FC_COARSE_CAL_TIME_SHIFT;
> +
> +		get_val_timeout = REG_INIT_TIMEOUT;
> +
> +		while (time_next <= time_now &&
> +		       --get_val_timeout) {
> +			regmap_read(info->rtc_map, FC_COARSE_CAL,
> +				    &time_next);
> +			time_next >>= FC_COARSE_CAL_TIME_SHIFT;
> +			udelay(DEALY_TIME_LOOP);
> +		}
> +
> +		if (!get_val_timeout)
> +			return -1;
> +
> +		udelay(DEALY_TIME_PREPARE);
> +
> +		regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val);
> +		coarse_val &= FC_COARSE_CAL_VAL_MASK;
> +
> +		if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) {
> +			calib_val += offset;
> +			offset >>= CALIB_OFFSET_SHIFT;
> +			regmap_write(info->rtc_map, ANA_CALIB,
> +				     calib_val);
> +		} else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) {
> +			calib_val -= offset;
> +			offset >>= CALIB_OFFSET_SHIFT;
> +			regmap_write(info->rtc_map, ANA_CALIB,
> +				     calib_val);
> +		} else {
> +			regmap_write(info->rtc_map, FC_COARSE_EN,
> +				     REG_DISABLE_FUN);
> +			break;
> +		}
> +
> +		if (offset == 0)
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info)
> +{
> +	uint32_t val;
> +	uint64_t freq = CALIB_FREQ;
> +	uint32_t sec_cnt;
> +	uint32_t timeout = REG_INIT_TIMEOUT;
> +	uint32_t time_now = 0;
> +	uint32_t time_next = 0;
> +
> +	regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN);
> +
> +	regmap_read(info->rtc_map, FC_FINE_CAL, &time_now);
> +	time_now >>= FC_FINE_CAL_TIME_SHIFT;
> +
> +	while (time_next <= time_now && --timeout) {
> +		regmap_read(info->rtc_map, FC_FINE_CAL, &time_next);
> +		time_next >>= FC_FINE_CAL_TIME_SHIFT;
> +		udelay(DEALY_TIME_LOOP);
> +	}
> +
> +	if (!timeout)
> +		return -1;
> +
> +	regmap_read(info->rtc_map, FC_FINE_CAL, &val);
> +	val &= FC_FINE_CAL_VAL_MASK;
> +
> +	do_div(freq, CALIB_FREQ_NS);
> +	freq = freq * CALIB_FRAC_EXT;
> +	do_div(freq, val);
> +
> +	sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) /
> +			   CALIB_FRAC_EXT &
> +		   SEC_PULSE_GEN_INT_MASK) +
> +		  (freq << SEC_PULSE_GEN_FRAC_SHIFT);
> +
> +	regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt);
> +	regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN);
> +
> +	return 0;
> +}
> +
> +static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info)
> +{
> +	/* select inner sec pulse and select reg set calibration val */
> +	regmap_update_bits(info->rtc_map, SEC_PULSE_GEN,
> +			   (unsigned int)(~SEC_PULSE_GEN_SEL_MASK),
> +			   (unsigned int)(~SEC_PULSE_SEL_INNER));
> +
> +	regmap_update_bits(info->rtc_map, ANA_CALIB,
> +			   (unsigned int)(~CALIB_SEL_FTUNE_MASK),
> +			   CALIB_SEL_FTUNE_INNER);
> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);

Don't disable alarms on probe.

> +}
> +
> +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned int sec;
> +	unsigned int sec_ro_t;
> +	unsigned long flag;
> +
> +	spin_lock_irqsave(&info->rtc_lock, flag);
> +
> +	regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec);
> +	regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t);
> +
> +	if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) {
> +		sec = sec_ro_t;
> +		regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> +		regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);

What does this do?

> +	}
> +
> +	spin_unlock_irqrestore(&info->rtc_lock, flag);
> +
> +	rtc_time64_to_tm(sec, tm);
> +
> +	return 0;
> +}
> +
> +static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	unsigned long sec;
> +	int ret;
> +	unsigned long flag;
> +
> +	ret = rtc_valid_tm(tm);

This is useless, tm will always be valid

> +	if (ret)
> +		return ret;
> +
> +	sec = rtc_tm_to_time64(tm);
> +
> +	spin_lock_irqsave(&info->rtc_lock, flag);
> +
> +	regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec);
> +	regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN);
> +
> +	regmap_write(info->rtc_map, MACRO_RG_SET_T, sec);
> +
> +	spin_unlock_irqrestore(&info->rtc_lock, flag);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +	struct cv1800_rtc_priv *info = dev_get_drvdata(dev);
> +	struct rtc_wkalrm alrm;
> +
> +	regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN);
> +
> +	rtc_read_alarm(info->rtc_dev, &alrm);
> +	alrm.enabled = 0;
> +	rtc_set_alarm(info->rtc_dev, &alrm);

I don't get what you are doing here, you should call rtc_update_irq, not
mess with alarms that have been set.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct rtc_class_ops cv800b_rtc_ops = {
> +	.read_time = cv1800_rtc_read_time,
> +	.set_time = cv1800_rtc_set_time,
> +	.read_alarm = cv1800_rtc_read_alarm,
> +	.set_alarm = cv1800_rtc_set_alarm,
> +	.alarm_irq_enable = cv1800_rtc_alarm_irq_enable,
> +};
> +
> +static int cv1800_rtc_probe(struct platform_device *pdev)
> +{
> +	struct cv1800_rtc_priv *rtc;
> +	uint32_t ctrl_val;
> +	int ret;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv),
> +			   GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	rtc->dev = &pdev->dev;
> +
> +	rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent);
> +	if (IS_ERR(rtc->rtc_map))
> +		return PTR_ERR(rtc->rtc_map);
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0)
> +		return rtc->irq;
> +
> +	ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler,
> +			       IRQF_TRIGGER_HIGH, "alarm", &pdev->dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "cannot register interrupt handler\n");
> +
> +	rtc->clk = devm_clk_get(rtc->dev, NULL);
> +	if (IS_ERR(rtc->clk))
> +		return PTR_ERR(rtc->clk);
> +

You are going to leak rtc->clk after the next call.

> +	rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(rtc->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk),
> +				     "clk not found\n");
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	spin_lock_init(&rtc->rtc_lock);
> +
> +	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev,
> +								dev_name(&pdev->dev),
> +								&cv800b_rtc_ops,
> +								THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev),
> +				     "can't register rtc device\n");

Please use devm_rtc_allocate_device and devm_rtc_register_device

> +
> +	/* if use internal clk,so coarse calibrate rtc */
> +	regmap_read(rtc->rtc_map, CTRL, &ctrl_val);
> +	ctrl_val &= CTRL_MODE_MASK;
> +
> +	if (ctrl_val == CTRL_MODE_OSC32K) {
> +		ret = cv1800_rtc_32k_coarse_val_calib(rtc);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to coarse RTC val !\n");
> +
> +		ret = cv1800_rtc_32k_fine_val_calib(rtc);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to fine RTC val !\n");
> +	}
> +
> +	rtc_enable_sec_counter(rtc);

I'm pretty sure you don't want to do that on every probe.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id cv1800_dt_ids[] = {
> +	{ .compatible = "sophgo,cv1800b-rtc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, cv1800_dt_ids);
> +
> +static struct platform_driver cv1800_rtc_driver = {
> +	.driver = {
> +			.name = "sophgo-cv800b-rtc",
> +			.of_match_table = cv1800_dt_ids,
> +		},
> +	.probe = cv1800_rtc_probe,
> +};
> +
> +module_platform_driver(cv1800_rtc_driver);
> +MODULE_AUTHOR("Jingbao Qiu");
> +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
> 

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

  reply	other threads:[~2023-12-26 12:37 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-26 10:04 [PATCH v3 0/4] riscv: rtc: sophgo: add mfd and rtc support for CV1800 Jingbao Qiu
2023-12-26 10:04 ` [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC Jingbao Qiu
2023-12-26 11:38   ` Rob Herring
2023-12-27  7:05     ` Jingbao Qiu
2023-12-26 12:18   ` Krzysztof Kozlowski
2023-12-27  7:35     ` Jingbao Qiu
2023-12-27 11:37       ` Krzysztof Kozlowski
2023-12-27 11:41         ` Jingbao Qiu
2023-12-26 12:21   ` Krzysztof Kozlowski
2023-12-27  7:37     ` Jingbao Qiu
2023-12-26 10:04 ` [PATCH v3 2/4] dt-bindings: rtc: sophgo: add RTC " Jingbao Qiu
2023-12-26 11:38   ` Rob Herring
2023-12-26 10:04 ` [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC Jingbao Qiu
2023-12-26 12:37   ` Alexandre Belloni [this message]
2023-12-27  8:03     ` Jingbao Qiu
2023-12-27 13:50       ` Alexandre Belloni
2023-12-28  4:12         ` Jingbao Qiu
2023-12-26 10:04 ` [PATCH v3 4/4] riscv: dts: sophgo: add rtc dt node for CV1800 Jingbao Qiu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231226123743bd2d3f0d@mail.local \
    --to=alexandre.belloni@bootlin.com \
    --cc=a.zummo@towertech.it \
    --cc=aou@eecs.berkeley.edu \
    --cc=chao.wei@sophgo.com \
    --cc=conor+dt@kernel.org \
    --cc=conor@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlan@gentoo.org \
    --cc=inochiama@outlook.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=qiujingbao.dlmu@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=unicorn_wang@outlook.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.