Hello Hans,
On Sun, Sep 1, 2013 at 7:04 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 08/31/2013 01:49 PM, edubezval@xxxxxxxxx wrote:
>> Hi Dinesh,
>>
>> On Fri, Aug 30, 2013 at 7:28 AM, Dinesh Ram <dinram@xxxxxxxxx> wrote:
>>> In the si4713_tx_tune_power() method, the args array element 'power' can take values between
>>> SI4713_MIN_POWER and SI4713_MAX_POWER. power = 0 is also valid.
>>> All the values (0 > power < SI4713_MIN_POWER) are illegal and hence
>>> are all mapped to SI4713_MIN_POWER.
>>
>> While do we need to assume min power in these cases?
s/While/Why! but I guess you already got it.
>
> It makes no sense to map 0 < powers < MIN_POWER to 0 (i.e. power off). I would never
> expect that selecting a power > 0 would actually turn off power, so just map to the
> lowest possible power value.
Hmm.. Interesting. Is this what you are seen currently?
0 < power < MIN_POWER == power off?
I would expect the driver to return an error code:
if (((power > 0) && (power < SI4713_MIN_POWER)) ||
power > SI4713_MAX_POWER || antcap > SI4713_MAX_ANTCAP)
return -EDOM;
And that is why I am asking why are we assigning a min value when we
see a value out of the expected range?
>
>>
>>>
>>> Signed-off-by: Dinesh Ram <dinram@xxxxxxxxx>
>>> ---
>>> drivers/media/radio/si4713/si4713.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
>>> index 55c4d27..5d0be87 100644
>>> --- a/drivers/media/radio/si4713/si4713.c
>>> +++ b/drivers/media/radio/si4713/si4713.c
>>> @@ -550,14 +550,14 @@ static int si4713_tx_tune_freq(struct si4713_device *sdev, u16 frequency)
>>> }
>>>
>>> /*
>>> - * si4713_tx_tune_power - Sets the RF voltage level between 88 and 115 dBuV in
>>> + * si4713_tx_tune_power - Sets the RF voltage level between 88 and 120 dBuV in
>>> * 1 dB units. A value of 0x00 indicates off. The command
>>> * also sets the antenna tuning capacitance. A value of 0
>>> * indicates autotuning, and a value of 1 - 191 indicates
>>> * a manual override, which results in a tuning
>>> * capacitance of 0.25 pF x @antcap.
>>> * @sdev: si4713_device structure for the device we are communicating
>>> - * @power: tuning power (88 - 115 dBuV, unit/step 1 dB)
>>> + * @power: tuning power (88 - 120 dBuV, unit/step 1 dB)
>>> * @antcap: value of antenna tuning capacitor (0 - 191)
>>> */
>>> static int si4713_tx_tune_power(struct si4713_device *sdev, u8 power,
>>> @@ -571,16 +571,16 @@ static int si4713_tx_tune_power(struct si4713_device *sdev, u8 power,
>>> * .Third byte = power
>>> * .Fourth byte = antcap
>>> */
>>> - const u8 args[SI4713_TXPWR_NARGS] = {
>>> + u8 args[SI4713_TXPWR_NARGS] = {
>>> 0x00,
>>> 0x00,
>>> power,
>>> antcap,
>>> };
>>>
>>> - if (((power > 0) && (power < SI4713_MIN_POWER)) ||
>>> - power > SI4713_MAX_POWER || antcap > SI4713_MAX_ANTCAP)
>>> - return -EDOM;
>>> + /* Map power values 1-87 to MIN_POWER (88) */
>>> + if (power > 0 && power < SI4713_MIN_POWER)
>>> + args[2] = power = SI4713_MIN_POWER;
>>
>> Why are you allowing antcap > SI4713_MAX_ANTCAP? and power >
>> SI4713_MAX_POWER too?
>
> The control framework already checks for that so you'll never see out-of-range values
> here. So it was an unnecessary check.
>
I see. Are you sure about that?
I am just a bit concerned about regulations here. One can really get
in trouble if it can transmit FM for longer than 10m in some
countries, without a license.
I know of some nasty ways to boost transmitting power, but I would
like to avoid making even easier with this driver. :-)
>>
>>>
>>> err = si4713_send_command(sdev, SI4713_CMD_TX_TUNE_POWER,
>>> args, ARRAY_SIZE(args), val,
>>> @@ -1457,9 +1457,9 @@ static int si4713_probe(struct i2c_client *client,
>>> V4L2_CID_TUNE_PREEMPHASIS,
>>> V4L2_PREEMPHASIS_75_uS, 0, V4L2_PREEMPHASIS_50_uS);
>>> sdev->tune_pwr_level = v4l2_ctrl_new_std(hdl, &si4713_ctrl_ops,
>>> - V4L2_CID_TUNE_POWER_LEVEL, 0, 120, 1, DEFAULT_POWER_LEVEL);
>>> + V4L2_CID_TUNE_POWER_LEVEL, 0, SI4713_MAX_POWER, 1, DEFAULT_POWER_LEVEL);
>>> sdev->tune_ant_cap = v4l2_ctrl_new_std(hdl, &si4713_ctrl_ops,
>>> - V4L2_CID_TUNE_ANTENNA_CAPACITOR, 0, 191, 1, 0);
>>> + V4L2_CID_TUNE_ANTENNA_CAPACITOR, 0, SI4713_MAX_ANTCAP, 1, 0);
>>>
>>> if (hdl->error) {
>>> rval = hdl->error;
>>> --
>>> 1.8.4.rc2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>
> Regards,
>
> Hans
--
Eduardo Bezerra Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
