| Message ID | 1516225967-21668-1-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/17/2018 11:52 PM, Brad Love wrote: > Some tuners produce inverted spectrum, but the si2168 is not > currently set up to accept it. This adds an optional parameter > to set the frontend up to receive inverted spectrum. > > Parameter is optional and only boards who enable inversion > will utilize this. > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > Changes since v1: > - Embarassing build failure due to missing declaration. > > drivers/media/dvb-frontends/si2168.c | 3 +++ > drivers/media/dvb-frontends/si2168.h | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c > index c041e79..048b815 100644 > --- a/drivers/media/dvb-frontends/si2168.c > +++ b/drivers/media/dvb-frontends/si2168.c > @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct dvb_frontend *fe) > struct i2c_client *client = fe->demodulator_priv; > struct si2168_dev *dev = i2c_get_clientdata(client); > struct dtv_frontend_properties *c = &fe->dtv_property_cache; > + struct si2168_config *config = client->dev.platform_data; hmmm, are you sure platform data pointer points is const? I usually tend to store all config information to device state. Then there is no need to care if pointer is valid or not anymore. And inversion happens when those wires are cross-connected > int ret; > struct si2168_cmd cmd; > u8 bandwidth, delivery_system; > @@ -339,6 +340,8 @@ static int si2168_set_frontend(struct dvb_frontend *fe) > > memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); > cmd.args[4] = delivery_system | bandwidth; > + if (config->spectral_inversion) > + cmd.args[5] |= 1; > cmd.wlen = 6; > cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > diff --git a/drivers/media/dvb-frontends/si2168.h b/drivers/media/dvb-frontends/si2168.h > index f48f0fb..d519edd 100644 > --- a/drivers/media/dvb-frontends/si2168.h > +++ b/drivers/media/dvb-frontends/si2168.h > @@ -46,6 +46,9 @@ struct si2168_config { > > /* TS clock gapped */ > bool ts_clock_gapped; > + > + /* Inverted spectrum */ > + bool spectral_inversion; > }; > > #endif >
On 2018-01-17 16:02, Antti Palosaari wrote: > > > On 01/17/2018 11:52 PM, Brad Love wrote: >> Some tuners produce inverted spectrum, but the si2168 is not >> currently set up to accept it. This adds an optional parameter >> to set the frontend up to receive inverted spectrum. >> >> Parameter is optional and only boards who enable inversion >> will utilize this. >> >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> Changes since v1: >> - Embarassing build failure due to missing declaration. >> >> drivers/media/dvb-frontends/si2168.c | 3 +++ >> drivers/media/dvb-frontends/si2168.h | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/media/dvb-frontends/si2168.c >> b/drivers/media/dvb-frontends/si2168.c >> index c041e79..048b815 100644 >> --- a/drivers/media/dvb-frontends/si2168.c >> +++ b/drivers/media/dvb-frontends/si2168.c >> @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct >> dvb_frontend *fe) >> struct i2c_client *client = fe->demodulator_priv; >> struct si2168_dev *dev = i2c_get_clientdata(client); >> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >> + struct si2168_config *config = client->dev.platform_data; > > hmmm, are you sure platform data pointer points is const? I usually > tend to store all config information to device state. Then there is no > need to care if pointer is valid or not anymore. > > And inversion happens when those wires are cross-connected It just dawned on me that the platform_data is stack allocated and therefore not safe to access outside of probe. I will fix this momentarily. I was informed by one of our hardware guys that the two models in patch 2/2 are inverted spectrum, so I guess they have wires cross-connected. I can verify this again to be sure. > >> int ret; >> struct si2168_cmd cmd; >> u8 bandwidth, delivery_system; >> @@ -339,6 +340,8 @@ static int si2168_set_frontend(struct >> dvb_frontend *fe) >> memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); >> cmd.args[4] = delivery_system | bandwidth; >> + if (config->spectral_inversion) >> + cmd.args[5] |= 1; >> cmd.wlen = 6; >> cmd.rlen = 4; >> ret = si2168_cmd_execute(client, &cmd); >> diff --git a/drivers/media/dvb-frontends/si2168.h >> b/drivers/media/dvb-frontends/si2168.h >> index f48f0fb..d519edd 100644 >> --- a/drivers/media/dvb-frontends/si2168.h >> +++ b/drivers/media/dvb-frontends/si2168.h >> @@ -46,6 +46,9 @@ struct si2168_config { >> /* TS clock gapped */ >> bool ts_clock_gapped; >> + >> + /* Inverted spectrum */ >> + bool spectral_inversion; >> }; >> #endif >> >
On 2018-01-17 16:08, Brad Love wrote: > On 2018-01-17 16:02, Antti Palosaari wrote: >> >> On 01/17/2018 11:52 PM, Brad Love wrote: >>> Some tuners produce inverted spectrum, but the si2168 is not >>> currently set up to accept it. This adds an optional parameter >>> to set the frontend up to receive inverted spectrum. >>> >>> Parameter is optional and only boards who enable inversion >>> will utilize this. >>> >>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>> --- >>> Changes since v1: >>> - Embarassing build failure due to missing declaration. >>> >>> drivers/media/dvb-frontends/si2168.c | 3 +++ >>> drivers/media/dvb-frontends/si2168.h | 3 +++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/drivers/media/dvb-frontends/si2168.c >>> b/drivers/media/dvb-frontends/si2168.c >>> index c041e79..048b815 100644 >>> --- a/drivers/media/dvb-frontends/si2168.c >>> +++ b/drivers/media/dvb-frontends/si2168.c >>> @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct >>> dvb_frontend *fe) >>> struct i2c_client *client = fe->demodulator_priv; >>> struct si2168_dev *dev = i2c_get_clientdata(client); >>> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >>> + struct si2168_config *config = client->dev.platform_data; >> hmmm, are you sure platform data pointer points is const? I usually >> tend to store all config information to device state. Then there is no >> need to care if pointer is valid or not anymore. >> >> And inversion happens when those wires are cross-connected > It just dawned on me that the platform_data is stack allocated and > therefore not safe to access outside of probe. I will fix this momentarily. > > I was informed by one of our hardware guys that the two models in patch > 2/2 are inverted spectrum, so I guess they have wires cross-connected. I > can verify this again to be sure. Hello Antti, I have confirmation. No 'cross-connected' / swapped differential pair polarities (if that's what you meant) on the IF pins. The si2157 inverted spectrum output is configurable though, and Hauppauge have the tuner set up to output inverted. Sounds like it was a decision based on interoperability with older demods. Cheers, Brad >>> int ret; >>> struct si2168_cmd cmd; >>> u8 bandwidth, delivery_system; >>> @@ -339,6 +340,8 @@ static int si2168_set_frontend(struct >>> dvb_frontend *fe) >>> memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); >>> cmd.args[4] = delivery_system | bandwidth; >>> + if (config->spectral_inversion) >>> + cmd.args[5] |= 1; >>> cmd.wlen = 6; >>> cmd.rlen = 4; >>> ret = si2168_cmd_execute(client, &cmd); >>> diff --git a/drivers/media/dvb-frontends/si2168.h >>> b/drivers/media/dvb-frontends/si2168.h >>> index f48f0fb..d519edd 100644 >>> --- a/drivers/media/dvb-frontends/si2168.h >>> +++ b/drivers/media/dvb-frontends/si2168.h >>> @@ -46,6 +46,9 @@ struct si2168_config { >>> /* TS clock gapped */ >>> bool ts_clock_gapped; >>> + >>> + /* Inverted spectrum */ >>> + bool spectral_inversion; >>> }; >>> #endif >>>
On 01/18/2018 03:58 AM, Brad Love wrote: > > On 2018-01-17 16:08, Brad Love wrote: >> On 2018-01-17 16:02, Antti Palosaari wrote: >>> >>> On 01/17/2018 11:52 PM, Brad Love wrote: >>>> Some tuners produce inverted spectrum, but the si2168 is not >>>> currently set up to accept it. This adds an optional parameter >>>> to set the frontend up to receive inverted spectrum. >>>> >>>> Parameter is optional and only boards who enable inversion >>>> will utilize this. >>>> >>>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>>> --- >>>> Changes since v1: >>>> - Embarassing build failure due to missing declaration. >>>> >>>> drivers/media/dvb-frontends/si2168.c | 3 +++ >>>> drivers/media/dvb-frontends/si2168.h | 3 +++ >>>> 2 files changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/media/dvb-frontends/si2168.c >>>> b/drivers/media/dvb-frontends/si2168.c >>>> index c041e79..048b815 100644 >>>> --- a/drivers/media/dvb-frontends/si2168.c >>>> +++ b/drivers/media/dvb-frontends/si2168.c >>>> @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct >>>> dvb_frontend *fe) >>>> struct i2c_client *client = fe->demodulator_priv; >>>> struct si2168_dev *dev = i2c_get_clientdata(client); >>>> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >>>> + struct si2168_config *config = client->dev.platform_data; >>> hmmm, are you sure platform data pointer points is const? I usually >>> tend to store all config information to device state. Then there is no >>> need to care if pointer is valid or not anymore. >>> >>> And inversion happens when those wires are cross-connected >> It just dawned on me that the platform_data is stack allocated and >> therefore not safe to access outside of probe. I will fix this momentarily. >> >> I was informed by one of our hardware guys that the two models in patch >> 2/2 are inverted spectrum, so I guess they have wires cross-connected. I >> can verify this again to be sure. > > > Hello Antti, > > I have confirmation. No 'cross-connected' / swapped differential pair > polarities (if that's what you meant) on the IF pins. The si2157 > inverted spectrum output is configurable though, and Hauppauge > have the tuner set up to output inverted. Sounds like it was a decision > based on interoperability with older demods. yeah, that was what I was thinking for. That board single tuner and two demods which other demod does not support if spectrum inversion? If there is just si2168 and si2157, you can set both to invert or both to non-invert - the end result is same. Antti
On 2018-01-17 20:36, Antti Palosaari wrote: > On 01/18/2018 03:58 AM, Brad Love wrote: >> >> On 2018-01-17 16:08, Brad Love wrote: >>> On 2018-01-17 16:02, Antti Palosaari wrote: >>>> >>>> On 01/17/2018 11:52 PM, Brad Love wrote: >>>>> Some tuners produce inverted spectrum, but the si2168 is not >>>>> currently set up to accept it. This adds an optional parameter >>>>> to set the frontend up to receive inverted spectrum. >>>>> >>>>> Parameter is optional and only boards who enable inversion >>>>> will utilize this. >>>>> >>>>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>>>> --- >>>>> Changes since v1: >>>>> - Embarassing build failure due to missing declaration. >>>>> >>>>> drivers/media/dvb-frontends/si2168.c | 3 +++ >>>>> drivers/media/dvb-frontends/si2168.h | 3 +++ >>>>> 2 files changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/media/dvb-frontends/si2168.c >>>>> b/drivers/media/dvb-frontends/si2168.c >>>>> index c041e79..048b815 100644 >>>>> --- a/drivers/media/dvb-frontends/si2168.c >>>>> +++ b/drivers/media/dvb-frontends/si2168.c >>>>> @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct >>>>> dvb_frontend *fe) >>>>> struct i2c_client *client = fe->demodulator_priv; >>>>> struct si2168_dev *dev = i2c_get_clientdata(client); >>>>> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >>>>> + struct si2168_config *config = client->dev.platform_data; >>>> hmmm, are you sure platform data pointer points is const? I usually >>>> tend to store all config information to device state. Then there is no >>>> need to care if pointer is valid or not anymore. >>>> >>>> And inversion happens when those wires are cross-connected >>> It just dawned on me that the platform_data is stack allocated and >>> therefore not safe to access outside of probe. I will fix this >>> momentarily. >>> >>> I was informed by one of our hardware guys that the two models in patch >>> 2/2 are inverted spectrum, so I guess they have wires >>> cross-connected. I >>> can verify this again to be sure. >> >> >> Hello Antti, >> >> I have confirmation. No 'cross-connected' / swapped differential pair >> polarities (if that's what you meant) on the IF pins. The si2157 >> inverted spectrum output is configurable though, and Hauppauge >> have the tuner set up to output inverted. Sounds like it was a decision >> based on interoperability with older demods. > > yeah, that was what I was thinking for. That board single tuner and > two demods which other demod does not support if spectrum inversion? > > If there is just si2168 and si2157, you can set both to invert or both > to non-invert - the end result is same. > > Antti I will check on the HVR975 tomorrow, if it's affected as well I'll submit a patch. The lgdt3306a frontend is already set up for auto spectrum inversion, so it is able handle the si2157 in either config.
On 2018-01-17 21:10, Brad Love wrote: > On 2018-01-17 20:36, Antti Palosaari wrote: >> On 01/18/2018 03:58 AM, Brad Love wrote: >>> On 2018-01-17 16:08, Brad Love wrote: >>>> On 2018-01-17 16:02, Antti Palosaari wrote: >>>>> On 01/17/2018 11:52 PM, Brad Love wrote: >>>>>> Some tuners produce inverted spectrum, but the si2168 is not >>>>>> currently set up to accept it. This adds an optional parameter >>>>>> to set the frontend up to receive inverted spectrum. >>>>>> >>>>>> Parameter is optional and only boards who enable inversion >>>>>> will utilize this. >>>>>> >>>>>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>>>>> --- >>>>>> Changes since v1: >>>>>> - Embarassing build failure due to missing declaration. >>>>>> >>>>>> drivers/media/dvb-frontends/si2168.c | 3 +++ >>>>>> drivers/media/dvb-frontends/si2168.h | 3 +++ >>>>>> 2 files changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/dvb-frontends/si2168.c >>>>>> b/drivers/media/dvb-frontends/si2168.c >>>>>> index c041e79..048b815 100644 >>>>>> --- a/drivers/media/dvb-frontends/si2168.c >>>>>> +++ b/drivers/media/dvb-frontends/si2168.c >>>>>> @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct >>>>>> dvb_frontend *fe) >>>>>> struct i2c_client *client = fe->demodulator_priv; >>>>>> struct si2168_dev *dev = i2c_get_clientdata(client); >>>>>> struct dtv_frontend_properties *c = &fe->dtv_property_cache; >>>>>> + struct si2168_config *config = client->dev.platform_data; >>>>> hmmm, are you sure platform data pointer points is const? I usually >>>>> tend to store all config information to device state. Then there is no >>>>> need to care if pointer is valid or not anymore. >>>>> >>>>> And inversion happens when those wires are cross-connected >>>> It just dawned on me that the platform_data is stack allocated and >>>> therefore not safe to access outside of probe. I will fix this >>>> momentarily. >>>> >>>> I was informed by one of our hardware guys that the two models in patch >>>> 2/2 are inverted spectrum, so I guess they have wires >>>> cross-connected. I >>>> can verify this again to be sure. >>> >>> Hello Antti, >>> >>> I have confirmation. No 'cross-connected' / swapped differential pair >>> polarities (if that's what you meant) on the IF pins. The si2157 >>> inverted spectrum output is configurable though, and Hauppauge >>> have the tuner set up to output inverted. Sounds like it was a decision >>> based on interoperability with older demods. >> yeah, that was what I was thinking for. That board single tuner and >> two demods which other demod does not support if spectrum inversion? >> >> If there is just si2168 and si2157, you can set both to invert or both >> to non-invert - the end result is same. >> >> Antti > > I will check on the HVR975 tomorrow, if it's affected as well I'll > submit a patch. The lgdt3306a frontend is already set up for auto > spectrum inversion, so it is able handle the si2157 in either config. > The HVR975 should be good as currently submitted. Cheers, Brad
diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c index c041e79..048b815 100644 --- a/drivers/media/dvb-frontends/si2168.c +++ b/drivers/media/dvb-frontends/si2168.c @@ -213,6 +213,7 @@ static int si2168_set_frontend(struct dvb_frontend *fe) struct i2c_client *client = fe->demodulator_priv; struct si2168_dev *dev = i2c_get_clientdata(client); struct dtv_frontend_properties *c = &fe->dtv_property_cache; + struct si2168_config *config = client->dev.platform_data; int ret; struct si2168_cmd cmd; u8 bandwidth, delivery_system; @@ -339,6 +340,8 @@ static int si2168_set_frontend(struct dvb_frontend *fe) memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); cmd.args[4] = delivery_system | bandwidth; + if (config->spectral_inversion) + cmd.args[5] |= 1; cmd.wlen = 6; cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); diff --git a/drivers/media/dvb-frontends/si2168.h b/drivers/media/dvb-frontends/si2168.h index f48f0fb..d519edd 100644 --- a/drivers/media/dvb-frontends/si2168.h +++ b/drivers/media/dvb-frontends/si2168.h @@ -46,6 +46,9 @@ struct si2168_config { /* TS clock gapped */ bool ts_clock_gapped; + + /* Inverted spectrum */ + bool spectral_inversion; }; #endif
Some tuners produce inverted spectrum, but the si2168 is not currently set up to accept it. This adds an optional parameter to set the frontend up to receive inverted spectrum. Parameter is optional and only boards who enable inversion will utilize this. Signed-off-by: Brad Love <brad@nextdimension.cc> --- Changes since v1: - Embarassing build failure due to missing declaration. drivers/media/dvb-frontends/si2168.c | 3 +++ drivers/media/dvb-frontends/si2168.h | 3 +++ 2 files changed, 6 insertions(+)