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.
>
>>
>>>
>>>> + 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
