Re: [PATCH/RFC v2] iio: gp2ap020a00f: Add a driver for the device

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 07/07/2013 06:09 PM, Jonathan Cameron wrote:
[..]
 >> +error_ret:
 >> +	mutex_unlock(&data->lock);
 >> +
 >> +	return err<  0 ? err : IIO_VAL_INT;
 >> +}
 >> +
 > Any info available on how to convert these inputs to an illuminance 
value?
 > (e.g. in LUX)

There is info available, but the formula depends on the IR factor
that is read from in_illuminance_ir channel, i.e, it isn't constant and
can vary from conversion to conversion.
Moreover, the other factors in the formula also vary depending on
the lux mode set (I added lux mode configuration in the third
version of the patch).
I wasn't sure how to implement it with IIO, so any hint on how
to handle dynamically changing scale factor is welcomed.

[...]
 >> +
 >> +static int gp2ap020a00f_buffer_preenable(struct iio_dev *indio_dev)
 >> +{
 >> +	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 >> +	size_t d_size = 0;
 >> +	int i, err = 0;
 >> +
 > Without datasheet I'd like you to talk me through exactly what is 
going on
 > in here...
 >> +	mutex_lock(&data->lock);
 >> +
 >> +	/* Enable triggers according to the scan_mask */
 >> +	for_each_set_bit(i, indio_dev->active_scan_mask,
 >> +		indio_dev->masklength) {
 >> +		switch (i) {
 >> +		case GP2AP020A00F_SCAN_MODE_LIGHT_CLEAR:
 >> +			err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_CLEAR_EN);
 > So these triggers correspond to actually capturing the data based on some
 > other signal?  e.g. after these are set we'll get a steady stream
 > of data off the relevant part of the chip?

Actually both LIGHT_CLEAR and LIGHT_IR conversions are started after
setting the device in ALS mode. If the channel is disabled with IIO
its output value is simply ignored.
In order to enable PROXIMITY channel the PS part of the device has to
be enabled. The device can be set in ALS, PS or ALS_AND_PS modes.

 >> +			if (err<  0)
 >> +				goto error_ret;
 >> +			break;
 >> +		case GP2AP020A00F_SCAN_MODE_LIGHT_IR:
 >> +			err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_IR_EN);
 >> +			if (err<  0)
 >> +				goto error_ret;
 >> +			break;
 >> +		case GP2AP020A00F_SCAN_MODE_PROXIMITY:
 >> +			err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_PROX_EN);
 >> +			if (err<  0)
 >> +				goto error_ret;
 >> +			break;
 >> +		}
 >> +		d_size += 2;
 >> +	}
 >> +
 > Is this always guaranteed to be aligned?  Doesn't immediately look like
 > it and quite a bit of the buffer handling assumes it is.

I fixed it in the newest patch by using indio_dev->scan_bytes property.

 >> +	if (indio_dev->scan_timestamp)
 >> +		d_size += sizeof(s64);
 >> +
 >> +	data->buffer = kmalloc(d_size, GFP_KERNEL);
 >> +	if (data->buffer == NULL) {
 >> +		err = -ENOMEM;
 >> +		goto error_ret;
 >> +	}
 > You might to better with the update_scan_mode callback and using generic
 > preenable/postdisable.  Perhaps it is simpler this way, I'm not sure
 > without actually implementing it!

 From what I figured out update_scan_mode callback is called only
while enabling the trigger. How an IIO driver can be informed about
the end of triggered capture in different way than through
the buffer_postdisable callback?

 >> +
 >> +	err = iio_sw_buffer_preenable(indio_dev);
 >> +
 >> +error_ret:
 >> +	mutex_unlock(&data->lock);
 >> +
 >> +	return err;
 >> +}
 >> +
 >> +static int gp2ap020a00f_buffer_predisable(struct iio_dev *indio_dev)
 >> +{
 >> +	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 >> +	int err;
 >> +
 >> +	mutex_lock(&data->lock);
 >> +
 > The fact you did the enable as a for_each_bit_set and this like this
 > is a little odd.  Actually I think the simple if statements here
 > are a little easier, but either way consistency of code structures
 > definitely makes my life easier, so pick one form or the other.

Fixed.

 >> +	if (test_bit(GP2AP020A00F_FLAG_ALS_CLEAR_TRIGGER,&data->flags)) {
 >> +		err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_CLEAR_DIS);
 >> +		if (err<  0)
 >> +			goto error_ret;
 >> +	}
 >> +
 >> +	if (test_bit(GP2AP020A00F_FLAG_ALS_IR_TRIGGER,&data->flags)) {
 >> +		err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_IR_DIS);
 >> +		if (err<  0)
 >> +			goto error_ret;
 >> +	}
 >> +
 >> +	if (test_bit(GP2AP020A00F_FLAG_PS_TRIGGER,&data->flags)) {
 >> +		err = gp2ap020a00f_exec_cmd(data,
 >> +					GP2AP020A00F_CMD_TRIGGER_PROX_DIS);
 >> +		if (err<  0)
 >> +			goto error_ret;
 >> +	}
 > In the interests of symetry I'd expect this generic call to be the
 > first thing called in this function.

Fixed.


 >> +static int gp2ap020a00f_remove(struct i2c_client *client)
 >> +{
 >> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 >> +	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 >> +
 >
 > It's small and probably doesn't actually matter, but I'd expect
 > this release order to be the opposite of what we have in the probe
 > function (and hence to match the ordering in the error handling
 > just above here).  In that the irq is freed beffor the buffer_cleanup.
 >

Fixed.

Thanks,
Jacek