[PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

[PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

[Christian Engelmayer]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

Some board pointer are assigned twice via comedi_board() in the comedi low
level driver attach functions. Remove the duplicate assignment from the
variable definition where the pointer is not used anyway until assigned later
in the function when dev->board_ptr, that comedi_board() relies on, is setup
correctly.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
Compile tested and applies against v3.15-rc2 as well as branch staging-next
of tree git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
---
 drivers/staging/comedi/drivers/das1800.c     | 2 +-
 drivers/staging/comedi/drivers/das800.c      | 2 +-
 drivers/staging/comedi/drivers/dt2801.c      | 2 +-
 drivers/staging/comedi/drivers/ni_at_a2150.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/das1800.c b/drivers/staging/comedi/drivers/das1800.c
index d581029..0335a70 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -1479,7 +1479,7 @@ static int das1800_probe(struct comedi_device *dev)
 static int das1800_attach(struct comedi_device *dev,
 			  struct comedi_devconfig *it)
 {
-	const struct das1800_board *thisboard = comedi_board(dev);
+	const struct das1800_board *thisboard;
 	struct das1800_private *devpriv;
 	struct comedi_subdevice *s;
 	unsigned int irq = it->options[1];
diff --git a/drivers/staging/comedi/drivers/das800.c b/drivers/staging/comedi/drivers/das800.c
index b23a12c..e2dc43d 100644
--- a/drivers/staging/comedi/drivers/das800.c
+++ b/drivers/staging/comedi/drivers/das800.c
@@ -683,7 +683,7 @@ static int das800_probe(struct comedi_device *dev)
 
 static int das800_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 {
-	const struct das800_board *thisboard = comedi_board(dev);
+	const struct das800_board *thisboard;
 	struct das800_private *devpriv;
 	struct comedi_subdevice *s;
 	unsigned int irq = it->options[1];
diff --git a/drivers/staging/comedi/drivers/dt2801.c b/drivers/staging/comedi/drivers/dt2801.c
index d4d4e4b..4263014 100644
--- a/drivers/staging/comedi/drivers/dt2801.c
+++ b/drivers/staging/comedi/drivers/dt2801.c
@@ -545,7 +545,7 @@ static int dt2801_dio_insn_config(struct comedi_device *dev,
 */
 static int dt2801_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 {
-	const struct dt2801_board *board = comedi_board(dev);
+	const struct dt2801_board *board;
 	struct dt2801_private *devpriv;
 	struct comedi_subdevice *s;
 	int board_code, type;
diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
index afbf251..fefd97e 100644
--- a/drivers/staging/comedi/drivers/ni_at_a2150.c
+++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
@@ -695,7 +695,7 @@ static int a2150_probe(struct comedi_device *dev)
 
 static int a2150_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 {
-	const struct a2150_board *thisboard = comedi_board(dev);
+	const struct a2150_board *thisboard;
 	struct a2150_private *devpriv;
 	struct comedi_subdevice *s;
 	unsigned int irq = it->options[1];
-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

[Ian Abbott]

On 26/04/14 15:04, Christian Engelmayer wrote:
> Some board pointer are assigned twice via comedi_board() in the comedi low
> level driver attach functions. Remove the duplicate assignment from the
> variable definition where the pointer is not used anyway until assigned later
> in the function when dev->board_ptr, that comedi_board() relies on, is setup
> correctly.
>
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> ---
> Compile tested and applies against v3.15-rc2 as well as branch staging-next
> of tree git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

Looks good!

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

RE: [PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

[Hartley Sweeten]

On Saturday, April 26, 2014 7:04 AM, Christian Engelmayer wrote:
> Some board pointer are assigned twice via comedi_board() in the comedi low
> level driver attach functions. Remove the duplicate assignment from the
> variable definition where the pointer is not used anyway until assigned later
> in the function when dev->board_ptr, that comedi_board() relies on, is setup
> correctly.
>
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> ---
> Compile tested and applies against v3.15-rc2 as well as branch staging-next
> of tree git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> ---
>  drivers/staging/comedi/drivers/das1800.c     | 2 +-
>  drivers/staging/comedi/drivers/das800.c      | 2 +-
>  drivers/staging/comedi/drivers/dt2801.c      | 2 +-
>  drivers/staging/comedi/drivers/ni_at_a2150.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

Technically, these drivers are fine as-is.

They are all legacy comedi drivers and use the manual attach mechanism. The
dev->board pointer is setup by the comedi core before calling the drivers
(*attach) so the foo = comedi_board(dev) is getting the board pointer that
was found by the core.

Unlike most comedi legacy drivers, these drivers then do an additional "probe"
to try and identify the board. This could result in the dev->board_ptr getting
changed which requires updating the local variable for the board pointer.

These probe functions need to be looked at to see if they are actually needed.

For now I would prefer that the existing code stay as-is.

Regards,
Hartley

Re: [PATCH] staging: comedi: remove duplicate pointer assignments in attach functions

[Christian Engelmayer]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Mon, 28 Apr 2014 22:36:13 +0000, Hartley Sweeten <HartleyS@visionengravers.com> wrote:
> Technically, these drivers are fine as-is.

They are. The proposed change falls under minor code maintenance only.

> They are all legacy comedi drivers and use the manual attach mechanism. The
> dev->board pointer is setup by the comedi core before calling the drivers
> (*attach) so the foo = comedi_board(dev) is getting the board pointer that
> was found by the core.

> Unlike most comedi legacy drivers, these drivers then do an additional "probe"
> to try and identify the board. This could result in the dev->board_ptr getting
> changed which requires updating the local variable for the board pointer.

The point is that while updating dev->board_ptr is necessary in case of the
manual attach use case, deriving the local pointer before dev->board_ptr is
decided is not. Furthermore it might be a bit risky to already have a local
pointer to a valid, but potentially wrong comedi struct preselected by the
core, although it cannot be used safely anyway until overwritten after the
manual probe is done.

Having had a short look over the comedi code I was under the impression that
the change would make the 4 affected functions consistent to the other parts
that seemingly follow the skeleton.

	static int skel_attach(struct comedi_device *dev, struct comedi_devconfig *it)
	{
		const struct skel_board *thisboard;
		struct skel_private *devpriv;

		/*
		* If you can probe the device to determine what device in a series
		* it is, this is the place to do it.  Otherwise, dev->board_ptr
		* should already be initialized.
		*/
		/* dev->board_ptr = skel_probe(dev, it); */

		thisboard = comedi_board(dev);

> These probe functions need to be looked at to see if they are actually needed.
> For now I would prefer that the existing code stay as-is.

That added about the intention of the patch, I'm fine if You want to question
the necessity of the probes as a whole and keep the legacy code meanwhile
untouched.

Regards,
Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]