[PATCH v2 1/3] [media] si2157: Add support for Si2141-A10

[PATCH v2 1/3] [media] si2157: Add support for Si2141-A10

[Stefan Brüns]

The Si2141 needs two distinct commands for powerup/reset, otherwise it
will not respond to chip revision requests. It also needs a firmware
to run properly.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/media/tuners/si2157.c      | 23 +++++++++++++++++++++--
 drivers/media/tuners/si2157_priv.h |  2 ++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 57b250847cd3..e35b1faf0ddc 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -106,6 +106,9 @@ static int si2157_init(struct dvb_frontend *fe)
 	if (dev->chiptype == SI2157_CHIPTYPE_SI2146) {
 		memcpy(cmd.args, "\xc0\x05\x01\x00\x00\x0b\x00\x00\x01", 9);
 		cmd.wlen = 9;
+	} else if (dev->chiptype == SI2157_CHIPTYPE_SI2141) {
+		memcpy(cmd.args, "\xc0\x00\x0d\x0e\x00\x01\x01\x01\x01\x03", 10);
+		cmd.wlen = 10;
 	} else {
 		memcpy(cmd.args, "\xc0\x00\x0c\x00\x00\x01\x01\x01\x01\x01\x01\x02\x00\x00\x01", 15);
 		cmd.wlen = 15;
@@ -115,6 +118,15 @@ static int si2157_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	/* Si2141 needs a second command before it answers the revision query */
+	if (dev->chiptype == SI2157_CHIPTYPE_SI2141) {
+		memcpy(cmd.args, "\xc0\x08\x01\x02\x00\x00\x01", 7);
+		cmd.wlen = 7;
+		ret = si2157_cmd_execute(client, &cmd);
+		if (ret)
+			goto err;
+	}
+
 	/* query chip revision */
 	memcpy(cmd.args, "\x02", 1);
 	cmd.wlen = 1;
@@ -131,12 +143,16 @@ static int si2157_init(struct dvb_frontend *fe)
 	#define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0)
 	#define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0)
 	#define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
+	#define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
 
 	switch (chip_id) {
 	case SI2158_A20:
 	case SI2148_A20:
 		fw_name = SI2158_A20_FIRMWARE;
 		break;
+	case SI2141_A10:
+		fw_name = SI2141_A10_FIRMWARE;
+		break;
 	case SI2157_A30:
 	case SI2147_A30:
 	case SI2146_A10:
@@ -371,7 +387,7 @@ static int si2157_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
 
 static const struct dvb_tuner_ops si2157_ops = {
 	.info = {
-		.name           = "Silicon Labs Si2146/2147/2148/2157/2158",
+		.name           = "Silicon Labs Si2141/Si2146/2147/2148/2157/2158",
 		.frequency_min  = 42000000,
 		.frequency_max  = 870000000,
 	},
@@ -471,6 +487,7 @@ static int si2157_probe(struct i2c_client *client,
 #endif
 
 	dev_info(&client->dev, "Silicon Labs %s successfully attached\n",
+			dev->chiptype == SI2157_CHIPTYPE_SI2141 ?  "Si2141" :
 			dev->chiptype == SI2157_CHIPTYPE_SI2146 ?
 			"Si2146" : "Si2147/2148/2157/2158");
 
@@ -508,6 +525,7 @@ static int si2157_remove(struct i2c_client *client)
 static const struct i2c_device_id si2157_id_table[] = {
 	{"si2157", SI2157_CHIPTYPE_SI2157},
 	{"si2146", SI2157_CHIPTYPE_SI2146},
+	{"si2141", SI2157_CHIPTYPE_SI2141},
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, si2157_id_table);
@@ -524,7 +542,8 @@ static struct i2c_driver si2157_driver = {
 
 module_i2c_driver(si2157_driver);
 
-MODULE_DESCRIPTION("Silicon Labs Si2146/2147/2148/2157/2158 silicon tuner driver");
+MODULE_DESCRIPTION("Silicon Labs Si2141/Si2146/2147/2148/2157/2158 silicon tuner driver");
 MODULE_AUTHOR("Antti Palosaari <crope@iki.fi>");
 MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(SI2158_A20_FIRMWARE);
+MODULE_FIRMWARE(SI2141_A10_FIRMWARE);
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index d6b2c7b44053..e6436f74abaa 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -42,6 +42,7 @@ struct si2157_dev {
 
 #define SI2157_CHIPTYPE_SI2157 0
 #define SI2157_CHIPTYPE_SI2146 1
+#define SI2157_CHIPTYPE_SI2141 2
 
 /* firmware command struct */
 #define SI2157_ARGLEN      30
@@ -52,5 +53,6 @@ struct si2157_cmd {
 };
 
 #define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
+#define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"
 
 #endif
-- 
2.11.0

[PATCH v2 2/3] [media] si2168: add support for Si2168-D60

[Stefan Brüns]

Add handling for new revision, requiring new firmware.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/media/dvb-frontends/si2168.c      | 4 ++++
 drivers/media/dvb-frontends/si2168_priv.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
index 20b4a659e2e4..28f3bbe0af25 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -674,6 +674,9 @@ static int si2168_probe(struct i2c_client *client,
 	case SI2168_CHIP_ID_B40:
 		dev->firmware_name = SI2168_B40_FIRMWARE;
 		break;
+	case SI2168_CHIP_ID_D60:
+		dev->firmware_name = SI2168_D60_FIRMWARE;
+		break;
 	default:
 		dev_dbg(&client->dev, "unknown chip version Si21%d-%c%c%c\n",
 			cmd.args[2], cmd.args[1], cmd.args[3], cmd.args[4]);
@@ -761,3 +764,4 @@ MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(SI2168_A20_FIRMWARE);
 MODULE_FIRMWARE(SI2168_A30_FIRMWARE);
 MODULE_FIRMWARE(SI2168_B40_FIRMWARE);
+MODULE_FIRMWARE(SI2168_D60_FIRMWARE);
diff --git a/drivers/media/dvb-frontends/si2168_priv.h b/drivers/media/dvb-frontends/si2168_priv.h
index 7843ccb448a0..4baa95b7d648 100644
--- a/drivers/media/dvb-frontends/si2168_priv.h
+++ b/drivers/media/dvb-frontends/si2168_priv.h
@@ -25,6 +25,7 @@
 #define SI2168_A20_FIRMWARE "dvb-demod-si2168-a20-01.fw"
 #define SI2168_A30_FIRMWARE "dvb-demod-si2168-a30-01.fw"
 #define SI2168_B40_FIRMWARE "dvb-demod-si2168-b40-01.fw"
+#define SI2168_D60_FIRMWARE "dvb-demod-si2168-d60-01.fw"
 #define SI2168_B40_FIRMWARE_FALLBACK "dvb-demod-si2168-02.fw"
 
 /* state struct */
@@ -37,6 +38,7 @@ struct si2168_dev {
 	#define SI2168_CHIP_ID_A20 ('A' << 24 | 68 << 16 | '2' << 8 | '0' << 0)
 	#define SI2168_CHIP_ID_A30 ('A' << 24 | 68 << 16 | '3' << 8 | '0' << 0)
 	#define SI2168_CHIP_ID_B40 ('B' << 24 | 68 << 16 | '4' << 8 | '0' << 0)
+	#define SI2168_CHIP_ID_D60 ('D' << 24 | 68 << 16 | '6' << 8 | '0' << 0)
 	unsigned int chip_id;
 	unsigned int version;
 	const char *firmware_name;
-- 
2.11.0

[PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Stefan Brüns]

Mygica T230 DVB-T/T2/C USB stick support. It uses the same FX2/Si2168
bridge/demodulator combo as the other devices supported by the driver,
but uses the Si2141 tuner.
Several DVB-T (MPEG2) and DVB-T2 (H.265) channels were tested, as well as
the include remote control.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
v2: add support to the dvbsky driver instead of cxusb, correct RC
model
---
 drivers/media/dvb-core/dvb-usb-ids.h  |  1 +
 drivers/media/usb/dvb-usb-v2/dvbsky.c | 91 +++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/drivers/media/dvb-core/dvb-usb-ids.h b/drivers/media/dvb-core/dvb-usb-ids.h
index a7a4674ccc40..ce4a3d574dd7 100644
--- a/drivers/media/dvb-core/dvb-usb-ids.h
+++ b/drivers/media/dvb-core/dvb-usb-ids.h
@@ -380,6 +380,7 @@
 #define USB_PID_SONY_PLAYTV				0x0003
 #define USB_PID_MYGICA_D689				0xd811
 #define USB_PID_MYGICA_T230				0xc688
+#define USB_PID_MYGICA_T230C				0xc689
 #define USB_PID_ELGATO_EYETV_DIVERSITY			0x0011
 #define USB_PID_ELGATO_EYETV_DTT			0x0021
 #define USB_PID_ELGATO_EYETV_DTT_2			0x003f
diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index 02dbc6c45423..729496e5a52e 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter *adap)
 	return ret;
 }
 
+static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
+{
+	struct dvbsky_state *state = adap_to_priv(adap);
+	struct dvb_usb_device *d = adap_to_d(adap);
+	int ret = 0;
+	struct i2c_adapter *i2c_adapter;
+	struct i2c_client *client_demod, *client_tuner;
+	struct i2c_board_info info;
+	struct si2168_config si2168_config;
+	struct si2157_config si2157_config;
+
+	/* attach demod */
+	memset(&si2168_config, 0, sizeof(si2168_config));
+	si2168_config.i2c_adapter = &i2c_adapter;
+	si2168_config.fe = &adap->fe[0];
+	si2168_config.ts_mode = SI2168_TS_PARALLEL;
+	si2168_config.ts_clock_inv = 1;
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "si2168", I2C_NAME_SIZE);
+	info.addr = 0x64;
+	info.platform_data = &si2168_config;
+
+	request_module(info.type);
+	client_demod = i2c_new_device(&d->i2c_adap, &info);
+	if (client_demod == NULL ||
+			client_demod->dev.driver == NULL)
+		goto fail_demod_device;
+	if (!try_module_get(client_demod->dev.driver->owner))
+		goto fail_demod_module;
+
+	/* attach tuner */
+	memset(&si2157_config, 0, sizeof(si2157_config));
+	si2157_config.fe = adap->fe[0];
+	si2157_config.if_port = 0;
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "si2141", I2C_NAME_SIZE);
+	info.addr = 0x60;
+	info.platform_data = &si2157_config;
+
+	request_module("si2157");
+	client_tuner = i2c_new_device(i2c_adapter, &info);
+	if (client_tuner == NULL ||
+			client_tuner->dev.driver == NULL)
+		goto fail_tuner_device;
+	if (!try_module_get(client_tuner->dev.driver->owner))
+		goto fail_tuner_module;
+
+	state->i2c_client_demod = client_demod;
+	state->i2c_client_tuner = client_tuner;
+	return ret;
+fail_tuner_module:
+	i2c_unregister_device(client_tuner);
+fail_tuner_device:
+	module_put(client_demod->dev.driver->owner);
+fail_demod_module:
+	i2c_unregister_device(client_demod);
+fail_demod_device:
+	ret = -ENODEV;
+	return ret;
+}
+
+
 static int dvbsky_identify_state(struct dvb_usb_device *d, const char **name)
 {
 	dvbsky_gpio_ctrl(d, 0x04, 1);
@@ -830,6 +892,32 @@ static struct dvb_usb_device_properties dvbsky_t330_props = {
 	}
 };
 
+static struct dvb_usb_device_properties mygica_t230c_props = {
+	.driver_name = KBUILD_MODNAME,
+	.owner = THIS_MODULE,
+	.adapter_nr = adapter_nr,
+	.size_of_priv = sizeof(struct dvbsky_state),
+
+	.generic_bulk_ctrl_endpoint = 0x01,
+	.generic_bulk_ctrl_endpoint_response = 0x81,
+	.generic_bulk_ctrl_delay = DVBSKY_MSG_DELAY,
+
+	.i2c_algo         = &dvbsky_i2c_algo,
+	.frontend_attach  = dvbsky_mygica_t230c_attach,
+	.init             = dvbsky_init,
+	.get_rc_config    = dvbsky_get_rc_config,
+	.streaming_ctrl   = dvbsky_streaming_ctrl,
+	.identify_state	  = dvbsky_identify_state,
+	.exit             = dvbsky_exit,
+
+	.num_adapters = 1,
+	.adapter = {
+		{
+			.stream = DVB_USB_STREAM_BULK(0x82, 8, 4096),
+		}
+	}
+};
+
 static const struct usb_device_id dvbsky_id_table[] = {
 	{ DVB_USB_DEVICE(0x0572, 0x6831,
 		&dvbsky_s960_props, "DVBSky S960/S860", RC_MAP_DVBSKY) },
@@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] = {
 	{ DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
 		&dvbsky_s960_props, "Terratec Cinergy S2 Rev.4",
 		RC_MAP_DVBSKY) },
+	{ DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
+		&mygica_t230c_props, "Mygica T230C DVB-T/T2/C",
+		RC_MAP_TOTAL_MEDIA_IN_HAND_02) },
 	{ }
 };
 MODULE_DEVICE_TABLE(usb, dvbsky_id_table);
-- 
2.11.0

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Antti Palosaari]

On 02/15/2017 03:51 AM, Stefan Brüns wrote:
> Mygica T230 DVB-T/T2/C USB stick support. It uses the same FX2/Si2168
> bridge/demodulator combo as the other devices supported by the driver,
> but uses the Si2141 tuner.
> Several DVB-T (MPEG2) and DVB-T2 (H.265) channels were tested, as well as
> the include remote control.
>
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
> v2: add support to the dvbsky driver instead of cxusb, correct RC
> model
> ---
>  drivers/media/dvb-core/dvb-usb-ids.h  |  1 +
>  drivers/media/usb/dvb-usb-v2/dvbsky.c | 91 +++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>
> diff --git a/drivers/media/dvb-core/dvb-usb-ids.h b/drivers/media/dvb-core/dvb-usb-ids.h
> index a7a4674ccc40..ce4a3d574dd7 100644
> --- a/drivers/media/dvb-core/dvb-usb-ids.h
> +++ b/drivers/media/dvb-core/dvb-usb-ids.h
> @@ -380,6 +380,7 @@
>  #define USB_PID_SONY_PLAYTV				0x0003
>  #define USB_PID_MYGICA_D689				0xd811
>  #define USB_PID_MYGICA_T230				0xc688
> +#define USB_PID_MYGICA_T230C				0xc689
>  #define USB_PID_ELGATO_EYETV_DIVERSITY			0x0011
>  #define USB_PID_ELGATO_EYETV_DTT			0x0021
>  #define USB_PID_ELGATO_EYETV_DTT_2			0x003f
> diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> index 02dbc6c45423..729496e5a52e 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> @@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter *adap)
>  	return ret;
>  }
>
> +static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
> +{
> +	struct dvbsky_state *state = adap_to_priv(adap);
> +	struct dvb_usb_device *d = adap_to_d(adap);
> +	int ret = 0;

you could return ret completely as you don't assign its value runtime

> +	struct i2c_adapter *i2c_adapter;
> +	struct i2c_client *client_demod, *client_tuner;
> +	struct i2c_board_info info;
> +	struct si2168_config si2168_config;
> +	struct si2157_config si2157_config;
> +
> +	/* attach demod */
> +	memset(&si2168_config, 0, sizeof(si2168_config));

prefer sizeof dst

> +	si2168_config.i2c_adapter = &i2c_adapter;
> +	si2168_config.fe = &adap->fe[0];
> +	si2168_config.ts_mode = SI2168_TS_PARALLEL;
> +	si2168_config.ts_clock_inv = 1;
it has boolean type

> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "si2168", I2C_NAME_SIZE);
I would prefer sizeof dst here too.

