Re: [REVIEW PATCH 11/41] af9035: basic support for IT9135 v2 chips
On 03/22/2013 11:30 AM, Mauro Carvalho Chehab wrote:
Em Fri, 22 Mar 2013 01:45:30 +0200
Antti Palosaari escreveu:
On 03/21/2013 11:54 PM, Mauro Carvalho Chehab wrote:
Em Sun, 10 Mar 2013 04:03:03 +0200
Antti Palosaari escreveu:
static struct ite_config af9035_it913x_config = {
- .chip_ver = 0x01,
+ .chip_ver = 0x02,
@@ -1153,6 +1161,7 @@ static int af9035_tuner_attach(struct dvb_usb_adapter
*adap)
case AF9033_TUNER_IT9135_38:
case AF9033_TUNER_IT9135_51:
case AF9033_TUNER_IT9135_52:
+ af9035_it913x_config.chip_ver = 0x01;
Hmmm... aren't you missing a break here? If not, please add a comment, as
otherwise reviewers think that this is a bug.
It is correct as it was set 0x02 by init. And variable was removed
totally few patches later.
Ok, so please send a patch latter adding a notice about that, like:
case AF9033_TUNER_IT9135_52:
af9035_it913x_config.chip_ver = 0x01;
/* fall trough */
case ...
This is a very common practice at the Kernel, as it helps to better
document it.
Also I'm pretty sure some janitor would otherwise send us sooner or later a
patch adding a break there.
I totally agree the issue, but it is totally irrelevant currently as the
whole piece of code does not exists anymore.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 11/41] af9035: basic support for IT9135 v2 chips
Em Fri, 22 Mar 2013 01:45:30 +0200
Antti Palosaari escreveu:
> On 03/21/2013 11:54 PM, Mauro Carvalho Chehab wrote:
> > Em Sun, 10 Mar 2013 04:03:03 +0200
> > Antti Palosaari escreveu:
> >> static struct ite_config af9035_it913x_config = {
> >> - .chip_ver = 0x01,
> >> + .chip_ver = 0x02,
>
> >> @@ -1153,6 +1161,7 @@ static int af9035_tuner_attach(struct
> >> dvb_usb_adapter *adap)
> >>case AF9033_TUNER_IT9135_38:
> >>case AF9033_TUNER_IT9135_51:
> >>case AF9033_TUNER_IT9135_52:
> >> + af9035_it913x_config.chip_ver = 0x01;
> >
> > Hmmm... aren't you missing a break here? If not, please add a comment, as
> > otherwise reviewers think that this is a bug.
>
> It is correct as it was set 0x02 by init. And variable was removed
> totally few patches later.
Ok, so please send a patch latter adding a notice about that, like:
case AF9033_TUNER_IT9135_52:
af9035_it913x_config.chip_ver = 0x01;
/* fall trough */
case ...
This is a very common practice at the Kernel, as it helps to better
document it.
Also I'm pretty sure some janitor would otherwise send us sooner or later a
patch adding a break there.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 11/41] af9035: basic support for IT9135 v2 chips
On 03/21/2013 11:54 PM, Mauro Carvalho Chehab wrote:
Em Sun, 10 Mar 2013 04:03:03 +0200
Antti Palosaari escreveu:
static struct ite_config af9035_it913x_config = {
- .chip_ver = 0x01,
+ .chip_ver = 0x02,
@@ -1153,6 +1161,7 @@ static int af9035_tuner_attach(struct dvb_usb_adapter
*adap)
case AF9033_TUNER_IT9135_38:
case AF9033_TUNER_IT9135_51:
case AF9033_TUNER_IT9135_52:
+ af9035_it913x_config.chip_ver = 0x01;
Hmmm... aren't you missing a break here? If not, please add a comment, as
otherwise reviewers think that this is a bug.
It is correct as it was set 0x02 by init. And variable was removed
totally few patches later.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW PATCH 11/41] af9035: basic support for IT9135 v2 chips
Em Sun, 10 Mar 2013 04:03:03 +0200
Antti Palosaari escreveu:
> Signed-off-by: Antti Palosaari
> ---
> drivers/media/usb/dvb-usb-v2/af9035.c | 44
> ---
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c
> b/drivers/media/usb/dvb-usb-v2/af9035.c
> index a1e953a..0b92277 100644
> --- a/drivers/media/usb/dvb-usb-v2/af9035.c
> +++ b/drivers/media/usb/dvb-usb-v2/af9035.c
> @@ -316,7 +316,7 @@ static int af9035_identify_state(struct dvb_usb_device
> *d, const char **name)
> state->chip_type);
>
> if (state->chip_type == 0x9135) {
> - if (state->chip_version == 2)
> + if (state->chip_version == 0x02)
> *name = AF9035_FIRMWARE_IT9135_V2;
> else
> *name = AF9035_FIRMWARE_IT9135_V1;
> @@ -595,18 +595,23 @@ static int af9035_read_config(struct dvb_usb_device *d)
>
> /* eeprom memory mapped location */
> if (state->chip_type == 0x9135) {
> + if (state->chip_version == 0x02) {
> + state->af9033_config[0].tuner = AF9033_TUNER_IT9135_60;
> + tmp16 = 0x00461d;
> + } else {
> + state->af9033_config[0].tuner = AF9033_TUNER_IT9135_38;
> + tmp16 = 0x00461b;
> + }
> +
> /* check if eeprom exists */
> - if (state->chip_version == 2)
> - ret = af9035_rd_reg(d, 0x00461d, &tmp);
> - else
> - ret = af9035_rd_reg(d, 0x00461b, &tmp);
> + ret = af9035_rd_reg(d, tmp16, &tmp);
> if (ret < 0)
> goto err;
>
> if (tmp) {
> addr = EEPROM_BASE_IT9135;
> } else {
> - state->af9033_config[0].tuner = AF9033_TUNER_IT9135_38;
> + dev_dbg(&d->udev->dev, "%s: no eeprom\n", __func__);
> goto skip_eeprom;
> }
> } else {
> @@ -639,12 +644,15 @@ static int af9035_read_config(struct dvb_usb_device *d)
> if (ret < 0)
> goto err;
>
> - state->af9033_config[i].tuner = tmp;
> - dev_dbg(&d->udev->dev, "%s: [%d]tuner=%02x\n",
> - __func__, i, tmp);
> + if (tmp == 0x00)
> + dev_dbg(&d->udev->dev,
> + "%s: [%d]tuner not set, using
> default\n",
> + __func__, i);
> + else
> + state->af9033_config[i].tuner = tmp;
>
> - if (state->chip_type == 0x9135 && tmp == 0x00)
> - state->af9033_config[i].tuner = AF9033_TUNER_IT9135_38;
> + dev_dbg(&d->udev->dev, "%s: [%d]tuner=%02x\n",
> + __func__, i, state->af9033_config[i].tuner);
>
> switch (state->af9033_config[i].tuner) {
> case AF9033_TUNER_TUA9001:
> @@ -975,12 +983,12 @@ static const struct fc0012_config
> af9035_fc0012_config[] = {
> };
>
> static struct ite_config af9035_it913x_config = {
> - .chip_ver = 0x01,
> + .chip_ver = 0x02,
> .chip_type = 0x9135,
> .firmware = 0x,
> .firmware_ver = 1,
> .adc_x2 = 1,
> - .tuner_id_0 = AF9033_TUNER_IT9135_38,
> + .tuner_id_0 = 0x00,
> .tuner_id_1 = 0x00,
> .dual_mode = 0x00,
> .adf = 0x00,
> @@ -1153,6 +1161,7 @@ static int af9035_tuner_attach(struct dvb_usb_adapter
> *adap)
> case AF9033_TUNER_IT9135_38:
> case AF9033_TUNER_IT9135_51:
> case AF9033_TUNER_IT9135_52:
> + af9035_it913x_config.chip_ver = 0x01;
Hmmm... aren't you missing a break here? If not, please add a comment, as
otherwise reviewers think that this is a bug.
> case AF9033_TUNER_IT9135_60:
> case AF9033_TUNER_IT9135_61:
> case AF9033_TUNER_IT9135_62:
> @@ -1453,6 +1462,7 @@ static const struct dvb_usb_device_properties
> af9035_props = {
> };
>
> static const struct usb_device_id af9035_id_table[] = {
> + /* AF9035 devices */
> { DVB_USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9035_9035,
> &af9035_props, "Afatech AF9035 reference design", NULL) },
> { DVB_USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9035_1000,
> @@ -1477,6 +1487,14 @@ static const struct usb_device_id af9035_id_table[] = {
> &af9035_props, "AVerMedia Twinstar (A825)", NULL) },
> { DVB_USB_DEVICE(USB_VID_ASUS, USB_PID_ASUS_U3100MINI_PLUS,
> &af9035_props, "Asus U3100Mini Plus", NULL) },
> +
> + /* IT9135 devices */
> +#if 0
> + { DVB_USB_DEVICE(0x048d, 0x9135,
> + &af9035_props, "IT9135 reference design", NULL) },
> + { DVB_USB_DEVICE(0x048d, 0x9006,
> + &af9035_props, "IT9135 reference design", NULL) },
>
