Em Mon, 14 May 2018 03:13:44 +0900
Akihiro TSUKADA <tskd08@xxxxxxxxx> escreveu:
> Hi,
> thanks for the review.
>
> >> +gl861_i2c_rawwrite(struct dvb_usb_device *d, u8 addr, u8 *wbuf, u16 wlen)>> +{>> + u8 *buf;>> + int ret;>> +>> + buf = kmalloc(wlen, GFP_KERNEL);>> + if (!buf)>> + return -ENOMEM;>> +>> + usleep_range(1000, 2000); /* avoid I2C errors */> > I guess this is probably also at the original code, but it seems really> weird to sleep here just after kmalloc().> > I would move any need for sleeps to the i2c xfer function, where it> makes clearer why it is needed and where. Same applies to other> usleep_range() calls at the functions below.
> those sleeps are for placing time gap between consecutive i2c transactions,
> but I agree that they should be moved to the i2c_xfer loop.
> I'll fix them in the next version.
Ok, thanks!
>
> >> +/*
> >> + * In Friio,
> >> + * I2C commnucations to the tuner are relay'ed via the demod (via the bridge),
> >> + * so its encapsulation to USB message is different from the one to the demod.
> >
> > This is quite common. I guess the rationale of using the demod's I2C mux
> > is to avoid noise at the tuner when there are I2C traffic that aren't related
> > to it.
> >
> > You should probably implement it via I2C_MUX support.
>
> I know that the i2c bus structure is common,
> but as I wrote to Antti before,
> i2c transactions to the tuner use the different USB transactions with
> different 'request' value from the one used in the other demod transactions.
>
> Here is the packet log example (I posted to linux-media before):
> "Re: [PATCH v4] dvb-usb/friio, dvb-usb-v2/gl861: decompose friio and merge with gl861"
> Message ID: <f047a680-436b-bf40-ae0a-68279366b668@xxxxxxxxx>
> Sent on: Sun, 1 Apr 2018 00:30:49 +0900
>
> I still do not understand how this can be implemented with I2C_MUX,
> or in the demod/tuner drivers.
> i2c to the tuner does not need gating/arbitration/locking,
> only needs to access via the demod reg,
> which is already implemented in the adapter by the demod,
> but it must be sent in the distinct USB transaction with its own 'request' value.
So what? Take a look, for example, at em28xx implementation:
drivers/media/usb/em28xx/em28xx-i2c.c
It has different functions for bus A (with is common on all em28xx
chipsets) and for bus B (with is present only on newer chips).
If you see at I2C xfer implementation, you'll see that the logic
there handles the bus:
static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
struct i2c_msg msgs[], int num)
{
...
/* Switch I2C bus if needed */
if (bus != dev->cur_i2c_bus &&
i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
if (bus == 1)
reg = EM2874_I2C_SECONDARY_BUS_SELECT;
else
reg = 0;
em28xx_write_reg_bits(dev, EM28XX_R06_I2C_CLK, reg,
EM2874_I2C_SECONDARY_BUS_SELECT);
dev->cur_i2c_bus = bus;
}
...
} else if (msgs[i].flags & I2C_M_RD) {
/* read bytes */
rc = i2c_recv_bytes(i2c_bus, msgs[i]);
There, not only the I2C bus need to be known by the routines,
but also switching from one bus to the other requires a register
write, depending on the device type (as can be seen when the I2C
buses are reigstered, at em28xx_cards.c:
/* register i2c bus 0 */
if (dev->board.is_em2800)
retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
else
retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
if (retval < 0) {
dev_err(&dev->intf->dev,
"%s: em28xx_i2c_register bus 0 - error [%d]!\n",
__func__, retval);
return retval;
}
/* register i2c bus 1 */
if (dev->def_i2c_bus) {
if (dev->is_em25xx)
retval = em28xx_i2c_register(dev, 1,
EM28XX_I2C_ALGO_EM25XX_BUS_B);
else
retval = em28xx_i2c_register(dev, 1,
EM28XX_I2C_ALGO_EM28XX);
if (retval < 0) {
dev_err(&dev->intf->dev,
"%s: em28xx_i2c_register bus 1 - error [%d]!\n",
__func__, retval);
em28xx_i2c_unregister(dev, 0);
return retval;
}
}
>
> >> +/* init/config of Friio demod chip(?) */
> >> +static int friio_reset(struct dvb_usb_device *d)
> >> +{
> >> + int i, ret;
> >> + u8 wbuf[2], rbuf[2];
> >> +
> >> + static const u8 friio_init_cmds[][2] = {
> >> + {0x33, 0x08}, {0x37, 0x40}, {0x3a, 0x1f}, {0x3b, 0xff},
> >> + {0x3c, 0x1f}, {0x3d, 0xff}, {0x38, 0x00}, {0x35, 0x00},
> >> + {0x39, 0x00}, {0x36, 0x00},
> >> + };
> >> +
> >> + ret = usb_set_interface(d->udev, 0, 0);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + wbuf[0] = 0x11;
> >> + wbuf[1] = 0x02;
> >> + ret = gl861_i2c_msg(d, 0x00, wbuf, 2, NULL, 0);
> >> + if (ret < 0)
> >> + return ret;
> >> + usleep_range(2000, 3000);
> >> +
> >> + wbuf[0] = 0x11;
> >> + wbuf[1] = 0x00;
> >> + ret = gl861_i2c_msg(d, 0x00, wbuf, 2, NULL, 0);
> >> + if (ret < 0)
> >> + return ret;
> >> + usleep_range(1000, 2000);
> >
> > Hmm... you had already usleeps() all over I2C xfer routines. Why adding
> > extra sleeps here?
>
> Those sleeps were added to (roughly) simulate the timing
> seen in the packet capture logs on a Windows box.
> They certainly include the time for application processing,
> but may include the necessary wait time after each command/i2c-transaction,
> so they are kept for safety.
Yes, but you have sleeps there and also at the I2C register setting
routines. Having both seems weird.
>
> > Also, why isn't it calling i2c_transfer() instead of gl861_i2c_msg()?
> > Same applies to other similar calls.
>
> Because friio_reset can be called in the following trace:
> friio_reset
> friio_power_ctrl
> d->props->power_ctrl
> dvb_usbv2_device_power_ctrl
> dvb_usbv2_init
>
> and in dvb_usbv2_init, dvb_usbv2_device_power_ctrl is called
> BEFORE i2c adapter initialization (dvb_usbv2_i2c_init).
Ah, I see. Please document it then.
>
> >> +static int friio_tuner_detach(struct dvb_usb_adapter *adap)
> >> +{
> >> + struct friio_priv *priv;
> >> +
> >> + priv = adap_to_priv(adap);
> >> +#ifndef CONFIG_MEDIA_ATTACH
> >> + /* Note:
> >> + * When CONFIG_MEDIA_ATTACH is defined,
> >> + * the tuner module is already "put" by the following call trace:
> >> + *
> >> + * symbol_put_addr(fe->ops.tuner_ops.release)
> >> + * dvb_frontend_invoke_release(fe, fe->ops.tuner_ops.release)
> >> + * dvb_frontend_detach(fe)
> >> + * dvb_usbv2_adapter_frontend_exit(adap)
> >> + * dvb_usbv2_adapter_exit(d)
> >> + * dvb_usbv2_exit(d)
> >> + * dvb_usbv2_disconnect(intf)
> >> + *
> >> + * Since the tuner driver of Friio (dvb-pll) has .release defined,
> >> + * dvb_module_release() should be skipped if CONFIG_MEDIA_ATTACH,
> >> + * to avoid double-putting the module.
> >> + * Even without dvb_module_release(),
> >> + * the tuner i2c device is automatically unregistered
> >> + * when its i2c adapter is unregistered with the demod i2c device.
> >> + *
> >> + * The same "double-putting" can happen to the demod module as well,
> >> + * but tc90522 does not define any _invoke_release()'ed functions,
> >> + * thus Friio is not affected.
> >> + */
> >> + dvb_module_release(priv->i2c_client_tuner);
> >> +#endif
> >
> > This looks really weird to me... why only at this driver we need this?
> > If we have some issues at the DVB core - or at dvb-usb-v2, the fix should
> > be there, and not here.
>
> dvb-usb-v2 handles attach/detach of frontend/tuner driver modules
> on behalf of each bridge driver.
>
> While sub drivers are attached via d->props->{frontend, tuner}_attach,
> they are implicitly 'dvb_detach'ed in dvb_frontend_detach() on disconnect,
> not detached explicitly in d->props->{frontend, tuner}_detach().
> But i2c drivers need to be 'dvb_module_release'ed in those *_detach(),
> so any i2c drivers that define any 'implicitly detached' ops function
> would be affected by this double-put problem.
> (I suspect that even legacy dvb_attached drivers are also affected
> if they define both ops{.detach, .release, ...} functions,
> but there seems to be no such drivers currently.)
>
> Fix in dvb-core/dvb-usb-v2 would be too complicated
> for this (transient?) problem, since it will get unnecessary
> once all dvb_attach's are replaced and removed.
>
> so I am going to fix it in the tuner driver (dvb_pll) instead,
> like af9013 or ts2020 drivers,
> by clearing tuner_ops.release and moving its code to i2c .remove().
Ok.
>
> regards,
> Akihiro
Thanks,
Mauro
