Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC

From: Joel Stanley
Date: Wed Mar 22 2017 - 05:48:13 EST

Next message: Chanwoo Choi: "[GIT PULL] extcon fixes for v4.11-rc3"
Previous message: Dan Carpenter: "Re: staging: iio: Replace a bit shift by a use of BIT."
In reply to: Rick Altherr: "Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC"
Next in thread: Rick Altherr: "Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr <raltherr@xxxxxxxxxx> 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 <raltherr@xxxxxxxxxx>

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 <raltherr@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.12.1.500.gab5fba24ee-goog
>

Next message: Chanwoo Choi: "[GIT PULL] extcon fixes for v4.11-rc3"
Previous message: Dan Carpenter: "Re: staging: iio: Replace a bit shift by a use of BIT."
In reply to: Rick Altherr: "Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC"
Next in thread: Rick Altherr: "Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]