| Message ID | 20210824091243.9393-1-billy_tsai@aspeedtech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for ast2600 ADC | expand |
On Tue, 24 Aug 2021 17:12:36 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch use need_prescaler and scaler_bit_width to set the adc clock > scaler. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Hi Billy, One minor comment inline. Thanks, Jonathan > --- > drivers/iio/adc/aspeed_adc.c | 39 +++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 2d6215a91f99..52db38be9699 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -202,9 +202,10 @@ static int aspeed_adc_probe(struct platform_device *pdev) > { > struct iio_dev *indio_dev; > struct aspeed_adc_data *data; > - const char *clk_parent_name; > int ret; > u32 adc_engine_control_reg_val; > + unsigned long scaler_flags = 0; > + char clk_name[32], clk_parent_name[32]; > > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data)); > if (!indio_dev) > @@ -221,24 +222,28 @@ static int aspeed_adc_probe(struct platform_device *pdev) > > /* Register ADC clock prescaler with source specified by device tree. */ > spin_lock_init(&data->clk_lock); > - clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0); > - > - data->clk_prescaler = clk_hw_register_divider( > - &pdev->dev, "prescaler", clk_parent_name, 0, > - data->base + ASPEED_REG_CLOCK_CONTROL, > - 17, 15, 0, &data->clk_lock); > - if (IS_ERR(data->clk_prescaler)) > - return PTR_ERR(data->clk_prescaler); > - > + snprintf(clk_parent_name, 32, of_clk_get_parent_name(pdev->dev.of_node, 0)); ARRAY_SIZE(clk_parent_name) instead of 32. Same for other places this pattern occurs. > + if (data->model_data->need_prescaler) { > + snprintf(clk_name, 32, "%s-prescaler", > + data->model_data->model_name); > + data->clk_prescaler = clk_hw_register_divider( > + &pdev->dev, clk_name, clk_parent_name, 0, > + data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, 0, > + &data->clk_lock); > + if (IS_ERR(data->clk_prescaler)) > + return PTR_ERR(data->clk_prescaler); > + snprintf(clk_parent_name, 32, clk_name); > + scaler_flags = CLK_SET_RATE_PARENT; > + } > /* > * Register ADC clock scaler downstream from the prescaler. Allow rate > * setting to adjust the prescaler as well. > */ > + snprintf(clk_name, 32, "%s-scaler", data->model_data->model_name); > data->clk_scaler = clk_hw_register_divider( > - &pdev->dev, "scaler", "prescaler", > - CLK_SET_RATE_PARENT, > - data->base + ASPEED_REG_CLOCK_CONTROL, > - 0, 10, 0, &data->clk_lock); > + &pdev->dev, clk_name, clk_parent_name, scaler_flags, > + data->base + ASPEED_REG_CLOCK_CONTROL, 0, > + data->model_data->scaler_bit_width, 0, &data->clk_lock); > if (IS_ERR(data->clk_scaler)) { > ret = PTR_ERR(data->clk_scaler); > goto scaler_error; > @@ -310,7 +315,8 @@ static int aspeed_adc_probe(struct platform_device *pdev) > reset_error: > clk_hw_unregister_divider(data->clk_scaler); > scaler_error: > - clk_hw_unregister_divider(data->clk_prescaler); > + if (data->model_data->need_prescaler) > + clk_hw_unregister_divider(data->clk_prescaler); > return ret; > } > > @@ -325,7 +331,8 @@ static int aspeed_adc_remove(struct platform_device *pdev) > clk_disable_unprepare(data->clk_scaler->clk); > reset_control_assert(data->rst); > clk_hw_unregister_divider(data->clk_scaler); > - clk_hw_unregister_divider(data->clk_prescaler); > + if (data->model_data->need_prescaler) > + clk_hw_unregister_divider(data->clk_prescaler); > > return 0; > }
On Tue, 24 Aug 2021 17:12:37 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch use devm_add_action_or_reset to handle the error in probe > phase. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/iio/adc/aspeed_adc.c | 92 +++++++++++++++++++++--------------- > 1 file changed, 55 insertions(+), 37 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 52db38be9699..1c87e12a0cab 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -187,6 +187,27 @@ static const struct iio_info aspeed_adc_iio_info = { > .debugfs_reg_access = aspeed_adc_reg_access, > }; > > +static void aspeed_adc_unregister_divider(void *data) > +{ > + struct clk_hw *clk = data; > + > + clk_hw_unregister_divider(clk); > +} > + > +static void aspeed_adc_reset_assert(void *data) > +{ > + struct reset_control *rst = data; > + > + reset_control_assert(rst); > +} > + > +static void aspeed_adc_clk_disable_unprepare(void *data) > +{ > + struct clk *clk = data; > + > + clk_disable_unprepare(clk); > +} > + > static int aspeed_adc_vref_config(struct iio_dev *indio_dev) > { > struct aspeed_adc_data *data = iio_priv(indio_dev); > @@ -232,6 +253,12 @@ static int aspeed_adc_probe(struct platform_device *pdev) > &data->clk_lock); > if (IS_ERR(data->clk_prescaler)) > return PTR_ERR(data->clk_prescaler); > + > + ret = devm_add_action_or_reset(data->dev, > + aspeed_adc_unregister_divider, > + data->clk_prescaler); > + if (ret) > + return ret; > snprintf(clk_parent_name, 32, clk_name); > scaler_flags = CLK_SET_RATE_PARENT; > } > @@ -244,23 +271,30 @@ static int aspeed_adc_probe(struct platform_device *pdev) > &pdev->dev, clk_name, clk_parent_name, scaler_flags, > data->base + ASPEED_REG_CLOCK_CONTROL, 0, > data->model_data->scaler_bit_width, 0, &data->clk_lock); > - if (IS_ERR(data->clk_scaler)) { > - ret = PTR_ERR(data->clk_scaler); > - goto scaler_error; > - } > + if (IS_ERR(data->clk_scaler)) > + return PTR_ERR(data->clk_scaler); > + > + ret = devm_add_action_or_reset(data->dev, aspeed_adc_unregister_divider, > + data->clk_scaler); > + if (ret) > + return ret; > > data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL); > if (IS_ERR(data->rst)) { > dev_err(&pdev->dev, > "invalid or missing reset controller device tree entry"); > - ret = PTR_ERR(data->rst); > - goto reset_error; > + return PTR_ERR(data->rst); > } > reset_control_deassert(data->rst); > > + ret = devm_add_action_or_reset(data->dev, aspeed_adc_reset_assert, > + data->rst); > + if (ret) > + return ret; > + > ret = aspeed_adc_vref_config(indio_dev); > if (ret) > - goto vref_config_error; > + return ret; > > if (data->model_data->wait_init_sequence) { > /* Enable engine in normal mode. */ > @@ -277,13 +311,19 @@ static int aspeed_adc_probe(struct platform_device *pdev) > ASPEED_ADC_INIT_POLLING_TIME, > ASPEED_ADC_INIT_TIMEOUT); > if (ret) > - goto poll_timeout_error; > + return ret; > } > > /* Start all channels in normal mode. */ > ret = clk_prepare_enable(data->clk_scaler->clk); > if (ret) > - goto clk_enable_error; > + return ret; > + > + ret = devm_add_action_or_reset(data->dev, > + aspeed_adc_clk_disable_unprepare, > + data->clk_scaler->clk); > + if (ret) > + return ret; > > adc_engine_control_reg_val = > ASPEED_ADC_CTRL_CHANNEL | > @@ -299,41 +339,19 @@ static int aspeed_adc_probe(struct platform_device *pdev) > indio_dev->num_channels = data->model_data->num_channels; > > ret = iio_device_register(indio_dev); > - if (ret) > - goto iio_register_error; > - > + if (ret) { > + writel(FIELD_PREP(ASPEED_ADC_OP_MODE, > + ASPEED_ADC_OP_MODE_PWR_DOWN), > + data->base + ASPEED_REG_ENGINE_CONTROL); > + return ret; > + } > return 0; > - > -iio_register_error: > - writel(FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_PWR_DOWN), > - data->base + ASPEED_REG_ENGINE_CONTROL); > - clk_disable_unprepare(data->clk_scaler->clk); > -clk_enable_error: > -poll_timeout_error: > -vref_config_error: > - reset_control_assert(data->rst); > -reset_error: > - clk_hw_unregister_divider(data->clk_scaler); > -scaler_error: > - if (data->model_data->need_prescaler) > - clk_hw_unregister_divider(data->clk_prescaler); > - return ret; > } > > static int aspeed_adc_remove(struct platform_device *pdev) > { > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > - struct aspeed_adc_data *data = iio_priv(indio_dev); > - > iio_device_unregister(indio_dev); Given there is no longer anything here, you should be safe to call devm_iio_device_unregister() and get rid of this function entirely. > - writel(FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_PWR_DOWN), > - data->base + ASPEED_REG_ENGINE_CONTROL); Unless I'm missing something this is not handled via any of the devm_add_action_or_reset() callbacks. You handle it in the error paths manually but if you move this to managed as well you can drop that handling and still have it automatically called on remove. > - clk_disable_unprepare(data->clk_scaler->clk); > - reset_control_assert(data->rst); > - clk_hw_unregister_divider(data->clk_scaler); > - if (data->model_data->need_prescaler) > - clk_hw_unregister_divider(data->clk_prescaler); > - > return 0; > } >
On Tue, 24 Aug 2021 17:12:41 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch adds a compensation phase to improve the accurate of ADC accuracy of the ADC measurement. > measurement. This is the built-in function though input half of the > reference voltage to get the ADC offset. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> This looks like fairly standard calibration which is good to have. Thanks, Jonathan > --- > drivers/iio/adc/aspeed_adc.c | 54 +++++++++++++++++++++++++++++++++++- > 1 file changed, 53 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 4d979dd7fe88..20caf28dff18 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -95,6 +95,7 @@ struct aspeed_adc_data { > struct reset_control *rst; > int vref; > u32 sample_period_ns; > + int cv; > }; > > #define ASPEED_CHAN(_idx, _data_reg_addr) { \ > @@ -104,7 +105,8 @@ struct aspeed_adc_data { > .address = (_data_reg_addr), \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > - BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > } > > static const struct iio_chan_spec aspeed_adc_iio_channels[] = { > @@ -126,6 +128,51 @@ static const struct iio_chan_spec aspeed_adc_iio_channels[] = { > ASPEED_CHAN(15, 0x2E), > }; > > +static int aspeed_adc_compensation(struct iio_dev *indio_dev) > +{ > + struct aspeed_adc_data *data = iio_priv(indio_dev); > + u32 index, adc_raw = 0; > + u32 adc_engine_control_reg_val; > + > + adc_engine_control_reg_val = > + readl(data->base + ASPEED_REG_ENGINE_CONTROL); > + adc_engine_control_reg_val &= ~ASPEED_ADC_OP_MODE; > + adc_engine_control_reg_val |= > + (FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_NORMAL) | > + ASPEED_ADC_ENGINE_ENABLE); > + /* > + * Enable compensating sensing: > + * After that, the input voltage of ADC will force to half of the reference > + * voltage. So the expected reading raw data will become half of the max > + * value. We can get compensating value = 0x200 - ADC read raw value. > + * It is recommended to average at least 10 samples to get a final CV. > + */ > + writel(adc_engine_control_reg_val | ASPEED_ADC_CTRL_COMPENSATION | > + ASPEED_ADC_CTRL_CHANNEL_ENABLE(0), > + data->base + ASPEED_REG_ENGINE_CONTROL); > + /* > + * After enable compensating sensing mode need to wait some time for ADC stable > + * Experiment result is 1ms. > + */ > + mdelay(1); > + > + for (index = 0; index < 16; index++) { > + /* > + * Waiting for the sampling period ensures that the value acquired > + * is fresh each time. > + */ > + ndelay(data->sample_period_ns); > + adc_raw += readw(data->base + aspeed_adc_iio_channels[0].address); > + } > + adc_raw >>= 4; > + data->cv = BIT(ASPEED_RESOLUTION_BITS - 1) - adc_raw; > + writel(adc_engine_control_reg_val, > + data->base + ASPEED_REG_ENGINE_CONTROL); > + dev_dbg(data->dev, "Compensating value = %d\n", data->cv); > + > + return 0; > +} > + > static int aspeed_adc_set_sampling_rate(struct iio_dev *indio_dev, u32 rate) > { > struct aspeed_adc_data *data = iio_priv(indio_dev); > @@ -155,6 +202,10 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, > *val = readw(data->base + chan->address); > return IIO_VAL_INT; > > + case IIO_CHAN_INFO_OFFSET: > + *val = data->cv; > + return IIO_VAL_INT; > + > case IIO_CHAN_INFO_SCALE: > *val = data->vref; > *val2 = ASPEED_RESOLUTION_BITS; > @@ -444,6 +495,7 @@ static int aspeed_adc_probe(struct platform_device *pdev) > return ret; > } > > + aspeed_adc_compensation(indio_dev); > /* Start all channels in normal mode. */ > adc_engine_control_reg_val = > readl(data->base + ASPEED_REG_ENGINE_CONTROL);
On Tue, 24 Aug 2021 17:12:43 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > The adc controller have trimming register for fine-tune the reference Nice to have ADC instead of adc (though not that important as meaning is clear). > voltage. The trimming value come from the otp register which will be > written before chip product. This patch will read this otp value and written during chip production? (perhaps that's what is intended?) > configure it to the adc register when adc controller probe and using dts > property "aspeed,trim-data-valid" to determine whether to execute this > flow. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/iio/adc/aspeed_adc.c | 68 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 0c5d84e82561..bd7fb23f3510 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -25,6 +25,8 @@ > #include <linux/spinlock.h> > #include <linux/types.h> > #include <linux/bitfield.h> > +#include <linux/regmap.h> > +#include <linux/mfd/syscon.h> > > #include <linux/iio/iio.h> > #include <linux/iio/driver.h> > @@ -72,6 +74,11 @@ > */ > #define ASPEED_ADC_DEF_SAMPLING_RATE 65000 > > +struct aspeed_adc_trim_locate { > + const unsigned int offset; > + const unsigned int field; > +}; > + > struct aspeed_adc_model_data { > const char *model_name; > unsigned int min_sampling_rate; // Hz > @@ -82,6 +89,7 @@ struct aspeed_adc_model_data { > bool bat_sense_sup; > u8 scaler_bit_width; > unsigned int num_channels; > + const struct aspeed_adc_trim_locate *trim_locate; > }; > > struct adc_gain { > @@ -136,6 +144,44 @@ static const struct iio_chan_spec aspeed_adc_iio_channels[] = { > ASPEED_CHAN(15, 0x2E), > }; > > +static int aspeed_adc_set_trim_data(struct iio_dev *indio_dev) > +{ > + struct device_node *syscon; > + struct regmap *scu; > + u32 scu_otp, trimming_val; > + struct aspeed_adc_data *data = iio_priv(indio_dev); > + > + syscon = of_find_node_by_name(NULL, "syscon"); > + if (syscon == NULL) { > + dev_warn(data->dev, "Couldn't find syscon node\n"); > + return -EOPNOTSUPP; > + } > + scu = syscon_node_to_regmap(syscon); > + if (IS_ERR(scu)) { > + dev_warn(data->dev, "Failed to get syscon regmap\n"); > + return -EOPNOTSUPP; > + } > + if (data->model_data->trim_locate) { > + if (regmap_read(scu, data->model_data->trim_locate->offset, > + &scu_otp)) { > + dev_warn(data->dev, > + "Failed to get adc trimming data\n"); > + trimming_val = 0x8; > + } else { > + trimming_val = > + ((scu_otp) & > + (data->model_data->trim_locate->field)) >> > + __ffs(data->model_data->trim_locate->field); > + } > + dev_dbg(data->dev, > + "trimming val = %d, offset = %08x, fields = %08x\n", > + trimming_val, data->model_data->trim_locate->offset, > + data->model_data->trim_locate->field); > + writel(trimming_val, data->base + ASPEED_REG_COMPENSATION_TRIM); > + } > + return 0; > +} > + > static int aspeed_adc_compensation(struct iio_dev *indio_dev) > { > struct aspeed_adc_data *data = iio_priv(indio_dev); > @@ -506,6 +552,10 @@ static int aspeed_adc_probe(struct platform_device *pdev) > if (ret) > return ret; > > + if (of_find_property(data->dev->of_node, "aspeed,trim-data-valid", > + NULL)) > + aspeed_adc_set_trim_data(indio_dev); > + > if (of_find_property(data->dev->of_node, "aspeed,battery-sensing", > NULL)) { > if (data->model_data->bat_sense_sup) { > @@ -579,6 +629,21 @@ static int aspeed_adc_remove(struct platform_device *pdev) > return 0; > } > > +static const struct aspeed_adc_trim_locate ast2500_adc_trim = { > + .offset = 0x154, > + .field = GENMASK(31, 28), > +}; > + > +static const struct aspeed_adc_trim_locate ast2600_adc0_trim = { > + .offset = 0x5d0, > + .field = GENMASK(3, 0), > +}; > + > +static const struct aspeed_adc_trim_locate ast2600_adc1_trim = { > + .offset = 0x5d0, > + .field = GENMASK(7, 4), > +}; > + > static const struct aspeed_adc_model_data ast2400_model_data = { > .model_name = "ast2400-adc", > .vref_fixed = 2500, // mV > @@ -598,6 +663,7 @@ static const struct aspeed_adc_model_data ast2500_model_data = { > .need_prescaler = true, > .scaler_bit_width = 10, > .num_channels = 16, > + .trim_locate = &ast2500_adc_trim, > }; > > static const struct aspeed_adc_model_data ast2600_adc0_model_data = { > @@ -608,6 +674,7 @@ static const struct aspeed_adc_model_data ast2600_adc0_model_data = { > .bat_sense_sup = true, > .scaler_bit_width = 16, > .num_channels = 8, > + .trim_locate = &ast2600_adc0_trim, > }; > > static const struct aspeed_adc_model_data ast2600_adc1_model_data = { > @@ -618,6 +685,7 @@ static const struct aspeed_adc_model_data ast2600_adc1_model_data = { > .bat_sense_sup = true, > .scaler_bit_width = 16, > .num_channels = 8, > + .trim_locate = &ast2600_adc1_trim, > }; > > static const struct of_device_id aspeed_adc_matches[] = {
This patch serials make aspeed_adc.c can support ast2600 and backward compatible. RESEND due to miss some patches when sent patch v4. Change since v3: dt-bindings: - Fix properties:aspeed,int_vref_mv type error. Change since v2: dt-bindings: - Create a new dt-bindings for ast2600 adc aspeed_adc.c: - Splits the patch for more details - Remove version enum and use the flags in model data to distinguish hardware feature - Support trimming data get and set. - Use devm_add_action_or_reset to simplify probe error handling. Changes since v1: dt-bindings: - Fix the aspeed,adc.yaml check error. - Add battery-sensing property. aspeed_adc.c: - Change the init flow: Clock and reference voltage setting should be completed before adc engine enable. - Change the default sampling rate to meet most user case. - Add patch #8 to suppoert battery sensing mode. Billy Tsai (15): dt-bindings: iio: adc: Add ast2600-adc bindings iio: adc: aspeed: completes the bitfield declare. iio: adc: aspeed: set driver data when adc probe. iio: adc: aspeed: Keep model data to driver data. iio: adc: aspeed: Refactory model data structure iio: adc: aspeed: Add vref config function iio: adc: aspeed: Set num_channels with model data iio: adc: aspeed: Use model_data to set clk scaler. iio: adc: aspeed: Use devm_add_action_or_reset. iio: adc: aspeed: Support ast2600 adc. iio: adc: aspeed: Fix the calculate error of clock. iio: adc: aspeed: Add func to set sampling rate. iio: adc: aspeed: Add compensation phase. iio: adc: aspeed: Support battery sensing. iio: adc: aspeed: Get and set trimming data. .../bindings/iio/adc/aspeed,ast2600-adc.yaml | 97 +++ drivers/iio/adc/aspeed_adc.c | 562 +++++++++++++++--- 2 files changed, 569 insertions(+), 90 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed,ast2600-adc.yaml