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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPLgG=mbBJd8+2vdGUO1RVnTJkgCcy4SCJG_YMmXKnq+zAW4EA@mail.gmail.com>
Date:   Thu, 23 Mar 2017 09:57:17 -0700
From:   Rick Altherr <raltherr@...gle.com>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        William Breathitt Gray <vilhelm.gray@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Rob Herring <robh@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "knaack.h@....de" <knaack.h@....de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Marek Vasut <marek.vasut+renesas@...il.com>,
        Matt Ranostay <mranostay@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Crestez Dan Leonard <leonard.crestez@...el.com>,
        Akinobu Mita <akinobu.mita@...il.com>,
        Fabrice Gasnier <fabrice.gasnier@...com>,
        Jonathan Cameron <jic23@...nel.org>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Jacopo Mondi <jacopo@...ndi.org>
Subject: Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC

On Thu, Mar 23, 2017 at 12:52 AM, Quentin Schulz
<quentin.schulz@...e-electrons.com> wrote:
> Hi,
>
> On 22/03/2017 21:46, Rick Altherr wrote:
>> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz
>> <quentin.schulz@...e-electrons.com> 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 <raltherr@...gle.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Rewritten as an IIO device
>>>> - Renamed register macros to describe the register's purpose
>>>> - Replaced awkward reading of 16-bit data registers with readw()
>>>> - Added Kconfig dependency on COMPILE_TEST
>>>>
> [...]
>>>> +#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