linux-kernel - Re: [PATCH v4 2/4] iio: health: Add driver for the TI AFE4404 heart monitor

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: <56B0D04B.1090208@ti.com>
Date:	Tue, 2 Feb 2016 09:50:35 -0600
From:	"Andrew F. Davis" <afd@...com>
To:	Jonathan Cameron <jic23@...nel.org>,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Peter Meerwald <pmeerw@...erw.net>,
	Dan Murphy <dmurphy@...com>, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>
CC:	<linux-iio@...r.kernel.org>, <linux-api@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] iio: health: Add driver for the TI AFE4404 heart
 monitor

On 01/30/2016 08:59 AM, Jonathan Cameron wrote:
> On 25/01/16 17:28, Andrew F. Davis wrote:
>> Add driver for the TI AFE4404 heart rate monitor and pulse oximeter.
>> This device detects reflected LED light fluctuations and presents an ADC
>> value to the user space for further signal processing.
>>
>> Datasheet: http://www.ti.com/product/AFE4404/datasheet
>>
>> Signed-off-by: Andrew F. Davis <afd@...com>
> Hi Andrew,
>
> This changed rather more than I was expecting, but fair enough.
>
> A few bits are missing from remove.  Few other little bits inline,
> though mostly those are about readability rather than what the code
> is doing.
>
> I was a little suprised to see no control over whether the trigger is
> enabled or not.  I'm trying to get my head around what the result of this will
> be.  I guess we'll have interrupts pinging away merilly doing nothing until
> the buffer is attached then the demux will get set up to actually do
> something with the data.
>
> hmm. Might be fine, if I ususual.  Normally devices have some means of
> masking the interrupt.

I wasn't sure about this ether, but it doesn't seem to break anything.

>
> Jonathan
>
>> ---
>>   .../ABI/testing/sysfs-bus-iio-health-afe440x       |  54 ++
>>   drivers/iio/health/Kconfig                         |  19 +-

[...]

>> +
>> +static const struct afe440x_reg_info afe4404_reg_info[] = {
>
> As noted below, I'd find
> [LED1] = AFE440X_REG_INFO(AFE440_LED1VAL...
> more readable.
>

ACK

[...]

>> +	int ret;
>> +
>> +	iio_device_unregister(indio_dev);
>> +
> Would expect unwinding of the triggered_buffer here.
> iio_triggered_buffer_cleanup()
>

ACK

>> +	iio_trigger_unregister(afe->trig);
> This should be protected by a check on afe->irq as for the register in probe.
> There is no protection against afe->trig == NULL in iio_trigger_unregister.
> That is kind of deliberate as it makes sure the probe / remove in drivers
> are balanced unlike here.
>

Makes sense, will add.

[...]

> I'm not overly keen on the hiding fo the indexing.  I'd prefer
> this was just the bit {...} and you had the fact it was filling in an
> indexed element explicit where used.

That works.

[...]

> I'd prefix reg_type to avoid possible name clashes in future.
>> +enum reg_type {
>> +	SIMPLE,
>> +	RESISTANCE,
>> +	CAPACITANCE,
>> +};

ACK

>> +
>> +/* this could be made more general for other IIO drivers if needed --------- */
>
> Yes, but lets do that at as a follow up.
>

Sure.

Thanks,
Andrew