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@xxxxxxxxxxxxxxxx>
>>>
>>> <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)
>>>>>>
>>>>>
>>>>
>>>
>>
>
--
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
