Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933607AbdCWQ55 (ORCPT ); Thu, 23 Mar 2017 12:57:57 -0400 Received: from mail-wr0-f170.google.com ([209.85.128.170]:33534 "EHLO mail-wr0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751444AbdCWQ5U (ORCPT ); Thu, 23 Mar 2017 12:57:20 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170321204828.31303-1-raltherr@google.com> <20170321204828.31303-2-raltherr@google.com> <5f245e55-4d4a-b744-3351-e8215432c315@free-electrons.com> From: Rick Altherr Date: Thu, 23 Mar 2017 09:57:17 -0700 Message-ID: Subject: Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC To: Quentin Schulz Cc: OpenBMC Maillist , "linux-kernel@vger.kernel.org" , Martin Blumenstingl , William Breathitt Gray , Andreas Klinger , Rob Herring , "linux-iio@vger.kernel.org" , "knaack.h@gmx.de" , Geert Uytterhoeven , Marek Vasut , Matt Ranostay , Lars-Peter Clausen , Crestez Dan Leonard , Akinobu Mita , Fabrice Gasnier , Jonathan Cameron , Peter Meerwald-Stadler , Maxime Ripard , Jacopo Mondi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3481 Lines: 98 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