Re: [PATCH v1 2/2] rtc: Add support for MT6685 Clock IC's RTC over SPMI

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 24/10/2025 10:33:18+0200, AngeloGioacchino Del Regno wrote:
> +static irqreturn_t rtc_mt6685_irq_handler_thread(int irq, void *data)
> +{
> +	u16 irqsta, irqen = 0, bbpu = RTC_BBPU_WR_UNLOCK_KEY | RTC_BBPU_PWREN;
> +	struct mt6685_rtc *rtc = data;
> +	struct device *dev = &rtc->rdev->dev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(rtc->mclk);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_bulk_read(rtc->regmap, RTC_REG_IRQ_STA, &irqsta, sizeof(irqsta));
> +	if (ret)
> +		goto end;
> +
> +	/* Only alarm interrupts are supported for now */
> +	if (!(irqsta & RTC_IRQ_AL)) {
> +		ret = -EINVAL;
> +		goto end;
> +	}
> +
> +	rtc_lock(rtc->rdev);
> +
> +	/* Enable BBPU power and clear the interrupt */
> +	ret = regmap_bulk_write(rtc->regmap, RTC_REG_BBPU, &bbpu, sizeof(bbpu));
> +	if (ret)
> +		goto end;
> +
> +	ret = regmap_bulk_write(rtc->regmap, RTC_REG_IRQ_EN, &irqen, sizeof(irqen));
> +	if (ret)
> +		goto end;
> +
> +	/* Trigger reset of the RTC BBPU Alarm */
> +	bbpu |= RTC_BBPU_RESET_AL;
> +	ret = regmap_bulk_write(rtc->regmap, RTC_REG_BBPU, &bbpu, sizeof(bbpu));
> +	if (ret) {
> +		dev_err(dev, "Cannot reset alarm: %d\n", ret);
> +		goto end;
> +	}
> +
> +	/* Trigger write synchronization */
> +	ret = rtc_mt6685_write_trigger(rtc);
> +	if (ret) {
> +		dev_err(dev, "Cannot synchronize write to RTC: %d\n", ret);

I'm not sure those two dev_err are necessary.

> +		goto end;
> +	}
> +	rtc_update_irq(rtc->rdev, 1, RTC_IRQF | RTC_AF);
> +end:
> +	rtc_unlock(rtc->rdev);
> +	clk_disable_unprepare(rtc->mclk);
> +	return ret ? IRQ_NONE : IRQ_HANDLED;
> +}
> +


> +static int rtc_mt6685_read_time_regs(struct mt6685_rtc *rtc, struct rtc_time *tm, u16 reg)
> +{
> +	u16 data[RTC_AL_TC_REGIDX_MAX];
> +	int ret;
> +
> +	ret = regmap_bulk_read(rtc->regmap, reg, &data,
> +			       RTC_AL_TC_REGIDX_MAX * sizeof(data[0]));
> +	if (ret) {
> +		dev_err(&rtc->rdev->dev, "Cannot read time regs\n");

Same here, there is not much the user can do and your are already
returning an error.

> +		return ret;
> +	}
> +
> +	tm->tm_sec  = FIELD_GET(RTC_AL_TC_SEC_MIN, data[RTC_AL_TC_REGIDX_SEC]);
> +	tm->tm_min  = FIELD_GET(RTC_AL_TC_SEC_MIN, data[RTC_AL_TC_REGIDX_MIN]);
> +	tm->tm_hour = FIELD_GET(RTC_AL_TC_HOUR_DOM, data[RTC_AL_TC_REGIDX_HOUR]);
> +	tm->tm_mday = FIELD_GET(RTC_AL_TC_HOUR_DOM, data[RTC_AL_TC_REGIDX_DOM]);
> +	tm->tm_wday = FIELD_GET(RTC_AL_TC_DOW, data[RTC_AL_TC_REGIDX_DOW]);
> +	tm->tm_mon  = FIELD_GET(RTC_AL_TC_MON, data[RTC_AL_TC_REGIDX_MON]);
> +	tm->tm_year = FIELD_GET(RTC_AL_TC_YEAR, data[RTC_AL_TC_REGIDX_YEAR]);
> +
> +	/* HW register start mon/wday from one, but tm_mon/tm_wday start from zero. */
> +	tm->tm_mon--;
> +	tm->tm_wday--;
> +
> +	return 0;
> +}
> +

> +static int rtc_mt6685_probe(struct platform_device *pdev)
> +{
> +	struct regmap_config mt6685_rtc_regmap_config = {
> +		.reg_bits = 16,
> +		.val_bits = 8,
> +		.max_register = 0x60,
> +		.fast_io = true,
> +		.use_single_read = true,
> +		.use_single_write = true,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct spmi_subdevice *sub_sdev;
> +	struct spmi_device *sparent;
> +	struct mt6685_rtc *rtc;
> +	int ret;
> +
> +	rtc = devm_kzalloc(dev, sizeof(struct mt6685_rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	sparent = to_spmi_device(dev->parent);
> +	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> +	if (IS_ERR(sub_sdev))
> +		return PTR_ERR(sub_sdev);
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "reg",
> +				   &mt6685_rtc_regmap_config.reg_base);
> +	if (ret)
> +		return ret;
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0)
> +		return rtc->irq;
> +
> +	rtc->mclk = devm_clk_get(dev, 0);
> +	if (IS_ERR(rtc->mclk))
> +		return PTR_ERR(rtc->mclk);
> +
> +	rtc->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &mt6685_rtc_regmap_config);
> +	if (IS_ERR(rtc->regmap))
> +		return PTR_ERR(rtc->regmap);
> +
> +	rtc->rdev = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(rtc->rdev))
> +		return PTR_ERR(rtc->rdev);
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	/* Clock is required to auto-synchronize IRQ enable to RTC */
> +	ret = clk_prepare_enable(rtc->mclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +					rtc_mt6685_irq_handler_thread,
> +					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +					"mt6685-rtc", rtc);
> +	clk_disable_unprepare(rtc->mclk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Cannot request alarm IRQ");
> +
> +	device_init_wakeup(&pdev->dev, true);

devm_device_init_wakeup ?


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