On Thu, Apr 11, 2013 at 11:24 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > On Wed April 10 2013 13:40:51 Dan Carpenter wrote: >> This is a static checker where it complains if we check for one function >> pointer and then call a different function on the next line. >> >> In most cases, the code does the same thing before and after this patch. >> For example, when ->phase_diversity is non-NULL then ->phase_div_status >> is also non-NULL. >> >> The one place where that's not true is when we check ->rds_blckcnt >> instead of ->rsq_status. In those cases, we would want to call >> ->rsq_status but we instead return -ENOENT. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> >> --- >> Please review this carefully. I don't have the hardware to test it. > > Andrey, can you review this? I think the first two chunks are correct, but > the last two chunks are probably not what you want. In the case of an AM > receiver there is no RDS data, so an error is probably correct. > Sorry, I suck at gmail-ing and my response to this letter was bounced for having HTML in it and I guess you didn't receive it either, I'll just copy it below: ==================================== On Wed, Apr 10, 2013 at 4:40 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > This is a static checker where it complains if we check for one function > pointer and then call a different function on the next line. > > In most cases, the code does the same thing before and after this patch. > For example, when ->phase_diversity is non-NULL then ->phase_div_status > is also non-NULL. > > The one place where that's not true is when we check ->rds_blckcnt > instead of ->rsq_status. In those cases, we would want to call > ->rsq_status but we instead return -ENOENT. > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > --- > Please review this carefully. I don't have the hardware to test it. > > diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c > index 9430c6a..817fc0c 100644 > --- a/drivers/media/radio/radio-si476x.c > +++ b/drivers/media/radio/radio-si476x.c > @@ -854,7 +854,7 @@ static int si476x_radio_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > switch (ctrl->id) { > case V4L2_CID_SI476X_INTERCHIP_LINK: > if (si476x_core_has_diversity(radio->core)) { > - if (radio->ops->phase_diversity) { > + if (radio->ops->phase_div_status) { > retval = radio->ops->phase_div_status(radio->core); > if (retval < 0) > break; I think I would prefer to use "si476x_core_is_in_am_receiver_mode" in the case above and then have additional BUG_ON(radio->ops->phase_div_status) for cases where it should not be NULL(tuner in FM mode, diversity feature present) also I probably should return -EINVAL in all the cases for this control. > @@ -1285,7 +1285,7 @@ static ssize_t si476x_radio_read_agc_blob(struct file *file, > struct si476x_agc_status_report report; > > si476x_core_lock(radio->core); > - if (radio->ops->rds_blckcnt) > + if (radio->ops->agc_status) > err = radio->ops->agc_status(radio->core, &report); > else > err = -ENOENT; > @@ -1320,7 +1320,7 @@ static ssize_t si476x_radio_read_rsq_blob(struct file *file, > }; > > si476x_core_lock(radio->core); > - if (radio->ops->rds_blckcnt) > + if (radio->ops->rsq_status) > err = radio->ops->rsq_status(radio->core, &args, &report); > else > err = -ENOENT; > @@ -1355,7 +1355,7 @@ static ssize_t si476x_radio_read_rsq_primary_blob(struct file *file, > }; > > si476x_core_lock(radio->core); > - if (radio->ops->rds_blckcnt) > + if (radio->ops->rsq_status) > err = radio->ops->rsq_status(radio->core, &args, &report); > else > err = -ENOENT; This all looks like a dumb copy-paste screw up on my part. I think I copied the body of "si476x_radio_read_rds_blckcnt_blob" and used it to implement all the other functions and I guess forgot to change this bit of the code. Thank you for catching this, mistakes like this are just embarrassing. I'll make a patch based on this one and send it alongside the patches for MFD subsystem -- 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