| Message ID | 87mxi1n7ql.fsf@nemi.mork.no (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Bjørn Mork <bjorn@mork.no> writes: > diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c > index 7904ca4..d994592 100644 > --- a/drivers/media/video/em28xx/em28xx-dvb.c > +++ b/drivers/media/video/em28xx/em28xx-dvb.c > @@ -669,7 +669,8 @@ static int dvb_init(struct em28xx *dev) > &em28xx_cxd2820r_config, &dev->i2c_adap, NULL); > if (dvb->fe[0]) { > struct i2c_adapter *i2c_tuner; > - i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]); > + /* we don't really attach i2c_tuner. Just reusing the symbol logic */ > + i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]); Except that this really messes up the reference count, and need to have a matching symbol_put... So you should probably code it with symbol_request()/symbol_put() if you want to go this way instead of the dvb_attach shortcut . Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/01/2011 08:18 PM, Bjørn Mork wrote: > Bjørn Mork<bjorn@mork.no> writes: > >> diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c >> index 7904ca4..d994592 100644 >> --- a/drivers/media/video/em28xx/em28xx-dvb.c >> +++ b/drivers/media/video/em28xx/em28xx-dvb.c >> @@ -669,7 +669,8 @@ static int dvb_init(struct em28xx *dev) >> &em28xx_cxd2820r_config,&dev->i2c_adap, NULL); >> if (dvb->fe[0]) { >> struct i2c_adapter *i2c_tuner; >> - i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]); >> + /* we don't really attach i2c_tuner. Just reusing the symbol logic */ >> + i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]); > > Except that this really messes up the reference count, and need to have > a matching symbol_put... So you should probably code it with > symbol_request()/symbol_put() if you want to go this way instead of > the dvb_attach shortcut . There is some other FEs having also I2C adapter, I wonder how those handle this situation. I looked example from cx24123 and s5h1420 drivers, both used by flexcop. Did you see what is magic used those devices? best regards, Antti
Antti Palosaari <crope@iki.fi> writes: > There is some other FEs having also I2C adapter, I wonder how those > handle this situation. I looked example from cx24123 and s5h1420 > drivers, both used by flexcop. > > Did you see what is magic used those devices? None. They have the same problem, creating hard module dependencies even if they use dvb_attach() and CONFIG_MEDIA_ATTACH is set: bjorn@canardo:~$ modinfo b2c2-flexcop filename: /lib/modules/2.6.32-5-amd64/kernel/drivers/media/dvb/b2c2/b2c2-flexcop.ko license: GPL description: B2C2 FlexcopII/II(b)/III digital TV receiver chip author: Patrick Boettcher <patrick.boettcher@desy.de depends: s5h1420,dvb-core,cx24113,cx24123,i2c-core vermagic: 2.6.32-5-amd64 SMP mod_unload modversions parm: debug:set debug level (1=info,2=tuner,4=i2c,8=ts,16=sram,32=reg (|-able)). (debugging is not enabled) (int) parm: adapter_nr:DVB adapter numbers (array of short) This probably means that a generic i2c_tuner wrapper, similar to dvb_attach, would be useful. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/03/2011 03:50 PM, Bjørn Mork wrote: > Antti Palosaari<crope@iki.fi> writes: > >> There is some other FEs having also I2C adapter, I wonder how those >> handle this situation. I looked example from cx24123 and s5h1420 >> drivers, both used by flexcop. >> >> Did you see what is magic used those devices? > > None. They have the same problem, creating hard module dependencies > even if they use dvb_attach() and CONFIG_MEDIA_ATTACH is set: > > bjorn@canardo:~$ modinfo b2c2-flexcop > filename: /lib/modules/2.6.32-5-amd64/kernel/drivers/media/dvb/b2c2/b2c2-flexcop.ko > license: GPL > description: B2C2 FlexcopII/II(b)/III digital TV receiver chip > author: Patrick Boettcher<patrick.boettcher@desy.de > depends: s5h1420,dvb-core,cx24113,cx24123,i2c-core > vermagic: 2.6.32-5-amd64 SMP mod_unload modversions > parm: debug:set debug level (1=info,2=tuner,4=i2c,8=ts,16=sram,32=reg (|-able)). (debugging is not enabled) (int) > parm: adapter_nr:DVB adapter numbers (array of short) > > > > This probably means that a generic i2c_tuner wrapper, similar to > dvb_attach, would be useful. For the cxd2820r it is also possible to return I2C adapter as pointer from dvb_attach like pointer to FE0 is carried for FE1 dvb_attach. What you think about that? regards, Antti
Antti Palosaari <crope@iki.fi> writes: > On 06/03/2011 03:50 PM, Bjørn Mork wrote: > >> This probably means that a generic i2c_tuner wrapper, similar to >> dvb_attach, would be useful. > > For the cxd2820r it is also possible to return I2C adapter as pointer > from dvb_attach like pointer to FE0 is carried for FE1 > dvb_attach. What you think about that? I don't feel competent to answer that at all. It does seem like overloading an existing interface, but it might be OK. I just grepped a bit around for EXPORT_SYMBOL of anything except foo_attach, and found that there are a few frontend drivers which exports multiple symbols: bjorn@canardo:/usr/local/src/git/linux-2.6$ grep EXPORT_SYMBOL drivers/media/dvb/frontends/*.c|grep -v _attach drivers/media/dvb/frontends/cx24113.c:EXPORT_SYMBOL(cx24113_agc_callback); drivers/media/dvb/frontends/cx24123.c:EXPORT_SYMBOL(cx24123_get_tuner_i2c_adapter); drivers/media/dvb/frontends/cxd2820r_core.c:EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter); drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_ctrl_agc_filter); drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_get_rf_output); drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_set_rf_output); drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_wbd_offset); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_dcc_freq); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_pwm_gain_reset); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_gain_control); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_current_gain); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_wbd_offset); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_tune_state); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_set_tune_state); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_register); drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_fw_register); drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_get_tuner_i2c_master); drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_pid_control); drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_pid_parse); drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_set_config); drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_i2c_enumeration); drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_get_i2c_master); drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_pid_filter_ctrl); drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_pid_filter); drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_i2c_enumeration); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_set_wbd_ref); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_update_pll); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_set_gpio); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_ctrl_timf); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000pc_detection); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_get_i2c_master); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_pid_filter_ctrl); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_pid_filter); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_i2c_enumeration); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_get_i2c_tuner); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_tuner_sleep); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_agc_restart); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_get_adc_power); drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_slave_reset); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_wbd_ref); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_gpio); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pwm_agc_reset); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_adc_power); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_tune_state); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_tune_state); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_slave_frontend); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_remove_slave_frontend); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_slave_frontend); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_i2c_enumeration); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_i2c_master); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pid_filter_ctrl); drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pid_filter); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_set_component_bus_speed); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_tuner_interface); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_component_bus_interface); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_i2c_master); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_i2c_adapter); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_gpio); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_pid_filter_ctrl); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_pid_filter); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_firmware_post_pll_init); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_i2c_enumeration); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_slave_frontend); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_remove_slave_frontend); drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_slave_frontend); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_i2c_set_speed); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_get_i2c_adapter); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_reset_i2c_master); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_init_i2c_master); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_exit_i2c_master); drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(systime); drivers/media/dvb/frontends/drxd_hard.c:EXPORT_SYMBOL(drxd_config_i2c); drivers/media/dvb/frontends/s5h1420.c:EXPORT_SYMBOL(s5h1420_get_tuner_i2c_adapter); drivers/media/dvb/frontends/stv090x.c:EXPORT_SYMBOL(stv090x_set_gpio); Which does show up as hard dependencies on your typical Debian installation (which does set CONFIG_MEDIA_ATTACH): bjorn@nemi:~$ modinfo -F depends dvb-usb-dib0700 dib8000,dvb-usb,i2c-core,dib0070,dib3000mc,usbcore,dib7000p,dib7000m So it looks like this problem is much more widespread than I initially thought, and it may therefore be perfectly OK to do what you did. However, the em28xx-dvb driver can be used with a large number of frontends and we certainly don't want them all as hard dependencies. I think it's time for someone with authority to state what is acceptable and what is not, with regard to frontends, tuners, dvb_attach(), EXPORT_SYMBOL and CONFIG_MEDIA_ATTACH. For all I know, this may already be documented somewhere. If so, please point me there. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From b570bbad12c1d164ed92c6711a1775db29c4c0a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no> Date: Wed, 1 Jun 2011 12:48:25 +0200 Subject: [PATCH] em28xx-dvb: Use dvb_attach to call cxd2820r_get_tuner_i2c_adapter() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This avoids a hard module dependency on cxd2820r. Even though we don't really attach anything in this call, we can stil reuse dvb_attach since the called function is expected to return a pointer. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/media/video/em28xx/em28xx-dvb.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c index 7904ca4..d994592 100644 --- a/drivers/media/video/em28xx/em28xx-dvb.c +++ b/drivers/media/video/em28xx/em28xx-dvb.c @@ -669,7 +669,8 @@ static int dvb_init(struct em28xx *dev) &em28xx_cxd2820r_config, &dev->i2c_adap, NULL); if (dvb->fe[0]) { struct i2c_adapter *i2c_tuner; - i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]); + /* we don't really attach i2c_tuner. Just reusing the symbol logic */ + i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]); /* FE 0 attach tuner */ if (!dvb_attach(tda18271_attach, dvb->fe[0], 0x60, i2c_tuner, &em28xx_cxd2820r_tda18271_config)) { -- 1.7.2.5