| Message ID | 20210831071458.2334-1-billy_tsai@aspeedtech.com |
|---|---|
| Headers | show |
| Series | Add support for ast2600 ADC | expand |
On Tue, 31 Aug 2021 15:14:44 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > Fix the issue when adc remove will get the null driver data. > > Fixed: commit 573803234e72 ("iio: Aspeed ADC") > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Thanks Billy Applied to the fixes-togreg branch of iio.git and marked for stable. Jonathan > --- > drivers/iio/adc/aspeed_adc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 19efaa41bc34..34ec0c28b2df 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -183,6 +183,7 @@ static int aspeed_adc_probe(struct platform_device *pdev) > > data = iio_priv(indio_dev); > data->dev = &pdev->dev; > + platform_set_drvdata(pdev, indio_dev); > > data->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(data->base))
On Tue, 31 Aug 2021 15:14:46 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch completes the declare of ADC register bitfields and uses the > same prefix ASPEED_ADC_* for these bitfields. In addition, tidy up space > alignment of the codes. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> LGTM Applied to the togreg branch of iio.git. Note this won't be going out as non rebasing for a while yet (given mid merge window) so if anyone else has time to look at this that would be much appreciated! Jonathan > --- > drivers/iio/adc/aspeed_adc.c | 64 ++++++++++++++++++++++++++---------- > 1 file changed, 47 insertions(+), 17 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 34ec0c28b2df..f055fe7b2c40 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -3,6 +3,7 @@ > * Aspeed AST2400/2500 ADC > * > * Copyright (C) 2017 Google, Inc. > + * Copyright (C) 2021 Aspeed Technology Inc. > */ > > #include <linux/clk.h> > @@ -16,6 +17,7 @@ > #include <linux/reset.h> > #include <linux/spinlock.h> > #include <linux/types.h> > +#include <linux/bitfield.h> > > #include <linux/iio/iio.h> > #include <linux/iio/driver.h> > @@ -28,15 +30,39 @@ > #define ASPEED_REG_INTERRUPT_CONTROL 0x04 > #define ASPEED_REG_VGA_DETECT_CONTROL 0x08 > #define ASPEED_REG_CLOCK_CONTROL 0x0C > -#define ASPEED_REG_MAX 0xC0 > - > -#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1) > -#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1) > -#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1) > - > -#define ASPEED_ENGINE_ENABLE BIT(0) > - > -#define ASPEED_ADC_CTRL_INIT_RDY BIT(8) > +#define ASPEED_REG_COMPENSATION_TRIM 0xC4 > +/* > + * The register offset between 0xC8~0xCC can be read and won't affect the > + * hardware logic in each version of ADC. > + */ > +#define ASPEED_REG_MAX 0xD0 > + > +#define ASPEED_ADC_ENGINE_ENABLE BIT(0) > +#define ASPEED_ADC_OP_MODE GENMASK(3, 1) > +#define ASPEED_ADC_OP_MODE_PWR_DOWN 0 > +#define ASPEED_ADC_OP_MODE_STANDBY 1 > +#define ASPEED_ADC_OP_MODE_NORMAL 7 > +#define ASPEED_ADC_CTRL_COMPENSATION BIT(4) > +#define ASPEED_ADC_AUTO_COMPENSATION BIT(5) > +/* > + * Bit 6 determines not only the reference voltage range but also the dividing > + * circuit for battery sensing. > + */ > +#define ASPEED_ADC_REF_VOLTAGE GENMASK(7, 6) > +#define ASPEED_ADC_REF_VOLTAGE_2500mV 0 > +#define ASPEED_ADC_REF_VOLTAGE_1200mV 1 > +#define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH 2 > +#define ASPEED_ADC_REF_VOLTAGE_EXT_LOW 3 > +#define ASPEED_ADC_BAT_SENSING_DIV BIT(6) > +#define ASPEED_ADC_BAT_SENSING_DIV_2_3 0 > +#define ASPEED_ADC_BAT_SENSING_DIV_1_3 1 > +#define ASPEED_ADC_CTRL_INIT_RDY BIT(8) > +#define ASPEED_ADC_CH7_MODE BIT(12) > +#define ASPEED_ADC_CH7_NORMAL 0 > +#define ASPEED_ADC_CH7_BAT 1 > +#define ASPEED_ADC_BAT_SENSING_ENABLE BIT(13) > +#define ASPEED_ADC_CTRL_CHANNEL GENMASK(31, 16) > +#define ASPEED_ADC_CTRL_CHANNEL_ENABLE(ch) FIELD_PREP(ASPEED_ADC_CTRL_CHANNEL, BIT(ch)) > > #define ASPEED_ADC_INIT_POLLING_TIME 500 > #define ASPEED_ADC_INIT_TIMEOUT 500000 > @@ -227,7 +253,9 @@ static int aspeed_adc_probe(struct platform_device *pdev) > > if (model_data->wait_init_sequence) { > /* Enable engine in normal mode. */ > - writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE, > + writel(FIELD_PREP(ASPEED_ADC_OP_MODE, > + ASPEED_ADC_OP_MODE_NORMAL) | > + ASPEED_ADC_ENGINE_ENABLE, > data->base + ASPEED_REG_ENGINE_CONTROL); > > /* Wait for initial sequence complete. */ > @@ -246,10 +274,12 @@ static int aspeed_adc_probe(struct platform_device *pdev) > if (ret) > goto clk_enable_error; > > - adc_engine_control_reg_val = GENMASK(31, 16) | > - ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE; > + adc_engine_control_reg_val = > + ASPEED_ADC_CTRL_CHANNEL | > + FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_NORMAL) | > + ASPEED_ADC_ENGINE_ENABLE; > writel(adc_engine_control_reg_val, > - data->base + ASPEED_REG_ENGINE_CONTROL); > + data->base + ASPEED_REG_ENGINE_CONTROL); > > model_data = of_device_get_match_data(&pdev->dev); > indio_dev->name = model_data->model_name; > @@ -265,8 +295,8 @@ static int aspeed_adc_probe(struct platform_device *pdev) > return 0; > > iio_register_error: > - writel(ASPEED_OPERATION_MODE_POWER_DOWN, > - data->base + ASPEED_REG_ENGINE_CONTROL); > + 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: > @@ -284,8 +314,8 @@ static int aspeed_adc_remove(struct platform_device *pdev) > struct aspeed_adc_data *data = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > - writel(ASPEED_OPERATION_MODE_POWER_DOWN, > - data->base + ASPEED_REG_ENGINE_CONTROL); > + 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); > reset_control_assert(data->rst); > clk_hw_unregister_divider(data->clk_scaler);
On Tue, 31 Aug 2021 15:14:48 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: Title. Refactor (refactory isn't a word as far as I know though it perhaps should be ;) If you are rerolling the latter part of this series, merge patch 7 down into this one. If not, it's fine as it stands. That one is trivial and a direct result of this one. Jonathan > This patch refactors the model data structure to distinguish the > function form different versions of aspeed ADC. > - Rename the vref_voltage to vref_fixed_mv and add vref_mv driver data > When driver probe will check vref_fixed_mv value and store it to vref_mv > which isn't const value. > - Add num_channels > Make num_channles of iio device can be changed by different model_data > - Add need_prescaler flag and scaler_bit_width > The need_prescaler flag is used to tell the driver the clock divider needs > another Prescaler and the scaler_bit_width to set the clock divider > bitfield width. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/iio/adc/aspeed_adc.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 76ae1c3f584b..6ce2f676c54a 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -71,8 +71,11 @@ struct aspeed_adc_model_data { > const char *model_name; > unsigned int min_sampling_rate; // Hz > unsigned int max_sampling_rate; // Hz > - unsigned int vref_voltage; // mV > + unsigned int vref_fixed_mv; > bool wait_init_sequence; > + bool need_prescaler; > + u8 scaler_bit_width; > + unsigned int num_channels; > }; > > struct aspeed_adc_data { > @@ -83,6 +86,7 @@ struct aspeed_adc_data { > struct clk_hw *clk_prescaler; > struct clk_hw *clk_scaler; > struct reset_control *rst; > + int vref_mv; > }; > > #define ASPEED_CHAN(_idx, _data_reg_addr) { \ > @@ -126,7 +130,7 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > - *val = data->model_data->vref_voltage; > + *val = data->model_data->vref_fixed_mv; > *val2 = ASPEED_RESOLUTION_BITS; > return IIO_VAL_FRACTIONAL_LOG2; > > @@ -320,17 +324,23 @@ static int aspeed_adc_remove(struct platform_device *pdev) > > static const struct aspeed_adc_model_data ast2400_model_data = { > .model_name = "ast2400-adc", > - .vref_voltage = 2500, // mV > + .vref_fixed_mv = 2500, > .min_sampling_rate = 10000, > .max_sampling_rate = 500000, > + .need_prescaler = true, > + .scaler_bit_width = 10, > + .num_channels = 16, > }; > > static const struct aspeed_adc_model_data ast2500_model_data = { > .model_name = "ast2500-adc", > - .vref_voltage = 1800, // mV > + .vref_fixed_mv = 1800, > .min_sampling_rate = 1, > .max_sampling_rate = 1000000, > .wait_init_sequence = true, > + .need_prescaler = true, > + .scaler_bit_width = 10, > + .num_channels = 16, > }; > > static const struct of_device_id aspeed_adc_matches[] = {
On Tue, 31 Aug 2021 15:14:54 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > The ADC clock formula is > ast2400/2500: > ADC clock period = PCLK * 2 * (ADC0C[31:17] + 1) * (ADC0C[9:0] + 1) > ast2600: > ADC clock period = PCLK * 2 * (ADC0C[15:0] + 1) > They all have one fixed divided 2 and the legacy driver didn't handle it. > This patch register the fixed factory clock device as the parent of ADC > clock scaler to fix this issue. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/iio/adc/aspeed_adc.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 40b7ba58c1dc..349377b9fac0 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -4,6 +4,12 @@ > * > * Copyright (C) 2017 Google, Inc. > * Copyright (C) 2021 Aspeed Technology Inc. > + * > + * ADC clock formula: > + * Ast2400/Ast2500: > + * clock period = period of PCLK * 2 * (ADC0C[31:17] + 1) * (ADC0C[9:0] + 1) > + * Ast2600: > + * clock period = period of PCLK * 2 * (ADC0C[15:0] + 1) > */ > > #include <linux/clk.h> > @@ -85,6 +91,7 @@ struct aspeed_adc_data { > struct regulator *regulator; > void __iomem *base; > spinlock_t clk_lock; > + struct clk_hw *fixed_div_clk; > struct clk_hw *clk_prescaler; > struct clk_hw *clk_scaler; > struct reset_control *rst; > @@ -204,6 +211,13 @@ static void aspeed_adc_unregister_divider(void *data) > clk_hw_unregister_divider(clk); > } > > +static void aspeed_adc_unregister_fixed_divider(void *data) > +{ > + struct clk_hw *clk = data; > + > + clk_hw_unregister_fixed_factor(clk); > +} > + > static void aspeed_adc_reset_assert(void *data) > { > struct reset_control *rst = data; > @@ -328,6 +342,19 @@ static int aspeed_adc_probe(struct platform_device *pdev) > spin_lock_init(&data->clk_lock); > snprintf(clk_parent_name, ARRAY_SIZE(clk_parent_name), "%s", > of_clk_get_parent_name(pdev->dev.of_node, 0)); > + snprintf(clk_name, ARRAY_SIZE(clk_name), "%s-fixed-div", > + data->model_data->model_name); > + data->fixed_div_clk = clk_hw_register_fixed_factor( > + &pdev->dev, clk_name, clk_parent_name, 0, 1, 2); Obvious follow on from Philipp's review - there is a devm_ version of this as well which you can use rather than rolling a custom version. As a side note, I'm fairly sure you could refactor all those devm_clk_hw functions to use devm_add_action_or_reset() internally and simplify that code nicely. A recent series did the same for all the similar functions in IIO. > + if (IS_ERR(data->fixed_div_clk)) > + return PTR_ERR(data->fixed_div_clk); > + > + ret = devm_add_action_or_reset(data->dev, > + aspeed_adc_unregister_fixed_divider, > + data->clk_prescaler); > + if (ret) > + return ret; > + snprintf(clk_parent_name, ARRAY_SIZE(clk_parent_name), clk_name); > > if (data->model_data->need_prescaler) { > snprintf(clk_name, ARRAY_SIZE(clk_name), "%s-prescaler",
This patch serials make aspeed_adc.c can support ast2600 and backward compatible. Change since v4: dt-bindings: - Add clocks maxItems. - Rename the property to meet the property-units.yaml. - Add the description for the difference between adc0 and adc1. aspeed_adc.c: - Use new property name to get internal reference voltage: units from mv to uv. - Fix -Wnonnull warning caused by snprintf parameters. - Add suffix mv to the vref parameters. - Use ARRAY_SIZE instead of 32. - Add a reset action for ADC power down and Use devm_iio_device_register. - Fix typo error. - Separate the offset interface of ch7 when battery sensing enable 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): iio: adc: aspeed: set driver data when adc probe. dt-bindings: iio: adc: Add ast2600-adc bindings iio: adc: aspeed: completes the bitfield declare. 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 | 100 +++ drivers/iio/adc/aspeed_adc.c | 617 +++++++++++++++--- 2 files changed, 619 insertions(+), 98 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed,ast2600-adc.yaml