* [PATCH 0/3] Make driver compatible with ast2600
@ 2020-10-13 10:32 Billy Tsai
2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Billy Tsai @ 2020-10-13 10:32 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel,
billy_tsai, alexandru.ardelean, linux-iio, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel
Cc: BMC-SW
The ast2600 is a new generation of SoC from ASPEED.
The adc device in this generation adds some changes and features.
This patch series handles the changes below:
1. Define the new register fields.
2. Split into two individual IPs and each contains 8 voltage channels.
3. Remove the pre-scaler and extend the field length of the scaler.
4. Ref_voltage becomes configurable.
Billy Tsai (3):
iio: adc: aspeed: Orgnaize and add the define of adc
iio: adc: aspeed: Make driver compatible with ast2600
iio: adc: aspeed: Setting ref_voltage in probe
.../bindings/iio/adc/aspeed_adc.txt | 16 +-
drivers/iio/adc/aspeed_adc.c | 168 ++++++++++++++----
2 files changed, 148 insertions(+), 36 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc 2020-10-13 10:32 [PATCH 0/3] Make driver compatible with ast2600 Billy Tsai @ 2020-10-13 10:32 ` Billy Tsai 2020-10-18 10:22 ` Jonathan Cameron 2020-10-13 10:32 ` [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 Billy Tsai 2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai 2 siblings, 1 reply; 9+ messages in thread From: Billy Tsai @ 2020-10-13 10:32 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, billy_tsai, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel Cc: BMC-SW This patch organizes the define of adc to multiple partitions and adds the new bit field define for ast2600 driver. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- drivers/iio/adc/aspeed_adc.c | 42 ++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c index 1e5375235cfe..ae400c4d6d40 100644 --- a/drivers/iio/adc/aspeed_adc.c +++ b/drivers/iio/adc/aspeed_adc.c @@ -21,23 +21,57 @@ #include <linux/iio/driver.h> #include <linux/iopoll.h> +/********************************************************** + * ADC feature define + *********************************************************/ #define ASPEED_RESOLUTION_BITS 10 #define ASPEED_CLOCKS_PER_SAMPLE 12 +/********************************************************** + * ADC HW register offset define + *********************************************************/ #define ASPEED_REG_ENGINE_CONTROL 0x00 #define ASPEED_REG_INTERRUPT_CONTROL 0x04 #define ASPEED_REG_VGA_DETECT_CONTROL 0x08 #define ASPEED_REG_CLOCK_CONTROL 0x0C +#define ASPEED_REG_COMPENSATION_TRIM 0xC4 #define ASPEED_REG_MAX 0xC0 +/********************************************************** + * ADC register Bit field + *********************************************************/ +/*ENGINE_CONTROL */ +/* [0] */ +#define ASPEED_ENGINE_ENABLE BIT(0) +/* [3:1] */ #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) - +/* [4] */ +#define ASPEED_CTRL_COMPENSATION BIT(4) +/* [5] */ +#define ASPEED_AUTOPENSATING BIT(5) +/* [7:6] */ +#define ASPEED_REF_VOLTAGE_2500mV (0 << 6) +#define ASPEED_REF_VOLTAGE_1200mV (1 << 6) +#define ASPEED_REF_VOLTAGE_EXT_HIGH (2 << 6) +#define ASPEED_REF_VOLTAGE_EXT_LOW (3 << 6) +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_2_3 (0 << 6) +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_1_3 (1 << 6) +/* [8] */ #define ASPEED_ADC_CTRL_INIT_RDY BIT(8) - +/* [12] */ +#define ASPEED_ADC_CH7_VOLTAGE_NORMAL (0 << 12) +#define ASPEED_ADC_CH7_VOLTAGE_BATTERY (1 << 12) +/* [13] */ +#define ASPEED_ADC_EN_BATTERY_SENSING BIT(13) +/* [31:16] */ +#define ASPEED_ADC_CTRL_CH_EN(n) (1 << (16 + n)) +#define ASPEED_ADC_CTRL_CH_EN_ALL GENMASK(31, 16) + +/********************************************************** + * Software setting + *********************************************************/ #define ASPEED_ADC_INIT_POLLING_TIME 500 #define ASPEED_ADC_INIT_TIMEOUT 500000 -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc 2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai @ 2020-10-18 10:22 ` Jonathan Cameron 2020-10-18 18:26 ` Andy Shevchenko 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Cameron @ 2020-10-18 10:22 UTC (permalink / raw) To: Billy Tsai Cc: knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel, BMC-SW On Tue, 13 Oct 2020 18:32:43 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > This patch organizes the define of adc to multiple partitions > and adds the new bit field define for ast2600 driver. Should be 2 patch patches. If you need to do a reorg, do it first, then add new bits in a second patch. That way we are reviewing one non functional change, and one that actually does something. As a general rule, I'd also prefer to just see the additional defines added as and when they are used (in the patch that first uses them). > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/iio/adc/aspeed_adc.c | 42 ++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index 1e5375235cfe..ae400c4d6d40 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -21,23 +21,57 @@ > #include <linux/iio/driver.h> > #include <linux/iopoll.h> > > +/********************************************************** > + * ADC feature define > + *********************************************************/ I'm generally of the view that block comments like this normally imply the defines are not sufficiently self identifying. It should be possible to know what sort of thing they are at the point of use without having to go find a comment in the header. So if you think these are needed, perhaps reconsider the naming of of the defines? Personally I just don't seem them as necessary. Like all comments, they tend to 'rot' over time so if they aren't adding information, better not to have them! > #define ASPEED_RESOLUTION_BITS 10 > #define ASPEED_CLOCKS_PER_SAMPLE 12 > > +/********************************************************** > + * ADC HW register offset define > + *********************************************************/ > #define ASPEED_REG_ENGINE_CONTROL 0x00 > #define ASPEED_REG_INTERRUPT_CONTROL 0x04 > #define ASPEED_REG_VGA_DETECT_CONTROL 0x08 > #define ASPEED_REG_CLOCK_CONTROL 0x0C > +#define ASPEED_REG_COMPENSATION_TRIM 0xC4 > #define ASPEED_REG_MAX 0xC0 > > +/********************************************************** > + * ADC register Bit field > + *********************************************************/ > +/*ENGINE_CONTROL */ Inconsistent spacing after /* > +/* [0] */ > +#define ASPEED_ENGINE_ENABLE BIT(0) > +/* [3:1] */ If the docs are needed, then better to use FIELD_PREP and GENMASK as that is going to be self documenting at the actual point of the defines. > #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) > - > +/* [4] */ > +#define ASPEED_CTRL_COMPENSATION BIT(4) > +/* [5] */ > +#define ASPEED_AUTOPENSATING BIT(5) > +/* [7:6] */ > +#define ASPEED_REF_VOLTAGE_2500mV (0 << 6) > +#define ASPEED_REF_VOLTAGE_1200mV (1 << 6) > +#define ASPEED_REF_VOLTAGE_EXT_HIGH (2 << 6) > +#define ASPEED_REF_VOLTAGE_EXT_LOW (3 << 6) > +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_2_3 (0 << 6) > +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_1_3 (1 << 6) > +/* [8] */ > #define ASPEED_ADC_CTRL_INIT_RDY BIT(8) > - > +/* [12] */ > +#define ASPEED_ADC_CH7_VOLTAGE_NORMAL (0 << 12) > +#define ASPEED_ADC_CH7_VOLTAGE_BATTERY (1 << 12) > +/* [13] */ > +#define ASPEED_ADC_EN_BATTERY_SENSING BIT(13) > +/* [31:16] */ > +#define ASPEED_ADC_CTRL_CH_EN(n) (1 << (16 + n)) > +#define ASPEED_ADC_CTRL_CH_EN_ALL GENMASK(31, 16) > + > +/********************************************************** > + * Software setting > + *********************************************************/ > #define ASPEED_ADC_INIT_POLLING_TIME 500 > #define ASPEED_ADC_INIT_TIMEOUT 500000 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc 2020-10-18 10:22 ` Jonathan Cameron @ 2020-10-18 18:26 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-10-18 18:26 UTC (permalink / raw) To: Jonathan Cameron Cc: Billy Tsai, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Rob Herring, Joel Stanley, Andrew Jeffery, Philipp Zabel, Alexandru Ardelean, linux-iio, devicetree, linux-arm Mailing List, linux-aspeed, Linux Kernel Mailing List, BMC-SW On Sun, Oct 18, 2020 at 1:32 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 13 Oct 2020 18:32:43 +0800 > Billy Tsai <billy_tsai@aspeedtech.com> wrote: ... > > +/* [31:16] */ Useless comment. > > +#define ASPEED_ADC_CTRL_CH_EN(n) (1 << (16 + n)) > > +#define ASPEED_ADC_CTRL_CH_EN_ALL GENMASK(31, 16) But the main point is here. First of all you may use BIT() in above. Second, it's a good practice to put variables in the additional parentheses in macros in case you are doing some operations with. So, something like BIT(16 + (n)) would be nice to have at the end. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 2020-10-13 10:32 [PATCH 0/3] Make driver compatible with ast2600 Billy Tsai 2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai @ 2020-10-13 10:32 ` Billy Tsai 2020-10-18 10:46 ` Jonathan Cameron 2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai 2 siblings, 1 reply; 9+ messages in thread From: Billy Tsai @ 2020-10-13 10:32 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, billy_tsai, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel Cc: BMC-SW This patch is used to handle the difference between ast2600 and previous versions. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- drivers/iio/adc/aspeed_adc.c | 129 ++++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 34 deletions(-) diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c index ae400c4d6d40..fc4bbccf9348 100644 --- a/drivers/iio/adc/aspeed_adc.c +++ b/drivers/iio/adc/aspeed_adc.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Aspeed AST2400/2500 ADC + * Aspeed AST2400/2500/2600 ADC * * Copyright (C) 2017 Google, Inc. */ @@ -81,6 +81,7 @@ struct aspeed_adc_model_data { unsigned int max_sampling_rate; // Hz unsigned int vref_voltage; // mV bool wait_init_sequence; + int num_channels; }; struct aspeed_adc_data { @@ -90,6 +91,7 @@ struct aspeed_adc_data { struct clk_hw *clk_prescaler; struct clk_hw *clk_scaler; struct reset_control *rst; + unsigned int vref_voltage; // mV }; #define ASPEED_CHAN(_idx, _data_reg_addr) { \ @@ -126,8 +128,6 @@ 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); - const struct aspeed_adc_model_data *model_data = - of_device_get_match_data(data->dev); switch (mask) { case IIO_CHAN_INFO_RAW: @@ -135,7 +135,7 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - *val = model_data->vref_voltage; + *val = data->vref_voltage; *val2 = ASPEED_RESOLUTION_BITS; return IIO_VAL_FRACTIONAL_LOG2; @@ -208,8 +208,13 @@ static int aspeed_adc_probe(struct platform_device *pdev) struct aspeed_adc_data *data; const struct aspeed_adc_model_data *model_data; const char *clk_parent_name; + char prescaler_clk_name[32]; + char scaler_clk_name[32]; int ret; u32 adc_engine_control_reg_val; + u32 ref_voltage_cfg = 0; + + model_data = of_device_get_match_data(&pdev->dev); indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data)); if (!indio_dev) @@ -225,29 +230,75 @@ 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); + snprintf(prescaler_clk_name, sizeof(prescaler_clk_name), + "prescaler-%s", pdev->name); + snprintf(scaler_clk_name, sizeof(scaler_clk_name), + "scaler-%s", pdev->name); + if (!strcmp(model_data->model_name, "ast2400-adc") || + !strcmp(model_data->model_name, "ast2500-adc")) { + /* Divider config */ + data->clk_prescaler = clk_hw_register_divider( + &pdev->dev, prescaler_clk_name, clk_parent_name, + 0, + data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, + CLK_DIVIDER_ONE_BASED, &data->clk_lock); + if (IS_ERR(data->clk_prescaler)) + return PTR_ERR(data->clk_prescaler); - 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); - - /* - * Register ADC clock scaler downstream from the prescaler. Allow rate - * setting to adjust the prescaler as well. - */ - 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); - if (IS_ERR(data->clk_scaler)) { - ret = PTR_ERR(data->clk_scaler); - goto scaler_error; + /* + * Register ADC clock scaler downstream from the prescaler. Allow rate + * setting to adjust the prescaler as well. + */ + data->clk_scaler = clk_hw_register_divider( + &pdev->dev, scaler_clk_name, prescaler_clk_name, + CLK_SET_RATE_PARENT, + data->base + ASPEED_REG_CLOCK_CONTROL, 0, 10, + CLK_DIVIDER_ONE_BASED, &data->clk_lock); + if (IS_ERR(data->clk_scaler)) { + ret = PTR_ERR(data->clk_scaler); + goto scaler_error; + } + /* Get ref_voltage */ + data->vref_voltage = model_data->vref_voltage; + } else if (!strcmp(model_data->model_name, "ast2600-adc")) { + /* Divider config */ + data->clk_scaler = clk_hw_register_divider( + &pdev->dev, scaler_clk_name, clk_parent_name, + CLK_SET_RATE_UNGATE, + data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16, + CLK_DIVIDER_ONE_BASED, &data->clk_lock); + if (IS_ERR(data->clk_scaler)) + return PTR_ERR(data->clk_scaler); + /* + * Get ref_voltage: + * If reference voltage is between 1550~1650mv, we can set + * fields either ASPEED_REF_VOLTAGE_EXT_HIGH or ASPEED_REF_VOLTAGE_EXT_LOW. + * In this place, we select ASPEED_REF_VOLTAGE_EXT_HIGH as higher priority. + */ + if (!of_property_read_u32(pdev->dev.of_node, "ref_voltage", + &data->vref_voltage)) { + if (data->vref_voltage == 2500) + ref_voltage_cfg = ASPEED_REF_VOLTAGE_2500mV; + else if (data->vref_voltage == 1200) + ref_voltage_cfg = ASPEED_REF_VOLTAGE_1200mV; + else if ((data->vref_voltage >= 1550) && + (data->vref_voltage <= 2700)) + ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_HIGH; + else if ((data->vref_voltage >= 900) && + (data->vref_voltage <= 1650)) + ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_LOW; + else { + dev_err(&pdev->dev, "ref_voltage property is out of range: %d\n", + data->vref_voltage); + return -EINVAL; + } + } else { + dev_err(&pdev->dev, "Couldn't read ref_voltage property\n"); + return -EINVAL; + } } - data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL); + data->rst = devm_reset_control_get_shared(&pdev->dev, NULL); if (IS_ERR(data->rst)) { dev_err(&pdev->dev, "invalid or missing reset controller device tree entry"); @@ -256,13 +307,14 @@ static int aspeed_adc_probe(struct platform_device *pdev) } reset_control_deassert(data->rst); - model_data = of_device_get_match_data(&pdev->dev); + adc_engine_control_reg_val = readl(data->base + ASPEED_REG_ENGINE_CONTROL); + /* Enable engine in normal mode and set ref_voltage */ + adc_engine_control_reg_val |= (ASPEED_OPERATION_MODE_NORMAL | + ASPEED_ENGINE_ENABLE | ref_voltage_cfg); + writel(adc_engine_control_reg_val, + data->base + ASPEED_REG_ENGINE_CONTROL); if (model_data->wait_init_sequence) { - /* Enable engine in normal mode. */ - writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE, - data->base + ASPEED_REG_ENGINE_CONTROL); - /* Wait for initial sequence complete. */ ret = readl_poll_timeout(data->base + ASPEED_REG_ENGINE_CONTROL, adc_engine_control_reg_val, @@ -279,18 +331,16 @@ 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_CH_EN_ALL; writel(adc_engine_control_reg_val, data->base + ASPEED_REG_ENGINE_CONTROL); - model_data = of_device_get_match_data(&pdev->dev); indio_dev->name = model_data->model_name; indio_dev->dev.parent = &pdev->dev; indio_dev->info = &aspeed_adc_iio_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = aspeed_adc_iio_channels; - indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels); + indio_dev->num_channels = model_data->num_channels; ret = iio_device_register(indio_dev); if (ret) @@ -333,6 +383,7 @@ static const struct aspeed_adc_model_data ast2400_model_data = { .vref_voltage = 2500, // mV .min_sampling_rate = 10000, .max_sampling_rate = 500000, + .num_channels = 16, }; static const struct aspeed_adc_model_data ast2500_model_data = { @@ -341,11 +392,21 @@ static const struct aspeed_adc_model_data ast2500_model_data = { .min_sampling_rate = 1, .max_sampling_rate = 1000000, .wait_init_sequence = true, + .num_channels = 16, +}; + +static const struct aspeed_adc_model_data ast2600_model_data = { + .model_name = "ast2600-adc", + .min_sampling_rate = 1, + .max_sampling_rate = 1000000, + .wait_init_sequence = true, + .num_channels = 8, }; static const struct of_device_id aspeed_adc_matches[] = { { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data }, { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data }, + { .compatible = "aspeed,ast2600-adc", .data = &ast2600_model_data }, {}, }; MODULE_DEVICE_TABLE(of, aspeed_adc_matches); @@ -362,5 +423,5 @@ static struct platform_driver aspeed_adc_driver = { module_platform_driver(aspeed_adc_driver); MODULE_AUTHOR("Rick Altherr <raltherr@google.com>"); -MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver"); +MODULE_DESCRIPTION("Aspeed AST2400/2500/2600 ADC Driver"); MODULE_LICENSE("GPL"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 2020-10-13 10:32 ` [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 Billy Tsai @ 2020-10-18 10:46 ` Jonathan Cameron 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2020-10-18 10:46 UTC (permalink / raw) To: Billy Tsai Cc: knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel, BMC-SW On Tue, 13 Oct 2020 18:32:44 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: Hi Billy, > This patch is used to handle the difference between ast2600 > and previous versions. Good to mention what they are. Not all of them are obvious (such as the reset change to being requested as shared). > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Patch is a mixture of adding the new support, and adding various 'hooks' to enable the differences. I'd rather see it as two patches. First one adds the hooks but makes no functional change, second patch adds the new device support. That makes for easier reviewing for me! > --- > drivers/iio/adc/aspeed_adc.c | 129 ++++++++++++++++++++++++++--------- > 1 file changed, 95 insertions(+), 34 deletions(-) > > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > index ae400c4d6d40..fc4bbccf9348 100644 > --- a/drivers/iio/adc/aspeed_adc.c > +++ b/drivers/iio/adc/aspeed_adc.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * Aspeed AST2400/2500 ADC > + * Aspeed AST2400/2500/2600 ADC > * > * Copyright (C) 2017 Google, Inc. > */ > @@ -81,6 +81,7 @@ struct aspeed_adc_model_data { > unsigned int max_sampling_rate; // Hz > unsigned int vref_voltage; // mV Add a comment to this to say that not all devices use a fixed vref_voltage. > bool wait_init_sequence; > + int num_channels; > }; > > struct aspeed_adc_data { > @@ -90,6 +91,7 @@ struct aspeed_adc_data { > struct clk_hw *clk_prescaler; > struct clk_hw *clk_scaler; > struct reset_control *rst; > + unsigned int vref_voltage; // mV If the naming can include the unit, then we both don't need the comment and the units are apparent at the point of use. vref_mV; for example. A good cleanup would be to do similar for the other cases in the driver. > }; > > #define ASPEED_CHAN(_idx, _data_reg_addr) { \ > @@ -126,8 +128,6 @@ 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); > - const struct aspeed_adc_model_data *model_data = > - of_device_get_match_data(data->dev); > > switch (mask) { > case IIO_CHAN_INFO_RAW: > @@ -135,7 +135,7 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev, > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > - *val = model_data->vref_voltage; > + *val = data->vref_voltage; > *val2 = ASPEED_RESOLUTION_BITS; > return IIO_VAL_FRACTIONAL_LOG2; > > @@ -208,8 +208,13 @@ static int aspeed_adc_probe(struct platform_device *pdev) > struct aspeed_adc_data *data; > const struct aspeed_adc_model_data *model_data; > const char *clk_parent_name; > + char prescaler_clk_name[32]; > + char scaler_clk_name[32]; > int ret; > u32 adc_engine_control_reg_val; > + u32 ref_voltage_cfg = 0; > + > + model_data = of_device_get_match_data(&pdev->dev); > > indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data)); > if (!indio_dev) > @@ -225,29 +230,75 @@ 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); > + snprintf(prescaler_clk_name, sizeof(prescaler_clk_name), > + "prescaler-%s", pdev->name); > + snprintf(scaler_clk_name, sizeof(scaler_clk_name), > + "scaler-%s", pdev->name); > + if (!strcmp(model_data->model_name, "ast2400-adc") || > + !strcmp(model_data->model_name, "ast2500-adc")) { It would be nice to avoid all of these string comparisons if possible. Various options come to mind: 1) Put a flag for each possible thing in the mode_data structures so we just check that to make decisions in probe. 2) Compare against the model_data pointers (bit ugly) 3) Do comparisons just once to convert to an enum. 4) Put an enum value in each mode_data structure to identify it without a string comparison. 1 is probably the best of these options as more extensible to additional models as it provides fine grained feature information in one location rather than many. > + /* Divider config */ > + data->clk_prescaler = clk_hw_register_divider( > + &pdev->dev, prescaler_clk_name, clk_parent_name, > + 0, > + data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, > + CLK_DIVIDER_ONE_BASED, &data->clk_lock); > + if (IS_ERR(data->clk_prescaler)) > + return PTR_ERR(data->clk_prescaler); > > - 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); > - > - /* > - * Register ADC clock scaler downstream from the prescaler. Allow rate > - * setting to adjust the prescaler as well. > - */ > - 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); > - if (IS_ERR(data->clk_scaler)) { > - ret = PTR_ERR(data->clk_scaler); > - goto scaler_error; > + /* > + * Register ADC clock scaler downstream from the prescaler. Allow rate > + * setting to adjust the prescaler as well. > + */ > + data->clk_scaler = clk_hw_register_divider( > + &pdev->dev, scaler_clk_name, prescaler_clk_name, > + CLK_SET_RATE_PARENT, > + data->base + ASPEED_REG_CLOCK_CONTROL, 0, 10, > + CLK_DIVIDER_ONE_BASED, &data->clk_lock); > + if (IS_ERR(data->clk_scaler)) { > + ret = PTR_ERR(data->clk_scaler); > + goto scaler_error; > + } > + /* Get ref_voltage */ If you do the 'feature flag' suggestion I make above, clearly this is a separate feature to the clock stuff and so needs it's own feature flag. > + data->vref_voltage = model_data->vref_voltage; > + } else if (!strcmp(model_data->model_name, "ast2600-adc")) { > + /* Divider config */ > + data->clk_scaler = clk_hw_register_divider( > + &pdev->dev, scaler_clk_name, clk_parent_name, > + CLK_SET_RATE_UNGATE, > + data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16, > + CLK_DIVIDER_ONE_BASED, &data->clk_lock); > + if (IS_ERR(data->clk_scaler)) > + return PTR_ERR(data->clk_scaler); > + /* > + * Get ref_voltage: > + * If reference voltage is between 1550~1650mv, we can set > + * fields either ASPEED_REF_VOLTAGE_EXT_HIGH or ASPEED_REF_VOLTAGE_EXT_LOW. > + * In this place, we select ASPEED_REF_VOLTAGE_EXT_HIGH as higher priority. > + */ > + if (!of_property_read_u32(pdev->dev.of_node, "ref_voltage", > + &data->vref_voltage)) { > + if (data->vref_voltage == 2500) > + ref_voltage_cfg = ASPEED_REF_VOLTAGE_2500mV; > + else if (data->vref_voltage == 1200) > + ref_voltage_cfg = ASPEED_REF_VOLTAGE_1200mV; > + else if ((data->vref_voltage >= 1550) && > + (data->vref_voltage <= 2700)) > + ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_HIGH; > + else if ((data->vref_voltage >= 900) && > + (data->vref_voltage <= 1650)) > + ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_LOW; > + else { > + dev_err(&pdev->dev, "ref_voltage property is out of range: %d\n", > + data->vref_voltage); > + return -EINVAL; > + } > + } else { Don't eat the error value from of_property_read_u32. It might potentially provide information on 'why' we couldn't read it. Also would reduce indentation (improve readability) to do ret = of_property_read_u32(pdev->dev.of_node, "ref_voltage", ... if (ret) { dev_err(..) return ret; } if (data->vref_voltage) ... > + dev_err(&pdev->dev, "Couldn't read ref_voltage property\n"); > + return -EINVAL; > + } > } > > - data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + data->rst = devm_reset_control_get_shared(&pdev->dev, NULL); I'd like a comment in the patch description saying why you made this change. > if (IS_ERR(data->rst)) { > dev_err(&pdev->dev, > "invalid or missing reset controller device tree entry"); > @@ -256,13 +307,14 @@ static int aspeed_adc_probe(struct platform_device *pdev) > } > reset_control_deassert(data->rst); > > - model_data = of_device_get_match_data(&pdev->dev); > + adc_engine_control_reg_val = readl(data->base + ASPEED_REG_ENGINE_CONTROL); Given this is during probe, I'd like to see an explanation comment on why we are leaving the other bits of the register in whatever state we find them in. i.e. why are we leaving some channels enabled? We weren't previously doing so... > + /* Enable engine in normal mode and set ref_voltage */ > + adc_engine_control_reg_val |= (ASPEED_OPERATION_MODE_NORMAL | > + ASPEED_ENGINE_ENABLE | ref_voltage_cfg); > + writel(adc_engine_control_reg_val, > + data->base + ASPEED_REG_ENGINE_CONTROL); > > if (model_data->wait_init_sequence) { > - /* Enable engine in normal mode. */ > - writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE, > - data->base + ASPEED_REG_ENGINE_CONTROL); > - > /* Wait for initial sequence complete. */ > ret = readl_poll_timeout(data->base + ASPEED_REG_ENGINE_CONTROL, > adc_engine_control_reg_val, > @@ -279,18 +331,16 @@ 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_CH_EN_ALL; > writel(adc_engine_control_reg_val, > data->base + ASPEED_REG_ENGINE_CONTROL); > > - model_data = of_device_get_match_data(&pdev->dev); > indio_dev->name = model_data->model_name; > indio_dev->dev.parent = &pdev->dev; > indio_dev->info = &aspeed_adc_iio_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = aspeed_adc_iio_channels; > - indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels); > + indio_dev->num_channels = model_data->num_channels; > > ret = iio_device_register(indio_dev); > if (ret) > @@ -333,6 +383,7 @@ static const struct aspeed_adc_model_data ast2400_model_data = { > .vref_voltage = 2500, // mV > .min_sampling_rate = 10000, > .max_sampling_rate = 500000, > + .num_channels = 16, > }; > > static const struct aspeed_adc_model_data ast2500_model_data = { > @@ -341,11 +392,21 @@ static const struct aspeed_adc_model_data ast2500_model_data = { > .min_sampling_rate = 1, > .max_sampling_rate = 1000000, > .wait_init_sequence = true, > + .num_channels = 16, > +}; > + > +static const struct aspeed_adc_model_data ast2600_model_data = { > + .model_name = "ast2600-adc", > + .min_sampling_rate = 1, > + .max_sampling_rate = 1000000, > + .wait_init_sequence = true, > + .num_channels = 8, > }; > > static const struct of_device_id aspeed_adc_matches[] = { > { .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data }, > { .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data }, > + { .compatible = "aspeed,ast2600-adc", .data = &ast2600_model_data }, > {}, > }; > MODULE_DEVICE_TABLE(of, aspeed_adc_matches); > @@ -362,5 +423,5 @@ static struct platform_driver aspeed_adc_driver = { > module_platform_driver(aspeed_adc_driver); > > MODULE_AUTHOR("Rick Altherr <raltherr@google.com>"); > -MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver"); > +MODULE_DESCRIPTION("Aspeed AST2400/2500/2600 ADC Driver"); > MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe 2020-10-13 10:32 [PATCH 0/3] Make driver compatible with ast2600 Billy Tsai 2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai 2020-10-13 10:32 ` [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 Billy Tsai @ 2020-10-13 10:32 ` Billy Tsai 2020-10-16 17:16 ` Rob Herring 2020-10-18 10:55 ` Jonathan Cameron 2 siblings, 2 replies; 9+ messages in thread From: Billy Tsai @ 2020-10-13 10:32 UTC (permalink / raw) To: jic23, knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, billy_tsai, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel Cc: BMC-SW At ast2600 ref_voltage becomes configurable and this property is board dependency. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- .../devicetree/bindings/iio/adc/aspeed_adc.txt | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt index 034fc2ba100e..0ba1980c4e06 100644 --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt @@ -3,8 +3,11 @@ Aspeed ADC This device is a 10-bit converter for 16 voltage channels. All inputs are single ended. +At ast2600, this device split into two individual IPs and each contains 8 voltage channels. + +Chip level dtsi: Required properties: -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc" - reg: memory window mapping address and length - clocks: Input clock used to derive the sample clock. Expected to be the SoC's APB clock. @@ -20,3 +23,14 @@ Example: resets = <&syscon ASPEED_RESET_ADC>; #io-channel-cells = <1>; }; + +Board level dts: +Required properties: +- ref_voltage: (ast2600 only) + - Reference voltage in millivolts for the conversions. + - The range of value is 900 to 2700 mv. + +Example: +&adc0 { + ref_voltage = <2500>; +}; \ No newline at end of file -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe 2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai @ 2020-10-16 17:16 ` Rob Herring 2020-10-18 10:55 ` Jonathan Cameron 1 sibling, 0 replies; 9+ messages in thread From: Rob Herring @ 2020-10-16 17:16 UTC (permalink / raw) To: Billy Tsai Cc: jic23, knaack.h, lars, pmeerw, joel, andrew, p.zabel, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel, BMC-SW On Tue, Oct 13, 2020 at 06:32:45PM +0800, Billy Tsai wrote: > At ast2600 ref_voltage becomes configurable and this property is board > dependency. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > .../devicetree/bindings/iio/adc/aspeed_adc.txt | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > index 034fc2ba100e..0ba1980c4e06 100644 > --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > @@ -3,8 +3,11 @@ Aspeed ADC > This device is a 10-bit converter for 16 voltage channels. All inputs are > single ended. > > +At ast2600, this device split into two individual IPs and each contains 8 voltage channels. > + > +Chip level dtsi: > Required properties: > -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" > +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc" > - reg: memory window mapping address and length > - clocks: Input clock used to derive the sample clock. Expected to be the > SoC's APB clock. > @@ -20,3 +23,14 @@ Example: > resets = <&syscon ASPEED_RESET_ADC>; > #io-channel-cells = <1>; > }; > + > +Board level dts: This split is convention, but not relevant to the binding. > +Required properties: > +- ref_voltage: (ast2600 only) s/_/-/ And needs a vendor prefix. > + - Reference voltage in millivolts for the conversions. > + - The range of value is 900 to 2700 mv. > + > +Example: > +&adc0 { > + ref_voltage = <2500>; > +}; > \ No newline at end of file Fix this. > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe 2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai 2020-10-16 17:16 ` Rob Herring @ 2020-10-18 10:55 ` Jonathan Cameron 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2020-10-18 10:55 UTC (permalink / raw) To: Billy Tsai Cc: knaack.h, lars, pmeerw, robh+dt, joel, andrew, p.zabel, alexandru.ardelean, linux-iio, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel, BMC-SW On Tue, 13 Oct 2020 18:32:45 +0800 Billy Tsai <billy_tsai@aspeedtech.com> wrote: > At ast2600 ref_voltage becomes configurable and this property is board > dependency. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> dt-bindings: etc for the patch title. Look at naming of similar patches for a reference. > --- > .../devicetree/bindings/iio/adc/aspeed_adc.txt | 16 +++++++++++++++- This has been converted to yaml. It is now in Linus' tree (though only very recently!) You will need to add the relevant logic to make the yaml binding express that this only exists and is required for ast600 The binding is making me wonder a bit on what this voltage actually is... I 'think' we have a setup where the reference voltage is either picked from 2 available internal references or uses an external regulator (presumably provided to a pin on the chip?) As such, the binding should be an optional regulator. If the regulator is present we use that, even if it matches the internal regulator votlage. Someone put it down on the board, so presumably they want to use it. If it's not present, then have to provide either of the two internal regulator voltages via a devicetree binding such as you have here. > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > index 034fc2ba100e..0ba1980c4e06 100644 > --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt > @@ -3,8 +3,11 @@ Aspeed ADC > This device is a 10-bit converter for 16 voltage channels. All inputs are > single ended. > > +At ast2600, this device split into two individual IPs and each contains 8 voltage channels. > + > +Chip level dtsi: > Required properties: > -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" > +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc" > - reg: memory window mapping address and length > - clocks: Input clock used to derive the sample clock. Expected to be the > SoC's APB clock. > @@ -20,3 +23,14 @@ Example: > resets = <&syscon ASPEED_RESET_ADC>; > #io-channel-cells = <1>; > }; > + > +Board level dts: > +Required properties: > +- ref_voltage: (ast2600 only) > + - Reference voltage in millivolts for the conversions. > + - The range of value is 900 to 2700 mv. See above for suggestion on on this. For the internal case, will need a name that expresses both what it is and what the units are. int_ref_voltage_mv perhaps? Yaml binding should check that either we have a regulator, or we have this element. That way we'll ensure we don't have any confusion over which one to use. Thanks, Jonathan > + > +Example: > +&adc0 { > + ref_voltage = <2500>; > +}; > \ No newline at end of file ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-18 18:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-13 10:32 [PATCH 0/3] Make driver compatible with ast2600 Billy Tsai 2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai 2020-10-18 10:22 ` Jonathan Cameron 2020-10-18 18:26 ` Andy Shevchenko 2020-10-13 10:32 ` [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 Billy Tsai 2020-10-18 10:46 ` Jonathan Cameron 2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai 2020-10-16 17:16 ` Rob Herring 2020-10-18 10:55 ` 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).