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/
