Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
On 22/03/17 09:47, Joel Stanley wrote: > 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. Few follow ups inline... Busy week so I'm playing catch up on this. > > 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- > 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- > 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? Yeah, that's an ugly quirk of IIO I wish we had done differently. We don't have separate masks for read and write attributes, so there is no way to have the attributes for the file set correctly. > > I suggest we add the reference in the device tree bindings, and also > allow the value to be updated from userspace. Not keen on updating from userspace, but indeed to providing it from device tree. If there is a board out there where it is wrong the devicetree should be fixed rather than applying a userspace hack. It's not as though this device is likely to be accurate enough to warant a calibration procedure. > >> --- >> >> 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. That's fine (and definitely a good thing if we end up supporting 20 different variants in a few years time) but make sure to add it to the help text below so there is something to grep for. > >> + 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 ..9220909aefd4 >> --- /dev/null > >> +#define ASPEED_ADC_NUM_CHANNELS16 >> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ >> +#define ASPEED_ADC_RESOLUTION_BITS 10 >> +#define ASPEED_ADC_MIN_SAMP_RATE 1 >> +#define ASPEED_ADC_MAX_SAMP_RATE 50 >> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12 >> + >> +#define ASPEED
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
On 22/03/17 10:08, Joel Stanley wrote: > Hello Quentin, > > On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz > wrote: > >>> + >>> +#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), >> >> It would make sense to name the registers (the _addr parameter of your >> macro) so it's easier to understand what it refers to. > > I appreciate the desire to not have magic values. However, I think > these are okay. We don't use them anywhere else, and it is obvious > from reading that they are the registers containing the ADC values for > each channel. > > The alternative would look like this: > > + ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA), > + ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA), > > Which doesn't really help me as someone reading the code. > >>> + /* 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); >> >> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end >> of the mail in the probe function). Better name it aspeed-adc or even >> better to have a different name per compatible: ast2400-adc or ast2500-adc. > > The label comes out as "adc.1e6e9000". This is the reg property and > the node name from the device tree, which seems sensible, even if it > is a bit strange to be grabbing the name of the parent device for it The intent is this provides the part number rather than a device tree handle. The devicetree handle is available via other routes if desired. . > > Could the iio core fill in a sensible name for us here? Rick is the > 31st person to make this mistake, so it would be nice to fix properly. It's not easy to actually generate it. Quite a few drivers do it by querying the hardware to get precise model numbers. Manufacturers of certain devices have an irritating habit of switching 'compatiblish' chips without bothering to update device tree blobs. Jonathan > > Cheers, > > Joel > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
On Thu, Mar 23, 2017 at 12:52 AM, Quentin Schulz wrote: > Hi, > > On 22/03/2017 21:46, Rick Altherr wrote: >> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz >> wrote: >>> Hi, >>> >>> On 21/03/2017 21:48, 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 --- 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 > [...] +#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), >>> >>> It would make sense to name the registers (the _addr parameter of your >>> macro) so it's easier to understand what it refers to. >>> >> >> I agree with Joel on this. There isn't really a better name than >> ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to >> make that clearer. >> > > Is it the name in the datasheet? > No, the datasheet has such helpful register names as ADC10 where 0x10 is the offset in the register map. > [...] + indio_dev->name = dev_name(&pdev->dev); >>> >>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end >>> of the mail in the probe function). Better name it aspeed-adc or even >>> better to have a different name per compatible: ast2400-adc or ast2500-adc. >> >> Ack. Will use aspeed-adc to avoid parsing the compatible match and >> stripping the 'aspeed,' prefix. >> > > You don't need to parse the compatible match. You could use the data > void pointer in your struct of_device_id > (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234) > like it's done here: https://lkml.org/lkml/2017/3/21/675 > (sun4i_gpadc_of_id). > I figured that out a bit later and did so in v3 I sent out last night. > Note: please reply to all :) Whoops. Looks like I did that for all the replies I did yesterday. > > Quentin > > -- > Quentin Schulz, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Restoring the list after an accidental direct reply. On Wed, Mar 22, 2017 at 2:32 PM, Rick Altherr wrote: > On Wed, Mar 22, 2017 at 2:47 AM, Joel Stanley wrote: >> 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>; > > This shouldn't be strictly necessary as the ADCs are the default > function for the pins they are available on. > >> }; >> >> iio-hwmon { >> compatible = "iio-hwmon"; >> io-channels = <&adc 0>; >> }; > > This is necessary to make it show up as hwmon. You can see the iio > device directly at /sys/bus/iio/devices/. > >> >> I got this output from lm-sensors when booted: >> >> iio_hwmon-isa- >> 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- >> 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. >> > > Good catch. AST2500 uses a 1.8Vref while AST2400 uses a 2.5Vref. I > decided against putting a vref-supply in the device tree as neither > part allows for an external reference. Both the 1.8V and 2.5V are > fixed, internal references. It just happens that the reference > voltage changes with the chip generation. Looking at the data sheet > more, I also see the min/max sampling rate has changed for AST2500 as > well. This is probably best handled by attaching data to the > of_device_ids in the of_match_table. That would solve all of these > per-chip variations. > >> 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 think all attributes become writable because I implement write_raw > even though only the sample frequency is actually writable. I'm of > two minds on allowing the scale to be written. If the user knows > there is a voltage divider before the ADC, why don't they apply that > correction in userspace? On the other hand, the existence of the > voltage divider _could_ be specified in the device tree and the kernel > takes care of the scaling entirely. The latter seems like a > general-purpose concept but I couldn't find an existing binding for > specifying it. I decided to start with the minimal functionality of > only reporting the ADC's natural scaling and letting userspace deal > with any additional information it has. > >> 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
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Restoring the list after an accidental direct reply. On Wed, Mar 22, 2017 at 1:30 PM, Rick Altherr wrote: > On Tue, Mar 21, 2017 at 2:14 PM, Peter Meerwald-Stadler > 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. >> >> comments below >> link to a datasheet would be nice >> > > No public datasheet is available. I've tried. > >>> Signed-off-by: Rick Altherr >>> --- >>> >>> 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 ..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 >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> +#include >>> + >>> +#define ASPEED_ADC_NUM_CHANNELS 16 >> >> _NUM_CHANNELS is not used, remove > > Ack > >> >>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ >> >> should be used below > > Ack > >> >>> +#define ASPEED_ADC_RESOLUTION_BITS 10 >> >> should be used below > > Ack > >> >>> +#define ASPEED_ADC_MIN_SAMP_RATE 1 >>> +#define ASPEED_ADC_MAX_SAMP_RATE 50 >>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12 >>> + >>> +#define ASPEED_ADC_REG_ENGINE_CONTROL0x00 >>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04 >>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL0x08 >>> +#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[] = { >
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Hi Rick, [auto build test ERROR on iio/togreg] [also build test ERROR on v4.11-rc3 next-20170322] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Rick-Altherr/Documentation-dt-bindings-Document-bindings-for-Aspeed-AST2400-AST2500-ADC/20170323-093517 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: i386-allmodconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/iio/adc/aspeed_adc: struct of_device_id is 196 bytes. The last of 2 is: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x61 0x73 0x70 0x65 0x65 0x64 0x2c 0x61 0x73 0x74 0x32 0x35 0x30 0x30 0x2d 0x61 0x64 0x63 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 >> FATAL: drivers/iio/adc/aspeed_adc: struct of_device_id is not terminated >> with a NULL entry! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Hi, On 22/03/2017 21:46, Rick Altherr wrote: > On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz > wrote: >> Hi, >> >> On 21/03/2017 21:48, 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 >>> --- >>> >>> 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 >>> [...] >>> +#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), >> >> It would make sense to name the registers (the _addr parameter of your >> macro) so it's easier to understand what it refers to. >> > > I agree with Joel on this. There isn't really a better name than > ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to > make that clearer. > Is it the name in the datasheet? [...] >>> + indio_dev->name = dev_name(&pdev->dev); >> >> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end >> of the mail in the probe function). Better name it aspeed-adc or even >> better to have a different name per compatible: ast2400-adc or ast2500-adc. > > Ack. Will use aspeed-adc to avoid parsing the compatible match and > stripping the 'aspeed,' prefix. > You don't need to parse the compatible match. You could use the data void pointer in your struct of_device_id (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234) like it's done here: https://lkml.org/lkml/2017/3/21/675 (sun4i_gpadc_of_id). Note: please reply to all :) Quentin -- Quentin Schulz, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Hi Rick, [auto build test ERROR on iio/togreg] [also build test ERROR on v4.11-rc3 next-20170322] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Rick-Altherr/Documentation-dt-bindings-Document-bindings-for-Aspeed-AST2400-AST2500-ADC/20170323-093517 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 6.2.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=ia64 All error/warnings (new ones prefixed by >>): drivers/iio/adc/aspeed_adc.c: In function 'aspeed_adc_read_raw': >> drivers/iio/adc/aspeed_adc.c:100:39: error: dereferencing pointer to >> incomplete type 'struct clk_hw' *val = clk_get_rate(data->clk_scaler->clk) / ^~ drivers/iio/adc/aspeed_adc.c: In function 'aspeed_adc_probe': >> drivers/iio/adc/aspeed_adc.c:177:20: error: implicit declaration of function >> 'of_clk_get_parent_name' [-Werror=implicit-function-declaration] clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0); ^~ >> drivers/iio/adc/aspeed_adc.c:177:18: warning: assignment makes pointer from >> integer without a cast [-Wint-conversion] clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0); ^ >> drivers/iio/adc/aspeed_adc.c:179:24: error: implicit declaration of function >> 'clk_hw_register_divider' [-Werror=implicit-function-declaration] data->clk_prescaler = clk_hw_register_divider( ^~~ drivers/iio/adc/aspeed_adc.c:179:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion] data->clk_prescaler = clk_hw_register_divider( ^ >> drivers/iio/adc/aspeed_adc.c:195:5: error: 'CLK_SET_RATE_PARENT' undeclared >> (first use in this function) CLK_SET_RATE_PARENT, ^~~ drivers/iio/adc/aspeed_adc.c:195:5: note: each undeclared identifier is reported only once for each function it appears in >> drivers/iio/adc/aspeed_adc.c:229:2: error: implicit declaration of function >> 'clk_hw_unregister_divider' [-Werror=implicit-function-declaration] clk_hw_unregister_divider(data->clk_scaler); ^ cc1: some warnings being treated as errors vim +100 drivers/iio/adc/aspeed_adc.c 94 case IIO_CHAN_INFO_SCALE: 95 *val = 2500; // mV 96 *val2 = 10; 97 return IIO_VAL_FRACTIONAL_LOG2; 98 99 case IIO_CHAN_INFO_SAMP_FREQ: > 100 *val = clk_get_rate(data->clk_scaler->clk) / 101 ASPEED_ADC_CLOCKS_PER_SAMPLE; 102 return IIO_VAL_INT; 103 104 default: 105 return -EINVAL; 106 } 107 } 108 109 static int aspeed_adc_write_raw(struct iio_dev *indio_dev, 110 struct iio_chan_spec const *chan, 111 int val, int val2, long mask) 112 { 113 struct aspeed_adc_data *data = iio_priv(indio_dev); 114 115 switch (mask) { 116 case IIO_CHAN_INFO_SAMP_FREQ: 117 if (val < ASPEED_ADC_MIN_SAMP_RATE || 118 val > ASPEED_ADC_MAX_SAMP_RATE) 119 return -EINVAL; 120 121 clk_set_rate(data->clk_scaler->clk, 122 val * ASPEED_ADC_CLOCKS_PER_SAMPLE); 123 return 0; 124 125 default: 126 return -EINVAL; 127 } 128 } 129 130 static int aspeed_adc_reg_access(struct iio_dev *indio_dev, 131 unsigned int reg, unsigned int writeval, 132 unsigned int *readval) 133 { 134 struct aspeed_adc_data *data = iio_priv(indio_dev); 135 136 if (!readval || reg % 4 || reg > ASPEED_ADC_REG_MAX) 137 return -EINVAL; 138 139 *readval = readl(data->base + reg); 140 return 0; 141 } 142 143 static const struct iio_info aspeed_adc_iio_info = { 144 .driver_module = THIS_MODULE, 145 .read_raw = &aspeed_adc_read_raw, 146 .write_raw = &aspeed_adc_write_raw, 147 .debugfs_reg_access = &aspeed_adc_reg_access, 148 }; 149 150 static int aspeed_adc_probe(struct platform_device *pdev) 151 { 152 struct iio_dev *indio_dev; 153 stru
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Hello Quentin, On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz wrote: >> + >> +#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), > > It would make sense to name the registers (the _addr parameter of your > macro) so it's easier to understand what it refers to. I appreciate the desire to not have magic values. However, I think these are okay. We don't use them anywhere else, and it is obvious from reading that they are the registers containing the ADC values for each channel. The alternative would look like this: + ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA), + ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA), Which doesn't really help me as someone reading the code. >> + /* 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); > > This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end > of the mail in the probe function). Better name it aspeed-adc or even > better to have a different name per compatible: ast2400-adc or ast2500-adc. The label comes out as "adc.1e6e9000". This is the reg property and the node name from the device tree, which seems sensible, even if it is a bit strange to be grabbing the name of the parent device for it. Could the iio core fill in a sensible name for us here? Rick is the 31st person to make this mistake, so it would be nice to fix properly. Cheers, Joel
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
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- 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- 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 ..9220909aefd4 > --- /dev/null > +#define ASPEED_ADC_NUM_CHANNELS16 > +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ > +#define ASPEED_ADC_RESOLUTION_BITS 10 > +#define ASPEED_ADC_MIN_SAMP_RATE 1 > +#define ASPEED_ADC_MAX_SAMP_RATE 50 > +#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); > +
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Hi, On 21/03/2017 21:48, 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 > --- > > 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 > [...] > +struct aspeed_adc_data { > + struct device *dev; > + void __iomem*base; > + Useless empty line? > + 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), It would make sense to name the registers (the _addr parameter of your macro) so it's easier to understand what it refers to. [...] > +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; > + } > + > + /* 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; > + } > + > + /* > + * 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); This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end of the mail in the probe function). Better name it aspeed-adc or even better to have a different name per compatible: ast2400-adc or ast2500-adc. > + indio_dev->dev.parent = &pdev->dev; > + indio_dev->info = &aspeed_adc_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->ch
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 > --- > > 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 ..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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#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 1 > +#define ASPEED_ADC_MAX_SAMP_RATE 50 > +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12 > + > +#define ASPEED_ADC_REG_ENGINE_CONTROL0x00 > +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04 > +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL0x08 > +#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
[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. Signed-off-by: Rick Altherr --- 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 ..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 +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#define ASPEED_ADC_NUM_CHANNELS16 +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ +#define ASPEED_ADC_RESOLUTION_BITS 10 +#define ASPEED_ADC_MIN_SAMP_RATE 1 +#define ASPEED_ADC_MAX_SAMP_RATE 50 +#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: +