| Message ID | 1513059137-21593-2-git-send-email-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Keerthy, On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: > Remove all the unwanted exports from the driver I'm adding event capture capability to the pwm-omap driver and so far used v4.15-rc3 as codebase. Intended use is an IR receiver; for that I need to measure pulses width and spaces between pulses. So DM timer was setup to generate interupt after both TCAR1 and TCAR2 are filled, values are passed to IR decoder and TCAR_IT_FLAG is cleared. Of course, this is just proof of concept and needs to be polished and generalized, but to make it at least work I need functions you just unexported (plus some new). Question is whenever we need this level of indirection (omap_dm_timer_ops) or plain exports are enough. Thank you, ladis > Signed-off-by: Keerthy <j-keerthy@ti.com> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > Changes in v3: > > * Added Sebastian's Reviewed-by. > > Changes in v2: > > * No code changes in this v2 version. Only enhanced patch > statistics for renames. > > arch/arm/plat-omap/dmtimer.c | 27 --------------------------- > 1 file changed, 27 deletions(-) > > diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c > index d443e48..72565fc 100644 > --- a/arch/arm/plat-omap/dmtimer.c > +++ b/arch/arm/plat-omap/dmtimer.c > @@ -302,7 +302,6 @@ struct omap_dm_timer *omap_dm_timer_request(void) > { > return _omap_dm_timer_request(REQUEST_ANY, NULL); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_request); > > struct omap_dm_timer *omap_dm_timer_request_specific(int id) > { > @@ -315,7 +314,6 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id) > > return _omap_dm_timer_request(REQUEST_BY_ID, &id); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific); > > /** > * omap_dm_timer_request_by_cap - Request a timer by capability > @@ -330,7 +328,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap) > { > return _omap_dm_timer_request(REQUEST_BY_CAP, &cap); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_cap); > > /** > * omap_dm_timer_request_by_node - Request a timer by device-tree node > @@ -346,7 +343,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np) > > return _omap_dm_timer_request(REQUEST_BY_NODE, np); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_node); > > int omap_dm_timer_free(struct omap_dm_timer *timer) > { > @@ -359,7 +355,6 @@ int omap_dm_timer_free(struct omap_dm_timer *timer) > timer->reserved = 0; > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_free); > > void omap_dm_timer_enable(struct omap_dm_timer *timer) > { > @@ -379,13 +374,11 @@ void omap_dm_timer_enable(struct omap_dm_timer *timer) > } > } > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_enable); > > void omap_dm_timer_disable(struct omap_dm_timer *timer) > { > pm_runtime_put_sync(&timer->pdev->dev); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_disable); > > int omap_dm_timer_get_irq(struct omap_dm_timer *timer) > { > @@ -393,7 +386,6 @@ int omap_dm_timer_get_irq(struct omap_dm_timer *timer) > return timer->irq; > return -EINVAL; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_get_irq); > > #if defined(CONFIG_ARCH_OMAP1) > #include <mach/hardware.h> > @@ -429,7 +421,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) > > return inputmask; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); > > #else > > @@ -439,7 +430,6 @@ struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer) > return timer->fclk; > return NULL; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_get_fclk); > > __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) > { > @@ -447,7 +437,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) > > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); > > #endif > > @@ -461,7 +450,6 @@ int omap_dm_timer_trigger(struct omap_dm_timer *timer) > omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_trigger); > > int omap_dm_timer_start(struct omap_dm_timer *timer) > { > @@ -482,7 +470,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer) > timer->context.tclr = l; > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_start); > > int omap_dm_timer_stop(struct omap_dm_timer *timer) > { > @@ -506,7 +493,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer) > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_stop); > > int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) > { > @@ -569,7 +555,6 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) > > return ret; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_source); > > int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, > unsigned int load) > @@ -595,7 +580,6 @@ int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load); > > /* Optimized set_load which removes costly spin wait in timer_start */ > int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, > @@ -625,7 +609,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, > timer->context.tcrr = load; > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load_start); > > int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, > unsigned int match) > @@ -650,7 +633,6 @@ int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_match); > > int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, > int toggle, int trigger) > @@ -676,7 +658,6 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_pwm); > > int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) > { > @@ -699,7 +680,6 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_prescaler); > > int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, > unsigned int value) > @@ -716,7 +696,6 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_enable); > > /** > * omap_dm_timer_set_int_disable - disable timer interrupts > @@ -747,7 +726,6 @@ int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask) > omap_dm_timer_disable(timer); > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_disable); > > unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) > { > @@ -762,7 +740,6 @@ unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) > > return l; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_read_status); > > int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) > { > @@ -773,7 +750,6 @@ int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) > > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_write_status); > > unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) > { > @@ -784,7 +760,6 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) > > return __omap_dm_timer_read_counter(timer, timer->posted); > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_read_counter); > > int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) > { > @@ -799,7 +774,6 @@ int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) > timer->context.tcrr = value; > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timer_write_counter); > > int omap_dm_timers_active(void) > { > @@ -816,7 +790,6 @@ int omap_dm_timers_active(void) > } > return 0; > } > -EXPORT_SYMBOL_GPL(omap_dm_timers_active); > > static const struct of_device_id omap_timer_match[]; > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: > Keerthy, > > On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: >> Remove all the unwanted exports from the driver > > I'm adding event capture capability to the pwm-omap driver and so far used > v4.15-rc3 as codebase. > > Intended use is an IR receiver; for that I need to measure pulses width and > spaces between pulses. So DM timer was setup to generate interupt after > both TCAR1 and TCAR2 are filled, values are passed to IR decoder and > TCAR_IT_FLAG is cleared. > > Of course, this is just proof of concept and needs to be polished and > generalized, but to make it at least work I need functions you just > unexported (plus some new). > > Question is whenever we need this level of indirection (omap_dm_timer_ops) > or plain exports are enough. The general guidance is not to do plain exports and go via omap_dm_timer_ops. > > Thank you, > ladis > >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> >> --- >> Changes in v3: >> >> * Added Sebastian's Reviewed-by. >> >> Changes in v2: >> >> * No code changes in this v2 version. Only enhanced patch >> statistics for renames. >> >> arch/arm/plat-omap/dmtimer.c | 27 --------------------------- >> 1 file changed, 27 deletions(-) >> >> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c >> index d443e48..72565fc 100644 >> --- a/arch/arm/plat-omap/dmtimer.c >> +++ b/arch/arm/plat-omap/dmtimer.c >> @@ -302,7 +302,6 @@ struct omap_dm_timer *omap_dm_timer_request(void) >> { >> return _omap_dm_timer_request(REQUEST_ANY, NULL); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_request); >> >> struct omap_dm_timer *omap_dm_timer_request_specific(int id) >> { >> @@ -315,7 +314,6 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id) >> >> return _omap_dm_timer_request(REQUEST_BY_ID, &id); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific); >> >> /** >> * omap_dm_timer_request_by_cap - Request a timer by capability >> @@ -330,7 +328,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap) >> { >> return _omap_dm_timer_request(REQUEST_BY_CAP, &cap); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_cap); >> >> /** >> * omap_dm_timer_request_by_node - Request a timer by device-tree node >> @@ -346,7 +343,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np) >> >> return _omap_dm_timer_request(REQUEST_BY_NODE, np); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_node); >> >> int omap_dm_timer_free(struct omap_dm_timer *timer) >> { >> @@ -359,7 +355,6 @@ int omap_dm_timer_free(struct omap_dm_timer *timer) >> timer->reserved = 0; >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_free); >> >> void omap_dm_timer_enable(struct omap_dm_timer *timer) >> { >> @@ -379,13 +374,11 @@ void omap_dm_timer_enable(struct omap_dm_timer *timer) >> } >> } >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_enable); >> >> void omap_dm_timer_disable(struct omap_dm_timer *timer) >> { >> pm_runtime_put_sync(&timer->pdev->dev); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_disable); >> >> int omap_dm_timer_get_irq(struct omap_dm_timer *timer) >> { >> @@ -393,7 +386,6 @@ int omap_dm_timer_get_irq(struct omap_dm_timer *timer) >> return timer->irq; >> return -EINVAL; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_get_irq); >> >> #if defined(CONFIG_ARCH_OMAP1) >> #include <mach/hardware.h> >> @@ -429,7 +421,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) >> >> return inputmask; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); >> >> #else >> >> @@ -439,7 +430,6 @@ struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer) >> return timer->fclk; >> return NULL; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_get_fclk); >> >> __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) >> { >> @@ -447,7 +437,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) >> >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); >> >> #endif >> >> @@ -461,7 +450,6 @@ int omap_dm_timer_trigger(struct omap_dm_timer *timer) >> omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_trigger); >> >> int omap_dm_timer_start(struct omap_dm_timer *timer) >> { >> @@ -482,7 +470,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer) >> timer->context.tclr = l; >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_start); >> >> int omap_dm_timer_stop(struct omap_dm_timer *timer) >> { >> @@ -506,7 +493,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer) >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_stop); >> >> int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) >> { >> @@ -569,7 +555,6 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) >> >> return ret; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_source); >> >> int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, >> unsigned int load) >> @@ -595,7 +580,6 @@ int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load); >> >> /* Optimized set_load which removes costly spin wait in timer_start */ >> int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, >> @@ -625,7 +609,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, >> timer->context.tcrr = load; >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load_start); >> >> int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, >> unsigned int match) >> @@ -650,7 +633,6 @@ int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_match); >> >> int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, >> int toggle, int trigger) >> @@ -676,7 +658,6 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_pwm); >> >> int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) >> { >> @@ -699,7 +680,6 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_prescaler); >> >> int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, >> unsigned int value) >> @@ -716,7 +696,6 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_enable); >> >> /** >> * omap_dm_timer_set_int_disable - disable timer interrupts >> @@ -747,7 +726,6 @@ int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask) >> omap_dm_timer_disable(timer); >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_disable); >> >> unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) >> { >> @@ -762,7 +740,6 @@ unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) >> >> return l; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_read_status); >> >> int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) >> { >> @@ -773,7 +750,6 @@ int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) >> >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_write_status); >> >> unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) >> { >> @@ -784,7 +760,6 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) >> >> return __omap_dm_timer_read_counter(timer, timer->posted); >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_read_counter); >> >> int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) >> { >> @@ -799,7 +774,6 @@ int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) >> timer->context.tcrr = value; >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timer_write_counter); >> >> int omap_dm_timers_active(void) >> { >> @@ -816,7 +790,6 @@ int omap_dm_timers_active(void) >> } >> return 0; >> } >> -EXPORT_SYMBOL_GPL(omap_dm_timers_active); >> >> static const struct of_device_id omap_timer_match[]; >> >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 12, 2017 at 01:01:51PM +0530, Keerthy wrote: > > > On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: > > Keerthy, > > > > On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: > >> Remove all the unwanted exports from the driver > > > > I'm adding event capture capability to the pwm-omap driver and so far used > > v4.15-rc3 as codebase. > > > > Intended use is an IR receiver; for that I need to measure pulses width and > > spaces between pulses. So DM timer was setup to generate interupt after > > both TCAR1 and TCAR2 are filled, values are passed to IR decoder and > > TCAR_IT_FLAG is cleared. > > > > Of course, this is just proof of concept and needs to be polished and > > generalized, but to make it at least work I need functions you just > > unexported (plus some new). > > > > Question is whenever we need this level of indirection (omap_dm_timer_ops) > > or plain exports are enough. > > The general guidance is not to do plain exports and go via > omap_dm_timer_ops. ...in contrary what other clocksource drivers are doing. Now I'm assuming it is okay to extend omap_dm_timer_ops. That would mean check for ops members to be assigned should be also extended or we should delete it altogether and assume all members are populated? Thanks, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 12 December 2017 01:31 PM, Ladislav Michl wrote: > On Tue, Dec 12, 2017 at 01:01:51PM +0530, Keerthy wrote: >> >> >> On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: >>> Keerthy, >>> >>> On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: >>>> Remove all the unwanted exports from the driver >>> >>> I'm adding event capture capability to the pwm-omap driver and so far used >>> v4.15-rc3 as codebase. >>> >>> Intended use is an IR receiver; for that I need to measure pulses width and >>> spaces between pulses. So DM timer was setup to generate interupt after >>> both TCAR1 and TCAR2 are filled, values are passed to IR decoder and >>> TCAR_IT_FLAG is cleared. >>> >>> Of course, this is just proof of concept and needs to be polished and >>> generalized, but to make it at least work I need functions you just >>> unexported (plus some new). >>> >>> Question is whenever we need this level of indirection (omap_dm_timer_ops) >>> or plain exports are enough. >> >> The general guidance is not to do plain exports and go via >> omap_dm_timer_ops. > > ...in contrary what other clocksource drivers are doing. > > Now I'm assuming it is okay to extend omap_dm_timer_ops. That would mean > check for ops members to be assigned should be also extended or we should > delete it altogether and assume all members are populated? It should be fine to extend omap_dm_timer_ops. What are the ops missing for your new implementation? Tony, Your thoughts on the above? Regards, Keerthy > > Thanks, > ladis > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 12, 2017 at 01:38:04PM +0530, Keerthy wrote: > On Tuesday 12 December 2017 01:31 PM, Ladislav Michl wrote: > > On Tue, Dec 12, 2017 at 01:01:51PM +0530, Keerthy wrote: > >> > >> > >> On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: > >>> Keerthy, > >>> > >>> On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: > >>>> Remove all the unwanted exports from the driver > >>> > >>> I'm adding event capture capability to the pwm-omap driver and so far used > >>> v4.15-rc3 as codebase. > >>> > >>> Intended use is an IR receiver; for that I need to measure pulses width and > >>> spaces between pulses. So DM timer was setup to generate interupt after > >>> both TCAR1 and TCAR2 are filled, values are passed to IR decoder and > >>> TCAR_IT_FLAG is cleared. > >>> > >>> Of course, this is just proof of concept and needs to be polished and > >>> generalized, but to make it at least work I need functions you just > >>> unexported (plus some new). > >>> > >>> Question is whenever we need this level of indirection (omap_dm_timer_ops) > >>> or plain exports are enough. > >> > >> The general guidance is not to do plain exports and go via > >> omap_dm_timer_ops. > > > > ...in contrary what other clocksource drivers are doing. > > > > Now I'm assuming it is okay to extend omap_dm_timer_ops. That would mean > > check for ops members to be assigned should be also extended or we should > > delete it altogether and assume all members are populated? > > It should be fine to extend omap_dm_timer_ops. What are the ops missing > for your new implementation? Read capture registers, configure capture and ack interrupt. Perhaps set_pwm could be extended to configure capture as well. I'll update my code on top of your changes and we'll see how it would work. > Tony, > > Your thoughts on the above? > > R -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 12 December 2017 01:49 PM, Ladislav Michl wrote: > On Tue, Dec 12, 2017 at 01:38:04PM +0530, Keerthy wrote: >> On Tuesday 12 December 2017 01:31 PM, Ladislav Michl wrote: >>> On Tue, Dec 12, 2017 at 01:01:51PM +0530, Keerthy wrote: >>>> >>>> >>>> On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: >>>>> Keerthy, >>>>> >>>>> On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: >>>>>> Remove all the unwanted exports from the driver >>>>> >>>>> I'm adding event capture capability to the pwm-omap driver and so far used >>>>> v4.15-rc3 as codebase. >>>>> >>>>> Intended use is an IR receiver; for that I need to measure pulses width and >>>>> spaces between pulses. So DM timer was setup to generate interupt after >>>>> both TCAR1 and TCAR2 are filled, values are passed to IR decoder and >>>>> TCAR_IT_FLAG is cleared. >>>>> >>>>> Of course, this is just proof of concept and needs to be polished and >>>>> generalized, but to make it at least work I need functions you just >>>>> unexported (plus some new). >>>>> >>>>> Question is whenever we need this level of indirection (omap_dm_timer_ops) >>>>> or plain exports are enough. >>>> >>>> The general guidance is not to do plain exports and go via >>>> omap_dm_timer_ops. >>> >>> ...in contrary what other clocksource drivers are doing. >>> >>> Now I'm assuming it is okay to extend omap_dm_timer_ops. That would mean >>> check for ops members to be assigned should be also extended or we should >>> delete it altogether and assume all members are populated? >> >> It should be fine to extend omap_dm_timer_ops. What are the ops missing >> for your new implementation? > > Read capture registers, configure capture and ack interrupt. Perhaps set_pwm > could be extended to configure capture as well. > > I'll update my code on top of your changes and we'll see how it would work. Okay Thanks! > >> Tony, >> >> Your thoughts on the above? >> >> R -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Keerthy <j-keerthy@ti.com> [171212 08:25]: > > > On Tuesday 12 December 2017 01:49 PM, Ladislav Michl wrote: > > On Tue, Dec 12, 2017 at 01:38:04PM +0530, Keerthy wrote: > >> On Tuesday 12 December 2017 01:31 PM, Ladislav Michl wrote: > >>> On Tue, Dec 12, 2017 at 01:01:51PM +0530, Keerthy wrote: > >>>> > >>>> > >>>> On Tuesday 12 December 2017 12:46 PM, Ladislav Michl wrote: > >>>>> Keerthy, > >>>>> > >>>>> On Tue, Dec 12, 2017 at 11:42:10AM +0530, Keerthy wrote: > >>>>>> Remove all the unwanted exports from the driver > >>>>> > >>>>> I'm adding event capture capability to the pwm-omap driver and so far used > >>>>> v4.15-rc3 as codebase. > >>>>> > >>>>> Intended use is an IR receiver; for that I need to measure pulses width and > >>>>> spaces between pulses. So DM timer was setup to generate interupt after > >>>>> both TCAR1 and TCAR2 are filled, values are passed to IR decoder and > >>>>> TCAR_IT_FLAG is cleared. > >>>>> > >>>>> Of course, this is just proof of concept and needs to be polished and > >>>>> generalized, but to make it at least work I need functions you just > >>>>> unexported (plus some new). > >>>>> > >>>>> Question is whenever we need this level of indirection (omap_dm_timer_ops) > >>>>> or plain exports are enough. > >>>> > >>>> The general guidance is not to do plain exports and go via > >>>> omap_dm_timer_ops. > >>> > >>> ...in contrary what other clocksource drivers are doing. Hmm what do you mean? We don't want to export tons of custom functions from the timers in and then be in trouble when at some point we have a Linux generic hw timer framework. We already had to deal with these custom exports earlier with conversion to multiarch and then again with device tree. For now, it's best to pass the timer information to the pwm driver in platform data. In the long run that will be much easier to deal with than fixing random drivers tinkering with the timer registers directly. > >>> Now I'm assuming it is okay to extend omap_dm_timer_ops. That would mean > >>> check for ops members to be assigned should be also extended or we should > >>> delete it altogether and assume all members are populated? > >> > >> It should be fine to extend omap_dm_timer_ops. What are the ops missing > >> for your new implementation? > > > > Read capture registers, configure capture and ack interrupt. Perhaps set_pwm > > could be extended to configure capture as well. > > > > I'll update my code on top of your changes and we'll see how it would work. Ideally the pwm driver would just do a request_irq from the dmtimer code where dmtimer code would implement an interrupt controller. That would be already most fo the Linux generic hardware timer framework right there :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 12, 2017 at 09:00:54AM -0800, Tony Lindgren wrote: > Hmm what do you mean? We don't want to export tons of custom functions from > the timers in and then be in trouble when at some point we have a Linux > generic hw timer framework. We already had to deal with these custom > exports earlier with conversion to multiarch and then again with > device tree. > > For now, it's best to pass the timer information to the pwm driver in > platform data. In the long run that will be much easier to deal with than > fixing random drivers tinkering with the timer registers directly. All that register access would happen only in drivers/clocksource/timer-dm.c? So platform data will hold all function pointers needed for event capture and the pwm driver will do only interface to pwm framework. > Ideally the pwm driver would just do a request_irq from the dmtimer code > where dmtimer code would implement an interrupt controller. That would > be already most fo the Linux generic hardware timer framework right there :) I do not follow. Each general-purpose timer module has its own interrupt line, so claiming that irq directly using request_irq seems enough. Could you explain interrupt controller idea a bit more? Thank you, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ladislav Michl <ladis@linux-mips.org> [171212 18:06]: > On Tue, Dec 12, 2017 at 09:00:54AM -0800, Tony Lindgren wrote: > > Hmm what do you mean? We don't want to export tons of custom functions from > > the timers in and then be in trouble when at some point we have a Linux > > generic hw timer framework. We already had to deal with these custom > > exports earlier with conversion to multiarch and then again with > > device tree. > > > > For now, it's best to pass the timer information to the pwm driver in > > platform data. In the long run that will be much easier to deal with than > > fixing random drivers tinkering with the timer registers directly. > > All that register access would happen only in drivers/clocksource/timer-dm.c? > So platform data will hold all function pointers needed for event capture > and the pwm driver will do only interface to pwm framework. Yes please. > > Ideally the pwm driver would just do a request_irq from the dmtimer code > > where dmtimer code would implement an interrupt controller. That would > > be already most fo the Linux generic hardware timer framework right there :) > > I do not follow. Each general-purpose timer module has its own interrupt line, > so claiming that irq directly using request_irq seems enough. Could you > explain interrupt controller idea a bit more? Well let's assume we have drivers/clocksource/timer-dm.c implement an irq controller. Then the pwm driver would just do: pwm9: dmtimer-pwm { compatible = "ti,omap-dmtimer-pwm"; #pwm-cells = <3>; ti,timers = <&timer9>; ti,clock-source = <0x00>; /* timer_sys_ck */ interrupts-extended = <&timer9 IRQ_TYPE_SOMETHING>; }; Then you can do whatever you need to in the pwm driver with enable_irq/disable_irq + a handler? If reading the line status is needed.. Then maybe the GPIO framework needs to have hardware timer support instead? Anyways, just thinking out loud how we could have a Linux generic hardware timer framework that drivers like pwm could then use. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 12, 2017 at 10:21:50AM -0800, Tony Lindgren wrote: > * Ladislav Michl <ladis@linux-mips.org> [171212 18:06]: > > I do not follow. Each general-purpose timer module has its own interrupt line, > > so claiming that irq directly using request_irq seems enough. Could you > > explain interrupt controller idea a bit more? > > Well let's assume we have drivers/clocksource/timer-dm.c implement > an irq controller. Then the pwm driver would just do: > > pwm9: dmtimer-pwm { > compatible = "ti,omap-dmtimer-pwm"; > #pwm-cells = <3>; > ti,timers = <&timer9>; > ti,clock-source = <0x00>; /* timer_sys_ck */ > interrupts-extended = <&timer9 IRQ_TYPE_SOMETHING>; > }; > > Then you can do whatever you need to in the pwm driver with > enable_irq/disable_irq + a handler? That seems to work. Now should we map 1:1 to timer interrupt or have separate interrupt for match, overflow and capture? Former would need some more dm_timer_ops to determine interrupt source, while later would work "automagically" - but I haven't tested it yet. > If reading the line status is needed.. Then maybe the GPIO framework > needs to have hardware timer support instead? It does not seem OMAP can read event pin value in event capture mode. > Anyways, just thinking out loud how we could have a Linux generic > hardware timer framework that drivers like pwm could then use. I need a bit longer chain: dmtimer -> pwm -> rc (which calls ir_raw_event_store from interrupt) Is extending pwm core with interrpt callback the right thing there? Something like: (*pulse_captured)(ktime_t width, ktime_t last_edge); Thank you, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ladislav Michl <ladis@linux-mips.org> [171213 09:17]: > On Tue, Dec 12, 2017 at 10:21:50AM -0800, Tony Lindgren wrote: > > * Ladislav Michl <ladis@linux-mips.org> [171212 18:06]: > > > I do not follow. Each general-purpose timer module has its own interrupt line, > > > so claiming that irq directly using request_irq seems enough. Could you > > > explain interrupt controller idea a bit more? > > > > Well let's assume we have drivers/clocksource/timer-dm.c implement > > an irq controller. Then the pwm driver would just do: > > > > pwm9: dmtimer-pwm { > > compatible = "ti,omap-dmtimer-pwm"; > > #pwm-cells = <3>; > > ti,timers = <&timer9>; > > ti,clock-source = <0x00>; /* timer_sys_ck */ > > interrupts-extended = <&timer9 IRQ_TYPE_SOMETHING>; > > }; > > > > Then you can do whatever you need to in the pwm driver with > > enable_irq/disable_irq + a handler? > > That seems to work. Now should we map 1:1 to timer interrupt or > have separate interrupt for match, overflow and capture? > Former would need some more dm_timer_ops to determine interrupt > source, while later would work "automagically" - but I haven't > tested it yet. Hmm the second option sounds appealing to me as then the device tree binding really follows the hardware: bit 2 capture interrupt bit 1 overflow interrupt bit 0 match interrupt and in the dts we can describe these with: interrupts-extended = <&timer 9 2 IRQ_TYPE_EDGE_SOMETHING>; interrupts-extended = <&timer 9 1 IRQ_TYPE_EDGE_SOMETHING>; interrupts-extended = <&timer 9 0 IRQ_TYPE_EDGE_SOMETHING>; I think these are all edge based on "Capture Mode Functionality" chapter in the TRM? > > If reading the line status is needed.. Then maybe the GPIO framework > > needs to have hardware timer support instead? > > It does not seem OMAP can read event pin value in event capture mode. Sounds like a bug somehwere, probably in software? > > Anyways, just thinking out loud how we could have a Linux generic > > hardware timer framework that drivers like pwm could then use. > > I need a bit longer chain: > dmtimer -> pwm -> rc (which calls ir_raw_event_store from interrupt) > Is extending pwm core with interrpt callback the right thing there? > Something like: > (*pulse_captured)(ktime_t width, ktime_t last_edge); OK seems like having drivers/clocksource/timer-dm.c a chained interrupt handler should do it :) Anyways, that's a different set of patches on top of these. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c index d443e48..72565fc 100644 --- a/arch/arm/plat-omap/dmtimer.c +++ b/arch/arm/plat-omap/dmtimer.c @@ -302,7 +302,6 @@ struct omap_dm_timer *omap_dm_timer_request(void) { return _omap_dm_timer_request(REQUEST_ANY, NULL); } -EXPORT_SYMBOL_GPL(omap_dm_timer_request); struct omap_dm_timer *omap_dm_timer_request_specific(int id) { @@ -315,7 +314,6 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id) return _omap_dm_timer_request(REQUEST_BY_ID, &id); } -EXPORT_SYMBOL_GPL(omap_dm_timer_request_specific); /** * omap_dm_timer_request_by_cap - Request a timer by capability @@ -330,7 +328,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap) { return _omap_dm_timer_request(REQUEST_BY_CAP, &cap); } -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_cap); /** * omap_dm_timer_request_by_node - Request a timer by device-tree node @@ -346,7 +343,6 @@ struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np) return _omap_dm_timer_request(REQUEST_BY_NODE, np); } -EXPORT_SYMBOL_GPL(omap_dm_timer_request_by_node); int omap_dm_timer_free(struct omap_dm_timer *timer) { @@ -359,7 +355,6 @@ int omap_dm_timer_free(struct omap_dm_timer *timer) timer->reserved = 0; return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_free); void omap_dm_timer_enable(struct omap_dm_timer *timer) { @@ -379,13 +374,11 @@ void omap_dm_timer_enable(struct omap_dm_timer *timer) } } } -EXPORT_SYMBOL_GPL(omap_dm_timer_enable); void omap_dm_timer_disable(struct omap_dm_timer *timer) { pm_runtime_put_sync(&timer->pdev->dev); } -EXPORT_SYMBOL_GPL(omap_dm_timer_disable); int omap_dm_timer_get_irq(struct omap_dm_timer *timer) { @@ -393,7 +386,6 @@ int omap_dm_timer_get_irq(struct omap_dm_timer *timer) return timer->irq; return -EINVAL; } -EXPORT_SYMBOL_GPL(omap_dm_timer_get_irq); #if defined(CONFIG_ARCH_OMAP1) #include <mach/hardware.h> @@ -429,7 +421,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) return inputmask; } -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); #else @@ -439,7 +430,6 @@ struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer) return timer->fclk; return NULL; } -EXPORT_SYMBOL_GPL(omap_dm_timer_get_fclk); __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) { @@ -447,7 +437,6 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask) return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask); #endif @@ -461,7 +450,6 @@ int omap_dm_timer_trigger(struct omap_dm_timer *timer) omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_trigger); int omap_dm_timer_start(struct omap_dm_timer *timer) { @@ -482,7 +470,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer) timer->context.tclr = l; return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_start); int omap_dm_timer_stop(struct omap_dm_timer *timer) { @@ -506,7 +493,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer) omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_stop); int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) { @@ -569,7 +555,6 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source) return ret; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_source); int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, unsigned int load) @@ -595,7 +580,6 @@ int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load); /* Optimized set_load which removes costly spin wait in timer_start */ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, @@ -625,7 +609,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, timer->context.tcrr = load; return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_load_start); int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, unsigned int match) @@ -650,7 +633,6 @@ int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_match); int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, int trigger) @@ -676,7 +658,6 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_pwm); int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) { @@ -699,7 +680,6 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler) omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_prescaler); int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value) @@ -716,7 +696,6 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_enable); /** * omap_dm_timer_set_int_disable - disable timer interrupts @@ -747,7 +726,6 @@ int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask) omap_dm_timer_disable(timer); return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_disable); unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) { @@ -762,7 +740,6 @@ unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer) return l; } -EXPORT_SYMBOL_GPL(omap_dm_timer_read_status); int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) { @@ -773,7 +750,6 @@ int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value) return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_write_status); unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) { @@ -784,7 +760,6 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer) return __omap_dm_timer_read_counter(timer, timer->posted); } -EXPORT_SYMBOL_GPL(omap_dm_timer_read_counter); int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) { @@ -799,7 +774,6 @@ int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value) timer->context.tcrr = value; return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timer_write_counter); int omap_dm_timers_active(void) { @@ -816,7 +790,6 @@ int omap_dm_timers_active(void) } return 0; } -EXPORT_SYMBOL_GPL(omap_dm_timers_active); static const struct of_device_id omap_timer_match[];