On 28/01/15 19:26, Octavian Purdila wrote:
> On Sun, Jan 4, 2015 at 7:08 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> On 21/12/14 00:42, Octavian Purdila wrote:
>>> Add a new watermark trigger and hardware fifo operations. When the
>>> watermark trigger is activated the watermark level is set and the
>>> hardware FIFO is activated.
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
>> Mostly good, though I spot a demux in here that definitely shouldn't
>> be there and connected access to indio_dev->buffer->scan_mask which is
>> very dangerous as it may well not be the same as indio_dev->active_scan_mask
>> (which is what controls which data is captured).
>>
>> This is also true of the original driver trigger handler and a number of
>> other drivers. Ooops, I've not been picking up on that in reviews recently
>> by the look of it.
>>
>> If anyone is feeling bored a quick grep highlights the buggy drivers...
>> If not I'll get to it, but isn't that critical as right now, no mainline
>> driver is using the interface that will cause this issue.
>>
>
> Oh, ok, didn't know that we should use indio_dev->active_scan_mask
> instead of indio_dev->buffer->scan_mask. I will take a closer look
> after I am done reworking this patch set.
>
>> Jonathan
>>> ---
>>> drivers/iio/accel/bmc150-accel.c | 194 ++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 190 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
>>> index 14509be..0aa3126 100644
>>> --- a/drivers/iio/accel/bmc150-accel.c
>>> +++ b/drivers/iio/accel/bmc150-accel.c
>>> @@ -67,7 +67,9 @@
>>> #define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE BIT(2)
>>>
>>> #define BMC150_ACCEL_REG_INT_MAP_1 0x1A
>>> -#define BMC150_ACCEL_INT_MAP_1_BIT_DATA BIT(0)
>>> +#define BMC150_ACCEL_INT_MAP_1_BIT_DATA BIT(0)
>>> +#define BMC150_ACCEL_INT_MAP_1_BIT_FWM BIT(1)
>>> +#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL BIT(2)
>>>
>>> #define BMC150_ACCEL_REG_INT_RST_LATCH 0x21
>>> #define BMC150_ACCEL_INT_MODE_LATCH_RESET 0x80
>>> @@ -80,7 +82,9 @@
>>> #define BMC150_ACCEL_INT_EN_BIT_SLP_Z BIT(2)
>>>
>>> #define BMC150_ACCEL_REG_INT_EN_1 0x17
>>> -#define BMC150_ACCEL_INT_EN_BIT_DATA_EN BIT(4)
>>> +#define BMC150_ACCEL_INT_EN_BIT_DATA_EN BIT(4)
>>> +#define BMC150_ACCEL_INT_EN_BIT_FFULL_EN BIT(5)
>>> +#define BMC150_ACCEL_INT_EN_BIT_FWM_EN BIT(6)
>>>
>>> #define BMC150_ACCEL_REG_INT_OUT_CTRL 0x20
>>> #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL BIT(0)
>>> @@ -119,6 +123,12 @@
>>> #define BMC150_ACCEL_AXIS_TO_REG(axis) (BMC150_ACCEL_REG_XOUT_L + (axis * 2))
>>> #define BMC150_AUTO_SUSPEND_DELAY_MS 2000
>>>
>>> +#define BMC150_ACCEL_REG_FIFO_STATUS 0x0E
>>> +#define BMC150_ACCEL_REG_FIFO_CONFIG0 0x30
>>> +#define BMC150_ACCEL_REG_FIFO_CONFIG1 0x3E
>>> +#define BMC150_ACCEL_REG_FIFO_DATA 0x3F
>>> +#define BMC150_ACCEL_FIFO_LENGTH 32
>>> +
>>> enum bmc150_accel_axis {
>>> AXIS_X,
>>> AXIS_Y,
>>> @@ -161,6 +171,7 @@ struct bmc150_accel_trigger {
>>> struct bmc150_accel_data *data;
>>> struct iio_trigger *indio_trig;
>>> bool enabled;
>>> + int (*setup)(struct bmc150_accel_trigger *t, bool state);
>>> };
>>>
>>> struct bmc150_accel_event {
>>> @@ -180,8 +191,8 @@ struct bmc150_accel_event {
>>> };
>>> };
>>>
>>> -#define BMC150_ACCEL_INTERRUPTS 2
>>> -#define BMC150_ACCEL_TRIGGERS 2
>>> +#define BMC150_ACCEL_INTERRUPTS 3
>>> +#define BMC150_ACCEL_TRIGGERS 3
>>> #define BMC150_ACCEL_EVENTS 1
>>>
>>> struct bmc150_accel_data {
>>> @@ -191,6 +202,7 @@ struct bmc150_accel_data {
>>> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
>>> struct bmc150_accel_event events[BMC150_ACCEL_EVENTS];
>>> struct mutex mutex;
>>> + u8 fifo_mode, watermark;
>>> s16 buffer[8];
>>> u8 bw_bits;
>>> u32 range;
>>> @@ -484,6 +496,12 @@ bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = {
>>> BMC150_ACCEL_INT_EN_BIT_SLP_Y |
>>> BMC150_ACCEL_INT_EN_BIT_SLP_Z
>>> },
>>> + { /* fifo watermark interrupt */
>>> + .map_reg = BMC150_ACCEL_REG_INT_MAP_1,
>>> + .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM,
>>> + .en_reg = BMC150_ACCEL_REG_INT_EN_1,
>>> + .en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN,
>>> + },
>>> };
>>>
>>> static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev,
>>> @@ -1020,6 +1038,8 @@ static const struct iio_info bmc150_accel_info = {
>>> .driver_module = THIS_MODULE,
>>> };
>>>
>>> +static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev);
>>> +
>>> static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>>> {
>>> struct iio_poll_func *pf = p;
>>> @@ -1027,6 +1047,12 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>>> struct bmc150_accel_data *data = iio_priv(indio_dev);
>>> int bit, ret, i = 0;
>>>
>>> + if (data->fifo_mode) {
>>> + bmc150_accel_fifo_flush(indio_dev);
>> When you flush here, you want to get the best possible timestamp as close
>> to the interrupt as possible. Perhaps even in the top half interrupt
>> handler - then pass it through to here...
>
> Yes, we already take the timestamp in the IRQ handler
> (bmc150_accel_data_rdy_trig_poll).
>
>>> +
>>> + if (!data->timestamp)
>>> + data->timestamp = iio_get_time_ns();
>> As this is on the flush rather than an interrupt these are going
>> to be of dubious benefit... There isn't an obvious way of doing better though
>> unless we do have an interrupt. In that case you want to grab them as
>> early as possible (typically even in the interrupt top half) and pass it
>> down to where you want to use it.
>
> Actually flush gets called from two places: from the watermark trigger
> and in this case we take the timestamp in the IRQ handler, and when we
> do a read or poll and there is not enough data available in the
> buffer.
>
> For this later case we will have an error of up to a maximum of a one
> sample period since flush was called in between one of the sampling
> periods - the watermark interrupt makes sure we don't stall forever.
> That is not that bad.
Not terrible. I wonder if we want to indicated a dubious timestamp
in some way? Or maybe event specify a jitter on the channel?
(been wondering if we want this sort of description for channels in
general - how noisy are they?) Can be very useful to userspace and
is often tied up with the particular settings of the device.
>
> Also, the later case should be an exception if the application sets
> the right watermark level and uses the right timeout. Otherwise it
> will not use power optimally which is the whole point of the hw fifo.
>
>>> +
>>> + tstamp = data->timestamp - count * sample_freq;
>>> +
>>> + for (i = 0; i < count; i++) {
>>> + u16 sample[8];
>>> + int j, bit;
>>> +
>>> + j = 0;
>>> + for_each_set_bit(bit, indio_dev->buffer->scan_mask,
>>> + indio_dev->masklength) {
>>> + memcpy(&sample[j++], &buffer[i * 3 + bit], 2);
>>> + }
>>
>> A local demux rather than using the main iio one. Given you clearly read the
>> lot anyway is there any reason not to just pass it all on and let the IIO
>> demux handling the demux on the way to the kfifo?
>>
>
> Hmm, isn't the demux part only used when we have client buffers? The
> demux part is not used at all in my tests, due to
> indio_dev->active_scan_mask being equal to buffer->scan_mask.
I'm guessing you figured this out. Yes, only used with client buffers,
but the normal buffered access is a client buffer :)
>
>> There should be no access to the buffer scan_mask by drivers.
>>
>> They should only see the indio_dev->active_scan_mask (they may well not
>> be the same due to client devices).
>>
>
> Right, after taking a closer look at the demux part I finally
> understand it. I will fix it in the next version.
>
>>> +
>>> + iio_push_to_buffers_with_timestamp(indio_dev, sample, tstamp);
>>> +
>>> + tstamp += sample_freq;
>>> + }
>>> +
>>> + data->timestamp = 0;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int bmc150_accel_fifo_mode_set(struct bmc150_accel_data *data)
>>> +{
>>> + u8 reg = BMC150_ACCEL_REG_FIFO_CONFIG1;
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(data->client, reg);
>>> +
>>> + /* writting the fifo config discards FIFO data - avoid it if possible */
>> Strikes me that caching the values of some registers would be a good idea
>> - probably by using regmap to handle it. Still a job for another day.
>> I will keep this in mind.
>
> Good point, I'll add this to my todo list.
>
>>> + if (ret == data->fifo_mode)
>>> + return 0;
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client, reg, data->fifo_mode);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev, "Error writing reg_fifo_config1\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (!data->fifo_mode)
>>> + return 0;
>>> +
>>> + /* we can set the the watermark value only after FIFO is enabled */
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + BMC150_ACCEL_REG_FIFO_CONFIG0,
>>> + data->watermark);
>>> +
>>> + if (ret < 0)
>>> + dev_err(&data->client->dev, "Error writing reg_fifo_config0\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int bmc150_accel_fifo_setup(struct bmc150_accel_trigger *t, bool state)
>>> +{
>>> + if (state)
>> a #define for the magic 0x40 perhaps?
>>> + t->data->fifo_mode = 0x40;
>>> + else
>>> + t->data->fifo_mode = 0;
>>> +
>>> + return bmc150_accel_fifo_mode_set(t->data);
>>> +}
>>> +
>>> +const struct iio_hwfifo bmc150_accel_hwfifo = {
>>> + .length = BMC150_ACCEL_FIFO_LENGTH,
>>> + .set_watermark = bmc150_accel_set_watermark,
>>> + .flush = bmc150_accel_fifo_flush,
>>> +};
>>> +
>>> static int bmc150_accel_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> @@ -1387,6 +1567,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
>>> "Failed: iio triggered buffer setup\n");
>>> goto err_trigger_unregister;
>>> }
>>> +
>>> + indio_dev->hwfifo = &bmc150_accel_hwfifo;
>>> }
>>>
>>> ret = iio_device_register(indio_dev);
>>> @@ -1458,6 +1640,7 @@ static int bmc150_accel_resume(struct device *dev)
>>> mutex_lock(&data->mutex);
>>> if (atomic_read(&data->active_intr))
>>> bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>>> + bmc150_accel_fifo_mode_set(data);
>>> mutex_unlock(&data->mutex);
>>>
>>> return 0;
>>> @@ -1487,6 +1670,9 @@ static int bmc150_accel_runtime_resume(struct device *dev)
>>> ret = bmc150_accel_set_mode(data, BMC150_ACCEL_SLEEP_MODE_NORMAL, 0);
>>> if (ret < 0)
>>> return ret;
>>> + ret = bmc150_accel_fifo_mode_set(data);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> sleep_val = bmc150_accel_get_startup_times(data);
>>> if (sleep_val < 20)
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