> +	info.addr = 0x64;
> +	info.platform_data = &si2168_config;
> +
> +	request_module(info.type);
Use module name here. Even it is same than device id on that case, it is 
not always the case.

> +	client_demod = i2c_new_device(&d->i2c_adap, &info);
> +	if (client_demod == NULL ||
> +			client_demod->dev.driver == NULL)

You did not ran checkpatch.pl for that patch? or doesn't it complain 
anymore about these?

> +		goto fail_demod_device;
> +	if (!try_module_get(client_demod->dev.driver->owner))
> +		goto fail_demod_module;
> +
> +	/* attach tuner */
> +	memset(&si2157_config, 0, sizeof(si2157_config));
> +	si2157_config.fe = adap->fe[0];
> +	si2157_config.if_port = 0;
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "si2141", I2C_NAME_SIZE);
> +	info.addr = 0x60;
> +	info.platform_data = &si2157_config;
> +
> +	request_module("si2157");
> +	client_tuner = i2c_new_device(i2c_adapter, &info);
> +	if (client_tuner == NULL ||
> +			client_tuner->dev.driver == NULL)
> +		goto fail_tuner_device;
> +	if (!try_module_get(client_tuner->dev.driver->owner))
> +		goto fail_tuner_module;
> +
> +	state->i2c_client_demod = client_demod;
> +	state->i2c_client_tuner = client_tuner;
> +	return ret;
> +fail_tuner_module:
> +	i2c_unregister_device(client_tuner);
> +fail_tuner_device:
> +	module_put(client_demod->dev.driver->owner);
> +fail_demod_module:
> +	i2c_unregister_device(client_demod);
> +fail_demod_device:
> +	ret = -ENODEV;
> +	return ret;
> +}
> +
> +
>  static int dvbsky_identify_state(struct dvb_usb_device *d, const char **name)
>  {
>  	dvbsky_gpio_ctrl(d, 0x04, 1);
> @@ -830,6 +892,32 @@ static struct dvb_usb_device_properties dvbsky_t330_props = {
>  	}
>  };
>
> +static struct dvb_usb_device_properties mygica_t230c_props = {
> +	.driver_name = KBUILD_MODNAME,
> +	.owner = THIS_MODULE,
> +	.adapter_nr = adapter_nr,
> +	.size_of_priv = sizeof(struct dvbsky_state),
> +
> +	.generic_bulk_ctrl_endpoint = 0x01,
> +	.generic_bulk_ctrl_endpoint_response = 0x81,
> +	.generic_bulk_ctrl_delay = DVBSKY_MSG_DELAY,
> +
> +	.i2c_algo         = &dvbsky_i2c_algo,
> +	.frontend_attach  = dvbsky_mygica_t230c_attach,
> +	.init             = dvbsky_init,
> +	.get_rc_config    = dvbsky_get_rc_config,
> +	.streaming_ctrl   = dvbsky_streaming_ctrl,
> +	.identify_state	  = dvbsky_identify_state,
> +	.exit             = dvbsky_exit,
> +
> +	.num_adapters = 1,
> +	.adapter = {
> +		{
> +			.stream = DVB_USB_STREAM_BULK(0x82, 8, 4096),
> +		}
> +	}
> +};
> +
>  static const struct usb_device_id dvbsky_id_table[] = {
>  	{ DVB_USB_DEVICE(0x0572, 0x6831,
>  		&dvbsky_s960_props, "DVBSky S960/S860", RC_MAP_DVBSKY) },
> @@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] = {
>  	{ DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
>  		&dvbsky_s960_props, "Terratec Cinergy S2 Rev.4",
>  		RC_MAP_DVBSKY) },
> +	{ DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
> +		&mygica_t230c_props, "Mygica T230C DVB-T/T2/C",

Drop supported DTV standard names from device name. Also it is MyGica 
not Mygica.

> +		RC_MAP_TOTAL_MEDIA_IN_HAND_02) },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(usb, dvbsky_id_table);
>

