| 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:40 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > Add the function to set the sampling rate and keep the sampling period > for a driver used to wait the lastest value. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Why move the code as well as factoring out the setter function? I doubt it does any harm, but I'd like to understand why you did it. Jonathan > --- > drivers/iio/adc/aspeed_adc.c | 58 +++++++++++++++++++++++++----------- > 1 file changed, 40 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 8fe7da1a651f..4d979dd7fe88 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -65,6 +65,12 @@ > > #define ASPEED_ADC_INIT_POLLING_TIME 500 > #define ASPEED_ADC_INIT_TIMEOUT 500000 > +/* > + * When the sampling rate is too high, the ADC may not have enough charging > + * time, resulting in a low voltage value. Thus, default use slow sampling > + * rate for most user case. > + */ > +#define ASPEED_ADC_DEF_SAMPLING_RATE 65000 > > struct aspeed_adc_model_data { > const char *model_name; > @@ -88,6 +94,7 @@ struct aspeed_adc_data { > struct clk_hw *clk_scaler; > struct reset_control *rst; > int vref; > + u32 sample_period_ns; > }; > > #define ASPEED_CHAN(_idx, _data_reg_addr) { \ > @@ -119,6 +126,24 @@ static const struct iio_chan_spec aspeed_adc_iio_channels[] = { > ASPEED_CHAN(15, 0x2E), > }; > > +static int aspeed_adc_set_sampling_rate(struct iio_dev *indio_dev, u32 rate) > +{ > + struct aspeed_adc_data *data = iio_priv(indio_dev); > + > + if (rate < data->model_data->min_sampling_rate || > + rate > data->model_data->max_sampling_rate) > + return -EINVAL; > + /* Each sampling needs 12 clocks to covert.*/ convert. Please run a spell checker over these patches. > + clk_set_rate(data->clk_scaler->clk, rate * ASPEED_CLOCKS_PER_SAMPLE); > + rate = clk_get_rate(data->clk_scaler->clk); > + data->sample_period_ns = DIV_ROUND_UP_ULL( > + (u64)NSEC_PER_SEC * ASPEED_CLOCKS_PER_SAMPLE, rate); > + dev_dbg(data->dev, "Adc clock = %d sample period = %d ns", rate, > + data->sample_period_ns); > + > + return 0; > +} > + > static int aspeed_adc_read_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int *val, int *val2, long mask) > @@ -149,17 +174,10 @@ static int aspeed_adc_write_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int val, int val2, long mask) > { > - struct aspeed_adc_data *data = iio_priv(indio_dev); > > switch (mask) { > case IIO_CHAN_INFO_SAMP_FREQ: > - if (val < data->model_data->min_sampling_rate || > - val > data->model_data->max_sampling_rate) > - return -EINVAL; > - > - clk_set_rate(data->clk_scaler->clk, > - val * ASPEED_CLOCKS_PER_SAMPLE); > - return 0; > + return aspeed_adc_set_sampling_rate(indio_dev, val); > > case IIO_CHAN_INFO_SCALE: > case IIO_CHAN_INFO_RAW: > @@ -386,6 +404,20 @@ static int aspeed_adc_probe(struct platform_device *pdev) > if (ret) > return ret; > > + ret = clk_prepare_enable(data->clk_scaler->clk); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(data->dev, > + aspeed_adc_clk_disable_unprepare, > + data->clk_scaler->clk); > + if (ret) > + return ret; > + > + ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE); > + if (ret) > + return ret; > + > ret = aspeed_adc_vref_config(indio_dev); > if (ret) > return ret; > @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev) > } > > /* Start all channels in normal mode. */ Why move this code up? > - ret = clk_prepare_enable(data->clk_scaler->clk); > - if (ret) > - 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 = > readl(data->base + ASPEED_REG_ENGINE_CONTROL); > adc_engine_control_reg_val |=
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:42 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > In ast2600, ADC integrate dividing circuit at last input channel for > battery sensing. This patch use the dts property "battery-sensing" to > enable this feature makes the last channel of each adc can tolerance > higher voltage than reference voltage. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> The slight issue here is this changes the userspace ABI for the older parts. Whilst a per channel offset is perfectly valid it's still an ABI change and someone might be relying on a dodgy script that uses it. So, whilst it's a pain you should have two different chan_spec arrays and pick between them dependent on model of device. That way you can leave the old ABI untouched and add this control for the ast2600 only. > --- > drivers/iio/adc/aspeed_adc.c | 62 +++++++++++++++++++++++++++++++++--- > 1 file changed, 57 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 20caf28dff18..0c5d84e82561 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -79,10 +79,16 @@ struct aspeed_adc_model_data { > unsigned int vref_fixed; // mV > bool wait_init_sequence; > bool need_prescaler; > + bool bat_sense_sup; > u8 scaler_bit_width; > unsigned int num_channels; > }; > > +struct adc_gain { > + u8 mult; > + u8 div; > +}; > + > struct aspeed_adc_data { > struct device *dev; > const struct aspeed_adc_model_data *model_data; > @@ -96,6 +102,8 @@ struct aspeed_adc_data { > int vref; > u32 sample_period_ns; > int cv; > + bool battery_sensing; > + struct adc_gain battery_mode_gain; > }; > > #define ASPEED_CHAN(_idx, _data_reg_addr) { \ > @@ -103,10 +111,10 @@ struct aspeed_adc_data { > .indexed = 1, \ > .channel = (_idx), \ > .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) | \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > BIT(IIO_CHAN_INFO_OFFSET), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > } > > static const struct iio_chan_spec aspeed_adc_iio_channels[] = { > @@ -196,14 +204,39 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, > int *val, int *val2, long mask) > { > struct aspeed_adc_data *data = iio_priv(indio_dev); > + u32 adc_engine_control_reg_val; > > switch (mask) { > case IIO_CHAN_INFO_RAW: > - *val = readw(data->base + chan->address); > + if (data->battery_sensing && chan->channel == 7) { > + adc_engine_control_reg_val = > + readl(data->base + ASPEED_REG_ENGINE_CONTROL); > + writel(adc_engine_control_reg_val | > + FIELD_PREP(ASPEED_ADC_CH7_MODE, > + ASPEED_ADC_CH7_BAT) | > + ASPEED_ADC_BAT_SENSING_ENABLE, > + data->base + ASPEED_REG_ENGINE_CONTROL); > + /* > + * After enable battery sensing mode need to wait some time for adc stable > + * Experiment result is 1ms. > + */ > + mdelay(1); > + *val = readw(data->base + chan->address); > + *val = (*val * data->battery_mode_gain.mult) / > + data->battery_mode_gain.div; > + /* Restore control register value */ > + writel(adc_engine_control_reg_val, > + data->base + ASPEED_REG_ENGINE_CONTROL); > + } else > + *val = readw(data->base + chan->address); > return IIO_VAL_INT; > > case IIO_CHAN_INFO_OFFSET: > - *val = data->cv; > + if (data->battery_sensing && chan->channel == 7) > + *val = (data->cv * data->battery_mode_gain.mult) / > + data->battery_mode_gain.div; > + else > + *val = data->cv; > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > @@ -473,6 +506,23 @@ static int aspeed_adc_probe(struct platform_device *pdev) > if (ret) > return ret; > > + if (of_find_property(data->dev->of_node, "aspeed,battery-sensing", > + NULL)) { > + if (data->model_data->bat_sense_sup) { > + data->battery_sensing = 1; > + if (readl(data->base + ASPEED_REG_ENGINE_CONTROL) & > + ASPEED_ADC_BAT_SENSING_DIV) { > + data->battery_mode_gain.mult = 3; > + data->battery_mode_gain.div = 1; > + } else { > + data->battery_mode_gain.mult = 3; > + data->battery_mode_gain.div = 2; > + } > + } else > + dev_warn(&pdev->dev, > + "Failed to enable battey-sensing mode\n"); > + } > + > if (data->model_data->wait_init_sequence) { > adc_engine_control_reg_val = > readl(data->base + ASPEED_REG_ENGINE_CONTROL); > @@ -555,6 +605,7 @@ static const struct aspeed_adc_model_data ast2600_adc0_model_data = { > .min_sampling_rate = 10000, > .max_sampling_rate = 500000, > .wait_init_sequence = true, > + .bat_sense_sup = true, > .scaler_bit_width = 16, > .num_channels = 8, > }; > @@ -564,6 +615,7 @@ static const struct aspeed_adc_model_data ast2600_adc1_model_data = { > .min_sampling_rate = 10000, > .max_sampling_rate = 500000, > .wait_init_sequence = true, > + .bat_sense_sup = true, > .scaler_bit_width = 16, > .num_channels = 8, > };
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[] = {
Hi Jonathan,
On 2021/8/29, 11:33 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:
On Tue, 24 Aug 2021 17:12:40 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>> Add the function to set the sampling rate and keep the sampling period
>> for a driver used to wait the lastest value.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Why move the code as well as factoring out the setter function?
> I doubt it does any harm, but I'd like to understand why you did it.
> Jonathan
>> + ret = clk_prepare_enable(data->clk_scaler->clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_add_action_or_reset(data->dev,
>> + aspeed_adc_clk_disable_unprepare,
>> + data->clk_scaler->clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
>> + if (ret)
>> + return ret;
>> +
>> ret = aspeed_adc_vref_config(indio_dev);
>> if (ret)
>> return ret;
>> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>> }
>>
>> /* Start all channels in normal mode. */
> Why move this code up?
Because the ADC clock is required when initializing the ADC device.
In our system, the clock is always on. Thus, the legacy driver won't encounter any issues.
I move the clk_prepare_enable ahead of initializing phase for making the driver probe logically closer to the hardware.
>> - ret = clk_prepare_enable(data->clk_scaler->clk);
>> - if (ret)
>> - 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 =
>> readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>> adc_engine_control_reg_val |=
Best Regards,
Billy Tsai
On Mon, 30 Aug 2021 08:35:53 +0000 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > Hi Jonathan, > > On 2021/8/29, 11:33 PM, "Jonathan Cameron" <jic23@kernel.org> wrote: > > On Tue, 24 Aug 2021 17:12:40 +0800 > Billy Tsai <billy_tsai@aspeedtech.com> wrote: > > >> Add the function to set the sampling rate and keep the sampling period > >> for a driver used to wait the lastest value. > >> > >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > > > Why move the code as well as factoring out the setter function? > > I doubt it does any harm, but I'd like to understand why you did it. > > > Jonathan > > >> + ret = clk_prepare_enable(data->clk_scaler->clk); > >> + if (ret) > >> + return ret; > >> + > >> + ret = devm_add_action_or_reset(data->dev, > >> + aspeed_adc_clk_disable_unprepare, > >> + data->clk_scaler->clk); > >> + if (ret) > >> + return ret; > >> + > >> + ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE); > >> + if (ret) > >> + return ret; > >> + > >> ret = aspeed_adc_vref_config(indio_dev); > >> if (ret) > >> return ret; > >> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev) > >> } > >> > >> /* Start all channels in normal mode. */ > > > Why move this code up? > > Because the ADC clock is required when initializing the ADC device. > In our system, the clock is always on. Thus, the legacy driver won't encounter any issues. > I move the clk_prepare_enable ahead of initializing phase for making the driver probe logically closer to the hardware. Thanks. Please add something to the patch description to say this. Jonathan > > >> - ret = clk_prepare_enable(data->clk_scaler->clk); > >> - if (ret) > >> - 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 = > >> readl(data->base + ASPEED_REG_ENGINE_CONTROL); > >> adc_engine_control_reg_val |= > > > Best Regards, > Billy Tsai >
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