Em Mon, 14 May 2018 03:13:44 +0900 Akihiro TSUKADA <tsk...@gmail.com> 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-68279366b...@gmail.com> > 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