Fix those mentioned issues and ran patch against checkpatch.pl to ensure 
there is no remaining issues. After that whole patch set should be ok.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Stefan Bruens]

Hi Antti,

first thanks for for the review. Note the t230c_attach is mostly a copy of the 
t330_attach (which is very similar to the t680c_attach), so any of your 
comments should probably applied to the other attach functions to have a 
common coding style.

On Mittwoch, 15. Februar 2017 10:27:09 CET Antti Palosaari wrote:
> On 02/15/2017 03:51 AM, Stefan Brüns wrote:
[...]
> > diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 02dbc6c45423..729496e5a52e
> > 100644
> > --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > @@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter
> > *adap)> 
> >  	return ret;
> >  
> >  }
> > 
> > +static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
> > +{
> > +	struct dvbsky_state *state = adap_to_priv(adap);
> > +	struct dvb_usb_device *d = adap_to_d(adap);
> > +	int ret = 0;
> 
> you could return ret completely as you don't assign its value runtime

Sure, so something like:

  ...
  return 0;
fail_foo:
  xxx;
fail bar:
  yyy;
  return -ENODEV;

Some of the other attach functions assign ret = -ENODEV and then goto one of 
the fail_foo: labels.

 
> > +	struct i2c_adapter *i2c_adapter;
> > +	struct i2c_client *client_demod, *client_tuner;
> > +	struct i2c_board_info info;
> > +	struct si2168_config si2168_config;
> > +	struct si2157_config si2157_config;
> > +
> > +	/* attach demod */
> > +	memset(&si2168_config, 0, sizeof(si2168_config));
> 
> prefer sizeof dst

You mean sizeof(struct si2168_config) ?
 
> > +	si2168_config.i2c_adapter = &i2c_adapter;
> > +	si2168_config.fe = &adap->fe[0];
> > +	si2168_config.ts_mode = SI2168_TS_PARALLEL;
> > +	si2168_config.ts_clock_inv = 1;
> 
> it has boolean type

Sure
 
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > +	strlcpy(info.type, "si2168", I2C_NAME_SIZE);
> 
> I would prefer sizeof dst here too.

Most occurences of similar code in media/usb/ use I2C_NAME_SIZE, found two 
occurences of "strlcpy(buf, ..., sizeof(buf)), but of course I can change 
this.
 
> > +	info.addr = 0x64;
> > +	info.platform_data = &si2168_config;
> > +
> > +	request_module(info.type);
> 
> Use module name here. Even it is same than device id on that case, it is
> not always the case.

While si2157 driver has several supported chip types, si2168 only supports 
si2168 (several revisions). Both request_module("foobar") and 
request_module(info.type) are common in media/usb/. Change nevertheless?
 
> > +	client_demod = i2c_new_device(&d->i2c_adap, &info);
> > +	if (client_demod == NULL ||
> > +			client_demod->dev.driver == NULL)
> 
> You did not ran checkpatch.pl for that patch? or doesn't it complain
> anymore about these?

Checkpatch did not complain.
 
[...]
> > @@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] =
> > {
> > 
> >  	{ DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
> >  	
> >  		&dvbsky_s960_props, "Terratec Cinergy S2 Rev.4",
> >  		RC_MAP_DVBSKY) },
> > 
> > +	{ DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
> > +		&mygica_t230c_props, "Mygica T230C DVB-T/T2/C",
> 
> Drop supported DTV standard names from device name. Also it is MyGica
> not Mygica.

The print on the stick says: "MyGica(R) DVB-T2", label on the backside says 
"T230C<serial number>". According to the USB descriptors it is a "Geniatech" 
"EyeTV Stick". According to the box it is a "MyGica(R)" "Mini DVB-T2 USB Stick 
T230C"

Would "MyGica DVB-T2 T230C" be ok?
 
Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Antti Palosaari]

