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? 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. > >> >> 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. > >> >> 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 -- 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