Message-ID: <alpine.DEB.2.20.1703212202090.26011@vps.pmeerw.net>
Date:   Tue, 21 Mar 2017 22:14:58 +0100 (CET)
From:   Peter Meerwald-Stadler <pmeerw@...erw.net>
To:     Rick Altherr <raltherr@...gle.com>
cc:     openbmc@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-iio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC


> 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.

comments below
link to a datasheet would be nice
 
> Signed-off-by: Rick Altherr <raltherr@...gle.com>
> ---
> 
> 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"
> +	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.
> +
>  config AT91_ADC
>  	tristate "Atmel AT91 ADC"
>  	depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe399f894..306f10ffca74 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> 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
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -0,0 +1,271 @@
> +/*
> + * Aspeed AST2400/2500 ADC
> + *
> + * Copyright (C) 2017 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + */
> +
> +#include <asm/io.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +
> +#define ASPEED_ADC_NUM_CHANNELS			16

_NUM_CHANNELS is not used, remove

> +#define ASPEED_ADC_REF_VOLTAGE			2500 /* millivolts */

should be used below

> +#define ASPEED_ADC_RESOLUTION_BITS		10

should be used below

> +#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)
> +
> +struct aspeed_adc_data {
> +	struct device	*dev;
> +	void __iomem	*base;
> +
> +	spinlock_t	clk_lock;
> +	struct clk_hw	*clk_prescaler;
> +	struct clk_hw	*clk_scaler;
> +};
> +
> +#define ASPEED_ADC_CHAN(_idx, _addr) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = (_idx),					\
> +	.address = (_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),	\
> +}
> +
> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
> +	ASPEED_ADC_CHAN(0, 0x10),
> +	ASPEED_ADC_CHAN(1, 0x12),
> +	ASPEED_ADC_CHAN(2, 0x14),
> +	ASPEED_ADC_CHAN(3, 0x16),
> +	ASPEED_ADC_CHAN(4, 0x18),
> +	ASPEED_ADC_CHAN(5, 0x1A),
> +	ASPEED_ADC_CHAN(6, 0x1C),
> +	ASPEED_ADC_CHAN(7, 0x1E),
> +	ASPEED_ADC_CHAN(8, 0x20),
> +	ASPEED_ADC_CHAN(9, 0x22),
> +	ASPEED_ADC_CHAN(10, 0x24),
> +	ASPEED_ADC_CHAN(11, 0x26),
> +	ASPEED_ADC_CHAN(12, 0x28),
> +	ASPEED_ADC_CHAN(13, 0x2A),
> +	ASPEED_ADC_CHAN(14, 0x2C),
> +	ASPEED_ADC_CHAN(15, 0x2E),
> +};
> +
> +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

REF_VOLTAGE?

> +		*val2 = 10;
> +		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) {
> +	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,

without & (matter of taste)

> +	.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");
> +		ret = PTR_ERR(data->base);
> +		goto resource_error;

just return here?

> +	}
> +
> +	/* 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);
> +	if (IS_ERR(data->clk_prescaler)) {
> +		dev_err(&pdev->dev, "Failed allocating prescaler clock\n");
> +		ret = PTR_ERR(data->clk_prescaler);
> +		goto prescaler_error;

just return here?

> +	}
> +
> +	/*
> +	 * 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);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Could't register the device.\n");

Couldn't

> +		goto iio_register_error;
> +	}
> +
> +	return 0;
> +
> +iio_register_error:
> +	writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
> +	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;
> +}
> +
> +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);

writel(0x0, data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
I guess this power off the device?

the MODE #defines are now used (powerdown, normal, etc.)

> +	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" },
> +};
> +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 <raltherr@...gle.com>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Powered by blists - more mailing lists