Re: [Device-drivers-devel] [PATCH] iio: dac - split ad5440 into common, i2c and spi modules

Re: [Device-drivers-devel] [PATCH] iio: dac - split ad5440 into common, i2c and spi modules

[Lars-Peter Clausen]

On 11/10/2011 05:07 PM, Jean-Fran=E7ois Dagenais wrote:
> v1: first proposal, works with i2c AD5622
>=20

Hi,

Thanks for your patch. I've recently send a patch which converts the ad=
5446
to channel_spec, it would be good if you could rebase v2 of this patch =
ontop
of it. Unfortunately the patch isn't in GregKH tree yet, but you can gr=
ab it
from the mailing list archives:
http://marc.info/?l=3Dlinux-iio&m=3D131914551420107&q=3Draw

Some further comments inline.

> The patch also contains some cleanup of defines that were not
> used.

Cleanups should go in a separate patch. Makes it easier to review them.

> ---
>  drivers/staging/iio/dac/Kconfig      |   27 +++-
>  drivers/staging/iio/dac/Makefile     |    2 +
>  drivers/staging/iio/dac/ad5446-i2c.c |  135 ++++++++++++++++++
>  drivers/staging/iio/dac/ad5446-spi.c |  259 ++++++++++++++++++++++++=
++++++++++
>  drivers/staging/iio/dac/ad5446.c     |  258 +++++++-----------------=
----------
>  drivers/staging/iio/dac/ad5446.h     |   93 +++++-------
>  6 files changed, 510 insertions(+), 264 deletions(-)
>  create mode 100644 drivers/staging/iio/dac/ad5446-i2c.c
>  create mode 100644 drivers/staging/iio/dac/ad5446-spi.c
>=20
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/da=
c/Kconfig
> index 7ddae35..bb960b5 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -11,16 +11,37 @@ config AD5624R_SPI
>  	  AD5664R convertors (DAC). This driver uses the common SPI interfa=
ce.
> =20
>  config AD5446
> -	tristate "Analog Devices AD5444/6, AD5620/40/60 and AD5542A/12A DAC=
 SPI driver"
> -	depends on SPI
> +	tristate "Analog Devices AD5444/6, AD5620/40/60/02/12/22 and AD5542=
A/12A DAC SPI/I2C driver"
>  	help
>  	  Say yes here to build support for Analog Devices AD5444, AD5446,
>  	  AD5512A, AD5542A, AD5543, AD5553, AD5601, AD5611, AD5620, AD5621,
> -	  AD5640, AD5660 DACs.
> +	  AD5640, AD5660, AD5601, AD5612, AD5622 DACs.
> =20
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5446.
> =20
> +config AD5446_SPI
> +	tristate "support SPI bus connection"
> +	depends on AD5446 && SPI
> +	default y
> +	help
> +	  Say Y here if you have AD5444/6, AD5620/40/60 and AD5542A/12A
> +	  DAC connected to a SPI bus.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5446-spi.
> +
> +config AD5446_I2C
> +	tristate "support I2C bus connection"
> +	depends on AD5446 && I2C
> +	default y
> +	help
> +	  Say Y here if you have AD5602/12/22 DAC connected to an I2C bus.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5446-i2c.
> +
> +

I don't think we need to split this into two separate modules. Having o=
ne
handling both should be fine. The I2C and SPI specific sections should =
be
rather small and can be put in a #ifdef CONFIG_... block.

>  config AD5504
>  	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>  	depends on SPI
> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/d=
ac/Makefile
> index 7f4f2ed..481a1e9 100644
> --- a/drivers/staging/iio/dac/Makefile
> +++ b/drivers/staging/iio/dac/Makefile
> @@ -5,6 +5,8 @@
>  obj-$(CONFIG_AD5624R_SPI) +=3D ad5624r_spi.o
>  obj-$(CONFIG_AD5504) +=3D ad5504.o
>  obj-$(CONFIG_AD5446) +=3D ad5446.o
> +obj-$(CONFIG_AD5446_I2C) +=3D ad5446-i2c.o
> +obj-$(CONFIG_AD5446_SPI) +=3D ad5446-spi.o
>  obj-$(CONFIG_AD5791) +=3D ad5791.o
>  obj-$(CONFIG_AD5686) +=3D ad5686.o
>  obj-$(CONFIG_MAX517) +=3D max517.o
> diff --git a/drivers/staging/iio/dac/ad5446-i2c.c b/drivers/staging/i=
io/dac/ad5446-i2c.c
> new file mode 100644
> index 0000000..cee606e
> --- /dev/null
> +++ b/drivers/staging/iio/dac/ad5446-i2c.c
> @@ -0,0 +1,135 @@
> +/*
> + * AD5446 DAC driver SPI specific
> + *
> + * Copyright 2010 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +
> +#include "../iio.h"
> +
> +#include "ad5446.h"
> +
> +static int ad5446_i2c_sync_to_chip(struct ad5446_state *st)
> +{
> +	struct i2c_client *client =3D to_i2c_client(st->dev);
> +	int error =3D i2c_transfer(client->adapter, &st->proto.i2c.msg, 1);
> +	if (unlikely(error < 0)) {
> +		dev_err(st->dev, "I2C write error: %d\n", error);
> +		return error;
> +	}

Just use i2c_master_send.

> +
> +	return 0;
> +}
> +

[...]

> @@ -52,8 +37,19 @@ struct ad5446_state {
>  	unsigned			cached_val;
>  	unsigned			pwr_down_mode;
>  	unsigned			pwr_down;
> -	struct spi_transfer		xfer;
> -	struct spi_message		msg;
> +	union {
> +#ifdef __enabled_CONFIG_AD5446_SPI

Uhm, I think the __enabled_ defines were not intended to be used direct=
ly.
You should rather use the IS_ENABLED macro instead.

> +		struct {
> +			struct spi_transfer		xfer;
> +			struct spi_message		msg;
> +		} spi;
> +#endif
> +#ifdef __enabled_CONFIG_AD5446_I2C
> +		struct {
> +			struct i2c_msg msg;
> +		} i2c;
> +#endif
> +	} proto;
>  	union {
>  		unsigned short		d16;
>  		unsigned char		d24[3];
> @@ -65,7 +61,7 @@ struct ad5446_state {
>   * @bits:		accuracy of the DAC in bits
>   * @storagebits:	number of bits written to the DAC
>   * @left_shift:		number of bits the datum must be shifted
> - * @int_vref_mv:	AD5620/40/60: the internal reference voltage
> + * @int_vref_mv:	AD5620/40/60: the internal reference voltage, 0 mea=
ns vcc ref
>   * @store_sample:	chip specific helper function to store the datum
>   * @store_sample:	chip specific helper function to store the powerpo=
wn cmd
>   */
> @@ -77,33 +73,22 @@ struct ad5446_chip_info {
>  	u16			int_vref_mv;
>  	void (*store_sample)	(struct ad5446_state *st, unsigned val);
>  	void (*store_pwr_down)	(struct ad5446_state *st, unsigned mode);
> +	int  (*sync_to_chip) (struct ad5446_state *st);

I would put the sync callback into the ad5446_state struct and initiali=
ze it
in the I2C and SPI probe function. No need to store this information on=
 a
per chip basis. And maybe call it "write" or something instead.

[...]

>  #endif /* IIO_DAC_AD5446_H_ */

Re: [Device-drivers-devel] [PATCH] iio: dac - split ad5440 into common, i2c and spi modules

[Lars-Peter Clausen]

On 11/10/2011 06:34 PM, Jean-Francois Dagenais wrote:
>=20
> On Nov 10, 2011, at 11:56, Lars-Peter Clausen wrote:
>=20
>> On 11/10/2011 05:07 PM, Jean-Fran=E7ois Dagenais wrote:
>>> v1: first proposal, works with i2c AD5622
>>>
>>
>> Hi,
>>
>> Thanks for your patch. I've recently send a patch which converts the=
 ad5446
>> to channel_spec, it would be good if you could rebase v2 of this pat=
ch ontop
>> of it. Unfortunately the patch isn't in GregKH tree yet, but you can=
 grab it
>> from the mailing list archives:
>> http://marc.info/?l=3Dlinux-iio&m=3D131914551420107&q=3Draw
> I am having trouble applying the patch, to you have a tree point (mai=
nline or elsewhere) I should apply the patch onto?
> Trying to be autonomous I have started digging my mailbox for "ad5446=
=2Ec" and found many patches affecting it recently. Starting from
> 3.1, should I do a patch-series with your patch ending the series may=
be?

Hm, looks like there are a few patches which were added in between 3.1 =
and my
patch. If you have a up-to-date linux git tree the following command sh=
ould
list you the missing commits: `git log v3.1..v3.2-rc1
drivers/staging/iio/dac/ad5446.c`

With those applied my patch applies fine ontop. But when sending patche=
s it is
always a good idea to base them on the subsystem maintainers developmen=
t tree.
=46or staging this is GregKH staging-next branch:
http://git.kernel.org/?p=3Dlinux/kernel/git/gregkh/staging.git;a=3Dshor=
tlog;h=3Drefs/heads/staging-next

>=20
>>
>> Some further comments inline.
>>
>>> The patch also contains some cleanup of defines that were not
>>> used.
>>
>> Cleanups should go in a separate patch. Makes it easier to review th=
em.
>>
>>> ---
>>> drivers/staging/iio/dac/Kconfig      |   27 +++-
>>> drivers/staging/iio/dac/Makefile     |    2 +
>>> drivers/staging/iio/dac/ad5446-i2c.c |  135 ++++++++++++++++++
>>> drivers/staging/iio/dac/ad5446-spi.c |  259 +++++++++++++++++++++++=
+++++++++++
>>> drivers/staging/iio/dac/ad5446.c     |  258 +++++++----------------=
-----------
>>> drivers/staging/iio/dac/ad5446.h     |   93 +++++-------
>>> 6 files changed, 510 insertions(+), 264 deletions(-)
>>> create mode 100644 drivers/staging/iio/dac/ad5446-i2c.c
>>> create mode 100644 drivers/staging/iio/dac/ad5446-spi.c
>>>
>>> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/=
dac/Kconfig
>>> index 7ddae35..bb960b5 100644
>>> --- a/drivers/staging/iio/dac/Kconfig
>>> +++ b/drivers/staging/iio/dac/Kconfig
>>> @@ -11,16 +11,37 @@ config AD5624R_SPI
>>> 	  AD5664R convertors (DAC). This driver uses the common SPI interf=
ace.
>>>
>>> config AD5446
>>> -	tristate "Analog Devices AD5444/6, AD5620/40/60 and AD5542A/12A D=
AC SPI driver"
>>> -	depends on SPI
>>> +	tristate "Analog Devices AD5444/6, AD5620/40/60/02/12/22 and AD55=
42A/12A DAC SPI/I2C driver"
>>> 	help
>>> 	  Say yes here to build support for Analog Devices AD5444, AD5446,
>>> 	  AD5512A, AD5542A, AD5543, AD5553, AD5601, AD5611, AD5620, AD5621=
,
>>> -	  AD5640, AD5660 DACs.
>>> +	  AD5640, AD5660, AD5601, AD5612, AD5622 DACs.
>>>
>>> 	  To compile this driver as a module, choose M here: the
>>> 	  module will be called ad5446.
>>>
>>> +config AD5446_SPI
>>> +	tristate "support SPI bus connection"
>>> +	depends on AD5446 && SPI
>>> +	default y
>>> +	help
>>> +	  Say Y here if you have AD5444/6, AD5620/40/60 and AD5542A/12A
>>> +	  DAC connected to a SPI bus.
>>> +
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called ad5446-spi.
>>> +
>>> +config AD5446_I2C
>>> +	tristate "support I2C bus connection"
>>> +	depends on AD5446 && I2C
>>> +	default y
>>> +	help
>>> +	  Say Y here if you have AD5602/12/22 DAC connected to an I2C bus=
=2E
>>> +
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called ad5446-i2c.
>>> +
>>> +
>>
>> I don't think we need to split this into two separate modules. Havin=
g one
>> handling both should be fine. The I2C and SPI specific sections shou=
ld be
>> rather small and can be put in a #ifdef CONFIG_... block.
> Yeah, it seemed cleaner, but a bit overkill. I was mimicking the ad71=
4x-i2c/spi strategy.
>>
>>> config AD5504
>>> 	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>>> 	depends on SPI
>>> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio=
/dac/Makefile
>>> index 7f4f2ed..481a1e9 100644
>>> --- a/drivers/staging/iio/dac/Makefile
>>> +++ b/drivers/staging/iio/dac/Makefile
>>> @@ -5,6 +5,8 @@
>>> obj-$(CONFIG_AD5624R_SPI) +=3D ad5624r_spi.o
>>> obj-$(CONFIG_AD5504) +=3D ad5504.o
>>> obj-$(CONFIG_AD5446) +=3D ad5446.o
>>> +obj-$(CONFIG_AD5446_I2C) +=3D ad5446-i2c.o
>>> +obj-$(CONFIG_AD5446_SPI) +=3D ad5446-spi.o
>>> obj-$(CONFIG_AD5791) +=3D ad5791.o
>>> obj-$(CONFIG_AD5686) +=3D ad5686.o
>>> obj-$(CONFIG_MAX517) +=3D max517.o
>>> diff --git a/drivers/staging/iio/dac/ad5446-i2c.c b/drivers/staging=
/iio/dac/ad5446-i2c.c
>>> new file mode 100644
>>> index 0000000..cee606e
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/dac/ad5446-i2c.c
>>> @@ -0,0 +1,135 @@
>>> +/*
>>> + * AD5446 DAC driver SPI specific
>>> + *
>>> + * Copyright 2010 Analog Devices Inc.
>>> + *
>>> + * Licensed under the GPL-2 or later.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/i2c.h>
>>> +
>>> +#include "../iio.h"
>>> +
>>> +#include "ad5446.h"
>>> +
>>> +static int ad5446_i2c_sync_to_chip(struct ad5446_state *st)
>>> +{
>>> +	struct i2c_client *client =3D to_i2c_client(st->dev);
>>> +	int error =3D i2c_transfer(client->adapter, &st->proto.i2c.msg, 1=
);
>>> +	if (unlikely(error < 0)) {
>>> +		dev_err(st->dev, "I2C write error: %d\n", error);
>>> +		return error;
>>> +	}
>>
>> Just use i2c_master_send.
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>
>> [...]
>>
>>> @@ -52,8 +37,19 @@ struct ad5446_state {
>>> 	unsigned			cached_val;
>>> 	unsigned			pwr_down_mode;
>>> 	unsigned			pwr_down;
>>> -	struct spi_transfer		xfer;
>>> -	struct spi_message		msg;
>>> +	union {
>>> +#ifdef __enabled_CONFIG_AD5446_SPI
>>
>> Uhm, I think the __enabled_ defines were not intended to be used dir=
ectly.
>> You should rather use the IS_ENABLED macro instead.
>>
>>> +		struct {
>>> +			struct spi_transfer		xfer;
>>> +			struct spi_message		msg;
>>> +		} spi;
>>> +#endif
>>> +#ifdef __enabled_CONFIG_AD5446_I2C
>>> +		struct {
>>> +			struct i2c_msg msg;
>>> +		} i2c;
>>> +#endif
>>> +	} proto;
>>> 	union {
>>> 		unsigned short		d16;
>>> 		unsigned char		d24[3];
>>> @@ -65,7 +61,7 @@ struct ad5446_state {
>>>  * @bits:		accuracy of the DAC in bits
>>>  * @storagebits:	number of bits written to the DAC
>>>  * @left_shift:		number of bits the datum must be shifted
>>> - * @int_vref_mv:	AD5620/40/60: the internal reference voltage
>>> + * @int_vref_mv:	AD5620/40/60: the internal reference voltage, 0 m=
eans vcc ref
>>>  * @store_sample:	chip specific helper function to store the datum
>>>  * @store_sample:	chip specific helper function to store the powerp=
own cmd
>>>  */
>>> @@ -77,33 +73,22 @@ struct ad5446_chip_info {
>>> 	u16			int_vref_mv;
>>> 	void (*store_sample)	(struct ad5446_state *st, unsigned val);
>>> 	void (*store_pwr_down)	(struct ad5446_state *st, unsigned mode);
>>> +	int  (*sync_to_chip) (struct ad5446_state *st);
>>
>> I would put the sync callback into the ad5446_state struct and initi=
alize it
>> in the I2C and SPI probe function. No need to store this information=
 on a
>> per chip basis. And maybe call it "write" or something instead.
>>
>> [...]
>>
>>> #endif /* IIO_DAC_AD5446_H_ */
>>
>=20
> As a general question, while digging, I found concerning mail about r=
andom kernel segfaults. I know staging gives out this big warning, but =
I thought I would be safe using a simple driver like the ad5446 for pro=
duction. Am I mistaken. I mean the chip is so dumb simple, I am now con=
sidering using i2cset from userspace instead since I will not really be=
 using the whole iio framework.
>

The IIO framework should be quite stable by now. So you shouldn't reall=
y get
any random segfaults by using it. To which mail are you referring to?

- Lars