Re: [PATCH 1/2] iio: bmi160: Support hardware fifo

[Jonathan Cameron <jic23@kernel.org>]

On 14/11/16 17:30, Marcin Niestroj wrote:
> On 12.11.2016 16:55, Jonathan Cameron wrote:
>> On 09/11/16 17:16, Marcin Niestroj wrote:
>>> On 09.11.2016 15:16, Marcin Niestroj wrote:
>>>> Hi,
>>>> Thanks for review, below are my comments.
>>>>
>>>> On 03.11.2016 13:09, Peter Meerwald-Stadler wrote:
>>>>>
>>>>>> This patch was developed primarily based on bmc150_accel hardware fifo
>>>>>> implementation.
>>>>>
>>>>> parts of the patch are cleanup and bugfixing; should be separate?
>>>>>
>>>>> more comments below
>>>>>
>>>>>> IRQ handler was added, which for now is responsible only for handling
>>>>>> watermark interrupts. The BMI160 chip has two interrupt outputs. By
>>>>>> default INT is considered to be connected. If INT2 is used instead, the
>>>>>> interrupt-names device-tree property can be used to specify that.
>>>>>>
>>>>>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>>>
>>> <snip>
>>>
>>>>>> @@ -574,8 +1114,11 @@ int bmi160_core_probe(struct device *dev,
>>>>>> struct regmap *regmap,
>>>>>>
>>>>>>      data = iio_priv(indio_dev);
>>>>>>      dev_set_drvdata(dev, indio_dev);
>>>>>> +    data->irq = irq;
>>>>>>      data->regmap = regmap;
>>>>>>
>>>>>> +    mutex_init(&data->mutex);
>>>>>> +
>>>>>>      ret = bmi160_chip_init(data, use_spi);
>>>>>>      if (ret < 0)
>>>>>>          return ret;
>>>>>> @@ -591,10 +1134,50 @@ int bmi160_core_probe(struct device *dev,
>>>>>> struct regmap *regmap,
>>>>>>      indio_dev->info = &bmi160_info;
>>>>>>
>>>>>>      ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>> -                     bmi160_trigger_handler, NULL);
>>>>>> +                     bmi160_trigger_handler,
>>>>>> +                     &bmi160_buffer_ops);
>>>>>>      if (ret < 0)
>>>>>>          goto uninit;
>>>>>>
>>>>>> +    if (data->irq > 0) {
>>>>>> +        /* Check which interrupt pin is connected to our board */
>>>>>> +        irq2 = of_irq_get_byname(dev->of_node, "INT2");
>>>>>> +        if (irq2 == data->irq) {
>>>>>> +            dev_dbg(dev, "Using interrupt line INT2\n");
>>>>>> +            data->irq_data = &bmi160_irq2_data;
>>>>>> +        } else {
>>>>>> +            dev_dbg(dev, "Using interrupt line INT1\n");
>>>>>> +            data->irq_data = &bmi160_irq1_data;
>>>>>> +        }
>>>>>> +
>>>>>> +        ret = devm_request_threaded_irq(dev,
>>>>>> +                        data->irq,
>>>>>> +                        bmi160_irq_handler,
>>>>>> +                        bmi160_irq_thread_handler,
>>>>>> +                        IRQF_ONESHOT,
>>>>>> +                        BMI160_IRQ_NAME,
>>>>>> +                        indio_dev);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>
>>> I just noticed, that there should be a "goto buffer_cleanup" instead.
>>>
>>>>>> +
>>>>>> +        ret = bmi160_enable_irq(data);
>>>>>> +        if (ret < 0)
>>>>>> +            goto buffer_cleanup;
>>>>>> +
>>>>>> +        if (block_supported) {
>>>>>> +            indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>>>>>> +            indio_dev->info = &bmi160_info_fifo;
>>>>>> +            indio_dev->buffer->attrs = bmi160_fifo_attributes;
>>>>>> +            data->fifo_buffer = devm_kmalloc(dev,
>>>>>> +                            BMI160_FIFO_LENGTH,
>>>>>> +                            GFP_KERNEL);
>>>>>> +            if (!data->fifo_buffer) {
>>>>>> +                ret = -ENOMEM;
>>>>>
>>>>> need to disable irq on failure?
>>>>
>>>> Yes, I missed that.
>>>
>>> I am just wondering now if it is really neccessary. bmi160_enable_irq()
>>> is just enabling IRQ output on INT1 or INT2 depending on device-tree.
>>> This alone will not trigger interrupts, as all should be masked at this
>>> stage.
>> You should always unwind everything in error paths as it makes them
>> 'obviously' correct rather than subject to subtle bugs.
> 
> Ok. And what about issuing a softreset of the device? This should
> revert all registers to defaults.
That's fine, but best to add a comment saying what it's effects are if they
can be specifically listed (such as 'will disable the interrupt'.)

Again makes it 'obviously correct' which I like as a reviewer!

Jonathan
> 
>>>
>>>>
>>>>>
>>>>>> +                goto buffer_cleanup;
>>>>>> +            }
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>>      ret = iio_device_register(indio_dev);
>>>>>>      if (ret < 0)
>>>>>>          goto buffer_cleanup;
>>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> index 07a179d..aa63f89 100644
>>>>>> --- a/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c
>>>>>> @@ -23,6 +23,10 @@ static int bmi160_i2c_probe(struct i2c_client
>>>>>> *client,
>>>>>>  {
>>>>>>      struct regmap *regmap;
>>>>>>      const char *name = NULL;
>>>>>> +    bool block_supported =
>>>>>> +        i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
>>>>>> +        i2c_check_functionality(client->adapter,
>>>>>> +                    I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>>>>>>
>>>>>>      regmap = devm_regmap_init_i2c(client, &bmi160_regmap_config);
>>>>>>      if (IS_ERR(regmap)) {
>>>>>> @@ -34,7 +38,8 @@ static int bmi160_i2c_probe(struct i2c_client *client,
>>>>>>      if (id)
>>>>>>          name = id->name;
>>>>>>
>>>>>> -    return bmi160_core_probe(&client->dev, regmap, name, false);
>>>>>> +    return bmi160_core_probe(&client->dev, regmap, name, client->irq,
>>>>>> +                false, block_supported);
>>>>>>  }
>>>>>>
>>>>>>  static int bmi160_i2c_remove(struct i2c_client *client)
>>>>>> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> index 1ec8b12..9b57fbe 100644
>>>>>> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
>>>>>> @@ -25,7 +25,8 @@ static int bmi160_spi_probe(struct spi_device *spi)
>>>>>>              (int)PTR_ERR(regmap));
>>>>>>          return PTR_ERR(regmap);
>>>>>>      }
>>>>>> -    return bmi160_core_probe(&spi->dev, regmap, id->name, true);
>>>>>> +    return bmi160_core_probe(&spi->dev, regmap, id->name, spi->irq,
>>>>>> +                true, true);
>>>>>>  }
>>>>>>
>>>>>>  static int bmi160_spi_remove(struct spi_device *spi)
>>>>>>
>>>>>
>>>>
>>>
>>
>