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

[Jonathan Cameron]

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 = <_apb>;
>   #io-channel-cells = <1>;
> 
>   pinctrl-names = "default";
>   pinctrl-0 = <_adc0_default>;
>   };
> 
>   iio-hwmon {
>   compatible = "iio-hwmon";
>   io-channels = < 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   

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

[Jonathan Cameron]

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 = <_apb>;
>   #io-channel-cells = <1>;
> 
>   pinctrl-names = "default";
>   pinctrl-0 = <_adc0_default>;
>   };
> 
>   iio-hwmon {
>   compatible = "iio-hwmon";
>   io-channels = < 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 

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

[Jonathan Cameron]

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

[Jonathan Cameron]

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

[Rick Altherr]

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

[Rick Altherr]

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 = <_apb>;
>>   #io-channel-cells = <1>;
>>
>>   pinctrl-names = "default";
>>   pinctrl-0 = <_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 = < 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
>>>   

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

[Rick Altherr]

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

[Rick Altherr]

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 = <_apb>;
>>   #io-channel-cells = <1>;
>>
>>   pinctrl-names = "default";
>>   pinctrl-0 = <_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 = < 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 "Aspeed 

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

[Rick Altherr]

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),   \
>>> +}
>>> +
>>> 

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

[Rick Altherr]

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

[kbuild test robot]

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

[kbuild test robot]

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

[Quentin Schulz]

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

[Quentin Schulz]

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

[kbuild test robot]

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 = _adc_read_raw,
   146  .write_raw = _adc_write_raw,
   147  .debugfs_reg_access = _adc_reg_access,
   148  };
   149  
   150  static int aspeed_adc_probe(struct platform_device *pdev)
   151  {
   152  struct iio_dev *indio_dev;
   153  struct aspeed_adc_data 

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

[kbuild test robot]

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 = _adc_read_raw,
   146  .write_raw = _adc_write_raw,
   147  .debugfs_reg_access = _adc_reg_access,
   148  };
   149  
   150  static int aspeed_adc_probe(struct platform_device *pdev)
   151  {
   152  struct iio_dev *indio_dev;
   153  struct aspeed_adc_data 

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

[Joel Stanley]

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

[Joel Stanley]

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

[Joel Stanley]

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 = <_apb>;
  #io-channel-cells = <1>;

  pinctrl-names = "default";
  pinctrl-0 = <_adc0_default>;
  };

  iio-hwmon {
  compatible = "iio-hwmon";
  io-channels = < 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 + 

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

[Joel Stanley]

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 = <_apb>;
  #io-channel-cells = <1>;

  pinctrl-names = "default";
  pinctrl-0 = <_adc0_default>;
  };

  iio-hwmon {
  compatible = "iio-hwmon";
  io-channels = < 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);
> +   return 

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

[Quentin Schulz]

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(>dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(>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(>dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(>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(>clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + >dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, >clk_lock);
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(>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(
> + >dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, >clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(>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(>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 = >dev;
> + indio_dev->info = _adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = aspeed_adc_iio_channels;
> + 

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

[Quentin Schulz]

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(>dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(>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(>dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(>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(>clk_lock);
> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +
> + data->clk_prescaler = clk_hw_register_divider(
> + >dev, "prescaler", clk_parent_name, 0,
> + data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
> + 17, 15, 0, >clk_lock);
> + if (IS_ERR(data->clk_prescaler)) {
> + dev_err(>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(
> + >dev, "scaler", "prescaler",
> + CLK_SET_RATE_PARENT,
> + data->base + ASPEED_ADC_REG_CLOCK_CONTROL,
> + 0, 10, 0, >clk_lock);
> + if (IS_ERR(data->clk_scaler)) {
> + dev_err(>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(>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 = >dev;
> + indio_dev->info = _adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = aspeed_adc_iio_channels;
> + indio_dev->num_channels = 

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

[Peter Meerwald-Stadler]

> 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),
> +  

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

[Peter Meerwald-Stadler]

> 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),
> +