Re: [PATCH v6 3/5] dvb-usb/friio, dvb-usb-v2/gl861: decompose friio and merge with gl861

[Mauro Carvalho Chehab]

Em Mon, 14 May 2018 03:13:44 +0900
Akihiro TSUKADA  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: 
> 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(>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(>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] = {
> >> +  

Re: [PATCH v6 3/5] dvb-usb/friio, dvb-usb-v2/gl861: decompose friio and merge with gl861

[Akihiro TSUKADA]

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.

>> +/*
>> + * 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: 
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.

>> +/* 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.

> 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).

>> +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 

Re: [PATCH v6 3/5] dvb-usb/friio, dvb-usb-v2/gl861: decompose friio and merge with gl861

[Mauro Carvalho Chehab]

Em Mon,  9 Apr 2018 02:21:36 +0900
tsk...@gmail.com escreveu:

> From: Akihiro Tsukada 
> 
> Friio device contains "gl861" bridge and "tc90522" demod,
> for which the separate drivers are already in the kernel.
> But friio driver was monolithic and did not use them,
> practically copying those features.
> This patch decomposes friio driver into sub drivers and
> re-uses existing ones, thus reduces some code.
> 
> It adds some features to gl861,
> to support the friio-specific init/config of the devices
> and implement i2c communications to the tuner via demod
> with USB vendor requests.

Hi Akihiro-san,

Patches 1 and 2 looked OK on my eyes. I'll be applying them.

I have comments to this one. See below. I won't apply the
remaining patches on this series, as they likely depend on it.

> 
> Signed-off-by: Akihiro Tsukada 
> ---
> Changes since v5:
> - specify chip name literally
> - i2c algo of gl861 is not (yet?) changed as proposed by Antti,
>   (which is to move the special case handling to demod driver),
>   since I do not yet understand
>   whether it should/can be really moved or not.
> 
> Changes since v4:
> - use new style of specifying pll_desc id of the tuner driver
> 
> Changes since v3:
> - make dvb_usb_device_properties static
> 
> Changes since v2:
> (patch #27928, dvb-usb-friio: split and merge into dvb-usbv2-gl861)
>  - used the new i2c binding helpers instead of my own one
>  - merged gl861-friio.c with gl861.c
> 
>  drivers/media/usb/dvb-usb-v2/Kconfig |   5 +-
>  drivers/media/usb/dvb-usb-v2/gl861.c | 465 +++-
>  drivers/media/usb/dvb-usb-v2/gl861.h |   1 +
>  drivers/media/usb/dvb-usb/Kconfig|   6 -
>  drivers/media/usb/dvb-usb/Makefile   |   3 -
>  drivers/media/usb/dvb-usb/friio-fe.c | 441 --
>  drivers/media/usb/dvb-usb/friio.c| 522 ---
>  drivers/media/usb/dvb-usb/friio.h|  99 -
>  8 files changed, 462 insertions(+), 1080 deletions(-)
>  delete mode 100644 drivers/media/usb/dvb-usb/friio-fe.c
>  delete mode 100644 drivers/media/usb/dvb-usb/friio.c
>  delete mode 100644 drivers/media/usb/dvb-usb/friio.h
> 
> diff --git a/drivers/media/usb/dvb-usb-v2/Kconfig 
> b/drivers/media/usb/dvb-usb-v2/Kconfig
> index 0e4944b2b0f..e0a1f377295 100644
> --- a/drivers/media/usb/dvb-usb-v2/Kconfig
> +++ b/drivers/media/usb/dvb-usb-v2/Kconfig
> @@ -95,10 +95,13 @@ config DVB_USB_GL861
>   tristate "Genesys Logic GL861 USB2.0 support"
>   depends on DVB_USB_V2
>   select DVB_ZL10353 if MEDIA_SUBDRV_AUTOSELECT
> + select DVB_TC90522 if MEDIA_SUBDRV_AUTOSELECT
>   select MEDIA_TUNER_QT1010 if MEDIA_SUBDRV_AUTOSELECT
> + select DVB_PLL if MEDIA_SUBDRV_AUTOSELECT
>   help
> Say Y here to support the MSI Megasky 580 (55801) DVB-T USB2.0
> -   receiver with USB ID 0db0:5581.
> +   receiver with USB ID 0db0:5581, Friio White ISDB-T receiver
> +   with USB ID 0x7a69:0001.
>  
>  config DVB_USB_LME2510
>   tristate "LME DM04/QQBOX DVB-S USB2.0 support"
> diff --git a/drivers/media/usb/dvb-usb-v2/gl861.c 
> b/drivers/media/usb/dvb-usb-v2/gl861.c
> index b1b09c54786..ecff0062bfb 100644
> --- a/drivers/media/usb/dvb-usb-v2/gl861.c
> +++ b/drivers/media/usb/dvb-usb-v2/gl861.c
> @@ -10,6 +10,8 @@
>  
>  #include "zl10353.h"
>  #include "qt1010.h"
> +#include "tc90522.h"
> +#include "dvb-pll.h"
>  
>  DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>  
> @@ -49,6 +51,80 @@ static int gl861_i2c_msg(struct dvb_usb_device *d, u8 addr,
>  value, index, rbuf, rlen, 2000);
>  }
>  
> +/* Friio specific I2C read/write */
> +/* special USB request is used in Friio's init/config */
> +static int
> +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.

> + memcpy(buf, wbuf, wlen);
> + ret = usb_control_msg(d->udev, usb_sndctrlpipe(d->udev, 0),
> +  GL861_REQ_I2C_RAW, GL861_WRITE,
> +  addr << (8 + 1), 0x0100, buf, wlen, 2000);
> + kfree(buf);
> + return ret;
> +}
> +
> +/*
> + * 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.