On Wed, Oct 16, 2013 at 1:01 PM, Michael Krufky <mkrufky@xxxxxxxxxxx> wrote: > On Wed, Oct 16, 2013 at 12:43 PM, Antti Palosaari <crope@xxxxxx> wrote: >> On 16.10.2013 18:54, Michael Krufky wrote: >>> >>> This kinda makes me a bit nervous. The patch itself looks OK but the >>> cascading effects that it will have across the DVB subsystem need to >>> be discussed. >> >> >> Basically, the only effect is that you must use i2c_new_device() or >> i2c_new_probed_device() instead of old proprietary dvb_attach(). Convert old >> proprietary method with Kernel standard one. >> >> But thats not all. It was only first step. My final goal is to study, test >> and finally make radio tuners as a own "standalone" I2C drivers. This means >> that I will remove DVB frontend dependency next from the given tuner driver. >> And before you ask why: I need some more sophisticated access to tuner in >> order to meet SDR needs. Also I don't see any reason why those tuners should >> be a part of DVB API or part of V4L2 API (or even both in case of "hybrid" >> model). Actual reasons are likely payload from the history, but as of today >> view it is stupid design :] >> >> >>> Is there a discussion about this kind of conversion on the mailing >>> list somewhere that I've missed? >> >> >> There has been many times around the years. i2c_new_probed_device() function >> [1] seems to be added for somehow for out needs too... >> >> [1] http://lists.lm-sensors.org/pipermail/i2c/2007-March/000990.html >> >> >> regards >> Antti >> >> >>> >>> -Mike >>> >>> On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab >>> <m.chehab@xxxxxxxxxxx> wrote: >>>> >>>> Em Wed, 16 Oct 2013 01:31:04 +0300 >>>> Antti Palosaari <crope@xxxxxx> escreveu: >>>> >>>>> Initial driver conversion from proprietary DVB tuner model to more >>>>> general I2C driver model. >>>>> >>>>> That commit has just basic binding stuff and driver itself still >>>>> needs to be converted more complete later. >>>>> >>>>> Cc: Jean Delvare <khali@xxxxxxxxxxxx> >>>>> Signed-off-by: Antti Palosaari <crope@xxxxxx> >>>>> --- >>>>> drivers/media/tuners/e4000.c | 73 >>>>> ++++++++++++++++++++++----------- >>>>> drivers/media/tuners/e4000.h | 9 +++- >>>>> drivers/media/tuners/e4000_priv.h | 4 +- >>>>> drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++----- >>>>> 4 files changed, 78 insertions(+), 39 deletions(-) >>>>> >>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >>>>> index 54e2d8a..f6a5dbd 100644 >>>>> --- a/drivers/media/tuners/e4000.c >>>>> +++ b/drivers/media/tuners/e4000.c >>>>> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 >>>>> reg, u8 *val, int len) >>>>> u8 buf[1 + len]; >>>>> struct i2c_msg msg[1] = { >>>>> { >>>>> - .addr = priv->cfg->i2c_addr, >>>>> + .addr = priv->i2c_addr, >>>>> .flags = 0, >>>>> .len = sizeof(buf), >>>>> .buf = buf, >>>>> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 >>>>> reg, u8 *val, int len) >>>>> u8 buf[len]; >>>>> struct i2c_msg msg[2] = { >>>>> { >>>>> - .addr = priv->cfg->i2c_addr, >>>>> + .addr = priv->i2c_addr, >>>>> .flags = 0, >>>>> .len = 1, >>>>> .buf = ®, >>>>> }, { >>>>> - .addr = priv->cfg->i2c_addr, >>>>> + .addr = priv->i2c_addr, >>>>> .flags = I2C_M_RD, >>>>> .len = sizeof(buf), >>>>> .buf = buf, >>>>> @@ -233,8 +233,8 @@ static int e4000_set_params(struct dvb_frontend *fe) >>>>> * or more. >>>>> */ >>>>> f_vco = c->frequency * e4000_pll_lut[i].mul; >>>>> - sigma_delta = div_u64(0x10000ULL * (f_vco % priv->cfg->clock), >>>>> priv->cfg->clock); >>>>> - buf[0] = f_vco / priv->cfg->clock; >>>>> + sigma_delta = div_u64(0x10000ULL * (f_vco % priv->clock), >>>>> priv->clock); >>>>> + buf[0] = f_vco / priv->clock; >>>>> buf[1] = (sigma_delta >> 0) & 0xff; >>>>> buf[2] = (sigma_delta >> 8) & 0xff; >>>>> buf[3] = 0x00; >>>>> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct >>>>> dvb_frontend *fe, u32 *frequency) >>>>> return 0; >>>>> } >>>>> >>>>> -static int e4000_release(struct dvb_frontend *fe) >>>>> -{ >>>>> - struct e4000_priv *priv = fe->tuner_priv; >>>>> - >>>>> - dev_dbg(&priv->i2c->dev, "%s:\n", __func__); >>>>> - >>>>> - kfree(fe->tuner_priv); >>>>> - >>>>> - return 0; >>>>> -} >>>>> - >>>>> static const struct dvb_tuner_ops e4000_tuner_ops = { >>>>> .info = { >>>>> .name = "Elonics E4000", >>>>> @@ -376,8 +365,6 @@ static const struct dvb_tuner_ops e4000_tuner_ops = >>>>> { >>>>> .frequency_max = 862000000, >>>>> }, >>>>> >>>>> - .release = e4000_release, >>>>> - >>>>> .init = e4000_init, >>>>> .sleep = e4000_sleep, >>>>> .set_params = e4000_set_params, >>>>> @@ -385,9 +372,12 @@ static const struct dvb_tuner_ops e4000_tuner_ops = >>>>> { >>>>> .get_if_frequency = e4000_get_if_frequency, >>>>> }; >>>>> >>>>> -struct dvb_frontend *e4000_attach(struct dvb_frontend *fe, >>>>> - struct i2c_adapter *i2c, const struct e4000_config *cfg) >>>>> +static int e4000_probe(struct i2c_client *client, >>>>> + const struct i2c_device_id *id) >>>>> { >>>>> + struct e4000_config *cfg = client->dev.platform_data; >>>>> + struct dvb_frontend *fe = cfg->fe; >>>>> + struct i2c_adapter *i2c = client->adapter; >>>>> struct e4000_priv *priv; >>>>> int ret; >>>>> u8 chip_id; >>>>> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct >>>>> dvb_frontend *fe, >>>>> goto err; >>>>> } >>>>> >>>>> - priv->cfg = cfg; >>>>> + priv->i2c_addr = cfg->i2c_addr; >>>>> + priv->clock = cfg->clock; >>>>> + priv->i2c_client = client; >>>>> priv->i2c = i2c; >>>>> >>>>> /* check if the tuner is there */ >>>>> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct >>>>> dvb_frontend *fe, >>>>> >>>>> dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, >>>>> chip_id); >>>>> >>>>> - if (chip_id != 0x40) >>>>> + if (chip_id != 0x40) { >>>>> + ret = -ENODEV; >>>>> goto err; >>>>> + } >>>>> >>>>> /* put sleep as chip seems to be in normal mode by default */ >>>>> ret = e4000_wr_reg(priv, 0x00, 0x00); >>>>> @@ -431,16 +425,45 @@ struct dvb_frontend *e4000_attach(struct >>>>> dvb_frontend *fe, >>>>> if (fe->ops.i2c_gate_ctrl) >>>>> fe->ops.i2c_gate_ctrl(fe, 0); >>>>> >>>>> - return fe; >>>>> + i2c_set_clientdata(client, priv); >>>>> + >>>>> + return 0; >>>>> err: >>>>> if (fe->ops.i2c_gate_ctrl) >>>>> fe->ops.i2c_gate_ctrl(fe, 0); >>>>> >>>>> dev_dbg(&i2c->dev, "%s: failed=%d\n", __func__, ret); >>>>> kfree(priv); >>>>> - return NULL; >>>>> + return ret; >>>>> } >>>>> -EXPORT_SYMBOL(e4000_attach); >>>>> + >>>>> +static int e4000_remove(struct i2c_client *client) >>>>> +{ >>>>> + struct e4000_priv *priv = i2c_get_clientdata(client); >>>>> + >>>>> + dev_dbg(&client->dev, "%s:\n", __func__); >>>>> + >>>>> + kfree(priv); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct i2c_device_id e4000_id[] = { >>>>> + {"e4000", 0}, >>>>> + {} >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(i2c, e4000_id); >>>>> + >>>>> +static struct i2c_driver e4000_driver = { >>>>> + .driver = { >>>>> + .owner = THIS_MODULE, >>>>> + .name = "e4000", >>>>> + }, >>>>> + .probe = e4000_probe, >>>>> + .remove = e4000_remove, >>>>> + .id_table = e4000_id, >>>>> +}; >>>>> + >>>>> +module_i2c_driver(e4000_driver); >>>>> >>>>> MODULE_DESCRIPTION("Elonics E4000 silicon tuner driver"); >>>>> MODULE_AUTHOR("Antti Palosaari <crope@xxxxxx>"); >>>>> diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h >>>>> index 25ee7c0..760b206 100644 >>>>> --- a/drivers/media/tuners/e4000.h >>>>> +++ b/drivers/media/tuners/e4000.h >>>>> @@ -26,6 +26,11 @@ >>>>> >>>>> struct e4000_config { >>>>> /* >>>>> + * frontend >>>>> + */ >>>>> + struct dvb_frontend *fe; >>>>> + >>>>> + /* >>>>> * I2C address >>>>> * 0x64, 0x65, 0x66, 0x67 >>>>> */ >>>>> @@ -39,10 +44,10 @@ struct e4000_config { >>>>> >>>>> #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000) >>>> >>>> >>>> You can get rid of #if IS_ENABLED, by replacing it by a generic attach >>>> function, using the subdev model. In other words, this is how a tuner >>>> subdev is defined: >>>> >>>> struct v4l2_subdev_tuner_ops { >>>> int (*s_radio)(struct v4l2_subdev *sd); >>>> int (*s_frequency)(struct v4l2_subdev *sd, const struct >>>> v4l2_frequency *freq); >>>> int (*g_frequency)(struct v4l2_subdev *sd, struct >>>> v4l2_frequency *freq); >>>> int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner >>>> *vt); >>>> int (*s_tuner)(struct v4l2_subdev *sd, const struct >>>> v4l2_tuner *vt); >>>> int (*g_modulator)(struct v4l2_subdev *sd, struct >>>> v4l2_modulator *vm); >>>> int (*s_modulator)(struct v4l2_subdev *sd, const struct >>>> v4l2_modulator *vm); >>>> int (*s_type_addr)(struct v4l2_subdev *sd, struct >>>> tuner_setup *type); >>>> int (*s_config)(struct v4l2_subdev *sd, const struct >>>> v4l2_priv_tun_config *config); >>>> }; >>>> >>>> As the presence of the device is known, there's no need to actually >>>> attach >>>> anything, but you need to pass the configuration for the subdevice. This >>>> is >>>> done by calling the subdev s_config ops to setup them: >>>> >>>> v4l2_subdev_call(sd, tuner, s_config, vfh, arg); >>>> >>>> A new callback will be needed there, in order to put the device into DVB >>>> mode, >>>> e. g. passing the DVB specific parameters. >>>> >>>> As the subdev callbacks are defined on a generic .h header and they >>>> always >>>> exist, if the driver is not compiled, i2c_new_device() won't find the >>>> driver, >>>> and the bridge driver won't call s_config() for this I2C client. >>>> >>>> That's IMHO, one of the biggest advantages of this conversion: as a >>>> bonus, >>>> we get rid of several Kconfig issues. Also, with this change, lsmod will >>>> properly show that the tuner is bound to the bridge driver. >>>> >>>>> extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe, >>>>> - struct i2c_adapter *i2c, const struct e4000_config *cfg); >>>>> + struct i2c_adapter *i2c, struct e4000_config *cfg); >>>>> #else >>>>> static inline struct dvb_frontend *e4000_attach(struct dvb_frontend >>>>> *fe, >>>>> - struct i2c_adapter *i2c, const struct e4000_config *cfg) >>>>> + struct i2c_adapter *i2c, struct e4000_config *cfg) >>>>> { >>>>> dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", >>>>> __func__); >>>>> return NULL; >>>>> diff --git a/drivers/media/tuners/e4000_priv.h >>>>> b/drivers/media/tuners/e4000_priv.h >>>>> index a385505..7f47068 100644 >>>>> --- a/drivers/media/tuners/e4000_priv.h >>>>> +++ b/drivers/media/tuners/e4000_priv.h >>>>> @@ -24,8 +24,10 @@ >>>>> #include "e4000.h" >>>>> >>>>> struct e4000_priv { >>>>> - const struct e4000_config *cfg; >>>>> struct i2c_adapter *i2c; >>>>> + struct i2c_client *i2c_client; >>>>> + u8 i2c_addr; >>>>> + u32 clock; >>>>> }; >>>>> >>>>> struct e4000_pll { >>>>> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c >>>>> b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c >>>>> index defc491..573805a 100644 >>>>> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c >>>>> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c >>>>> @@ -843,11 +843,6 @@ err: >>>>> return ret; >>>>> } >>>>> >>>>> -static const struct e4000_config rtl2832u_e4000_config = { >>>>> - .i2c_addr = 0x64, >>>>> - .clock = 28800000, >>>>> -}; >>>>> - >>>>> static const struct fc2580_config rtl2832u_fc2580_config = { >>>>> .i2c_addr = 0x56, >>>>> .clock = 16384000, >>>>> @@ -874,10 +869,14 @@ static int rtl2832u_tuner_attach(struct >>>>> dvb_usb_adapter *adap) >>>>> int ret; >>>>> struct dvb_usb_device *d = adap_to_d(adap); >>>>> struct rtl28xxu_priv *priv = d_to_priv(d); >>>>> - struct dvb_frontend *fe; >>>>> + struct dvb_frontend *fe = NULL; >>>>> + struct i2c_client *client = NULL; >>>>> + struct i2c_board_info info; >>>>> >>>>> dev_dbg(&d->udev->dev, "%s:\n", __func__); >>>>> >>>>> + memset(&info, 0, sizeof(struct i2c_board_info)); >>>>> + >>>>> switch (priv->tuner) { >>>>> case TUNER_RTL2832_FC0012: >>>>> fe = dvb_attach(fc0012_attach, adap->fe[0], >>>>> @@ -897,9 +896,20 @@ static int rtl2832u_tuner_attach(struct >>>>> dvb_usb_adapter *adap) >>>>> adap->fe[0]->ops.read_signal_strength = >>>>> >>>>> adap->fe[0]->ops.tuner_ops.get_rf_strength; >>>>> return 0; >>>>> - case TUNER_RTL2832_E4000: >>>>> - fe = dvb_attach(e4000_attach, adap->fe[0], &d->i2c_adap, >>>>> - &rtl2832u_e4000_config); >>>>> + case TUNER_RTL2832_E4000: { >>>>> + struct e4000_config e4000_config = { >>>>> + .fe = adap->fe[0], >>>>> + .i2c_addr = 0x64, >>>>> + .clock = 28800000, >>>>> + }; >>>>> + >>>>> + strlcpy(info.type, "e4000", I2C_NAME_SIZE); >>>>> + info.addr = 0x64; >>>>> + info.platform_data = &e4000_config; >>>>> + >>>>> + request_module("e4000"); >>>>> + client = i2c_new_device(&d->i2c_adap, &info); >>>>> + } >>>>> break; >>>>> case TUNER_RTL2832_FC2580: >>>>> fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap, >>>>> @@ -927,12 +937,11 @@ static int rtl2832u_tuner_attach(struct >>>>> dvb_usb_adapter *adap) >>>>> >>>>> adap->fe[0]->ops.tuner_ops.get_rf_strength; >>>>> break; >>>>> default: >>>>> - fe = NULL; >>>>> dev_err(&d->udev->dev, "%s: unknown tuner=%d\n", >>>>> KBUILD_MODNAME, >>>>> priv->tuner); >>>>> } >>>>> >>>>> - if (fe == NULL) { >>>>> + if (fe == NULL && (client == NULL || client->driver == NULL)) { >>>>> ret = -ENODEV; >>>>> goto err; >>>>> } >>>> >>>> >>>> >>>> -- >>>> >>>> Cheers, >>>> Mauro >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> http://palosaari.fi/ > > > > Ugh, apologies for my previous toppost(again) ... I wish gmail had an > option for replies below quoted text, instead of the default. (If > someone knows a way - please let me know ;-) ) > > I only ask because I'm used to seeing an RFC proposal on the mailing > list for this type of change - I can't spend all day on irc like I had > in the past. > > Anyway, cool... I have been wanting to do something like this for > years, too... but not exactly this way. Meanwhile, it does indeed > seem like i2c_new_probed_device() will achieve what we're looking for, > so perhaps this will indeed be a good solution. > > Are we sure that this leaves total control of the driver attachment to > the driver itself? I mean, no worries of the wrong i2c tuner driver > trying to attach? No worries about automatic probing that could > damage the hardware? It seems OK to me but somebody needs to ask > those questions ;-) > > -Mike YIKES!! i2c_new_probed_device() does indeed probe the hardware -- this is unacceptable, as such an action can damage the ic. Is there some additional information that I'm missing that lets this perform an attach without probe? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
