From: Antti Palosaari <crope@iki.fi>
To: oliver@schinagl.nl
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH] Support for Asus MyCinema U3100Mini Plus
Date: Thu, 20 Sep 2012 00:36:28 +0300 [thread overview]
Message-ID: <505A3ADC.4000709@iki.fi> (raw)
In-Reply-To: <1348080243-3818-1-git-send-email-oliver+list@schinagl.nl>
This is review, please fix those minor findings mentioned and resend
fixed patch!
On 09/19/2012 09:44 PM, oliver@schinagl.nl wrote:
> From: Oliver Schinagl <oliver@schinagl.nl>
>
> This is initial support for the Asus MyCinema U3100Mini Plus. The driver
> in its current form gets detected and loads properly.
>
> Scanning using dvbscan works without problems, Locking onto a channel
> using tzap also works fine. Only playback using tzap -r + mplayer was
> tested and was fully functional.
>
> It uses the af9035 USB Bridge chip, with an af9033 demodulator. The tuner
> used is the FCI FC2580.
>
> Signed-off-by: Oliver Schinagl <oliver@schinagl.nl>
> ---
> drivers/media/dvb-core/dvb-usb-ids.h | 1 +
> drivers/media/dvb-frontends/af9033.c | 4 ++++
> drivers/media/dvb-frontends/af9033.h | 1 +
> drivers/media/dvb-frontends/af9033_priv.h | 38 +++++++++++++++++++++++++++++++
> drivers/media/tuners/fc2580.c | 3 ++-
> drivers/media/usb/dvb-usb-v2/Kconfig | 1 +
> drivers/media/usb/dvb-usb-v2/af9035.c | 27 ++++++++++++++++++++++
> drivers/media/usb/dvb-usb-v2/af9035.h | 1 +
> 8 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb-core/dvb-usb-ids.h b/drivers/media/dvb-core/dvb-usb-ids.h
> index d572307..58e0220 100644
> --- a/drivers/media/dvb-core/dvb-usb-ids.h
> +++ b/drivers/media/dvb-core/dvb-usb-ids.h
> @@ -329,6 +329,7 @@
> #define USB_PID_ASUS_U3000 0x171f
> #define USB_PID_ASUS_U3000H 0x1736
> #define USB_PID_ASUS_U3100 0x173f
> +#define USB_PID_ASUS_U3100MINI_PLUS 0x1779
> #define USB_PID_YUAN_EC372S 0x1edc
> #define USB_PID_YUAN_STK7700PH 0x1f08
> #define USB_PID_YUAN_PD378S 0x2edc
> diff --git a/drivers/media/dvb-frontends/af9033.c b/drivers/media/dvb-frontends/af9033.c
> index 0979ada..b40f5a0 100644
> --- a/drivers/media/dvb-frontends/af9033.c
> +++ b/drivers/media/dvb-frontends/af9033.c
> @@ -314,6 +314,10 @@ static int af9033_init(struct dvb_frontend *fe)
> len = ARRAY_SIZE(tuner_init_tda18218);
> init = tuner_init_tda18218;
> break;
> + case AF9033_TUNER_FC2580:
> + len = ARRAY_SIZE(tuner_init_fc2580);
> + init = tuner_init_fc2580;
> + break;
> default:
> dev_dbg(&state->i2c->dev, "%s: unsupported tuner ID=%d\n",
> __func__, state->cfg.tuner);
> diff --git a/drivers/media/dvb-frontends/af9033.h b/drivers/media/dvb-frontends/af9033.h
> index 288622b..739137e 100644
> --- a/drivers/media/dvb-frontends/af9033.h
> +++ b/drivers/media/dvb-frontends/af9033.h
> @@ -42,6 +42,7 @@ struct af9033_config {
> #define AF9033_TUNER_FC0011 0x28 /* Fitipower FC0011 */
> #define AF9033_TUNER_MXL5007T 0xa0 /* MaxLinear MxL5007T */
> #define AF9033_TUNER_TDA18218 0xa1 /* NXP TDA 18218HN */
> +#define AF9033_TUNER_FC2580 0x32 /* FIC FC2580 */
typo in name
> u8 tuner;
>
> /*
> diff --git a/drivers/media/dvb-frontends/af9033_priv.h b/drivers/media/dvb-frontends/af9033_priv.h
> index 0b783b9..d2c9ae6 100644
> --- a/drivers/media/dvb-frontends/af9033_priv.h
> +++ b/drivers/media/dvb-frontends/af9033_priv.h
> @@ -466,5 +466,43 @@ static const struct reg_val tuner_init_tda18218[] = {
> {0x80f1e6, 0x00},
> };
>
> +/* FIC FC2580 tuner init
> + AF9033_TUNER_FC2580 = 0x32 */
> +static const struct reg_val tuner_init_fc2580[] = {
> + { 0x800046, AF9033_TUNER_FC2580 },
Personally I don't like that define here. It causes some extra work last
time I looked USB sniffs as *just* that field is the field you are first
looking for. I am going to change it anyway, feel free to leave if you
wish, it is not error, thus I am not going to say you must change it.
> + { 0x800057, 0x01 },
> + { 0x800058, 0x00 },
> + { 0x80005f, 0x00 },
> + { 0x800060, 0x00 },
> + { 0x800071, 0x05 },
> + { 0x800072, 0x02 },
> + { 0x800074, 0x01 },
> + { 0x800079, 0x01 },
> + { 0x800093, 0x00 },
> + { 0x800094, 0x00 },
> + { 0x800095, 0x00 },
> + { 0x800096, 0x05 },
> + { 0x8000b3, 0x01 },
> + { 0x8000c3, 0x01 },
> + { 0x8000c4, 0x00 },
> + { 0x80f007, 0x00 },
> + { 0x80f00c, 0x19 },
> + { 0x80f00d, 0x1A },
> + { 0x80f00e, 0x00 },
> + { 0x80f00f, 0x02 },
> + { 0x80f010, 0x00 },
> + { 0x80f011, 0x02 },
> + { 0x80f012, 0x00 },
> + { 0x80f013, 0x02 },
> + { 0x80f014, 0x00 },
> + { 0x80f015, 0x02 },
> + { 0x80f01f, 0x96 },
> + { 0x80f020, 0x00 },
> + { 0x80f029, 0x96 },
> + { 0x80f02a, 0x00 },
> + { 0x80f077, 0x01 },
> + { 0x80f1e6, 0x01 },
> +};
> +
> #endif /* AF9033_PRIV_H */
>
> diff --git a/drivers/media/tuners/fc2580.c b/drivers/media/tuners/fc2580.c
> index afc0491..51bc39c 100644
> --- a/drivers/media/tuners/fc2580.c
> +++ b/drivers/media/tuners/fc2580.c
> @@ -498,8 +498,9 @@ struct dvb_frontend *fc2580_attach(struct dvb_frontend *fe,
>
> dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
>
> - if (chip_id != 0x56)
> + if ((chip_id != 0x56) && (chip_id != 0x5a)) {
> goto err;
> + }
hmm, braces added without a reason. That is Kernel CodingStyle
violation. You didn't ran checkpatch.pl ?
>
> dev_info(&priv->i2c->dev,
> "%s: FCI FC2580 successfully identified\n",
> diff --git a/drivers/media/usb/dvb-usb-v2/Kconfig b/drivers/media/usb/dvb-usb-v2/Kconfig
> index e09930c..834bfec 100644
> --- a/drivers/media/usb/dvb-usb-v2/Kconfig
> +++ b/drivers/media/usb/dvb-usb-v2/Kconfig
> @@ -40,6 +40,7 @@ config DVB_USB_AF9035
> select MEDIA_TUNER_FC0011 if MEDIA_SUBDRV_AUTOSELECT
> select MEDIA_TUNER_MXL5007T if MEDIA_SUBDRV_AUTOSELECT
> select MEDIA_TUNER_TDA18218 if MEDIA_SUBDRV_AUTOSELECT
> + select MEDIA_TUNER_FC2580 if MEDIA_SUBDRV_AUTOSELECT
> help
> Say Y here to support the Afatech AF9035 based DVB USB receiver.
>
> diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c
> index 89cc901..8ca13e4 100644
> --- a/drivers/media/usb/dvb-usb-v2/af9035.c
> +++ b/drivers/media/usb/dvb-usb-v2/af9035.c
> @@ -513,6 +513,7 @@ static int af9035_read_config(struct dvb_usb_device *d)
> case AF9033_TUNER_FC0011:
> case AF9033_TUNER_MXL5007T:
> case AF9033_TUNER_TDA18218:
> + case AF9033_TUNER_FC2580:
> state->af9033_config[i].spec_inv = 1;
> break;
> default:
> @@ -750,6 +751,11 @@ static struct tda18218_config af9035_tda18218_config = {
> .i2c_wr_max = 21,
> };
>
> +static struct fc2580_config af9035_fc2580_config = {
> + .i2c_addr = 0x56,
> + .clock = 16384000,
> +};
I usually define config structures as a const. Is there some reason it
is not static? Compiler should also print warning as it is likely
defined as const in fc2580. ?
> +
> static int af9035_tuner_attach(struct dvb_usb_adapter *adap)
> {
> struct state *state = adap_to_priv(adap);
> @@ -851,6 +857,25 @@ static int af9035_tuner_attach(struct dvb_usb_adapter *adap)
> fe = dvb_attach(tda18218_attach, adap->fe[0],
> &d->i2c_adap, &af9035_tda18218_config);
> break;
> + case AF9033_TUNER_FC2580:
> + /* Tuner enable using gpiot2_o, gpiot2_en and gpiot2_on */
> + ret = af9035_wr_reg(d, 0xd8eb, 1);
> + if (ret < 0)
> + goto err;
> +
> + ret = af9035_wr_reg(d, 0xd8ec, 1);
> + if (ret < 0)
> + goto err;
> +
> + ret = af9035_wr_reg(d, 0xd8ed, 1);
> + if (ret < 0)
> + goto err;
> +
> + usleep_range(10000, 50000);
> + /* attach tuner */
> + fe = dvb_attach(fc2580_attach, adap->fe[0],
> + &d->i2c_adap, &af9035_fc2580_config);
> + break;
These GPIO config registers are bitfields. You should use masked write
to change only the bit needed leaving undefined / unused bits to default.
> default:
> fe = NULL;
> }
> @@ -1075,6 +1100,8 @@ static const struct usb_device_id af9035_id_table[] = {
> &af9035_props, "AVerMedia HD Volar (A867)", NULL) },
> { DVB_USB_DEVICE(USB_VID_AVERMEDIA, USB_PID_AVERMEDIA_TWINSTAR,
> &af9035_props, "AVerMedia Twinstar (A825)", NULL) },
> + { DVB_USB_DEVICE(USB_VID_ASUS, USB_PID_ASUS_U3100MINI_PLUS,
> + &af9035_props, "Asus U3100Mini Plus", NULL) },
> { }
> };
> MODULE_DEVICE_TABLE(usb, af9035_id_table);
> diff --git a/drivers/media/usb/dvb-usb-v2/af9035.h b/drivers/media/usb/dvb-usb-v2/af9035.h
> index de8e761..75ef1ec 100644
> --- a/drivers/media/usb/dvb-usb-v2/af9035.h
> +++ b/drivers/media/usb/dvb-usb-v2/af9035.h
> @@ -28,6 +28,7 @@
> #include "fc0011.h"
> #include "mxl5007t.h"
> #include "tda18218.h"
> +#include "fc2580.h"
>
> struct reg_val {
> u32 reg;
>
And ran the checkpatch.pl. I suspect it was not ran as there was at
least that braces violation which is reported by checkpatch what I remember.
regards
Antti
--
http://palosaari.fi/
next prev parent reply other threads:[~2012-09-19 21:36 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-19 18:44 [PATCH] Support for Asus MyCinema U3100Mini Plus oliver
2012-09-19 20:52 ` Devin Heitmueller
2012-09-19 22:47 ` Oliver Schinagl
2012-09-20 18:54 ` Oliver Schinagl
2012-09-20 19:21 ` Devin Heitmueller
2012-09-19 21:36 ` Antti Palosaari [this message]
2012-09-20 18:54 ` Oliver Schinagl
-- strict thread matches above, loose matches on Subject: below --
2012-09-20 18:57 oliver
2012-09-20 19:15 ` Antti Palosaari
2012-09-20 19:28 ` Oliver Schinagl
2012-09-20 19:37 ` Antti Palosaari
2012-09-09 20:47 oliver
2012-09-09 20:49 ` Oliver Schinagl
2012-09-09 21:51 ` Antti Palosaari
2012-09-09 22:26 ` Oliver Schinagl
2012-09-09 22:29 ` Antti Palosaari
2012-09-10 9:58 ` Oliver Schinagl
2012-09-10 11:46 ` Antti Palosaari
2012-09-10 14:29 ` Oliver Schinagl
2012-09-10 17:28 ` Oliver Schinagl
2012-09-16 14:07 ` Oliver Schinagl
2012-09-16 16:43 ` Antti Palosaari
2012-09-16 15:03 ` Oliver Schinagl
2012-09-16 17:25 ` Antti Palosaari
2012-09-16 22:10 ` Oliver Schinagl
2012-09-16 23:36 ` Antti Palosaari
2012-09-17 8:25 ` Oliver Schinagl
2012-09-17 13:02 ` Oliver Schinagl
2012-09-17 13:16 ` Antti Palosaari
2012-09-17 13:26 ` Oliver Schinagl
2012-09-17 13:52 ` Antti Palosaari
2012-09-17 15:20 ` Oliver Schinagl
2012-09-17 20:43 ` Oliver Schinagl
2012-09-17 21:07 ` Antti Palosaari
2012-09-17 21:57 ` Oliver Schinagl
2012-09-18 17:18 ` Oliver Schinagl
2012-09-18 22:51 ` Antti Palosaari
2012-09-19 10:41 ` Oliver Schinagl
2012-09-19 10:53 ` Antti Palosaari
2012-09-17 20:43 ` Oliver Schinagl
2012-09-18 22:59 ` Antti Palosaari
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=505A3ADC.4000709@iki.fi \
--to=crope@iki.fi \
--cc=linux-media@vger.kernel.org \
--cc=oliver@schinagl.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).