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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACPK8XcoQGo4uErK71aeUhmcJ-6Yt0k41q8N7wsemz6HLRSL4w@mail.gmail.com>
Date:   Wed, 22 Mar 2017 20:38:40 +1030
From:   Joel Stanley <joel@....id.au>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     Rick Altherr <raltherr@...gle.com>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Linux Kernel Mailing List <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,
        Hartmut Knaack <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

Hello Quentin,

On Wed, Mar 22, 2017 at 5:51 PM, Quentin Schulz
<quentin.schulz@...e-electrons.com> wrote:

>> +
>> +#define ASPEED_ADC_CHAN(_idx, _addr) {                               \
>> +     .type = IIO_VOLTAGE,                                    \
>> +     .indexed = 1,                                           \
>> +     .channel = (_idx),                                      \
>> +     .address = (_addr),                                     \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |  \
>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ),   \
>> +}
>> +
>> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>> +     ASPEED_ADC_CHAN(0, 0x10),
>> +     ASPEED_ADC_CHAN(1, 0x12),
>> +     ASPEED_ADC_CHAN(2, 0x14),
>> +     ASPEED_ADC_CHAN(3, 0x16),
>> +     ASPEED_ADC_CHAN(4, 0x18),
>> +     ASPEED_ADC_CHAN(5, 0x1A),
>> +     ASPEED_ADC_CHAN(6, 0x1C),
>> +     ASPEED_ADC_CHAN(7, 0x1E),
>> +     ASPEED_ADC_CHAN(8, 0x20),
>> +     ASPEED_ADC_CHAN(9, 0x22),
>> +     ASPEED_ADC_CHAN(10, 0x24),
>> +     ASPEED_ADC_CHAN(11, 0x26),
>> +     ASPEED_ADC_CHAN(12, 0x28),
>> +     ASPEED_ADC_CHAN(13, 0x2A),
>> +     ASPEED_ADC_CHAN(14, 0x2C),
>> +     ASPEED_ADC_CHAN(15, 0x2E),
>
> It would make sense to name the registers (the _addr parameter of your
> macro) so it's easier to understand what it refers to.

I appreciate the desire to not have magic values. However, I think
these are okay. We don't use them anywhere else, and it is obvious
from reading that they are the registers containing the ADC values for
each channel.

The alternative would look like this:

+     ASPEED_ADC_CHAN(14, ASPEED_ADC_CHAN_14_DATA),
+     ASPEED_ADC_CHAN(15, ASPEED_ADC_CHAN_15_DATA),

Which doesn't really help me as someone reading the code.

>> +     /* Start all channels in normal mode. */
>> +     clk_prepare_enable(data->clk_scaler->clk);
>> +     adc_engine_control_reg_val = GENMASK(31, 16) |
>> +             ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>> +     writel(adc_engine_control_reg_val,
>> +             data->base + ASPEED_ADC_REG_ENGINE_CONTROL);
>> +
>> +     indio_dev->name = dev_name(&pdev->dev);
>
> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
> of the mail in the probe function). Better name it aspeed-adc or even
> better to have a different name per compatible: ast2400-adc or ast2500-adc.

The label comes out as "adc.1e6e9000". This is the reg property and
the node name from the device tree, which seems sensible, even if it
is a bit strange to be grabbing the name of the parent device for it.

Could the iio core fill in a sensible name for us here? Rick is the
31st person to make this mistake, so it would be nice to fix properly.

Cheers,

Joel