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: <866cabad-d151-a54f-e316-201aa05cbbf5@kernel.org>
Date:   Sat, 25 Mar 2017 17:11:48 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Joel Stanley <joel@....id.au>,
        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>,
        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 22/03/17 10:08, Joel Stanley wrote:
> 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
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 majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>