Hello

On 02/16/2017 01:31 AM, Stefan Bruens wrote:
> Hi Antti,
>
> first thanks for for the review. Note the t230c_attach is mostly a copy of the
> t330_attach (which is very similar to the t680c_attach), so any of your
> comments should probably applied to the other attach functions to have a
> common coding style.

Old code could be bad, but imho you could make new code better even it 
makes existing diver coding style slightly inconsistent.

>
> On Mittwoch, 15. Februar 2017 10:27:09 CET Antti Palosaari wrote:
>> On 02/15/2017 03:51 AM, Stefan Brüns wrote:
> [...]
>>> diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
>>> b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 02dbc6c45423..729496e5a52e
>>> 100644
>>> --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
>>> +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
>>> @@ -665,6 +665,68 @@ static int dvbsky_t330_attach(struct dvb_usb_adapter
>>> *adap)>
>>>  	return ret;
>>>
>>>  }
>>>
>>> +static int dvbsky_mygica_t230c_attach(struct dvb_usb_adapter *adap)
>>> +{
>>> +	struct dvbsky_state *state = adap_to_priv(adap);
>>> +	struct dvb_usb_device *d = adap_to_d(adap);
>>> +	int ret = 0;
>>
>> you could return ret completely as you don't assign its value runtime
>
> Sure, so something like:
>
>   ...
>   return 0;
> fail_foo:
>   xxx;
> fail bar:
>   yyy;
>   return -ENODEV;
>
> Some of the other attach functions assign ret = -ENODEV and then goto one of
> the fail_foo: labels.
>
>
>>> +	struct i2c_adapter *i2c_adapter;
>>> +	struct i2c_client *client_demod, *client_tuner;
>>> +	struct i2c_board_info info;
>>> +	struct si2168_config si2168_config;
>>> +	struct si2157_config si2157_config;
>>> +
>>> +	/* attach demod */
>>> +	memset(&si2168_config, 0, sizeof(si2168_config));
>>
>> prefer sizeof dst
>
> You mean sizeof(struct si2168_config) ?

yeah. See chapter 14 from kernel coding style documentation, it handles 
issue slightly.

>
>>> +	si2168_config.i2c_adapter = &i2c_adapter;
>>> +	si2168_config.fe = &adap->fe[0];
>>> +	si2168_config.ts_mode = SI2168_TS_PARALLEL;
>>> +	si2168_config.ts_clock_inv = 1;
>>
>> it has boolean type
>
> Sure
>
>>> +	memset(&info, 0, sizeof(struct i2c_board_info));
>>> +	strlcpy(info.type, "si2168", I2C_NAME_SIZE);
>>
>> I would prefer sizeof dst here too.
>
> Most occurences of similar code in media/usb/ use I2C_NAME_SIZE, found two
> occurences of "strlcpy(buf, ..., sizeof(buf)), but of course I can change
> this.
>
>>> +	info.addr = 0x64;
>>> +	info.platform_data = &si2168_config;
>>> +
>>> +	request_module(info.type);
>>
>> Use module name here. Even it is same than device id on that case, it is
>> not always the case.
>
> While si2157 driver has several supported chip types, si2168 only supports
> si2168 (several revisions). Both request_module("foobar") and
> request_module(info.type) are common in media/usb/. Change nevertheless?
>
>>> +	client_demod = i2c_new_device(&d->i2c_adap, &info);
>>> +	if (client_demod == NULL ||
>>> +			client_demod->dev.driver == NULL)
>>
>> You did not ran checkpatch.pl for that patch? or doesn't it complain
>> anymore about these?
>
> Checkpatch did not complain.

Indentation seem seems to be wrong (see again coding style doc). Also 
those might fit into single line. And not sure comparing even to NULL, 
at least some point preferred style was !foo, but personally I don't mind.

>
> [...]
>>> @@ -858,6 +946,9 @@ static const struct usb_device_id dvbsky_id_table[] =
>>> {
>>>
>>>  	{ DVB_USB_DEVICE(USB_VID_TERRATEC, USB_PID_TERRATEC_CINERGY_S2_R4,
>>>  	
>>>  		&dvbsky_s960_props, "Terratec Cinergy S2 Rev.4",
>>>  		RC_MAP_DVBSKY) },
>>>
>>> +	{ DVB_USB_DEVICE(USB_VID_CONEXANT, USB_PID_MYGICA_T230C,
>>> +		&mygica_t230c_props, "Mygica T230C DVB-T/T2/C",
>>
>> Drop supported DTV standard names from device name. Also it is MyGica
>> not Mygica.
>
> The print on the stick says: "MyGica(R) DVB-T2", label on the backside says
> "T230C<serial number>". According to the USB descriptors it is a "Geniatech"
> "EyeTV Stick". According to the box it is a "MyGica(R)" "Mini DVB-T2 USB Stick
> T230C"
>
> Would "MyGica DVB-T2 T230C" be ok?

I would just use device commercial name, which one seems to be most 
official. Geniatech is manufacturer, but commercial brand they sell 
these is MyGica so at least it is not Geniatech EyeTV Stick which is 
something like design name.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH v2 3/3] [media] dvbsky: MyGica T230C support

[Antti Palosaari]

On 02/16/2017 10:48 AM, Antti Palosaari wrote:
> On 02/16/2017 01:31 AM, Stefan Bruens wrote:

>>>> +    /* attach demod */
>>>> +    memset(&si2168_config, 0, sizeof(si2168_config));
>>>
>>> prefer sizeof dst
>>
>> You mean sizeof(struct si2168_config) ?
>
> yeah. See chapter 14 from kernel coding style documentation, it handles
> issue slightly.

argh, I looked wrong! It is *correct*, do not change it. It is just as 
it should. Sorry about noise.

regards
Antti




-- 
http://palosaari.fi/