* [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
* 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
* [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 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-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 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).