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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 08, 2026 at 07:18:30AM +0800, Ke Sun wrote:
> 
> On 1/8/26 00:12, Greg KH wrote:
> > 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,
> 
> Since ldata's lifecycle is now tied to the RTC device (stored via
> dev_set_drvdata(&ldata->rtc->dev, ldata)), and the RTC device's lifecycle
> is tied to the amba_device (via devm_rtc_allocate_device(&adev->dev)),
> using devm_request_irq(&adev->dev, ...) allows us to remove the manual IRQ
> release in pl031_remove, as the IRQ will be automatically released along
> with the amba_device lifecycle.

Please test this.  There are loads of race conditions that can happen
when irqs are bound to devm lifecycles.  You are changing the behavior
here, so be very careful.

And again, this is a change that was not documented in the changelog,
and should not be part of this patch, it should be stand-alone.

thanks,

greg k-h