| Message ID | 20130410114051.GA21419@longonot.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@oracle.com> > --- > 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. Regards, Hans > > 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; > @@ -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(ZZradio->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; > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 11, 2013 at 11:24 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@oracle.com> >> --- >> 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@oracle.com> 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@oracle.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; @@ -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 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@oracle.com> --- Please review this carefully. I don't have the hardware to test it. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html