diff mbox series

[3/4] rtc: Introduce devm_rtc_allocate_device_priv

Message ID 20250120-rtc-v1-3-08c50830bac9@nxp.com (mailing list archive)
State New
Headers show
Series rtc/scmi: Support multiple RTCs | expand

Commit Message

Peng Fan (OSS) Jan. 20, 2025, 2:25 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Current users of rtc_class_ops->x are using 'rtc->dev.parent', so
there is no way for rtc drivers get rtc private information. Take
i.MX95 for example, i.MX95 SCMI BBM Protocol supports two RTCs
for i.MX95 EVK board. Using 'rtc->dev.parent' causing driver not
not able to know the exact RTC device. So introduce 'priv' and
devm_rtc_allocate_device_priv for driver to set rtc device private
information.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/rtc/class.c     |  9 ++++++++-
 drivers/rtc/dev.c       |  8 +++++---
 drivers/rtc/interface.c | 16 ++++++++--------
 drivers/rtc/proc.c      |  2 +-
 include/linux/rtc.h     |  2 ++
 5 files changed, 24 insertions(+), 13 deletions(-)

Comments

Dan Carpenter Jan. 20, 2025, 10:57 a.m. UTC | #1 
On Mon, Jan 20, 2025 at 10:25:35AM +0800, Peng Fan (OSS) wrote:
>  int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
> diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
> index c4a3ab53dcd4b7280a3a2981fe842729603a1feb..e0e1a488b795645d7c9453490d6cdba510cc5db5 100644
> --- a/drivers/rtc/dev.c
> +++ b/drivers/rtc/dev.c
> @@ -410,7 +410,8 @@ 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->priv ?
> +							  &rtc->dev : rtc->dev.parent, &param);

This seems kind of horrible...  I can't think of anywhere else which
does something like this.

It would almost be better to do something like:

	err = rtc->ops->param_get(rtc->priv ? (void *)rtc : rtc->dev.parent, &param);

The advatange of this is that it looks totally horrible from the get go
instead of only subtly wrong.  And it would immediately crash if you got
it wrong implementing the ->param_get() function pointer.

regards,
dan carpenter
Peng Fan Jan. 21, 2025, 2:35 p.m. UTC | #2 
Hi Dan,

> Subject: Re: [PATCH 3/4] rtc: Introduce devm_rtc_allocate_device_priv
> 
> On Mon, Jan 20, 2025 at 10:25:35AM +0800, Peng Fan (OSS) wrote:
> >  int __devm_rtc_register_device(struct module *owner, struct
> > rtc_device *rtc) diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
> > index
> >
> c4a3ab53dcd4b7280a3a2981fe842729603a1feb..e0e1a488b795645d
> 7c9453490d6c
> > dba510cc5db5 100644
> > --- a/drivers/rtc/dev.c
> > +++ b/drivers/rtc/dev.c
> > @@ -410,7 +410,8 @@ 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->priv ?
> > +							  &rtc->dev :
> rtc->dev.parent, &param);
> 
> This seems kind of horrible...  I can't think of anywhere else which does
> something like this.
> 
> It would almost be better to do something like:
> 
> 	err = rtc->ops->param_get(rtc->priv ? (void *)rtc : rtc-
> >dev.parent, &param);
> 
> The advatange of this is that it looks totally horrible from the get go
> instead of only subtly wrong.  And it would immediately crash if you
> got it wrong implementing the ->param_get() function pointer.

Thanks for help improving the code. I will include this in V2 and post
out after we reach a goal on how to support the 2nd RTC on i.MX95.

Thanks,
Peng.

> 
> regards,
> dan carpenter
Dan Carpenter Jan. 21, 2025, 3:15 p.m. UTC | #3 
On Tue, Jan 21, 2025 at 02:35:59PM +0000, Peng Fan wrote:
> Hi Dan,
> 
> > Subject: Re: [PATCH 3/4] rtc: Introduce devm_rtc_allocate_device_priv
> > 
> > On Mon, Jan 20, 2025 at 10:25:35AM +0800, Peng Fan (OSS) wrote:
> > >  int __devm_rtc_register_device(struct module *owner, struct
> > > rtc_device *rtc) diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
> > > index
> > >
> > c4a3ab53dcd4b7280a3a2981fe842729603a1feb..e0e1a488b795645d
> > 7c9453490d6c
> > > dba510cc5db5 100644
> > > --- a/drivers/rtc/dev.c
> > > +++ b/drivers/rtc/dev.c
> > > @@ -410,7 +410,8 @@ 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->priv ?
> > > +							  &rtc->dev :
> > rtc->dev.parent, &param);
> > 
> > This seems kind of horrible...  I can't think of anywhere else which does
> > something like this.
> > 
> > It would almost be better to do something like:
> > 
> > 	err = rtc->ops->param_get(rtc->priv ? (void *)rtc : rtc-
> > >dev.parent, &param);
> > 
> > The advatange of this is that it looks totally horrible from the get go
> > instead of only subtly wrong.  And it would immediately crash if you
> > got it wrong implementing the ->param_get() function pointer.
> 
> Thanks for help improving the code. I will include this in V2 and post
> out after we reach a goal on how to support the 2nd RTC on i.MX95.

Don't do what I said actually...  Let's find a better way.  I don't
know why rtc_class_ops function pointers take a device pointer instead
of an rtc_device pointer.  Or if they did take a device pointer why
not the &rtc->dev like you suggested?  But let's not do both like this.

Migrating all the function pointers is a lot of work but not impossible.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index e31fa0ad127e9545afac745a621d4ccbcd5fca28..67413600785d806fe4da441483ce1280357d8791 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -361,7 +361,7 @@  static void devm_rtc_release_device(void *res)
 	put_device(&rtc->dev);
 }
 
-struct rtc_device *devm_rtc_allocate_device(struct device *dev)
+struct rtc_device *devm_rtc_allocate_device_priv(struct device *dev, void *priv)
 {
 	struct rtc_device *rtc;
 	int id, err;
@@ -378,6 +378,7 @@  struct rtc_device *devm_rtc_allocate_device(struct device *dev)
 
 	rtc->id = id;
 	rtc->dev.parent = dev;
+	rtc->priv = priv;
 	err = devm_add_action_or_reset(dev, devm_rtc_release_device, rtc);
 	if (err)
 		return ERR_PTR(err);
@@ -388,6 +389,12 @@  struct rtc_device *devm_rtc_allocate_device(struct device *dev)
 
 	return rtc;
 }
+EXPORT_SYMBOL_GPL(devm_rtc_allocate_device_priv);
+
+struct rtc_device *devm_rtc_allocate_device(struct device *dev)
+{
+	return devm_rtc_allocate_device_priv(dev, NULL);
+}
 EXPORT_SYMBOL_GPL(devm_rtc_allocate_device);
 
 int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c
index c4a3ab53dcd4b7280a3a2981fe842729603a1feb..e0e1a488b795645d7c9453490d6cdba510cc5db5 100644
--- a/drivers/rtc/dev.c
+++ b/drivers/rtc/dev.c
@@ -410,7 +410,8 @@  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->priv ?
+							  &rtc->dev : rtc->dev.parent, &param);
 			else
 				err = -EINVAL;
 		}
@@ -440,7 +441,8 @@  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->priv ?
+							  &rtc->dev : rtc->dev.parent, &param);
 			else
 				err = -EINVAL;
 		}
@@ -450,7 +452,7 @@  static long rtc_dev_ioctl(struct file *file,
 	default:
 		/* Finally try the driver's ioctl interface */
 		if (ops->ioctl) {
-			err = ops->ioctl(rtc->dev.parent, cmd, arg);
+			err = ops->ioctl(rtc->priv ? &rtc->dev : rtc->dev.parent, cmd, arg);
 			if (err == -ENOIOCTLCMD)
 				err = -ENOTTY;
 		} else {
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index aaf76406cd7d7d2cfd5479fc1fc892fcb5efbb6b..06d51761900ee5d6cc3916d31d907505c193c6ad 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->priv ? &rtc->dev : rtc->dev.parent, 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->priv ? &rtc->dev : rtc->dev.parent, 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->priv ? &rtc->dev : rtc->dev.parent, 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->priv ? &rtc->dev : rtc->dev.parent, alarm);
 
 	trace_rtc_set_alarm(rtc_tm_to_time64(&alarm->time), err);
 	return err;
@@ -545,7 +545,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->priv ? &rtc->dev : rtc->dev.parent, enabled);
 
 	mutex_unlock(&rtc->ops_lock);
 
@@ -847,7 +847,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->priv ? &rtc->dev : rtc->dev.parent, false);
 	trace_rtc_alarm_irq_enable(0, 0);
 }
 
@@ -1049,7 +1049,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->priv ? &rtc->dev : rtc->dev.parent, offset);
 	mutex_unlock(&rtc->ops_lock);
 
 	trace_rtc_read_offset(*offset, ret);
@@ -1084,7 +1084,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->priv ? &rtc->dev : rtc->dev.parent, offset);
 	mutex_unlock(&rtc->ops_lock);
 
 	trace_rtc_set_offset(offset, ret);
diff --git a/drivers/rtc/proc.c b/drivers/rtc/proc.c
index cbcdbb19d848e78e6674bd626833151a99773ef0..94cc4f9d62b7867018d876f7933468fbd1552ffc 100644
--- a/drivers/rtc/proc.c
+++ b/drivers/rtc/proc.c
@@ -73,7 +73,7 @@  static int rtc_proc_show(struct seq_file *seq, void *offset)
 	seq_printf(seq, "24hr\t\t: yes\n");
 
 	if (ops->proc)
-		ops->proc(rtc->dev.parent, seq);
+		ops->proc(rtc->priv ? &rtc->dev : rtc->dev.parent, seq);
 
 	return 0;
 }
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3f4d315aaec9e641e35c1c86a522f2879bec19ba..a6f3c86a08e1e214062b2a68d9a6a437afb186b3 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -110,6 +110,7 @@  struct rtc_device {
 	struct hrtimer pie_timer; /* sub second exp, so needs hrtimer */
 	int pie_enabled;
 	struct work_struct irqwork;
+	void *priv;
 
 	/*
 	 * This offset specifies the update timing of the RTC.
@@ -182,6 +183,7 @@  extern struct rtc_device *devm_rtc_device_register(struct device *dev,
 					const struct rtc_class_ops *ops,
 					struct module *owner);
 struct rtc_device *devm_rtc_allocate_device(struct device *dev);
+struct rtc_device *devm_rtc_allocate_device_priv(struct device *dev, void *priv);
 int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc);
 
 extern int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm);