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

[Andrew F. Davis]

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@xxxxxx>
> 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