| 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
| ||
|
Message-ID: <20150401130608.GB10772@kwain>
Date: Wed, 1 Apr 2015 15:06:08 +0200
From: Antoine Tenart <antoine.tenart@...e-electrons.com>
To: Peter Meerwald <pmeerw@...erw.net>
Cc: Antoine Tenart <antoine.tenart@...e-electrons.com>,
sebastian.hesselbarth@...il.com, jic23@...nel.org, knaack.h@....de,
lars@...afoo.de, zmxu@...vell.com, jszhang@...vell.com,
yrliao@...vell.com, linux-iio@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] iio: adc: add support for Berlin
Peter,
On Fri, Mar 20, 2015 at 06:43:15PM +0100, Peter Meerwald wrote:
>
> probably these regmap_read() / _write() pairs could be MACRO()'d away
> somehow
Sure.
>
> the prefix SYSCTL prefix is unexpected, couldn't you use BERLIN_ or
> something?
I'll use BERLIN2_ instead of SYSCTL_
>
> > +#define SYSCTL_SM_CTRL 0x14
> > +#define SYSCTL_SM_CTRL_SM_SOC_INT BIT(1)
>
> whitespace issue?
I added a whitespace to put the bit definitions of a register together,
so that the reading is easier.
>
> > +#define SYSCTL_SM_CTRL_SOC_SM_INT BIT(2)
> > +#define SYSCTL_SM_CTRL_ADC_SEL(x) (BIT(x) << 5) /* 0-15 */
> > +#define SYSCTL_SM_CTRL_ADC_SEL_MASK (0xf << 5)
> > +#define SYSCTL_SM_CTRL_ADC_POWER BIT(9)
> > +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2 (0x0 << 10)
> > +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3 (0x1 << 10)
> > +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4 (0x2 << 10)
> > +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8 (0x3 << 10)
> > +#define SYSCTL_SM_CTRL_ADC_CLKSEL_MASK (0x3 << 10)
> > +#define SYSCTL_SM_CTRL_ADC_START BIT(12)
> > +#define SYSCTL_SM_CTRL_ADC_RESET BIT(13)
> > +#define SYSCTL_SM_CTRL_ADC_BANDGAP_RDY BIT(14)
> > +#define SYSCTL_SM_CTRL_ADC_CONT_SINGLE (0x0 << 15)
> > +#define SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS (0x1 << 15)
> > +#define SYSCTL_SM_CTRL_ADC_BUFFER_EN BIT(16)
> > +#define SYSCTL_SM_CTRL_ADC_VREF_EXT (0x0 << 17)
> > +#define SYSCTL_SM_CTRL_ADC_VREF_INT (0x1 << 17)
> > +#define SYSCTL_SM_CTRL_ADC_ROTATE BIT(19)
> > +#define SYSCTL_SM_CTRL_TSEN_EN BIT(20)
> > +#define SYSCTL_SM_CTRL_TSEN_CLK_SEL_125 (0x0 << 21) /* 1.25 MHz */
> > +#define SYSCTL_SM_CTRL_TSEN_CLK_SEL_250 (0x1 << 21) /* 2.5 MHz */
> > +#define SYSCTL_SM_CTRL_TSEN_MODE_0_125 (0x0 << 22) /* 0-125 C */
> > +#define SYSCTL_SM_CTRL_TSEN_MODE_10_50 (0x1 << 22) /* 10-50 C */
> > + { \
> > + .channel = n, \
> > + .datasheet_name = "channel"#n, \
> > + .type = t, \
> > + .indexed = 1, \
> > + .scan_index = n, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 64, \
> > + .storagebits = 64, \
> > + }, \
>
> the data read is not 64 bit I think
>
> the driver does not seem to support buffered reads, so scan_type and
> scan_index can be removed
OK.
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + }
> > +
> > +#define N_CHANNELS 8
>
> not prefixed; would be good if you could do with ARRAY_SIZE over
> _adc_channels instead
OK.
>
> > + BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE), /* external input */
> > + BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE), /* external input */
> > + BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE), /* external input */
> > + BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE), /* external input */
> > + BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE), /* reserved */
> > + BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE), /* reserved */
> > + BERLIN2_ADC_CHANNEL(6, IIO_TEMP), /* temperature sensor */
>
> the temperature channel is not indexed (there is only one)
I'll update.
>
> > + BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE), /* reserved */
> > + { /* timestamp */
>
>
> use IIO_CHAN_SOFT_TIMESTAMP(N_CHANNELS) here
Oh, nice! I'll update.
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (chan->type == IIO_VOLTAGE) {
> > + *val = berlin2_adc_read(idev, chan->channel);
> > + if (*val < 0)
> > + return *val;
>
> is this milli-Volts?
Good question. I think so, but I do not have much information about
the !tsen channels.
>
> > +
> > + return IIO_VAL_INT;
> > + } else if (chan->type == IIO_TEMP) {
> > + temp = berlin2_adc_tsen_read(idev);
> > + if (temp < 0)
> > + return temp;
> > +
> > + if (temp > 2047)
> > + temp = -(4096 - temp);
> > +
> > + /* Convert to Celsius */
> > + *val = (temp * 100) / 264 - 270;
>
> use SCALE and OFFSET IIO_CHAN_INFO_s to that the temperature can be
> interpreted as milli-Celsius (or use _PROCESSED, not _RAW)
I'll fix that.
> > +
> > +static int berlin2_adc_probe(struct platform_device *pdev)
> > +{
> > + struct iio_dev *idev;
>
> people prefer indio_dev instead of idev
OK.
>
> > + struct berlin2_adc_priv *priv;
> > + struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > + int ret, val;
> > +
> > + idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));
>
> devm_iio_device_alloc?
Yep, that's better.
> > +
> > + ret = iio_device_register(idev);
>
> devm_iio_device_register?
Ditto.
>
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to register the IIO device: %d\n",
> > + ret);
>
> probably not the most useful error msg and about the only logging; drop
> it?
>
> and just do
> return devm_iio_device_register(idev);
OK.
Thanks for the review!
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists