| Message ID | 20190402034253.4928-3-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AM437x: Add rtc-only + DDR mode support | expand |
* Keerthy <j-keerthy@ti.com> [190402 03:43]: > Add an interface function to set up the rtc for a power_off > mode. > > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > drivers/rtc/interface.c | 12 ++++++++++++ > drivers/rtc/rtc-omap.c | 1 + > include/linux/rtc.h | 2 ++ > 3 files changed, 15 insertions(+) > > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > index ccb7d6b4da3b..4846ec897067 100644 > --- a/drivers/rtc/interface.c > +++ b/drivers/rtc/interface.c > @@ -1070,3 +1070,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset) > trace_rtc_set_offset(offset, ret); > return ret; > } > + > +/** > + * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN > + * line and can be used to power off the SoC. > + * > + * Kernel interface to program rtc to power off > + */ > +int rtc_power_off_program(struct rtc_device *rtc) > +{ > + return rtc->ops->power_off_program(rtc->dev.parent); > +} > +EXPORT_SYMBOL_GPL(rtc_power_off_program); This whole series looks OK to me. Not sure about the naming for rtc_power_off_program(), would rtc_power_off_device() be more generic? Regards, Tony
On 02/04/2019 10:32:32-0700, Tony Lindgren wrote: > * Keerthy <j-keerthy@ti.com> [190402 03:43]: > > Add an interface function to set up the rtc for a power_off > > mode. > > > > Signed-off-by: Keerthy <j-keerthy@ti.com> > > --- > > drivers/rtc/interface.c | 12 ++++++++++++ > > drivers/rtc/rtc-omap.c | 1 + > > include/linux/rtc.h | 2 ++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > > index ccb7d6b4da3b..4846ec897067 100644 > > --- a/drivers/rtc/interface.c > > +++ b/drivers/rtc/interface.c > > @@ -1070,3 +1070,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset) > > trace_rtc_set_offset(offset, ret); > > return ret; > > } > > + > > +/** > > + * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN > > + * line and can be used to power off the SoC. > > + * > > + * Kernel interface to program rtc to power off > > + */ > > +int rtc_power_off_program(struct rtc_device *rtc) > > +{ > > + return rtc->ops->power_off_program(rtc->dev.parent); > > +} > > +EXPORT_SYMBOL_GPL(rtc_power_off_program); > > This whole series looks OK to me. Not sure about the naming for > rtc_power_off_program(), would rtc_power_off_device() be more > generic? > Well, even if this is more generic, this feels way too ad-hoc to me. The series is adding a function and a callback to the core for, from what I understand, only one particular board. You may as well simply export the function directly from the rtc-omap driver as anyway, this is added as a dependency in the last patch. Something else I would be open to but I'm not completely sure this fits your use case is a new interface that would take an alarm index as a parameter to allow setting any alarm on the RTC. This would at least be usable for multiple other drivers.
On 03/04/19 2:37 AM, Alexandre Belloni wrote: > On 02/04/2019 10:32:32-0700, Tony Lindgren wrote: >> * Keerthy <j-keerthy@ti.com> [190402 03:43]: >>> Add an interface function to set up the rtc for a power_off >>> mode. >>> >>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>> --- >>> drivers/rtc/interface.c | 12 ++++++++++++ >>> drivers/rtc/rtc-omap.c | 1 + >>> include/linux/rtc.h | 2 ++ >>> 3 files changed, 15 insertions(+) >>> >>> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c >>> index ccb7d6b4da3b..4846ec897067 100644 >>> --- a/drivers/rtc/interface.c >>> +++ b/drivers/rtc/interface.c >>> @@ -1070,3 +1070,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset) >>> trace_rtc_set_offset(offset, ret); >>> return ret; >>> } >>> + >>> +/** >>> + * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN >>> + * line and can be used to power off the SoC. >>> + * >>> + * Kernel interface to program rtc to power off >>> + */ >>> +int rtc_power_off_program(struct rtc_device *rtc) >>> +{ >>> + return rtc->ops->power_off_program(rtc->dev.parent); >>> +} >>> +EXPORT_SYMBOL_GPL(rtc_power_off_program); >> >> This whole series looks OK to me. Not sure about the naming for >> rtc_power_off_program(), would rtc_power_off_device() be more >> generic? >> > > Well, even if this is more generic, this feels way too ad-hoc to me. > The series is adding a function and a callback to the core for, from > what I understand, only one particular board. > > You may as well simply export the function directly from the rtc-omap > driver as anyway, this is added as a dependency in the last patch. > > Something else I would be open to but I'm not completely sure this fits > your use case is a new interface that would take an alarm index as a > parameter to allow setting any alarm on the RTC. This would at least be > usable for multiple other drivers. Agreed that this is pretty specific to am43 as of now. As you suggested i will export out omap_rtc_poweroff_program. I will post v3 with the interface patch removed. Thanks for your feedback. >
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index ccb7d6b4da3b..4846ec897067 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -1070,3 +1070,15 @@ int rtc_set_offset(struct rtc_device *rtc, long offset) trace_rtc_set_offset(offset, ret); return ret; } + +/** + * rtc_power_off_program - Some of the rtc are hooked on to PMIC_EN + * line and can be used to power off the SoC. + * + * Kernel interface to program rtc to power off + */ +int rtc_power_off_program(struct rtc_device *rtc) +{ + return rtc->ops->power_off_program(rtc->dev.parent); +} +EXPORT_SYMBOL_GPL(rtc_power_off_program); diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index b467154443a4..0e7a65c105e7 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -507,6 +507,7 @@ static const struct rtc_class_ops omap_rtc_ops = { .read_alarm = omap_rtc_read_alarm, .set_alarm = omap_rtc_set_alarm, .alarm_irq_enable = omap_rtc_alarm_irq_enable, + .power_off_program = omap_rtc_power_off_program, }; static const struct omap_rtc_device_type omap_rtc_default_type = { diff --git a/include/linux/rtc.h b/include/linux/rtc.h index 48d3f8e0b64f..430e038a2374 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -84,6 +84,7 @@ struct rtc_class_ops { int (*alarm_irq_enable)(struct device *, unsigned int enabled); int (*read_offset)(struct device *, long *offset); int (*set_offset)(struct device *, long offset); + int (*power_off_program)(struct device *dev); }; struct rtc_device; @@ -211,6 +212,7 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer); int rtc_read_offset(struct rtc_device *rtc, long *offset); int rtc_set_offset(struct rtc_device *rtc, long offset); void rtc_timer_do_work(struct work_struct *work); +int rtc_power_off_program(struct rtc_device *rtc); static inline bool is_leap_year(unsigned int year) {
Add an interface function to set up the rtc for a power_off mode. Signed-off-by: Keerthy <j-keerthy@ti.com> --- drivers/rtc/interface.c | 12 ++++++++++++ drivers/rtc/rtc-omap.c | 1 + include/linux/rtc.h | 2 ++ 3 files changed, 15 insertions(+)