linux-kernel - Re: [PATCH v3 1/3] iio: adc: add support for Berlin

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]

Date:	Tue, 5 May 2015 11:16:35 +0200
From:	Antoine Tenart <antoine.tenart@...e-electrons.com>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Antoine Tenart <antoine.tenart@...e-electrons.com>,
	sebastian.hesselbarth@...il.com, knaack.h@....de, lars@...afoo.de,
	pmeerw@...erw.net, 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 v3 1/3] iio: adc: add support for Berlin

Jonathan,

On Mon, May 04, 2015 at 05:21:11PM +0100, Jonathan Cameron wrote:
> On 04/05/15 10:06, Antoine Tenart wrote:

[...]

> Also, I'm travelling and without internet so can't look it up directly.
> So.. On the basis I'll forget to check later..  What is the status
> of the two series you say this is based upon?

These two series haven't been taken yet, but they should be merged in
the next merge window (at least that's what we aim). Sebastian may have
more information on this matter.

> 
> There are also some overly long lines where spitting them would not
> significantly hurt readability so clean those up as well to avoid
> the inevitable code style fix patch from someone with too much time on
> their hands!

I'll have a look.

> > diff --git a/drivers/iio/adc/berlin2-adc.c b/drivers/iio/adc/berlin2-adc.c
> > new file mode 100644
> > index 000000000000..0adfef035d79
> > --- /dev/null
> > +++ b/drivers/iio/adc/berlin2-adc.c

[...]

> > +
> > +struct berlin2_adc_priv {
> > +	struct regmap		*regmap;
> > +	struct mutex		lock;
> > +	wait_queue_head_t	wq;
> > +	int			irq;
> > +	int			tsen_irq;
> tsen_irq and irq are only used in one function I think. If so just use local
> variables rather than cluttering up this structure.

Sure.

[...]

> > +
> > +	regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> > +			BERLIN2_SM_CTRL_ADC_START, 0);
> > +
> > +	data = priv->data;
> > +	priv->data = -1;
> Is setting data = -1 not overkill with data_available there as well?

It is, we can remove this line.

[...]

> > +
> > +		temp = berlin2_adc_tsen_read(indio_dev);
> > +		if (temp < 0)
> > +			return temp;
> > +
> > +		if (temp > 2047)
> > +			temp = -(4096 - temp);
> > +
> > +		/* Convert to Celsius */
> Just to check... Celsius or milli degrees celsius?
> (which is the ABI units - matched with hwmon)

Celsuis, I'll update to return milli Celsius.

Thanks!

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/