[PATCH 1/4] staging: iio: ad5933: rename ext_clk_Hz -> ext_clk_hz

[PATCH 1/4] staging: iio: ad5933: rename ext_clk_Hz -> ext_clk_hz

[Alexandru Ardelean]

The checkpatch script doesn't like camel-case names.
Renamed the variable.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/staging/iio/impedance-analyzer/ad5933.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index a2370dd1e1a8..8a920f675b83 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -84,13 +84,13 @@
 
 /**
  * struct ad5933_platform_data - platform specific data
- * @ext_clk_Hz:		the external clock frequency in Hz, if not set
+ * @ext_clk_hz:		the external clock frequency in Hz, if not set
  *			the driver uses the internal clock (16.776 MHz)
  * @vref_mv:		the external reference voltage in millivolt
  */
 
 struct ad5933_platform_data {
-	unsigned long			ext_clk_Hz;
+	unsigned long			ext_clk_hz;
 	unsigned short			vref_mv;
 };
 
@@ -726,8 +726,8 @@ static int ad5933_probe(struct i2c_client *client,
 	else
 		st->vref_mv = pdata->vref_mv;
 
-	if (pdata->ext_clk_Hz) {
-		st->mclk_hz = pdata->ext_clk_Hz;
+	if (pdata->ext_clk_hz) {
+		st->mclk_hz = pdata->ext_clk_hz;
 		st->ctrl_lb = AD5933_CTRL_EXT_SYSCLK;
 	} else {
 		st->mclk_hz = AD5933_INT_OSC_FREQ_Hz;
-- 
2.17.1

[PATCH 2/4] staging: iio: ad5933: Add regulator binding for vref

[Alexandru Ardelean]

From: Dragos Bogdan <dragos.bogdan@analog.com>

In order to add full device tree support, add the option of specifying
the vref using the regulator framework.

Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/staging/iio/impedance-analyzer/ad5933.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 8a920f675b83..ec54ab4471b9 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -97,6 +97,7 @@ struct ad5933_platform_data {
 struct ad5933_state {
 	struct i2c_client		*client;
 	struct regulator		*reg;
+	struct regulator		*vref;
 	struct delayed_work		work;
 	struct mutex			lock; /* Protect sensor state */
 	unsigned long			mclk_hz;
@@ -721,6 +722,14 @@ static int ad5933_probe(struct i2c_client *client,
 	}
 	voltage_uv = regulator_get_voltage(st->reg);
 
+	st->reg = devm_regulator_get(&client->dev, "vref");
+	if (!IS_ERR(st->vref)) {
+		ret = regulator_enable(st->vref);
+		if (ret)
+			goto error_disable_reg;
+		pdata->vref_mv = regulator_get_voltage(st->vref) / 1000;
+	}
+
 	if (voltage_uv)
 		st->vref_mv = voltage_uv / 1000;
 	else
@@ -747,7 +756,7 @@ static int ad5933_probe(struct i2c_client *client,
 
 	ret = ad5933_register_ring_funcs_and_init(indio_dev);
 	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
 	ret = ad5933_setup(st);
 	if (ret)
@@ -761,6 +770,8 @@ static int ad5933_probe(struct i2c_client *client,
 
 error_unreg_ring:
 	iio_kfifo_free(indio_dev->buffer);
+error_disable_vref:
+	regulator_disable(st->vref);
 error_disable_reg:
 	regulator_disable(st->reg);
 
@@ -775,6 +786,7 @@ static int ad5933_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 	iio_kfifo_free(indio_dev->buffer);
 	regulator_disable(st->reg);
+	regulator_disable(st->vref);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 2/4] staging: iio: ad5933: Add regulator binding for vref

[Jonathan Cameron]

On Tue, 2 Oct 2018 14:57:58 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Dragos Bogdan <dragos.bogdan@analog.com>
> 
> In order to add full device tree support, add the option of specifying
> the vref using the regulator framework.
> 
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 8a920f675b83..ec54ab4471b9 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -97,6 +97,7 @@ struct ad5933_platform_data {
>  struct ad5933_state {
>  	struct i2c_client		*client;
>  	struct regulator		*reg;
> +	struct regulator		*vref;
>  	struct delayed_work		work;
>  	struct mutex			lock; /* Protect sensor state */
>  	unsigned long			mclk_hz;
> @@ -721,6 +722,14 @@ static int ad5933_probe(struct i2c_client *client,
>  	}
>  	voltage_uv = regulator_get_voltage(st->reg);
>  
> +	st->reg = devm_regulator_get(&client->dev, "vref");

You want devm_regulator_get_optional here I think.

Otherwise you'll get a stub regulator if one isn't specified in
the device tree.


Also, I'm confused, you have an existing regulator which you
override with a second one?  Please provide some more details of
this..

> +	if (!IS_ERR(st->vref)) {
> +		ret = regulator_enable(st->vref);
> +		if (ret)
> +			goto error_disable_reg;
> +		pdata->vref_mv = regulator_get_voltage(st->vref) / 1000;
> +	}
> +
>  	if (voltage_uv)
>  		st->vref_mv = voltage_uv / 1000;
>  	else
> @@ -747,7 +756,7 @@ static int ad5933_probe(struct i2c_client *client,
>  
>  	ret = ad5933_register_ring_funcs_and_init(indio_dev);
>  	if (ret)
> -		goto error_disable_reg;
> +		goto error_disable_vref;
>  
>  	ret = ad5933_setup(st);
>  	if (ret)
> @@ -761,6 +770,8 @@ static int ad5933_probe(struct i2c_client *client,
>  
>  error_unreg_ring:
>  	iio_kfifo_free(indio_dev->buffer);
> +error_disable_vref:
> +	regulator_disable(st->vref);
>  error_disable_reg:
>  	regulator_disable(st->reg);
>  
> @@ -775,6 +786,7 @@ static int ad5933_remove(struct i2c_client *client)
>  	iio_device_unregister(indio_dev);
>  	iio_kfifo_free(indio_dev->buffer);
>  	regulator_disable(st->reg);
> +	regulator_disable(st->vref);
>  
>  	return 0;
>  }

[PATCH 3/4] staging: iio: ad5933: Use the clock framework for the external clock

[Alexandru Ardelean]

From: Dragos Bogdan <dragos.bogdan@analog.com>

In order to add full device tree support, add the option of specifying
the external clock (MCLK) using the clock framework.

Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../staging/iio/impedance-analyzer/ad5933.c   | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index ec54ab4471b9..04208a1df48e 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/clk.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -98,6 +99,7 @@ struct ad5933_state {
 	struct i2c_client		*client;
 	struct regulator		*reg;
 	struct regulator		*vref;
+	struct clk			*mclk;
 	struct delayed_work		work;
 	struct mutex			lock; /* Protect sensor state */
 	unsigned long			mclk_hz;
@@ -735,6 +737,19 @@ static int ad5933_probe(struct i2c_client *client,
 	else
 		st->vref_mv = pdata->vref_mv;
 
+	st->mclk = devm_clk_get(&client->dev, "mclk");
+	if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
+		ret = PTR_ERR(st->mclk);
+		goto error_disable_vref;
+	}
+
+	if (!IS_ERR(st->mclk)) {
+		ret = clk_prepare_enable(st->mclk);
+		if (ret < 0)
+			goto error_disable_vref;
+		pdata->ext_clk_hz = clk_get_rate(st->mclk);
+	}
+
 	if (pdata->ext_clk_hz) {
 		st->mclk_hz = pdata->ext_clk_hz;
 		st->ctrl_lb = AD5933_CTRL_EXT_SYSCLK;
@@ -756,7 +771,7 @@ static int ad5933_probe(struct i2c_client *client,
 
 	ret = ad5933_register_ring_funcs_and_init(indio_dev);
 	if (ret)
-		goto error_disable_vref;
+		goto error_disable_mclk;
 
 	ret = ad5933_setup(st);
 	if (ret)
@@ -770,6 +785,8 @@ static int ad5933_probe(struct i2c_client *client,
 
 error_unreg_ring:
 	iio_kfifo_free(indio_dev->buffer);
+error_disable_mclk:
+	clk_disable_unprepare(st->mclk);
 error_disable_vref:
 	regulator_disable(st->vref);
 error_disable_reg:
@@ -787,6 +804,7 @@ static int ad5933_remove(struct i2c_client *client)
 	iio_kfifo_free(indio_dev->buffer);
 	regulator_disable(st->reg);
 	regulator_disable(st->vref);
+	clk_disable_unprepare(st->mclk);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 3/4] staging: iio: ad5933: Use the clock framework for the external clock

[Jonathan Cameron]

On Tue, 2 Oct 2018 14:57:59 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Dragos Bogdan <dragos.bogdan@analog.com>
> 
> In order to add full device tree support, add the option of specifying
> the external clock (MCLK) using the clock framework.
> 
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

This looks fine, but I would like to know if the intent is to get
rid of the platform data support entirely once we have DT able to
do everything. Do we still have usecases for platform data with this
chip that can't be converted over?

Thanks,

Jonathan

> ---
>  .../staging/iio/impedance-analyzer/ad5933.c   | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index ec54ab4471b9..04208a1df48e 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -16,6 +16,7 @@
>  #include <linux/err.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
> +#include <linux/clk.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -98,6 +99,7 @@ struct ad5933_state {
>  	struct i2c_client		*client;
>  	struct regulator		*reg;
>  	struct regulator		*vref;
> +	struct clk			*mclk;
>  	struct delayed_work		work;
>  	struct mutex			lock; /* Protect sensor state */
>  	unsigned long			mclk_hz;
> @@ -735,6 +737,19 @@ static int ad5933_probe(struct i2c_client *client,
>  	else
>  		st->vref_mv = pdata->vref_mv;
>  
> +	st->mclk = devm_clk_get(&client->dev, "mclk");
> +	if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
> +		ret = PTR_ERR(st->mclk);
> +		goto error_disable_vref;
> +	}
> +
> +	if (!IS_ERR(st->mclk)) {
> +		ret = clk_prepare_enable(st->mclk);
> +		if (ret < 0)
> +			goto error_disable_vref;
> +		pdata->ext_clk_hz = clk_get_rate(st->mclk);
> +	}
> +
>  	if (pdata->ext_clk_hz) {
>  		st->mclk_hz = pdata->ext_clk_hz;
>  		st->ctrl_lb = AD5933_CTRL_EXT_SYSCLK;
> @@ -756,7 +771,7 @@ static int ad5933_probe(struct i2c_client *client,
>  
>  	ret = ad5933_register_ring_funcs_and_init(indio_dev);
>  	if (ret)
> -		goto error_disable_vref;
> +		goto error_disable_mclk;
>  
>  	ret = ad5933_setup(st);
>  	if (ret)
> @@ -770,6 +785,8 @@ static int ad5933_probe(struct i2c_client *client,
>  
>  error_unreg_ring:
>  	iio_kfifo_free(indio_dev->buffer);
> +error_disable_mclk:
> +	clk_disable_unprepare(st->mclk);
>  error_disable_vref:
>  	regulator_disable(st->vref);
>  error_disable_reg:
> @@ -787,6 +804,7 @@ static int ad5933_remove(struct i2c_client *client)
>  	iio_kfifo_free(indio_dev->buffer);
>  	regulator_disable(st->reg);
>  	regulator_disable(st->vref);
> +	clk_disable_unprepare(st->mclk);
>  
>  	return 0;
>  }

[PATCH 4/4] staging: iio: ad5933: Add of_device_id table

[Alexandru Ardelean]

From: Dragos Bogdan <dragos.bogdan@analog.com>

As part of finalizing the device-tree support for this driver, this patch
adds the of_device_id table with supported devices.

Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/staging/iio/impedance-analyzer/ad5933.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 04208a1df48e..ab8b93fd39ed 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -809,6 +809,14 @@ static int ad5933_remove(struct i2c_client *client)
 	return 0;
 }
 
+static const struct of_device_id of_ad5933_match[] = {
+	{ .compatible = "adi,ad5933", 0},
+	{ .compatible = "adi,ad5934", 0},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_ad5933_match);
+
 static const struct i2c_device_id ad5933_id[] = {
 	{ "ad5933", 0 },
 	{ "ad5934", 0 },
-- 
2.17.1

Re: [PATCH 4/4] staging: iio: ad5933: Add of_device_id table

[Jonathan Cameron]

On Tue, 2 Oct 2018 14:58:00 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Dragos Bogdan <dragos.bogdan@analog.com>
> 
> As part of finalizing the device-tree support for this driver, this patch
> adds the of_device_id table with supported devices.
> 
> Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 04208a1df48e..ab8b93fd39ed 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -809,6 +809,14 @@ static int ad5933_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static const struct of_device_id of_ad5933_match[] = {
> +	{ .compatible = "adi,ad5933", 0},
> +	{ .compatible = "adi,ad5934", 0},
> +	{},
> +};
I would expect this to also be assigned to the driver of_match_table.

I know it'll pick it up via the device_id if not, but it's not how
it is ultimately intended to work.

Jonathan
> +
> +MODULE_DEVICE_TABLE(of, of_ad5933_match);
> +
>  static const struct i2c_device_id ad5933_id[] = {
>  	{ "ad5933", 0 },
>  	{ "ad5934", 0 },

Re: [PATCH 1/4] staging: iio: ad5933: rename ext_clk_Hz -> ext_clk_hz

[Jonathan Cameron]

On Tue, 2 Oct 2018 14:57:57 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The checkpatch script doesn't like camel-case names.
> Renamed the variable.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

I did briefly wonder we had units of pounds for the ctrl value
below it ;)

Jonathan

> ---
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index a2370dd1e1a8..8a920f675b83 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -84,13 +84,13 @@
>  
>  /**
>   * struct ad5933_platform_data - platform specific data
> - * @ext_clk_Hz:		the external clock frequency in Hz, if not set
> + * @ext_clk_hz:		the external clock frequency in Hz, if not set
>   *			the driver uses the internal clock (16.776 MHz)
>   * @vref_mv:		the external reference voltage in millivolt
>   */
>  
>  struct ad5933_platform_data {
> -	unsigned long			ext_clk_Hz;
> +	unsigned long			ext_clk_hz;
>  	unsigned short			vref_mv;
>  };
>  
> @@ -726,8 +726,8 @@ static int ad5933_probe(struct i2c_client *client,
>  	else
>  		st->vref_mv = pdata->vref_mv;
>  
> -	if (pdata->ext_clk_Hz) {
> -		st->mclk_hz = pdata->ext_clk_Hz;
> +	if (pdata->ext_clk_hz) {
> +		st->mclk_hz = pdata->ext_clk_hz;
>  		st->ctrl_lb = AD5933_CTRL_EXT_SYSCLK;
>  	} else {
>  		st->mclk_hz = AD5933_INT_OSC_FREQ_Hz;