Re: [PATCH 3/6] si4713 : Bug fix for si4713_tx_tune_power() method in the i2c driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux