linux-kernel - Re: [PATCH v2 3/6] Add commands abstraction layer for SI476X MFD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201210091142.54364.hverkuil@xs4all.nl>
Date:	Tue, 9 Oct 2012 11:42:54 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Andrey Smirnov <andrey.smirnov@...vergeddevices.net>
Cc:	mchehab@...hat.com, sameo@...ux.intel.com,
	broonie@...nsource.wolfsonmicro.com, perex@...ex.cz, tiwai@...e.de,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/6] Add commands abstraction layer for SI476X MFD

On Mon 8 October 2012 22:06:29 Andrey Smirnov wrote:
> On 10/08/2012 01:56 AM, Hans Verkuil wrote:
> > On Sat October 6 2012 03:54:59 Andrey Smirnov wrote:
> >> This patch adds all the functions used for exchanging commands with
> >> the chip.
> >>
> >> Signed-off-by: Andrey Smirnov <andrey.smirnov@...vergeddevices.net>
> >> ---
> >>  drivers/mfd/si476x-cmd.c | 1493 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 1493 insertions(+)
> >>  create mode 100644 drivers/mfd/si476x-cmd.c
> >>
> >> diff --git a/drivers/mfd/si476x-cmd.c b/drivers/mfd/si476x-cmd.c
> >> new file mode 100644
> >> index 0000000..f11cf58
> >> --- /dev/null
> >> +++ b/drivers/mfd/si476x-cmd.c

<snip>

> >> +/**
> >> + * si476x_cmd_am_rsq_status - send 'FM_TUNE_FREQ' command to the
> >> + * device
> >> + * @core  - device to send the command to
> >> + * @rsqack - if set command clears RSQINT, SNRINT, SNRLINT, RSSIHINT,
> >> + *           RSSSILINT, BLENDINT, MULTHINT and MULTLINT
> >> + * @attune - when set the values in the status report are the values
> >> + *           that were calculated at tune
> >> + * @cancel - abort ongoing seek/tune opertation
> >> + * @stcack - clear the STCINT bin in status register
> >> + * @report - all signal quality information retured by the command
> >> + *           (if NULL then the output of the command is ignored)

I've just noticed that this comment block does not correspond at all to
the code. It's a good idea to check the other comment blocks for similar
copy/paste errors.

> >> + *
> >> + * Function returns 0 on success and negative error code on failure
> >> + */
> >> +int si476x_core_cmd_am_rsq_status(struct si476x_core *core,
> >> +				  struct si476x_rsq_status_args *rsqargs,
> >> +				  struct si476x_rsq_status_report *report)
> >> +{
> >> +	int err;
> >> +	u8       resp[CMD_AM_RSQ_STATUS_NRESP];
> >> +	const u8 args[CMD_AM_RSQ_STATUS_NARGS] = {
> >> +		rsqargs->rsqack << 3 | rsqargs->attune << 2 |
> >> +		rsqargs->cancel << 1 | rsqargs->stcack,
> >> +	};
> >> +
> >> +	err = CORE_SEND_COMMAND(core, CMD_AM_RSQ_STATUS,
> >> +				args, resp,
> >> +				atomic_read(&core->timeouts.command));
> >> +
> >> +	if (report) {
> > Do you really need to test 'report'? Does it ever make sense if this is
> > called with a NULL report pointer?
> 
> Unfortunately yes. This command is also used to acknowledge and
> STC(seek-tune completed)
> interrupt.

A comment would be welcome here, and in similar cases. It's not obvious.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/