linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions
@ 2020-06-23 23:30 Heiko Stuebner
  2020-06-23 23:30 ` [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant Heiko Stuebner
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Heiko Stuebner @ 2020-06-23 23:30 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, heiko, linux-iio, linux-kernel,
	linux-rockchip, xxm, Heiko Stuebner

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Parts of the saradc probe rely on devm functions and later parts do not.
This makes it more difficult to for example enable triggers via their
devm-functions and would need more undo-work in remove.

So to make life easier for the driver, move the rest of probe calls
also to their devm-equivalents.

This includes moving the clk- and regulator-disabling to a devm_action
so that they gets disabled both during remove and in the error case
in probe, after the action is registered.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v6:
- move devm actions into separate functions as suggested by Jonathan
changes in v5:
- none
changes in v4:
- new patch as suggested by Jonathan

 drivers/iio/adc/rockchip_saradc.c | 72 ++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 582ba047c4a6..1a7990d60f9f 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -193,6 +193,27 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
 	reset_control_deassert(reset);
 }
 
+static void rockchip_saradc_clk_disable(void *data)
+{
+	struct rockchip_saradc *info = data;
+
+	clk_disable_unprepare(info->clk);
+}
+
+static void rockchip_saradc_pclk_disable(void *data)
+{
+	struct rockchip_saradc *info = data;
+
+	clk_disable_unprepare(info->pclk);
+}
+
+static void rockchip_saradc_regulator_disable(void *data)
+{
+	struct rockchip_saradc *info = data;
+
+	regulator_disable(info->vref);
+}
+
 static int rockchip_saradc_probe(struct platform_device *pdev)
 {
 	struct rockchip_saradc *info = NULL;
@@ -291,17 +312,38 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to enable vref regulator\n");
 		return ret;
 	}
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rockchip_saradc_regulator_disable, info);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register devm action, %d\n",
+			ret);
+		return ret;
+	}
 
 	ret = clk_prepare_enable(info->pclk);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to enable pclk\n");
-		goto err_reg_voltage;
+		return ret;
+	}
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rockchip_saradc_pclk_disable, info);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register devm action, %d\n",
+			ret);
+		return ret;
 	}
 
 	ret = clk_prepare_enable(info->clk);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to enable converter clock\n");
-		goto err_pclk;
+		return ret;
+	}
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rockchip_saradc_clk_disable, info);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register devm action, %d\n",
+			ret);
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, indio_dev);
@@ -315,30 +357,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	indio_dev->channels = info->data->channels;
 	indio_dev->num_channels = info->data->num_channels;
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret)
-		goto err_clk;
-
-	return 0;
-
-err_clk:
-	clk_disable_unprepare(info->clk);
-err_pclk:
-	clk_disable_unprepare(info->pclk);
-err_reg_voltage:
-	regulator_disable(info->vref);
-	return ret;
-}
-
-static int rockchip_saradc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct rockchip_saradc *info = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	clk_disable_unprepare(info->clk);
-	clk_disable_unprepare(info->pclk);
-	regulator_disable(info->vref);
+		return ret;
 
 	return 0;
 }
@@ -383,7 +404,6 @@ static SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
 
 static struct platform_driver rockchip_saradc_driver = {
 	.probe		= rockchip_saradc_probe,
-	.remove		= rockchip_saradc_remove,
 	.driver		= {
 		.name	= "rockchip-saradc",
 		.of_match_table = rockchip_saradc_match,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant
  2020-06-23 23:30 [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Heiko Stuebner
@ 2020-06-23 23:30 ` Heiko Stuebner
  2020-06-27 12:04   ` Jonathan Cameron
  2020-06-23 23:30 ` [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers Heiko Stuebner
  2020-06-27 12:03 ` [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Jonathan Cameron
  2 siblings, 1 reply; 8+ messages in thread
From: Heiko Stuebner @ 2020-06-23 23:30 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, heiko, linux-iio, linux-kernel,
	linux-rockchip, xxm, Heiko Stuebner

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

As suggested give the current ADC_CHANNEL constant a distinct
and consistent prefix.

Suggested-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v6:
- none
changes in v5:
- new patch

 drivers/iio/adc/rockchip_saradc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 1a7990d60f9f..5d2e07dc72fd 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -118,7 +118,7 @@ static const struct iio_info rockchip_saradc_iio_info = {
 	.read_raw = rockchip_saradc_read_raw,
 };
 
-#define ADC_CHANNEL(_index, _id) {				\
+#define SARADC_CHANNEL(_index, _id) {				\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
 	.channel = _index,					\
@@ -128,9 +128,9 @@ static const struct iio_info rockchip_saradc_iio_info = {
 }
 
 static const struct iio_chan_spec rockchip_saradc_iio_channels[] = {
-	ADC_CHANNEL(0, "adc0"),
-	ADC_CHANNEL(1, "adc1"),
-	ADC_CHANNEL(2, "adc2"),
+	SARADC_CHANNEL(0, "adc0"),
+	SARADC_CHANNEL(1, "adc1"),
+	SARADC_CHANNEL(2, "adc2"),
 };
 
 static const struct rockchip_saradc_data saradc_data = {
@@ -141,8 +141,8 @@ static const struct rockchip_saradc_data saradc_data = {
 };
 
 static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
-	ADC_CHANNEL(0, "adc0"),
-	ADC_CHANNEL(1, "adc1"),
+	SARADC_CHANNEL(0, "adc0"),
+	SARADC_CHANNEL(1, "adc1"),
 };
 
 static const struct rockchip_saradc_data rk3066_tsadc_data = {
@@ -153,12 +153,12 @@ static const struct rockchip_saradc_data rk3066_tsadc_data = {
 };
 
 static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
-	ADC_CHANNEL(0, "adc0"),
-	ADC_CHANNEL(1, "adc1"),
-	ADC_CHANNEL(2, "adc2"),
-	ADC_CHANNEL(3, "adc3"),
-	ADC_CHANNEL(4, "adc4"),
-	ADC_CHANNEL(5, "adc5"),
+	SARADC_CHANNEL(0, "adc0"),
+	SARADC_CHANNEL(1, "adc1"),
+	SARADC_CHANNEL(2, "adc2"),
+	SARADC_CHANNEL(3, "adc3"),
+	SARADC_CHANNEL(4, "adc4"),
+	SARADC_CHANNEL(5, "adc5"),
 };
 
 static const struct rockchip_saradc_data rk3399_saradc_data = {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers
  2020-06-23 23:30 [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Heiko Stuebner
  2020-06-23 23:30 ` [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant Heiko Stuebner
@ 2020-06-23 23:30 ` Heiko Stuebner
  2020-06-27 12:10   ` Jonathan Cameron
  2020-06-27 12:03 ` [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Jonathan Cameron
  2 siblings, 1 reply; 8+ messages in thread
From: Heiko Stuebner @ 2020-06-23 23:30 UTC (permalink / raw)
  To: jic23
  Cc: knaack.h, lars, pmeerw, heiko, linux-iio, linux-kernel,
	linux-rockchip, xxm, Heiko Stuebner

From: Simon Xue <xxm@rock-chips.com>

Add the ability to also support access via (triggered) buffers
next to the existing direct mode.

Device in question is the Odroid Go Advance that connects a joystick
to two of the saradc channels for X and Y axis and the new (and still
pending) adc joystick driver of course wants to use triggered buffers
from the iio subsystem.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
[some simplifications and added commit description]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v6:
- remove unneeded header
- remove unneeded blank line change
changes in v5:
- use IIO_CPU instead of IIO_LE as suggested by Peter
changes in v4:
- comment for the channel-num sanity check in probe
- move to struct for data+timestamp, that way we get the 8-byte
  alignment automatically - checked by comparing sizeof results
changes in v3:
- split buffer struct into values and timestamp area similar to dln2-adc
  and make sure timestamp gets 8-byte aligned - ALIGN uses 4 as it aligns
  u16 elements not bytes - hopefully I got it right this time ;-)
changes in v2:
- use devm_iio_triggered_buffer_setup
- calculate data array size from channel number (curtesy of at91-sama5d2_adc)

 drivers/iio/adc/Kconfig           |   2 +
 drivers/iio/adc/rockchip_saradc.c | 147 +++++++++++++++++++++++-------
 2 files changed, 114 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5b57437cef75..ddf830267e24 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -865,6 +865,8 @@ config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
 	depends on RESET_CONTROLLER
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for the SARADC found in SoCs from
 	  Rockchip.
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 5d2e07dc72fd..eb2222c312da 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -15,7 +15,10 @@
 #include <linux/delay.h>
 #include <linux/reset.h>
 #include <linux/regulator/consumer.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #define SARADC_DATA			0x00
 
@@ -32,9 +35,9 @@
 #define SARADC_DLY_PU_SOC_MASK		0x3f
 
 #define SARADC_TIMEOUT			msecs_to_jiffies(100)
+#define SARADC_MAX_CHANNELS		6
 
 struct rockchip_saradc_data {
-	int				num_bits;
 	const struct iio_chan_spec	*channels;
 	int				num_channels;
 	unsigned long			clk_rate;
@@ -49,8 +52,37 @@ struct rockchip_saradc {
 	struct reset_control	*reset;
 	const struct rockchip_saradc_data *data;
 	u16			last_val;
+	const struct iio_chan_spec *last_chan;
 };
 
+static void rockchip_saradc_power_down(struct rockchip_saradc *info)
+{
+	/* Clear irq & power down adc */
+	writel_relaxed(0, info->regs + SARADC_CTRL);
+}
+
+static int rockchip_saradc_conversion(struct rockchip_saradc *info,
+				   struct iio_chan_spec const *chan)
+{
+	reinit_completion(&info->completion);
+
+	/* 8 clock periods as delay between power up and start cmd */
+	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
+
+	info->last_chan = chan;
+
+	/* Select the channel to be used and trigger conversion */
+	writel(SARADC_CTRL_POWER_CTRL
+			| (chan->channel & SARADC_CTRL_CHN_MASK)
+			| SARADC_CTRL_IRQ_ENABLE,
+		   info->regs + SARADC_CTRL);
+
+	if (!wait_for_completion_timeout(&info->completion, SARADC_TIMEOUT))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
 				    struct iio_chan_spec const *chan,
 				    int *val, int *val2, long mask)
@@ -62,22 +94,11 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&indio_dev->mlock);
 
-		reinit_completion(&info->completion);
-
-		/* 8 clock periods as delay between power up and start cmd */
-		writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
-
-		/* Select the channel to be used and trigger conversion */
-		writel(SARADC_CTRL_POWER_CTRL
-				| (chan->channel & SARADC_CTRL_CHN_MASK)
-				| SARADC_CTRL_IRQ_ENABLE,
-		       info->regs + SARADC_CTRL);
-
-		if (!wait_for_completion_timeout(&info->completion,
-						 SARADC_TIMEOUT)) {
-			writel_relaxed(0, info->regs + SARADC_CTRL);
+		ret = rockchip_saradc_conversion(info, chan);
+		if (ret) {
+			rockchip_saradc_power_down(info);
 			mutex_unlock(&indio_dev->mlock);
-			return -ETIMEDOUT;
+			return ret;
 		}
 
 		*val = info->last_val;
@@ -91,7 +112,7 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
 		}
 
 		*val = ret / 1000;
-		*val2 = info->data->num_bits;
+		*val2 = chan->scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	default:
 		return -EINVAL;
@@ -104,10 +125,9 @@ static irqreturn_t rockchip_saradc_isr(int irq, void *dev_id)
 
 	/* Read value */
 	info->last_val = readl_relaxed(info->regs + SARADC_DATA);
-	info->last_val &= GENMASK(info->data->num_bits - 1, 0);
+	info->last_val &= GENMASK(info->last_chan->scan_type.realbits - 1, 0);
 
-	/* Clear irq & power down adc */
-	writel_relaxed(0, info->regs + SARADC_CTRL);
+	rockchip_saradc_power_down(info);
 
 	complete(&info->completion);
 
@@ -118,51 +138,55 @@ static const struct iio_info rockchip_saradc_iio_info = {
 	.read_raw = rockchip_saradc_read_raw,
 };
 
-#define SARADC_CHANNEL(_index, _id) {				\
+#define SARADC_CHANNEL(_index, _id, _res) {			\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
 	.channel = _index,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 	.datasheet_name = _id,					\
+	.scan_index = _index,					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = _res,				\
+		.storagebits = 16,				\
+		.endianness = IIO_CPU,				\
+	},							\
 }
 
 static const struct iio_chan_spec rockchip_saradc_iio_channels[] = {
-	SARADC_CHANNEL(0, "adc0"),
-	SARADC_CHANNEL(1, "adc1"),
-	SARADC_CHANNEL(2, "adc2"),
+	SARADC_CHANNEL(0, "adc0", 10),
+	SARADC_CHANNEL(1, "adc1", 10),
+	SARADC_CHANNEL(2, "adc2", 10),
 };
 
 static const struct rockchip_saradc_data saradc_data = {
-	.num_bits = 10,
 	.channels = rockchip_saradc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_saradc_iio_channels),
 	.clk_rate = 1000000,
 };
 
 static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
-	SARADC_CHANNEL(0, "adc0"),
-	SARADC_CHANNEL(1, "adc1"),
+	SARADC_CHANNEL(0, "adc0", 12),
+	SARADC_CHANNEL(1, "adc1", 12),
 };
 
 static const struct rockchip_saradc_data rk3066_tsadc_data = {
-	.num_bits = 12,
 	.channels = rockchip_rk3066_tsadc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_rk3066_tsadc_iio_channels),
 	.clk_rate = 50000,
 };
 
 static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
-	SARADC_CHANNEL(0, "adc0"),
-	SARADC_CHANNEL(1, "adc1"),
-	SARADC_CHANNEL(2, "adc2"),
-	SARADC_CHANNEL(3, "adc3"),
-	SARADC_CHANNEL(4, "adc4"),
-	SARADC_CHANNEL(5, "adc5"),
+	SARADC_CHANNEL(0, "adc0", 10),
+	SARADC_CHANNEL(1, "adc1", 10),
+	SARADC_CHANNEL(2, "adc2", 10),
+	SARADC_CHANNEL(3, "adc3", 10),
+	SARADC_CHANNEL(4, "adc4", 10),
+	SARADC_CHANNEL(5, "adc5", 10),
 };
 
 static const struct rockchip_saradc_data rk3399_saradc_data = {
-	.num_bits = 10,
 	.channels = rockchip_rk3399_saradc_iio_channels,
 	.num_channels = ARRAY_SIZE(rockchip_rk3399_saradc_iio_channels),
 	.clk_rate = 1000000,
@@ -214,6 +238,47 @@ static void rockchip_saradc_regulator_disable(void *data)
 	regulator_disable(info->vref);
 }
 
+static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *i_dev = pf->indio_dev;
+	struct rockchip_saradc *info = iio_priv(i_dev);
+	/*
+	 * @values: each channel takes an u16 value
+	 * @timestamp: will be 8-byte aligned automatically
+	 */
+	struct {
+		u16 values[SARADC_MAX_CHANNELS];
+		int64_t timestamp;
+	} data;
+	int ret;
+	int i, j = 0;
+
+	mutex_lock(&i_dev->mlock);
+
+	for_each_set_bit(i, i_dev->active_scan_mask, i_dev->masklength) {
+		const struct iio_chan_spec *chan = &i_dev->channels[i];
+
+		ret = rockchip_saradc_conversion(info, chan);
+		if (ret) {
+			rockchip_saradc_power_down(info);
+			goto out;
+		}
+
+		data.values[j] = info->last_val;
+		j++;
+	}
+
+	iio_push_to_buffers_with_timestamp(i_dev, &data,
+					   iio_get_time_ns(i_dev));
+out:
+	mutex_unlock(&i_dev->mlock);
+
+	iio_trigger_notify_done(i_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int rockchip_saradc_probe(struct platform_device *pdev)
 {
 	struct rockchip_saradc *info = NULL;
@@ -242,6 +307,12 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 
 	info->data = match->data;
 
+	/* Sanity check for possible later IP variants with more channels */
+	if (info->data->num_channels > SARADC_MAX_CHANNELS) {
+		dev_err(&pdev->dev, "max channels exceeded");
+		return -EINVAL;
+	}
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	info->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(info->regs))
@@ -357,6 +428,12 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	indio_dev->channels = info->data->channels;
 	indio_dev->num_channels = info->data->num_channels;
 
+	ret = devm_iio_triggered_buffer_setup(&indio_dev->dev, indio_dev, NULL,
+					      rockchip_saradc_trigger_handler,
+					      NULL);
+	if (ret)
+		return ret;
+
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret)
 		return ret;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions
  2020-06-23 23:30 [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Heiko Stuebner
  2020-06-23 23:30 ` [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant Heiko Stuebner
  2020-06-23 23:30 ` [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers Heiko Stuebner
@ 2020-06-27 12:03 ` Jonathan Cameron
  2020-07-03  7:48   ` Heiko Stuebner
  2 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2020-06-27 12:03 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, linux-rockchip,
	xxm, Heiko Stuebner

On Wed, 24 Jun 2020 01:30:09 +0200
Heiko Stuebner <heiko@sntech.de> wrote:

> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Parts of the saradc probe rely on devm functions and later parts do not.
> This makes it more difficult to for example enable triggers via their
> devm-functions and would need more undo-work in remove.
> 
> So to make life easier for the driver, move the rest of probe calls
> also to their devm-equivalents.
> 
> This includes moving the clk- and regulator-disabling to a devm_action
> so that they gets disabled both during remove and in the error case
> in probe, after the action is registered.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Looks good to me. Applied to the togreg branch of iio.git with one small
tweak. See inline.

One other thing whilst we are here. Why do we need the build dependence
on ARM?  I just scrapped it and the driver builds fine on x86 so would
be good to get the additional build coverage if we can.

Jonathan

> ---
> changes in v6:
> - move devm actions into separate functions as suggested by Jonathan
> changes in v5:
> - none
> changes in v4:
> - new patch as suggested by Jonathan
> 
>  drivers/iio/adc/rockchip_saradc.c | 72 ++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 582ba047c4a6..1a7990d60f9f 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -193,6 +193,27 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
>  	reset_control_deassert(reset);
>  }
>  
> +static void rockchip_saradc_clk_disable(void *data)
> +{
> +	struct rockchip_saradc *info = data;
> +
> +	clk_disable_unprepare(info->clk);
> +}
> +
> +static void rockchip_saradc_pclk_disable(void *data)
> +{
> +	struct rockchip_saradc *info = data;
> +
> +	clk_disable_unprepare(info->pclk);
> +}
> +
> +static void rockchip_saradc_regulator_disable(void *data)
> +{
> +	struct rockchip_saradc *info = data;
> +
> +	regulator_disable(info->vref);
> +}
> +
>  static int rockchip_saradc_probe(struct platform_device *pdev)
>  {
>  	struct rockchip_saradc *info = NULL;
> @@ -291,17 +312,38 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "failed to enable vref regulator\n");
>  		return ret;
>  	}
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rockchip_saradc_regulator_disable, info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> +			ret);
> +		return ret;
> +	}
>  
>  	ret = clk_prepare_enable(info->pclk);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to enable pclk\n");
> -		goto err_reg_voltage;
> +		return ret;
> +	}
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rockchip_saradc_pclk_disable, info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> +			ret);
> +		return ret;
>  	}
>  
>  	ret = clk_prepare_enable(info->clk);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to enable converter clock\n");
> -		goto err_pclk;
> +		return ret;
> +	}
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rockchip_saradc_clk_disable, info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> +			ret);
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, indio_dev);
> @@ -315,30 +357,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	indio_dev->channels = info->data->channels;
>  	indio_dev->num_channels = info->data->num_channels;
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret)
> -		goto err_clk;
> -
> -	return 0;
> -
> -err_clk:
> -	clk_disable_unprepare(info->clk);
> -err_pclk:
> -	clk_disable_unprepare(info->pclk);
> -err_reg_voltage:
> -	regulator_disable(info->vref);
> -	return ret;
> -}
> -
> -static int rockchip_saradc_remove(struct platform_device *pdev)
> -{
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct rockchip_saradc *info = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -	clk_disable_unprepare(info->clk);
> -	clk_disable_unprepare(info->pclk);
> -	regulator_disable(info->vref);
> +		return ret;
Small tweak rather hidden by how diff presents this but
might as well just

return devm_iio_device_register

>  
>  	return 0;
>  }
> @@ -383,7 +404,6 @@ static SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
>  
>  static struct platform_driver rockchip_saradc_driver = {
>  	.probe		= rockchip_saradc_probe,
> -	.remove		= rockchip_saradc_remove,
>  	.driver		= {
>  		.name	= "rockchip-saradc",
>  		.of_match_table = rockchip_saradc_match,


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant
  2020-06-23 23:30 ` [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant Heiko Stuebner
@ 2020-06-27 12:04   ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2020-06-27 12:04 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, linux-rockchip,
	xxm, Heiko Stuebner

On Wed, 24 Jun 2020 01:30:10 +0200
Heiko Stuebner <heiko@sntech.de> wrote:

> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> As suggested give the current ADC_CHANNEL constant a distinct
> and consistent prefix.
> 
> Suggested-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan

> ---
> changes in v6:
> - none
> changes in v5:
> - new patch
> 
>  drivers/iio/adc/rockchip_saradc.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 1a7990d60f9f..5d2e07dc72fd 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -118,7 +118,7 @@ static const struct iio_info rockchip_saradc_iio_info = {
>  	.read_raw = rockchip_saradc_read_raw,
>  };
>  
> -#define ADC_CHANNEL(_index, _id) {				\
> +#define SARADC_CHANNEL(_index, _id) {				\
>  	.type = IIO_VOLTAGE,					\
>  	.indexed = 1,						\
>  	.channel = _index,					\
> @@ -128,9 +128,9 @@ static const struct iio_info rockchip_saradc_iio_info = {
>  }
>  
>  static const struct iio_chan_spec rockchip_saradc_iio_channels[] = {
> -	ADC_CHANNEL(0, "adc0"),
> -	ADC_CHANNEL(1, "adc1"),
> -	ADC_CHANNEL(2, "adc2"),
> +	SARADC_CHANNEL(0, "adc0"),
> +	SARADC_CHANNEL(1, "adc1"),
> +	SARADC_CHANNEL(2, "adc2"),
>  };
>  
>  static const struct rockchip_saradc_data saradc_data = {
> @@ -141,8 +141,8 @@ static const struct rockchip_saradc_data saradc_data = {
>  };
>  
>  static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
> -	ADC_CHANNEL(0, "adc0"),
> -	ADC_CHANNEL(1, "adc1"),
> +	SARADC_CHANNEL(0, "adc0"),
> +	SARADC_CHANNEL(1, "adc1"),
>  };
>  
>  static const struct rockchip_saradc_data rk3066_tsadc_data = {
> @@ -153,12 +153,12 @@ static const struct rockchip_saradc_data rk3066_tsadc_data = {
>  };
>  
>  static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
> -	ADC_CHANNEL(0, "adc0"),
> -	ADC_CHANNEL(1, "adc1"),
> -	ADC_CHANNEL(2, "adc2"),
> -	ADC_CHANNEL(3, "adc3"),
> -	ADC_CHANNEL(4, "adc4"),
> -	ADC_CHANNEL(5, "adc5"),
> +	SARADC_CHANNEL(0, "adc0"),
> +	SARADC_CHANNEL(1, "adc1"),
> +	SARADC_CHANNEL(2, "adc2"),
> +	SARADC_CHANNEL(3, "adc3"),
> +	SARADC_CHANNEL(4, "adc4"),
> +	SARADC_CHANNEL(5, "adc5"),
>  };
>  
>  static const struct rockchip_saradc_data rk3399_saradc_data = {


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers
  2020-06-23 23:30 ` [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers Heiko Stuebner
@ 2020-06-27 12:10   ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2020-06-27 12:10 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, linux-rockchip,
	xxm, Heiko Stuebner

On Wed, 24 Jun 2020 01:30:11 +0200
Heiko Stuebner <heiko@sntech.de> wrote:

> From: Simon Xue <xxm@rock-chips.com>
> 
> Add the ability to also support access via (triggered) buffers
> next to the existing direct mode.
> 
> Device in question is the Odroid Go Advance that connects a joystick
> to two of the saradc channels for X and Y axis and the new (and still
> pending) adc joystick driver of course wants to use triggered buffers
> from the iio subsystem.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> [some simplifications and added commit description]
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
I made one trivial tweak...  Applied to the togreg branch of iio.git and
pushed out as testing to see if they autobuilders can find anything we
missed.

Thanks,

Jonathan

> ---
> changes in v6:
> - remove unneeded header
> - remove unneeded blank line change
> changes in v5:
> - use IIO_CPU instead of IIO_LE as suggested by Peter
> changes in v4:
> - comment for the channel-num sanity check in probe
> - move to struct for data+timestamp, that way we get the 8-byte
>   alignment automatically - checked by comparing sizeof results
> changes in v3:
> - split buffer struct into values and timestamp area similar to dln2-adc
>   and make sure timestamp gets 8-byte aligned - ALIGN uses 4 as it aligns
>   u16 elements not bytes - hopefully I got it right this time ;-)
> changes in v2:
> - use devm_iio_triggered_buffer_setup
> - calculate data array size from channel number (curtesy of at91-sama5d2_adc)
> 
>  drivers/iio/adc/Kconfig           |   2 +
>  drivers/iio/adc/rockchip_saradc.c | 147 +++++++++++++++++++++++-------
>  2 files changed, 114 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5b57437cef75..ddf830267e24 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -865,6 +865,8 @@ config ROCKCHIP_SARADC
>  	tristate "Rockchip SARADC driver"
>  	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
>  	depends on RESET_CONTROLLER
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for the SARADC found in SoCs from
>  	  Rockchip.
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 5d2e07dc72fd..eb2222c312da 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -15,7 +15,10 @@
>  #include <linux/delay.h>
>  #include <linux/reset.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #define SARADC_DATA			0x00
>  
> @@ -32,9 +35,9 @@
>  #define SARADC_DLY_PU_SOC_MASK		0x3f
>  
>  #define SARADC_TIMEOUT			msecs_to_jiffies(100)
> +#define SARADC_MAX_CHANNELS		6
>  
>  struct rockchip_saradc_data {
> -	int				num_bits;
>  	const struct iio_chan_spec	*channels;
>  	int				num_channels;
>  	unsigned long			clk_rate;
> @@ -49,8 +52,37 @@ struct rockchip_saradc {
>  	struct reset_control	*reset;
>  	const struct rockchip_saradc_data *data;
>  	u16			last_val;
> +	const struct iio_chan_spec *last_chan;
>  };
>  
> +static void rockchip_saradc_power_down(struct rockchip_saradc *info)
> +{
> +	/* Clear irq & power down adc */
> +	writel_relaxed(0, info->regs + SARADC_CTRL);
> +}
> +
> +static int rockchip_saradc_conversion(struct rockchip_saradc *info,
> +				   struct iio_chan_spec const *chan)
> +{
> +	reinit_completion(&info->completion);
> +
> +	/* 8 clock periods as delay between power up and start cmd */
> +	writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
> +
> +	info->last_chan = chan;
> +
> +	/* Select the channel to be used and trigger conversion */
> +	writel(SARADC_CTRL_POWER_CTRL
> +			| (chan->channel & SARADC_CTRL_CHN_MASK)
> +			| SARADC_CTRL_IRQ_ENABLE,
> +		   info->regs + SARADC_CTRL);
> +
> +	if (!wait_for_completion_timeout(&info->completion, SARADC_TIMEOUT))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
>  static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec const *chan,
>  				    int *val, int *val2, long mask)
> @@ -62,22 +94,11 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&indio_dev->mlock);
>  
> -		reinit_completion(&info->completion);
> -
> -		/* 8 clock periods as delay between power up and start cmd */
> -		writel_relaxed(8, info->regs + SARADC_DLY_PU_SOC);
> -
> -		/* Select the channel to be used and trigger conversion */
> -		writel(SARADC_CTRL_POWER_CTRL
> -				| (chan->channel & SARADC_CTRL_CHN_MASK)
> -				| SARADC_CTRL_IRQ_ENABLE,
> -		       info->regs + SARADC_CTRL);
> -
> -		if (!wait_for_completion_timeout(&info->completion,
> -						 SARADC_TIMEOUT)) {
> -			writel_relaxed(0, info->regs + SARADC_CTRL);
> +		ret = rockchip_saradc_conversion(info, chan);
> +		if (ret) {
> +			rockchip_saradc_power_down(info);
>  			mutex_unlock(&indio_dev->mlock);
> -			return -ETIMEDOUT;
> +			return ret;
>  		}
>  
>  		*val = info->last_val;
> @@ -91,7 +112,7 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
>  		}
>  
>  		*val = ret / 1000;
> -		*val2 = info->data->num_bits;
> +		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	default:
>  		return -EINVAL;
> @@ -104,10 +125,9 @@ static irqreturn_t rockchip_saradc_isr(int irq, void *dev_id)
>  
>  	/* Read value */
>  	info->last_val = readl_relaxed(info->regs + SARADC_DATA);
> -	info->last_val &= GENMASK(info->data->num_bits - 1, 0);
> +	info->last_val &= GENMASK(info->last_chan->scan_type.realbits - 1, 0);
>  
> -	/* Clear irq & power down adc */
> -	writel_relaxed(0, info->regs + SARADC_CTRL);
> +	rockchip_saradc_power_down(info);
>  
>  	complete(&info->completion);
>  
> @@ -118,51 +138,55 @@ static const struct iio_info rockchip_saradc_iio_info = {
>  	.read_raw = rockchip_saradc_read_raw,
>  };
>  
> -#define SARADC_CHANNEL(_index, _id) {				\
> +#define SARADC_CHANNEL(_index, _id, _res) {			\
>  	.type = IIO_VOLTAGE,					\
>  	.indexed = 1,						\
>  	.channel = _index,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>  	.datasheet_name = _id,					\
> +	.scan_index = _index,					\
> +	.scan_type = {						\
> +		.sign = 'u',					\
> +		.realbits = _res,				\
> +		.storagebits = 16,				\
> +		.endianness = IIO_CPU,				\
> +	},							\
>  }
>  
>  static const struct iio_chan_spec rockchip_saradc_iio_channels[] = {
> -	SARADC_CHANNEL(0, "adc0"),
> -	SARADC_CHANNEL(1, "adc1"),
> -	SARADC_CHANNEL(2, "adc2"),
> +	SARADC_CHANNEL(0, "adc0", 10),
> +	SARADC_CHANNEL(1, "adc1", 10),
> +	SARADC_CHANNEL(2, "adc2", 10),
>  };
>  
>  static const struct rockchip_saradc_data saradc_data = {
> -	.num_bits = 10,
>  	.channels = rockchip_saradc_iio_channels,
>  	.num_channels = ARRAY_SIZE(rockchip_saradc_iio_channels),
>  	.clk_rate = 1000000,
>  };
>  
>  static const struct iio_chan_spec rockchip_rk3066_tsadc_iio_channels[] = {
> -	SARADC_CHANNEL(0, "adc0"),
> -	SARADC_CHANNEL(1, "adc1"),
> +	SARADC_CHANNEL(0, "adc0", 12),
> +	SARADC_CHANNEL(1, "adc1", 12),
>  };
>  
>  static const struct rockchip_saradc_data rk3066_tsadc_data = {
> -	.num_bits = 12,
>  	.channels = rockchip_rk3066_tsadc_iio_channels,
>  	.num_channels = ARRAY_SIZE(rockchip_rk3066_tsadc_iio_channels),
>  	.clk_rate = 50000,
>  };
>  
>  static const struct iio_chan_spec rockchip_rk3399_saradc_iio_channels[] = {
> -	SARADC_CHANNEL(0, "adc0"),
> -	SARADC_CHANNEL(1, "adc1"),
> -	SARADC_CHANNEL(2, "adc2"),
> -	SARADC_CHANNEL(3, "adc3"),
> -	SARADC_CHANNEL(4, "adc4"),
> -	SARADC_CHANNEL(5, "adc5"),
> +	SARADC_CHANNEL(0, "adc0", 10),
> +	SARADC_CHANNEL(1, "adc1", 10),
> +	SARADC_CHANNEL(2, "adc2", 10),
> +	SARADC_CHANNEL(3, "adc3", 10),
> +	SARADC_CHANNEL(4, "adc4", 10),
> +	SARADC_CHANNEL(5, "adc5", 10),
>  };
>  
>  static const struct rockchip_saradc_data rk3399_saradc_data = {
> -	.num_bits = 10,
>  	.channels = rockchip_rk3399_saradc_iio_channels,
>  	.num_channels = ARRAY_SIZE(rockchip_rk3399_saradc_iio_channels),
>  	.clk_rate = 1000000,
> @@ -214,6 +238,47 @@ static void rockchip_saradc_regulator_disable(void *data)
>  	regulator_disable(info->vref);
>  }
>  
> +static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *i_dev = pf->indio_dev;
> +	struct rockchip_saradc *info = iio_priv(i_dev);
> +	/*
> +	 * @values: each channel takes an u16 value
> +	 * @timestamp: will be 8-byte aligned automatically
> +	 */
> +	struct {
> +		u16 values[SARADC_MAX_CHANNELS];
> +		int64_t timestamp;
> +	} data;
> +	int ret;
> +	int i, j = 0;
> +
> +	mutex_lock(&i_dev->mlock);
> +
> +	for_each_set_bit(i, i_dev->active_scan_mask, i_dev->masklength) {
> +		const struct iio_chan_spec *chan = &i_dev->channels[i];
> +
> +		ret = rockchip_saradc_conversion(info, chan);
> +		if (ret) {
> +			rockchip_saradc_power_down(info);
> +			goto out;
> +		}
> +
> +		data.values[j] = info->last_val;
> +		j++;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(i_dev, &data,
> +					   iio_get_time_ns(i_dev));
Given new relaxed view on lines just over 80 chars I might tweak that whilst 
applying to a more readable single line.

> +out:
> +	mutex_unlock(&i_dev->mlock);
> +
> +	iio_trigger_notify_done(i_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int rockchip_saradc_probe(struct platform_device *pdev)
>  {
>  	struct rockchip_saradc *info = NULL;
> @@ -242,6 +307,12 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  
>  	info->data = match->data;
>  
> +	/* Sanity check for possible later IP variants with more channels */
> +	if (info->data->num_channels > SARADC_MAX_CHANNELS) {
> +		dev_err(&pdev->dev, "max channels exceeded");
> +		return -EINVAL;
> +	}
> +
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	info->regs = devm_ioremap_resource(&pdev->dev, mem);
>  	if (IS_ERR(info->regs))
> @@ -357,6 +428,12 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	indio_dev->channels = info->data->channels;
>  	indio_dev->num_channels = info->data->num_channels;
>  
> +	ret = devm_iio_triggered_buffer_setup(&indio_dev->dev, indio_dev, NULL,
> +					      rockchip_saradc_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret)
>  		return ret;


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions
  2020-06-27 12:03 ` [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Jonathan Cameron
@ 2020-07-03  7:48   ` Heiko Stuebner
  2020-07-04 15:38     ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Heiko Stuebner @ 2020-07-03  7:48 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, linux-rockchip, xxm

Am Samstag, 27. Juni 2020, 14:03:29 CEST schrieb Jonathan Cameron:
> On Wed, 24 Jun 2020 01:30:09 +0200
> Heiko Stuebner <heiko@sntech.de> wrote:
> 
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > Parts of the saradc probe rely on devm functions and later parts do not.
> > This makes it more difficult to for example enable triggers via their
> > devm-functions and would need more undo-work in remove.
> > 
> > So to make life easier for the driver, move the rest of probe calls
> > also to their devm-equivalents.
> > 
> > This includes moving the clk- and regulator-disabling to a devm_action
> > so that they gets disabled both during remove and in the error case
> > in probe, after the action is registered.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Looks good to me. Applied to the togreg branch of iio.git with one small
> tweak. See inline.
> 
> One other thing whilst we are here. Why do we need the build dependence
> on ARM?

I guess originally it was there simply, because Rockchip only makes
ARM-SoCs so as not to pollute the config with too many unusable
options and it had the COMPILE_TEST for the test-builds.

But if one driver more doesn't really matter, it can of course also just
go away.

> I just scrapped it and the driver builds fine on x86 so would
> be good to get the additional build coverage if we can.

So I guess you'd like a patch removing that part, right?
Because I guess the "I just scrapped it" meant locally, as I can't find
a commit of that sort in the testing branch ;-)


Heiko


> > ---
> > changes in v6:
> > - move devm actions into separate functions as suggested by Jonathan
> > changes in v5:
> > - none
> > changes in v4:
> > - new patch as suggested by Jonathan
> > 
> >  drivers/iio/adc/rockchip_saradc.c | 72 ++++++++++++++++++++-----------
> >  1 file changed, 46 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> > index 582ba047c4a6..1a7990d60f9f 100644
> > --- a/drivers/iio/adc/rockchip_saradc.c
> > +++ b/drivers/iio/adc/rockchip_saradc.c
> > @@ -193,6 +193,27 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
> >  	reset_control_deassert(reset);
> >  }
> >  
> > +static void rockchip_saradc_clk_disable(void *data)
> > +{
> > +	struct rockchip_saradc *info = data;
> > +
> > +	clk_disable_unprepare(info->clk);
> > +}
> > +
> > +static void rockchip_saradc_pclk_disable(void *data)
> > +{
> > +	struct rockchip_saradc *info = data;
> > +
> > +	clk_disable_unprepare(info->pclk);
> > +}
> > +
> > +static void rockchip_saradc_regulator_disable(void *data)
> > +{
> > +	struct rockchip_saradc *info = data;
> > +
> > +	regulator_disable(info->vref);
> > +}
> > +
> >  static int rockchip_saradc_probe(struct platform_device *pdev)
> >  {
> >  	struct rockchip_saradc *info = NULL;
> > @@ -291,17 +312,38 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> >  		dev_err(&pdev->dev, "failed to enable vref regulator\n");
> >  		return ret;
> >  	}
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rockchip_saradc_regulator_disable, info);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> >  
> >  	ret = clk_prepare_enable(info->pclk);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "failed to enable pclk\n");
> > -		goto err_reg_voltage;
> > +		return ret;
> > +	}
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rockchip_saradc_pclk_disable, info);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > +			ret);
> > +		return ret;
> >  	}
> >  
> >  	ret = clk_prepare_enable(info->clk);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "failed to enable converter clock\n");
> > -		goto err_pclk;
> > +		return ret;
> > +	}
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rockchip_saradc_clk_disable, info);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > +			ret);
> > +		return ret;
> >  	}
> >  
> >  	platform_set_drvdata(pdev, indio_dev);
> > @@ -315,30 +357,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> >  	indio_dev->channels = info->data->channels;
> >  	indio_dev->num_channels = info->data->num_channels;
> >  
> > -	ret = iio_device_register(indio_dev);
> > +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> >  	if (ret)
> > -		goto err_clk;
> > -
> > -	return 0;
> > -
> > -err_clk:
> > -	clk_disable_unprepare(info->clk);
> > -err_pclk:
> > -	clk_disable_unprepare(info->pclk);
> > -err_reg_voltage:
> > -	regulator_disable(info->vref);
> > -	return ret;
> > -}
> > -
> > -static int rockchip_saradc_remove(struct platform_device *pdev)
> > -{
> > -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > -	struct rockchip_saradc *info = iio_priv(indio_dev);
> > -
> > -	iio_device_unregister(indio_dev);
> > -	clk_disable_unprepare(info->clk);
> > -	clk_disable_unprepare(info->pclk);
> > -	regulator_disable(info->vref);
> > +		return ret;
> Small tweak rather hidden by how diff presents this but
> might as well just
> 
> return devm_iio_device_register
> 
> >  
> >  	return 0;
> >  }
> > @@ -383,7 +404,6 @@ static SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
> >  
> >  static struct platform_driver rockchip_saradc_driver = {
> >  	.probe		= rockchip_saradc_probe,
> > -	.remove		= rockchip_saradc_remove,
> >  	.driver		= {
> >  		.name	= "rockchip-saradc",
> >  		.of_match_table = rockchip_saradc_match,
> 
> 





^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions
  2020-07-03  7:48   ` Heiko Stuebner
@ 2020-07-04 15:38     ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2020-07-04 15:38 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, linux-rockchip, xxm

On Fri, 03 Jul 2020 09:48:42 +0200
Heiko Stuebner <heiko@sntech.de> wrote:

> Am Samstag, 27. Juni 2020, 14:03:29 CEST schrieb Jonathan Cameron:
> > On Wed, 24 Jun 2020 01:30:09 +0200
> > Heiko Stuebner <heiko@sntech.de> wrote:
> >   
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > 
> > > Parts of the saradc probe rely on devm functions and later parts do not.
> > > This makes it more difficult to for example enable triggers via their
> > > devm-functions and would need more undo-work in remove.
> > > 
> > > So to make life easier for the driver, move the rest of probe calls
> > > also to their devm-equivalents.
> > > 
> > > This includes moving the clk- and regulator-disabling to a devm_action
> > > so that they gets disabled both during remove and in the error case
> > > in probe, after the action is registered.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>  
> > Looks good to me. Applied to the togreg branch of iio.git with one small
> > tweak. See inline.
> > 
> > One other thing whilst we are here. Why do we need the build dependence
> > on ARM?  
> 
> I guess originally it was there simply, because Rockchip only makes
> ARM-SoCs so as not to pollute the config with too many unusable
> options and it had the COMPILE_TEST for the test-builds.

I'd have no problem with
depends on ARCH_ROCKCHIP || COMPILE_TEST
which would mean the unusable option only applied to those of us
deliberately doing test builds rather than anyone configuring a
real platform (where COMPILE_TEST makes no sense).

> 
> But if one driver more doesn't really matter, it can of course also just
> go away.
> 
> > I just scrapped it and the driver builds fine on x86 so would
> > be good to get the additional build coverage if we can.  
> 
> So I guess you'd like a patch removing that part, right?
> Because I guess the "I just scrapped it" meant locally, as I can't find
> a commit of that sort in the testing branch ;-)

Indeed I did this locally.

Patch would be idea, or I can do it if you prefer.

Thanks,

Jonathan


> 
> 
> Heiko
> 
> 
> > > ---
> > > changes in v6:
> > > - move devm actions into separate functions as suggested by Jonathan
> > > changes in v5:
> > > - none
> > > changes in v4:
> > > - new patch as suggested by Jonathan
> > > 
> > >  drivers/iio/adc/rockchip_saradc.c | 72 ++++++++++++++++++++-----------
> > >  1 file changed, 46 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> > > index 582ba047c4a6..1a7990d60f9f 100644
> > > --- a/drivers/iio/adc/rockchip_saradc.c
> > > +++ b/drivers/iio/adc/rockchip_saradc.c
> > > @@ -193,6 +193,27 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
> > >  	reset_control_deassert(reset);
> > >  }
> > >  
> > > +static void rockchip_saradc_clk_disable(void *data)
> > > +{
> > > +	struct rockchip_saradc *info = data;
> > > +
> > > +	clk_disable_unprepare(info->clk);
> > > +}
> > > +
> > > +static void rockchip_saradc_pclk_disable(void *data)
> > > +{
> > > +	struct rockchip_saradc *info = data;
> > > +
> > > +	clk_disable_unprepare(info->pclk);
> > > +}
> > > +
> > > +static void rockchip_saradc_regulator_disable(void *data)
> > > +{
> > > +	struct rockchip_saradc *info = data;
> > > +
> > > +	regulator_disable(info->vref);
> > > +}
> > > +
> > >  static int rockchip_saradc_probe(struct platform_device *pdev)
> > >  {
> > >  	struct rockchip_saradc *info = NULL;
> > > @@ -291,17 +312,38 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> > >  		dev_err(&pdev->dev, "failed to enable vref regulator\n");
> > >  		return ret;
> > >  	}
> > > +	ret = devm_add_action_or_reset(&pdev->dev,
> > > +				       rockchip_saradc_regulator_disable, info);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > >  
> > >  	ret = clk_prepare_enable(info->pclk);
> > >  	if (ret < 0) {
> > >  		dev_err(&pdev->dev, "failed to enable pclk\n");
> > > -		goto err_reg_voltage;
> > > +		return ret;
> > > +	}
> > > +	ret = devm_add_action_or_reset(&pdev->dev,
> > > +				       rockchip_saradc_pclk_disable, info);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > > +			ret);
> > > +		return ret;
> > >  	}
> > >  
> > >  	ret = clk_prepare_enable(info->clk);
> > >  	if (ret < 0) {
> > >  		dev_err(&pdev->dev, "failed to enable converter clock\n");
> > > -		goto err_pclk;
> > > +		return ret;
> > > +	}
> > > +	ret = devm_add_action_or_reset(&pdev->dev,
> > > +				       rockchip_saradc_clk_disable, info);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> > > +			ret);
> > > +		return ret;
> > >  	}
> > >  
> > >  	platform_set_drvdata(pdev, indio_dev);
> > > @@ -315,30 +357,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> > >  	indio_dev->channels = info->data->channels;
> > >  	indio_dev->num_channels = info->data->num_channels;
> > >  
> > > -	ret = iio_device_register(indio_dev);
> > > +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > >  	if (ret)
> > > -		goto err_clk;
> > > -
> > > -	return 0;
> > > -
> > > -err_clk:
> > > -	clk_disable_unprepare(info->clk);
> > > -err_pclk:
> > > -	clk_disable_unprepare(info->pclk);
> > > -err_reg_voltage:
> > > -	regulator_disable(info->vref);
> > > -	return ret;
> > > -}
> > > -
> > > -static int rockchip_saradc_remove(struct platform_device *pdev)
> > > -{
> > > -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > -	struct rockchip_saradc *info = iio_priv(indio_dev);
> > > -
> > > -	iio_device_unregister(indio_dev);
> > > -	clk_disable_unprepare(info->clk);
> > > -	clk_disable_unprepare(info->pclk);
> > > -	regulator_disable(info->vref);
> > > +		return ret;  
> > Small tweak rather hidden by how diff presents this but
> > might as well just
> > 
> > return devm_iio_device_register
> >   
> > >  
> > >  	return 0;
> > >  }
> > > @@ -383,7 +404,6 @@ static SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
> > >  
> > >  static struct platform_driver rockchip_saradc_driver = {
> > >  	.probe		= rockchip_saradc_probe,
> > > -	.remove		= rockchip_saradc_remove,
> > >  	.driver		= {
> > >  		.name	= "rockchip-saradc",
> > >  		.of_match_table = rockchip_saradc_match,  
> > 
> >   
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-04 15:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23 23:30 [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Heiko Stuebner
2020-06-23 23:30 ` [PATCH v6 2/3] iio: adc: rockchip_saradc: better prefix for channel constant Heiko Stuebner
2020-06-27 12:04   ` Jonathan Cameron
2020-06-23 23:30 ` [PATCH v6 3/3] iio: adc: rockchip_saradc: Add support iio buffers Heiko Stuebner
2020-06-27 12:10   ` Jonathan Cameron
2020-06-27 12:03 ` [PATCH v6 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions Jonathan Cameron
2020-07-03  7:48   ` Heiko Stuebner
2020-07-04 15:38     ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).