Re: [RFC PATCH v2 1/5] rtc: migrate driver data to RTC device

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jan 07, 2026 at 10:37:33PM +0800, Ke Sun wrote:
> Unify RTC driver interface by storing driver data on the RTC device
> instead of the parent device. Update RTC ops callbacks to pass the RTC
> device itself rather than its parent. This change enables better
> support for Rust RTC drivers that store data on the RTC device.
> 
> Signed-off-by: Ke Sun <sunke@kylinos.cn>
> ---
>  drivers/rtc/dev.c       |  4 ++--
>  drivers/rtc/interface.c | 18 +++++++++---------
>  drivers/rtc/rtc-pl031.c |  9 ++-------
>  3 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
> index baf1a8ca8b2b1..0f62ba9342e3e 100644
> --- a/drivers/rtc/dev.c
> +++ b/drivers/rtc/dev.c
> @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file,
>  		}
>  		default:
>  			if (rtc->ops->param_get)
> -				err = rtc->ops->param_get(rtc->dev.parent, &param);
> +				err = rtc->ops->param_get(&rtc->dev, &param);
>  			else
>  				err = -EINVAL;
>  		}
> @@ -440,7 +440,7 @@ static long rtc_dev_ioctl(struct file *file,
>  
>  		default:
>  			if (rtc->ops->param_set)
> -				err = rtc->ops->param_set(rtc->dev.parent, &param);
> +				err = rtc->ops->param_set(&rtc->dev, &param);
>  			else
>  				err = -EINVAL;
>  		}
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index b8b298efd9a9c..783a3ec3bb93d 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -91,7 +91,7 @@ static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm)
>  		err = -EINVAL;
>  	} else {
>  		memset(tm, 0, sizeof(struct rtc_time));
> -		err = rtc->ops->read_time(rtc->dev.parent, tm);
> +		err = rtc->ops->read_time(&rtc->dev, tm);
>  		if (err < 0) {
>  			dev_dbg(&rtc->dev, "read_time: fail to read: %d\n",
>  				err);
> @@ -155,7 +155,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>  	if (!rtc->ops)
>  		err = -ENODEV;
>  	else if (rtc->ops->set_time)
> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
> +		err = rtc->ops->set_time(&rtc->dev, tm);
>  	else
>  		err = -EINVAL;
>  
> @@ -200,7 +200,7 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc,
>  		alarm->time.tm_wday = -1;
>  		alarm->time.tm_yday = -1;
>  		alarm->time.tm_isdst = -1;
> -		err = rtc->ops->read_alarm(rtc->dev.parent, alarm);
> +		err = rtc->ops->read_alarm(&rtc->dev, alarm);
>  	}
>  
>  	mutex_unlock(&rtc->ops_lock);
> @@ -441,7 +441,7 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
>  	else if (!test_bit(RTC_FEATURE_ALARM, rtc->features))
>  		err = -EINVAL;
>  	else
> -		err = rtc->ops->set_alarm(rtc->dev.parent, alarm);
> +		err = rtc->ops->set_alarm(&rtc->dev, alarm);
>  
>  	/*
>  	 * Check for potential race described above. If the waiting for next
> @@ -568,7 +568,7 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
>  	else if (!test_bit(RTC_FEATURE_ALARM, rtc->features) || !rtc->ops->alarm_irq_enable)
>  		err = -EINVAL;
>  	else
> -		err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
> +		err = rtc->ops->alarm_irq_enable(&rtc->dev, enabled);
>  
>  	mutex_unlock(&rtc->ops_lock);
>  
> @@ -618,7 +618,7 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
>  		rtc->uie_rtctimer.period = ktime_set(1, 0);
>  		err = rtc_timer_enqueue(rtc, &rtc->uie_rtctimer);
>  		if (!err && rtc->ops && rtc->ops->alarm_irq_enable)
> -			err = rtc->ops->alarm_irq_enable(rtc->dev.parent, 1);
> +			err = rtc->ops->alarm_irq_enable(&rtc->dev, 1);
>  		if (err)
>  			goto out;
>  	} else {
> @@ -874,7 +874,7 @@ static void rtc_alarm_disable(struct rtc_device *rtc)
>  	if (!rtc->ops || !test_bit(RTC_FEATURE_ALARM, rtc->features) || !rtc->ops->alarm_irq_enable)
>  		return;
>  
> -	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
> +	rtc->ops->alarm_irq_enable(&rtc->dev, false);
>  	trace_rtc_alarm_irq_enable(0, 0);
>  }
>  
> @@ -1076,7 +1076,7 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset)
>  		return -EINVAL;
>  
>  	mutex_lock(&rtc->ops_lock);
> -	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
> +	ret = rtc->ops->read_offset(&rtc->dev, offset);
>  	mutex_unlock(&rtc->ops_lock);
>  
>  	trace_rtc_read_offset(*offset, ret);
> @@ -1111,7 +1111,7 @@ int rtc_set_offset(struct rtc_device *rtc, long offset)
>  		return -EINVAL;
>  
>  	mutex_lock(&rtc->ops_lock);
> -	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
> +	ret = rtc->ops->set_offset(&rtc->dev, offset);
>  	mutex_unlock(&rtc->ops_lock);
>  
>  	trace_rtc_set_offset(offset, ret);
> diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
> index eab39dfa4e5fe..a605034d44cb7 100644
> --- a/drivers/rtc/rtc-pl031.c
> +++ b/drivers/rtc/rtc-pl031.c
> @@ -284,10 +284,6 @@ static int pl031_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  
>  static void pl031_remove(struct amba_device *adev)
>  {
> -	struct pl031_local *ldata = dev_get_drvdata(&adev->dev);
> -
> -	if (adev->irq[0])
> -		free_irq(adev->irq[0], ldata);
>  	amba_release_regions(adev);
>  }
>  
> @@ -320,8 +316,6 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out;
>  	}
>  
> -	amba_set_drvdata(adev, ldata);
> -
>  	dev_dbg(&adev->dev, "designer ID = 0x%02x\n", amba_manf(adev));
>  	dev_dbg(&adev->dev, "revision = 0x%01x\n", amba_rev(adev));
>  
> @@ -356,6 +350,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
>  		ret = PTR_ERR(ldata->rtc);
>  		goto out;
>  	}
> +	dev_set_drvdata(&ldata->rtc->dev, ldata);
>  
>  	if (!adev->irq[0])
>  		clear_bit(RTC_FEATURE_ALARM, ldata->rtc->features);
> @@ -369,7 +364,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out;
>  
>  	if (adev->irq[0]) {
> -		ret = request_irq(adev->irq[0], pl031_interrupt,
> +		ret = devm_request_irq(&adev->dev, adev->irq[0], pl031_interrupt,
>  				  vendor->irqflags, "rtc-pl031", ldata);

Are you _SURE_ you can use devm for this?  it is a functional change,
one that trips lots of people up.  I wouldn't make this change without
at least saying why you are doing so in the changelog text, which I
didn't see at all here.

thanks,

greg k-h