SPI ad7739 driver

[Alexander Pazdnikov]

Hello.

Here is a driver for an SPI A/D converter - ad7739

1. Do anybody has some time to make a breif review, please ?
2. Is it possible to add this driver to comedi mainstream ?

Thank you.

[Ian Abbott]

Here's a very brief review:

The coding style needs to be cleaned up to conform to Linux kernel
standards: see Documentation/CodingStyle in the Linux sources.

If request_irq() fails and you return -EBUSY, your ad7739_detach routine
will be called anyway. As devpriv(dev)->spi is still non-NULL at this
point, it will try and free the IRQ which it never successfully
requested, and call put_device() without a matching get_device(). Also,
it would be better if the request_irq() was done *after* resetting the
chip (and preferably after setting up the subdevices). You should
record whether the request_irq() call was successful so that your detect
routine knows whether to free it. (Most comedi drivers just set
dev->irq to the IRQ value, but that assumes IRQ 0 is invalid. If IRQ 0
could be valid on your platform, it's better to set something in your
private data to indicate if request_irq() was successful.)

While on the subject of ad7739_detach(), devpriv(dev) might be NULL at
this point, so devpriv(dev)->spi might be an invalid NULL dereference.
You need to check that devpriv(dev) is valid before checking
devpriv(dev)->spi.

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

[Alexander Pazdnikov]

В Mon, 27 Jun 2011 14:01:55 +0100
Ian Abbott <abb...@mev.co.uk> пишет:

Thank you Ian
Go to work on errors :-)

--
Best Regards

[Alexander Pazdnikov]

В Mon, 27 Jun 2011 14:01:55 +0100
Ian Abbott <abb...@mev.co.uk> пишет:

"detach" function will be executed after "attach" function etheir on error and good result of "attach" ?

Do understand right ?

--
Best regards

[Ian Abbott]

That's correct. The "detach" function is always called even if the
"attach" function fails. (This is quite an unusual way of doing things
as the usual convention is for functions to clean up after themselves on
failure. Nevertheless, that's the way comedi does it. I think the
Linux serial driver did something similar in the dim and distant past,
but it doesn't any more.)

[Alexander Pazdnikov]

Fixed:
1. coding style
2. irq leak on errors.

--
Best regards

[Ian Abbott]

On 05/07/11 05:18, Alexander Pazdnikov wrote:
> Fixed:
> 1. coding style
> 2. irq leak on errors.

That's a lot better, but the Linux kernel folks don't like //-style
comments.

This part of the attach routine looks wrong:

> /* hold device */
> get_device(&priv->spi->dev);
>
> priv->spi = to_spi_device(d);

Shouldn't those lines be the other way round?

It would be useful if the driver source had a Comedi driver description
block like most of the other Comedi drivers. See
drivers/staging/comedi/drivers/skel.c for a description of the fields in
this block and the other drivers for numerous examples.

When you're ready to submit the driver for inclusion in the kernel,
you'll need to submit it as a patch (preferably using git send-email)
against the current kernel sources. This should include the necessary
Makefile and Kconfig changes and should have a "Signed-off-by:". The
place to submit the patch is:

To: <de...@linuxdriverproject.org>
Cc: Greg Kroah-Hartman <gre...@suse.de>
Cc: Frank Mori Hess <fmh...@users.sourceforge.net>
Cc: Ian Abbott <abb...@mev.co.uk>

See the Linux Driver Project email archive at
<http://dir.gmane.org/gmane.linux.drivers.driver-project.devel> to get a
feel for how patches should be submitted.

Good luck!

[Alexander Pazdnikov]

> That's a lot better, but the Linux kernel folks don't like //-style
> comments.

Thank you, going to fix it.

> This part of the attach routine looks wrong:
>
> > /* hold device */
> > get_device(&priv->spi->dev);
> >
> > priv->spi = to_spi_device(d);
>
> Shouldn't those lines be the other way round?

Ooooh no, what a shame on me.
Thank you very much. I didn't test last version.
Thank you for the lesson, I'll try my best not to hurry up next time.

>
> It would be useful if the driver source had a Comedi driver description
> block like most of the other Comedi drivers. See
> drivers/staging/comedi/drivers/skel.c for a description of the fields in
> this block and the other drivers for numerous examples.

Thank you, going to fix it.

>
> When you're ready to submit the driver for inclusion in the kernel,
> you'll need to submit it as a patch (preferably using git send-email)
> against the current kernel sources. This should include the necessary
> Makefile and Kconfig changes and should have a "Signed-off-by:". The
> place to submit the patch is:
>
> To: <de...@linuxdriverproject.org>
> Cc: Greg Kroah-Hartman <gre...@suse.de>
> Cc: Frank Mori Hess <fmh...@users.sourceforge.net>
> Cc: Ian Abbott <abb...@mev.co.uk>
>
> See the Linux Driver Project email archive at
> <http://dir.gmane.org/gmane.linux.drivers.driver-project.devel> to get a
> feel for how patches should be submitted.
>
> Good luck!

Thank you very much for your help and time.
Good luck to you too! :-)