Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758777AbdCVJr6 (ORCPT ); Wed, 22 Mar 2017 05:47:58 -0400 Received: from mail-yw0-f196.google.com ([209.85.161.196]:32840 "EHLO mail-yw0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758788AbdCVJrn (ORCPT ); Wed, 22 Mar 2017 05:47:43 -0400 MIME-Version: 1.0 In-Reply-To: <20170321204828.31303-2-raltherr@google.com> References: <20170321204828.31303-1-raltherr@google.com> <20170321204828.31303-2-raltherr@google.com> From: Joel Stanley Date: Wed, 22 Mar 2017 20:17:12 +1030 X-Google-Sender-Auth: 8WqGEOQcEjjTGc3Ui4a9Lo2t-is Message-ID: Subject: Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC To: Rick Altherr Cc: OpenBMC Maillist , Linux Kernel Mailing List , Martin Blumenstingl , William Breathitt Gray , Andreas Klinger , Rob Herring , linux-iio@vger.kernel.org, Hartmut Knaack , Quentin Schulz , Geert Uytterhoeven , Marek Vasut , Matt Ranostay , Lars-Peter Clausen , Crestez Dan Leonard , Akinobu Mita , Fabrice Gasnier , Jonathan Cameron , Peter Meerwald-Stadler , Maxime Ripard , Jacopo Mondi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12587 Lines: 369 On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr wrote: > Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low > and high threshold interrupts are supported by the hardware but are not > currently implemented. > > Signed-off-by: Rick Altherr Looks good Rick. I gave it a review from the perspective of the Aspeed soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly worked, but uncovered some things that need addressing. My device tree additions looked like this: adc: adc@1e6e9000 { compatible = "aspeed,ast2500-adc"; reg = <0x1e6e9000 0xb0>; clocks = <&clk_apb>; #io-channel-cells = <1>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_adc0_default>; }; iio-hwmon { compatible = "iio-hwmon"; io-channels = <&adc 0>; }; I got this output from lm-sensors when booted: iio_hwmon-isa-0000 Adapter: ISA adapter in1: +1.28 V I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to: iio_hwmon-isa-0000 Adapter: ISA adapter in1: +1.80 V ADC_12V_TW is the 12V rail sampled through a voltage divider. The voltage should be: 12 * 680 / ( 5600 + 680) = 1.299 cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw 738 738 / 1023 * 1.8 = 1.2975 Looks like the first channel is working! However our reference is incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it. It does hardcode 2500 in the aspeed_adc_read_raw callback: case IIO_CHAN_INFO_SCALE: *val = 2500; // mV *val2 = 10; return IIO_VAL_FRACTIONAL_LOG2; Should this value the the constant you define? Regardless, I don't think the reference voltage should be a constant. This is going to vary from system to system. Can we put it in the device tree? I notice other devices have vref-supply in their bindings. I noticed that in_voltage_scale is writable. However, it did not accept any of the values I give it. Is this because we do not handle it in aspeed_adc_write_raw? I suggest we add the reference in the device tree bindings, and also allow the value to be updated from userspace. > --- > > Changes in v2: > - Rewritten as an IIO device > - Renamed register macros to describe the register's purpose > - Replaced awkward reading of 16-bit data registers with readw() > - Added Kconfig dependency on COMPILE_TEST > > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/aspeed_adc.c | 271 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 282 insertions(+) > create mode 100644 drivers/iio/adc/aspeed_adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 2268a6fb9865..9672d799a3fb 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -130,6 +130,16 @@ config AD799X > To compile this driver as a module, choose M here: the module will be > called ad799x. > > +config ASPEED_ADC > + tristate "Aspeed AST2400/AST2500 ADC" You could just say Aspeed ADC to save us having to update it when the ast2600 comes out. > + depends on ARCH_ASPEED || COMPILE_TEST > + help > + If you say yes here you get support for the Aspeed AST2400/AST2500 > + ADC. > + > + To compile this driver as a module, choose M here: the module will be > + called aspeed_adc. Don't forget to test compiling as a module. > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c > new file mode 100644 > index 000000000000..9220909aefd4 > --- /dev/null > +#define ASPEED_ADC_NUM_CHANNELS 16 > +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ > +#define ASPEED_ADC_RESOLUTION_BITS 10 > +#define ASPEED_ADC_MIN_SAMP_RATE 10000 > +#define ASPEED_ADC_MAX_SAMP_RATE 500000 > +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12 > + > +#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00 > +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04 > +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08 > +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C > +#define ASPEED_ADC_REG_MAX 0xC0 > + > +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1) > +#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1) > +#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1) > + > +#define ASPEED_ADC_ENGINE_ENABLE BIT(0) Nit: You could chose to label these with a shorter prefix. Drop the aspeed or adc, or both. > + > +static int aspeed_adc_read_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_RAW: > + *val = readw(data->base + chan->address); > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 2500; // mV > + *val2 = 10; What does 10 mean? > + return IIO_VAL_FRACTIONAL_LOG2; > + > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = clk_get_rate(data->clk_scaler->clk) / > + ASPEED_ADC_CLOCKS_PER_SAMPLE; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > +} > + > +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) { Handle IIO_CHAN_INFO_SCALE here too. > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (val < ASPEED_ADC_MIN_SAMP_RATE || > + val > ASPEED_ADC_MAX_SAMP_RATE) > + return -EINVAL; > + > + clk_set_rate(data->clk_scaler->clk, > + val * ASPEED_ADC_CLOCKS_PER_SAMPLE); > + return 0; > + > + default: > + return -EINVAL; > + } > +} > + > +static int aspeed_adc_reg_access(struct iio_dev *indio_dev, > + unsigned int reg, unsigned int writeval, > + unsigned int *readval) > +{ > + struct aspeed_adc_data *data = iio_priv(indio_dev); > + > + if (!readval || reg % 4 || reg > ASPEED_ADC_REG_MAX) > + return -EINVAL; > + > + *readval = readl(data->base + reg); > + return 0; > +} > + > +static const struct iio_info aspeed_adc_iio_info = { > + .driver_module = THIS_MODULE, > + .read_raw = &aspeed_adc_read_raw, > + .write_raw = &aspeed_adc_write_raw, > + .debugfs_reg_access = &aspeed_adc_reg_access, > +}; > + > +static int aspeed_adc_probe(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev; > + struct aspeed_adc_data *data; > + struct resource *res; > + const char *clk_parent_name; > + int ret; > + u32 adc_engine_control_reg_val; > + > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data)); > + if (!indio_dev) { > + dev_err(&pdev->dev, "Failed allocating iio device\n"); > + return -ENOMEM; > + } > + > + data = iio_priv(indio_dev); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->base)) { > + dev_err(&pdev->dev, "Failed allocating device resources\n"); The function you're calling will do that for you http://lxr.free-electrons.com/source/lib/devres.c?v=4.10#L134 Just return the error here. I'd consider dropping the dev_errs for the other cases in the probe. We still get a reasonable error message without printing something ourselves. For example, when bailing out with ENOMEM: [ 5.510000] aspeed_adc: probe of 1e6e9000.adc failed with error -12 > + ret = PTR_ERR(data->base); > + goto resource_error; > + } > + > + /* 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_ADC_REG_CLOCK_CONTROL, > + 17, 15, 0, &data->clk_lock); I couldn't see any other drivers that use these functions outside of drivers/clk. I like what you've done here, but someone who understands the clock framework should take a look. > + if (IS_ERR(data->clk_prescaler)) { > + dev_err(&pdev->dev, "Failed allocating prescaler clock\n"); > + ret = PTR_ERR(data->clk_prescaler); > + goto prescaler_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", "prescaler", > + CLK_SET_RATE_PARENT, > + data->base + ASPEED_ADC_REG_CLOCK_CONTROL, > + 0, 10, 0, &data->clk_lock); > + if (IS_ERR(data->clk_scaler)) { > + dev_err(&pdev->dev, "Failed allocating scaler clock\n"); > + ret = PTR_ERR(data->clk_scaler); > + goto scaler_error; > + } > + > + /* Start all channels in normal mode. */ > + clk_prepare_enable(data->clk_scaler->clk); > + adc_engine_control_reg_val = GENMASK(31, 16) | > + ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE; > + writel(adc_engine_control_reg_val, > + data->base + ASPEED_ADC_REG_ENGINE_CONTROL); > + > + indio_dev->name = dev_name(&pdev->dev); > + 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); Should we be able to enable just the channels that we want? Perhaps only the ones that are requested through the device tree? > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + dev_err(&pdev->dev, "Could't register the device.\n"); > + goto iio_register_error; > + } > + > + return 0; > + > +iio_register_error: > + writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL); Should this be done in remove as well? > + clk_disable_unprepare(data->clk_scaler->clk); > + clk_hw_unregister_divider(data->clk_scaler); > + > +scaler_error: > + clk_hw_unregister_divider(data->clk_prescaler); > + > +prescaler_error: > +resource_error: > + return ret; You could just return from the error where it happens in the case where no cleanup is required. > +} > + > +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); > + clk_disable_unprepare(data->clk_scaler->clk); > + clk_hw_unregister_divider(data->clk_scaler); > + clk_hw_unregister_divider(data->clk_prescaler); > + > + return 0; > +} > + > +const struct of_device_id aspeed_adc_matches[] = { > + { .compatible = "aspeed,ast2400-adc" }, > + { .compatible = "aspeed,ast2500-adc" }, > +}; This is missing a null entry to terminate. > +MODULE_DEVICE_TABLE(of, aspeed_adc_matches); > + > +static struct platform_driver aspeed_adc_driver = { > + .probe = aspeed_adc_probe, > + .remove = aspeed_adc_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = aspeed_adc_matches, > + } > +}; > + > +module_platform_driver(aspeed_adc_driver); > + > +MODULE_AUTHOR("Rick Altherr "); > +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.12.1.500.gab5fba24ee-goog >