v2: fixed missing description for iio: imu: bmi160: added regulator
support
Convert txt format documentation to yaml.
Add documentation about vdd-supply, vddio-supply and mount-matrix.
Add vdd-supply and vddio-supply support. Without this support, vdd and
vddio should be set to always-on in device tree.
Add mount-matrix binding support. As chip could have different
orientations a mount matrix support is needed to correctly translate
these differences
Jonathan Albrieux (4):
dt-bindings: iio: imu: bmi160: convert txt format to yaml
dt-bindings: iio: imu: bmi160: add regulators and mount-matrix
iio: imu: bmi160: added regulator support
iio: imu: bmi160: added mount-matrix support
.../devicetree/bindings/iio/imu/bmi160.txt | 37 ------
.../devicetree/bindings/iio/imu/bmi160.yaml | 105 ++++++++++++++++++
drivers/iio/imu/bmi160/bmi160.h | 3 +
drivers/iio/imu/bmi160/bmi160_core.c | 47 +++++++-
4 files changed, 154 insertions(+), 38 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
create mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.yaml
--
2.17.1
Add vdd-supply and vddio-supply support.
Add mount-matrix support.
Signed-off-by: Jonathan Albrieux <[email protected]>
---
.../devicetree/bindings/iio/imu/bmi160.yaml | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
index 6b464ce5ed0b..5b13af7a209f 100644
--- a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
@@ -46,6 +46,21 @@ properties:
set if the specified interrupt pin should be configured as
open drain. If not set, defaults to push-pull.
+ vdd-supply:
+ maxItems: 1
+ description: |
+ an optional regulator that needs to be on to provide VDD power to
+ the sensor.
+
+ vddio-supply:
+ maxItems: 1
+ description: |
+ an optional regulator that needs to be on to provide the VDD IO power to
+ the sensor.
+
+ mount-matrix:
+ description: an optional 3x3 mounting rotation matrix
+
required:
- compatible
- reg
@@ -61,9 +76,15 @@ examples:
bmi160@68 {
compatible = "bosch,bmi160";
reg = <0x68>;
+ vdd-supply = <&pm8916_l17>;
+ vddio-supply = <&pm8916_l6>;
interrupt-parent = <&gpio4>;
interrupts = <12 1>;
interrupt-names = "INT1";
+ mount-matrix = "0", "1", "0",
+ "-1", "0", "0",
+ "0", "0", "1";
+ };
};
- |
// Example for SPI
--
2.17.1
v2: fixed missing description
Add vdd-supply and vddio-supply support. Without this support vdd and vddio
should be set to always-on in device tree
Signed-off-by: Jonathan Albrieux <[email protected]>
---
drivers/iio/imu/bmi160/bmi160.h | 2 ++
drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 621f5309d735..923c3b274fde 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -3,10 +3,12 @@
#define BMI160_H_
#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
struct bmi160_data {
struct regmap *regmap;
struct iio_trigger *trig;
+ struct regulator_bulk_data supplies[2];
};
extern const struct regmap_config bmi160_regmap_config;
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 6af65d6f1d28..9bbe0d8e6720 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -15,6 +15,7 @@
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
#include <linux/iio/iio.h>
#include <linux/iio/triggered_buffer.h>
@@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
unsigned int val;
struct device *dev = regmap_get_device(data->regmap);
+ ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to enable regulators: %d\n", ret);
+ return ret;
+ }
+
ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
if (ret)
return ret;
@@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
static void bmi160_chip_uninit(void *data)
{
struct bmi160_data *bmi_data = data;
+ struct device *dev = regmap_get_device(bmi_data->regmap);
+ int ret;
bmi160_set_mode(bmi_data, BMI160_GYRO, false);
bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
+
+ ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
+ bmi_data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to disable regulators: %d\n", ret);
+ }
}
int bmi160_core_probe(struct device *dev, struct regmap *regmap,
@@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
dev_set_drvdata(dev, indio_dev);
data->regmap = regmap;
+ data->supplies[0].supply = "vdd";
+ data->supplies[1].supply = "vddio";
+ ret = devm_regulator_bulk_get(dev,
+ ARRAY_SIZE(data->supplies),
+ data->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to get regulators: %d\n", ret);
+ return ret;
+ }
+
ret = bmi160_chip_init(data, use_spi);
if (ret)
return ret;
@@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
}
EXPORT_SYMBOL_GPL(bmi160_core_probe);
-MODULE_AUTHOR("Daniel Baluta <[email protected]");
+MODULE_AUTHOR("Daniel Baluta <[email protected]>");
MODULE_DESCRIPTION("Bosch BMI160 driver");
MODULE_LICENSE("GPL v2");
--
2.17.1
Add mount-matrix binding support. As chip could have different orientations
a mount matrix support is needed to correctly translate these differences
Signed-off-by: Jonathan Albrieux <[email protected]>
---
drivers/iio/imu/bmi160/bmi160.h | 1 +
drivers/iio/imu/bmi160/bmi160_core.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
index 923c3b274fde..a82e040bd109 100644
--- a/drivers/iio/imu/bmi160/bmi160.h
+++ b/drivers/iio/imu/bmi160/bmi160.h
@@ -9,6 +9,7 @@ struct bmi160_data {
struct regmap *regmap;
struct iio_trigger *trig;
struct regulator_bulk_data supplies[2];
+ struct iio_mount_matrix orientation;
};
extern const struct regmap_config bmi160_regmap_config;
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 9bbe0d8e6720..78c8ca962359 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -110,6 +110,7 @@
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .ext_info = bmi160_ext_info, \
}
/* scan indexes follow DATA register order */
@@ -265,6 +266,20 @@ static const struct bmi160_odr_item bmi160_odr_table[] = {
},
};
+static const struct iio_mount_matrix *
+bmi160_get_mount_matrix(const struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct bmi160_data *data = iio_priv(indio_dev);
+
+ return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bmi160_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmi160_get_mount_matrix),
+ { }
+};
+
static const struct iio_chan_spec bmi160_channels[] = {
BMI160_CHANNEL(IIO_ACCEL, X, BMI160_SCAN_ACCEL_X),
BMI160_CHANNEL(IIO_ACCEL, Y, BMI160_SCAN_ACCEL_Y),
@@ -840,6 +855,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
return ret;
}
+ ret = iio_read_mount_matrix(dev, "mount-matrix",
+ &data->orientation);
+ if (ret)
+ return ret;
+
ret = bmi160_chip_init(data, use_spi);
if (ret)
return ret;
--
2.17.1
On Tue, 19 May 2020 09:50:58 +0200
Jonathan Albrieux <[email protected]> wrote:
> Add vdd-supply and vddio-supply support.
> Add mount-matrix support.
>
> Signed-off-by: Jonathan Albrieux <[email protected]>
A few minor comments inline.
> ---
> .../devicetree/bindings/iio/imu/bmi160.yaml | 21 +++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> index 6b464ce5ed0b..5b13af7a209f 100644
> --- a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> @@ -46,6 +46,21 @@ properties:
> set if the specified interrupt pin should be configured as
> open drain. If not set, defaults to push-pull.
>
> + vdd-supply:
> + maxItems: 1
> + description: |
> + an optional regulator that needs to be on to provide VDD power to
> + the sensor.
They aren't optional. Whether we specify them or rely on stub regulators
being provided because they aren't controllable is the optional bit.
That's clearly defined by them not being in the required list below.
So say something li.e
description: |
provide VDD power to the sensor.
> +
> + vddio-supply:
> + maxItems: 1
> + description: |
> + an optional regulator that needs to be on to provide the VDD IO power to
> + the sensor.
> +
> + mount-matrix:
> + description: an optional 3x3 mounting rotation matrix
> +
> required:
> - compatible
> - reg
> @@ -61,9 +76,15 @@ examples:
> bmi160@68 {
> compatible = "bosch,bmi160";
> reg = <0x68>;
> + vdd-supply = <&pm8916_l17>;
> + vddio-supply = <&pm8916_l6>;
> interrupt-parent = <&gpio4>;
> interrupts = <12 1>;
> interrupt-names = "INT1";
> + mount-matrix = "0", "1", "0",
> + "-1", "0", "0",
> + "0", "0", "1";
> + };
> };
> - |
> // Example for SPI
On Tue, 19 May 2020 09:50:59 +0200
Jonathan Albrieux <[email protected]> wrote:
> v2: fixed missing description
Don't put change log here....
>
> Add vdd-supply and vddio-supply support. Without this support vdd and vddio
> should be set to always-on in device tree
Kind of the opposite. If they are always on we don't have to provide them
in the device tree.
A few trivial things inline.
>
> Signed-off-by: Jonathan Albrieux <[email protected]>
> ---
Change log goes here so we don't end up keeping it in the git log.
> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 621f5309d735..923c3b274fde 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -3,10 +3,12 @@
> #define BMI160_H_
>
> #include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
>
> struct bmi160_data {
> struct regmap *regmap;
> struct iio_trigger *trig;
> + struct regulator_bulk_data supplies[2];
> };
>
> extern const struct regmap_config bmi160_regmap_config;
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 6af65d6f1d28..9bbe0d8e6720 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -15,6 +15,7 @@
> #include <linux/delay.h>
> #include <linux/irq.h>
> #include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/triggered_buffer.h>
> @@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> unsigned int val;
> struct device *dev = regmap_get_device(data->regmap);
>
> + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to enable regulators: %d\n", ret);
> + return ret;
> + }
> +
> ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> if (ret)
> return ret;
> @@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> static void bmi160_chip_uninit(void *data)
> {
> struct bmi160_data *bmi_data = data;
> + struct device *dev = regmap_get_device(bmi_data->regmap);
> + int ret;
>
> bmi160_set_mode(bmi_data, BMI160_GYRO, false);
> bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
> +
> + ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
> + bmi_data->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to disable regulators: %d\n", ret);
> + }
No need for brackets around a 1 line if block
if (ret)
dev_err(dev, "failed to disable regulators: %d\n", ret);
> }
>
> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> @@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> dev_set_drvdata(dev, indio_dev);
> data->regmap = regmap;
>
> + data->supplies[0].supply = "vdd";
> + data->supplies[1].supply = "vddio";
> + ret = devm_regulator_bulk_get(dev,
> + ARRAY_SIZE(data->supplies),
> + data->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to get regulators: %d\n", ret);
> + return ret;
> + }
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret)
> return ret;
> @@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> }
> EXPORT_SYMBOL_GPL(bmi160_core_probe);
>
> -MODULE_AUTHOR("Daniel Baluta <[email protected]");
> +MODULE_AUTHOR("Daniel Baluta <[email protected]>");
Good fix but shouldn't be in this patch. Put it a separate patch on it's own.
> MODULE_DESCRIPTION("Bosch BMI160 driver");
> MODULE_LICENSE("GPL v2");
On Tue, 19 May 2020 09:51:00 +0200
Jonathan Albrieux <[email protected]> wrote:
> Add mount-matrix binding support. As chip could have different orientations
> a mount matrix support is needed to correctly translate these differences
>
> Signed-off-by: Jonathan Albrieux <[email protected]>
Hi Jonathan,
Looks good to me. I'll pick this up once 1-3 are tidied up and
we have a device tree review in for the binding doc. I'm rubbish
at reviewing those as Rob will certify so may well have missed something!
Jonathan
> ---
> drivers/iio/imu/bmi160/bmi160.h | 1 +
> drivers/iio/imu/bmi160/bmi160_core.c | 20 ++++++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 923c3b274fde..a82e040bd109 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -9,6 +9,7 @@ struct bmi160_data {
> struct regmap *regmap;
> struct iio_trigger *trig;
> struct regulator_bulk_data supplies[2];
> + struct iio_mount_matrix orientation;
> };
>
> extern const struct regmap_config bmi160_regmap_config;
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 9bbe0d8e6720..78c8ca962359 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -110,6 +110,7 @@
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .ext_info = bmi160_ext_info, \
> }
>
> /* scan indexes follow DATA register order */
> @@ -265,6 +266,20 @@ static const struct bmi160_odr_item bmi160_odr_table[] = {
> },
> };
>
> +static const struct iio_mount_matrix *
> +bmi160_get_mount_matrix(const struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct bmi160_data *data = iio_priv(indio_dev);
> +
> + return &data->orientation;
> +}
> +
> +static const struct iio_chan_spec_ext_info bmi160_ext_info[] = {
> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmi160_get_mount_matrix),
> + { }
> +};
> +
> static const struct iio_chan_spec bmi160_channels[] = {
> BMI160_CHANNEL(IIO_ACCEL, X, BMI160_SCAN_ACCEL_X),
> BMI160_CHANNEL(IIO_ACCEL, Y, BMI160_SCAN_ACCEL_Y),
> @@ -840,6 +855,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> return ret;
> }
>
> + ret = iio_read_mount_matrix(dev, "mount-matrix",
> + &data->orientation);
> + if (ret)
> + return ret;
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret)
> return ret;
On Tue, May 19, 2020 at 06:51:59PM +0100, Jonathan Cameron wrote:
> On Tue, 19 May 2020 09:50:58 +0200
> Jonathan Albrieux <[email protected]> wrote:
>
> > Add vdd-supply and vddio-supply support.
> > Add mount-matrix support.
> >
> > Signed-off-by: Jonathan Albrieux <[email protected]>
>
> A few minor comments inline.
>
> > ---
> > .../devicetree/bindings/iio/imu/bmi160.yaml | 21 +++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> > index 6b464ce5ed0b..5b13af7a209f 100644
> > --- a/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> > +++ b/Documentation/devicetree/bindings/iio/imu/bmi160.yaml
> > @@ -46,6 +46,21 @@ properties:
> > set if the specified interrupt pin should be configured as
> > open drain. If not set, defaults to push-pull.
> >
> > + vdd-supply:
> > + maxItems: 1
> > + description: |
> > + an optional regulator that needs to be on to provide VDD power to
> > + the sensor.
>
> They aren't optional. Whether we specify them or rely on stub regulators
> being provided because they aren't controllable is the optional bit.
> That's clearly defined by them not being in the required list below.
> So say something li.e
>
> description: |
> provide VDD power to the sensor.
>
Oh ok thank you, will work on that
> > +
> > + vddio-supply:
> > + maxItems: 1
> > + description: |
> > + an optional regulator that needs to be on to provide the VDD IO power to
> > + the sensor.
> > +
> > + mount-matrix:
> > + description: an optional 3x3 mounting rotation matrix
> > +
> > required:
> > - compatible
> > - reg
> > @@ -61,9 +76,15 @@ examples:
> > bmi160@68 {
> > compatible = "bosch,bmi160";
> > reg = <0x68>;
> > + vdd-supply = <&pm8916_l17>;
> > + vddio-supply = <&pm8916_l6>;
> > interrupt-parent = <&gpio4>;
> > interrupts = <12 1>;
> > interrupt-names = "INT1";
> > + mount-matrix = "0", "1", "0",
> > + "-1", "0", "0",
> > + "0", "0", "1";
> > + };
> > };
> > - |
> > // Example for SPI
>
>
On Tue, May 19, 2020 at 06:55:35PM +0100, Jonathan Cameron wrote:
> On Tue, 19 May 2020 09:50:59 +0200
> Jonathan Albrieux <[email protected]> wrote:
>
> > v2: fixed missing description
>
> Don't put change log here....
Yep I will put it in the cover letter
> >
> > Add vdd-supply and vddio-supply support. Without this support vdd and vddio
> > should be set to always-on in device tree
>
> Kind of the opposite. If they are always on we don't have to provide them
> in the device tree.
>
I wrote that because, testing on msm8916, without setting the regulators to
always on they were controlled by other components and it happened that
the line wasn't ready during probe causing failure to load the module.
I will try to reword based on your comment, thank you.
> A few trivial things inline.
>
> >
> > Signed-off-by: Jonathan Albrieux <[email protected]>
> > ---
>
> Change log goes here so we don't end up keeping it in the git log.
>
> > drivers/iio/imu/bmi160/bmi160.h | 2 ++
> > drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
> > 2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> > index 621f5309d735..923c3b274fde 100644
> > --- a/drivers/iio/imu/bmi160/bmi160.h
> > +++ b/drivers/iio/imu/bmi160/bmi160.h
> > @@ -3,10 +3,12 @@
> > #define BMI160_H_
> >
> > #include <linux/iio/iio.h>
> > +#include <linux/regulator/consumer.h>
> >
> > struct bmi160_data {
> > struct regmap *regmap;
> > struct iio_trigger *trig;
> > + struct regulator_bulk_data supplies[2];
> > };
> >
> > extern const struct regmap_config bmi160_regmap_config;
> > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > index 6af65d6f1d28..9bbe0d8e6720 100644
> > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > @@ -15,6 +15,7 @@
> > #include <linux/delay.h>
> > #include <linux/irq.h>
> > #include <linux/of_irq.h>
> > +#include <linux/regulator/consumer.h>
> >
> > #include <linux/iio/iio.h>
> > #include <linux/iio/triggered_buffer.h>
> > @@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> > unsigned int val;
> > struct device *dev = regmap_get_device(data->regmap);
> >
> > + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > + return ret;
> > + }
> > +
> > ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> > if (ret)
> > return ret;
> > @@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> > static void bmi160_chip_uninit(void *data)
> > {
> > struct bmi160_data *bmi_data = data;
> > + struct device *dev = regmap_get_device(bmi_data->regmap);
> > + int ret;
> >
> > bmi160_set_mode(bmi_data, BMI160_GYRO, false);
> > bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
> > +
> > + ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
> > + bmi_data->supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to disable regulators: %d\n", ret);
> > + }
> No need for brackets around a 1 line if block
>
Thank you, I didn't noticed that :-)
> if (ret)
> dev_err(dev, "failed to disable regulators: %d\n", ret);
>
> > }
> >
> > int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > @@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > dev_set_drvdata(dev, indio_dev);
> > data->regmap = regmap;
> >
> > + data->supplies[0].supply = "vdd";
> > + data->supplies[1].supply = "vddio";
> > + ret = devm_regulator_bulk_get(dev,
> > + ARRAY_SIZE(data->supplies),
> > + data->supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to get regulators: %d\n", ret);
> > + return ret;
> > + }
> > +
> > ret = bmi160_chip_init(data, use_spi);
> > if (ret)
> > return ret;
> > @@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > }
> > EXPORT_SYMBOL_GPL(bmi160_core_probe);
> >
> > -MODULE_AUTHOR("Daniel Baluta <[email protected]");
> > +MODULE_AUTHOR("Daniel Baluta <[email protected]>");
>
> Good fix but shouldn't be in this patch. Put it a separate patch on it's own.
>
Ok will separate this fix into another patch, thank you!
> > MODULE_DESCRIPTION("Bosch BMI160 driver");
> > MODULE_LICENSE("GPL v2");
>
>
Best regards,
Jonathan Albrieux
On Tue, May 19, 2020 at 06:57:11PM +0100, Jonathan Cameron wrote:
> On Tue, 19 May 2020 09:51:00 +0200
> Jonathan Albrieux <[email protected]> wrote:
>
> > Add mount-matrix binding support. As chip could have different orientations
> > a mount matrix support is needed to correctly translate these differences
> >
> > Signed-off-by: Jonathan Albrieux <[email protected]>
> Hi Jonathan,
>
> Looks good to me. I'll pick this up once 1-3 are tidied up and
> we have a device tree review in for the binding doc. I'm rubbish
> at reviewing those as Rob will certify so may well have missed something!
>
> Jonathan
>
>
Thank you! I'm going to work on suggestions now,
Best regards,
Jonathan Albrieux
>
> > ---
> > drivers/iio/imu/bmi160/bmi160.h | 1 +
> > drivers/iio/imu/bmi160/bmi160_core.c | 20 ++++++++++++++++++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> > index 923c3b274fde..a82e040bd109 100644
> > --- a/drivers/iio/imu/bmi160/bmi160.h
> > +++ b/drivers/iio/imu/bmi160/bmi160.h
> > @@ -9,6 +9,7 @@ struct bmi160_data {
> > struct regmap *regmap;
> > struct iio_trigger *trig;
> > struct regulator_bulk_data supplies[2];
> > + struct iio_mount_matrix orientation;
> > };
> >
> > extern const struct regmap_config bmi160_regmap_config;
> > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > index 9bbe0d8e6720..78c8ca962359 100644
> > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > @@ -110,6 +110,7 @@
> > .storagebits = 16, \
> > .endianness = IIO_LE, \
> > }, \
> > + .ext_info = bmi160_ext_info, \
> > }
> >
> > /* scan indexes follow DATA register order */
> > @@ -265,6 +266,20 @@ static const struct bmi160_odr_item bmi160_odr_table[] = {
> > },
> > };
> >
> > +static const struct iio_mount_matrix *
> > +bmi160_get_mount_matrix(const struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct bmi160_data *data = iio_priv(indio_dev);
> > +
> > + return &data->orientation;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info bmi160_ext_info[] = {
> > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmi160_get_mount_matrix),
> > + { }
> > +};
> > +
> > static const struct iio_chan_spec bmi160_channels[] = {
> > BMI160_CHANNEL(IIO_ACCEL, X, BMI160_SCAN_ACCEL_X),
> > BMI160_CHANNEL(IIO_ACCEL, Y, BMI160_SCAN_ACCEL_Y),
> > @@ -840,6 +855,11 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > return ret;
> > }
> >
> > + ret = iio_read_mount_matrix(dev, "mount-matrix",
> > + &data->orientation);
> > + if (ret)
> > + return ret;
> > +
> > ret = bmi160_chip_init(data, use_spi);
> > if (ret)
> > return ret;
>
>
On Wed, 20 May 2020 09:17:51 +0200
Jonathan Albrieux <[email protected]> wrote:
> On Tue, May 19, 2020 at 06:55:35PM +0100, Jonathan Cameron wrote:
> > On Tue, 19 May 2020 09:50:59 +0200
> > Jonathan Albrieux <[email protected]> wrote:
> >
> > > v2: fixed missing description
> >
> > Don't put change log here....
>
> Yep I will put it in the cover letter
>
> > >
> > > Add vdd-supply and vddio-supply support. Without this support vdd and vddio
> > > should be set to always-on in device tree
> >
> > Kind of the opposite. If they are always on we don't have to provide them
> > in the device tree.
> >
>
> I wrote that because, testing on msm8916, without setting the regulators to
> always on they were controlled by other components and it happened that
> the line wasn't ready during probe causing failure to load the module.
>
> I will try to reword based on your comment, thank you.
Ah. Understood. I'd give that explicit example in the patch description.
I'd assumed this was the normal case of they weren't being described
at all in DT, whereas you case is more complex.
Jonathan
>
> > A few trivial things inline.
> >
> > >
> > > Signed-off-by: Jonathan Albrieux <[email protected]>
> > > ---
> >
> > Change log goes here so we don't end up keeping it in the git log.
> >
> > > drivers/iio/imu/bmi160/bmi160.h | 2 ++
> > > drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
> > > 2 files changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> > > index 621f5309d735..923c3b274fde 100644
> > > --- a/drivers/iio/imu/bmi160/bmi160.h
> > > +++ b/drivers/iio/imu/bmi160/bmi160.h
> > > @@ -3,10 +3,12 @@
> > > #define BMI160_H_
> > >
> > > #include <linux/iio/iio.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > > struct bmi160_data {
> > > struct regmap *regmap;
> > > struct iio_trigger *trig;
> > > + struct regulator_bulk_data supplies[2];
> > > };
> > >
> > > extern const struct regmap_config bmi160_regmap_config;
> > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > > index 6af65d6f1d28..9bbe0d8e6720 100644
> > > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > > @@ -15,6 +15,7 @@
> > > #include <linux/delay.h>
> > > #include <linux/irq.h>
> > > #include <linux/of_irq.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > > #include <linux/iio/iio.h>
> > > #include <linux/iio/triggered_buffer.h>
> > > @@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> > > unsigned int val;
> > > struct device *dev = regmap_get_device(data->regmap);
> > >
> > > + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> > > if (ret)
> > > return ret;
> > > @@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> > > static void bmi160_chip_uninit(void *data)
> > > {
> > > struct bmi160_data *bmi_data = data;
> > > + struct device *dev = regmap_get_device(bmi_data->regmap);
> > > + int ret;
> > >
> > > bmi160_set_mode(bmi_data, BMI160_GYRO, false);
> > > bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
> > > +
> > > + ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
> > > + bmi_data->supplies);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to disable regulators: %d\n", ret);
> > > + }
> > No need for brackets around a 1 line if block
> >
>
> Thank you, I didn't noticed that :-)
>
> > if (ret)
> > dev_err(dev, "failed to disable regulators: %d\n", ret);
> >
> > > }
> > >
> > > int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > @@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > dev_set_drvdata(dev, indio_dev);
> > > data->regmap = regmap;
> > >
> > > + data->supplies[0].supply = "vdd";
> > > + data->supplies[1].supply = "vddio";
> > > + ret = devm_regulator_bulk_get(dev,
> > > + ARRAY_SIZE(data->supplies),
> > > + data->supplies);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to get regulators: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > ret = bmi160_chip_init(data, use_spi);
> > > if (ret)
> > > return ret;
> > > @@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > }
> > > EXPORT_SYMBOL_GPL(bmi160_core_probe);
> > >
> > > -MODULE_AUTHOR("Daniel Baluta <[email protected]");
> > > +MODULE_AUTHOR("Daniel Baluta <[email protected]>");
> >
> > Good fix but shouldn't be in this patch. Put it a separate patch on it's own.
> >
>
> Ok will separate this fix into another patch, thank you!
>
> > > MODULE_DESCRIPTION("Bosch BMI160 driver");
> > > MODULE_LICENSE("GPL v2");
> >
> >
>
> Best regards,
> Jonathan Albrieux
On Thu, May 21, 2020 at 07:30:55PM +0100, Jonathan Cameron wrote:
> On Wed, 20 May 2020 09:17:51 +0200
> Jonathan Albrieux <[email protected]> wrote:
>
> > On Tue, May 19, 2020 at 06:55:35PM +0100, Jonathan Cameron wrote:
> > > On Tue, 19 May 2020 09:50:59 +0200
> > > Jonathan Albrieux <[email protected]> wrote:
> > >
> > > > v2: fixed missing description
> > >
> > > Don't put change log here....
> >
> > Yep I will put it in the cover letter
> >
> > > >
> > > > Add vdd-supply and vddio-supply support. Without this support vdd and vddio
> > > > should be set to always-on in device tree
> > >
> > > Kind of the opposite. If they are always on we don't have to provide them
> > > in the device tree.
> > >
> >
> > I wrote that because, testing on msm8916, without setting the regulators to
> > always on they were controlled by other components and it happened that
> > the line wasn't ready during probe causing failure to load the module.
> >
> > I will try to reword based on your comment, thank you.
>
> Ah. Understood. I'd give that explicit example in the patch description.
> I'd assumed this was the normal case of they weren't being described
> at all in DT, whereas you case is more complex.
>
> Jonathan
>
Yep, I omitted to describe the case I was in. I'll add it to next patch, thank
you,
> >
> > > A few trivial things inline.
> > >
> > > >
> > > > Signed-off-by: Jonathan Albrieux <[email protected]>
> > > > ---
> > >
> > > Change log goes here so we don't end up keeping it in the git log.
> > >
> > > > drivers/iio/imu/bmi160/bmi160.h | 2 ++
> > > > drivers/iio/imu/bmi160/bmi160_core.c | 27 ++++++++++++++++++++++++++-
> > > > 2 files changed, 28 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> > > > index 621f5309d735..923c3b274fde 100644
> > > > --- a/drivers/iio/imu/bmi160/bmi160.h
> > > > +++ b/drivers/iio/imu/bmi160/bmi160.h
> > > > @@ -3,10 +3,12 @@
> > > > #define BMI160_H_
> > > >
> > > > #include <linux/iio/iio.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >
> > > > struct bmi160_data {
> > > > struct regmap *regmap;
> > > > struct iio_trigger *trig;
> > > > + struct regulator_bulk_data supplies[2];
> > > > };
> > > >
> > > > extern const struct regmap_config bmi160_regmap_config;
> > > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > > > index 6af65d6f1d28..9bbe0d8e6720 100644
> > > > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > > > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > > > @@ -15,6 +15,7 @@
> > > > #include <linux/delay.h>
> > > > #include <linux/irq.h>
> > > > #include <linux/of_irq.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >
> > > > #include <linux/iio/iio.h>
> > > > #include <linux/iio/triggered_buffer.h>
> > > > @@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> > > > unsigned int val;
> > > > struct device *dev = regmap_get_device(data->regmap);
> > > >
> > > > + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> > > > if (ret)
> > > > return ret;
> > > > @@ -793,9 +800,17 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> > > > static void bmi160_chip_uninit(void *data)
> > > > {
> > > > struct bmi160_data *bmi_data = data;
> > > > + struct device *dev = regmap_get_device(bmi_data->regmap);
> > > > + int ret;
> > > >
> > > > bmi160_set_mode(bmi_data, BMI160_GYRO, false);
> > > > bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
> > > > +
> > > > + ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
> > > > + bmi_data->supplies);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to disable regulators: %d\n", ret);
> > > > + }
> > > No need for brackets around a 1 line if block
> > >
> >
> > Thank you, I didn't noticed that :-)
> >
> > > if (ret)
> > > dev_err(dev, "failed to disable regulators: %d\n", ret);
> > >
> > > > }
> > > >
> > > > int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > > @@ -815,6 +830,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > > dev_set_drvdata(dev, indio_dev);
> > > > data->regmap = regmap;
> > > >
> > > > + data->supplies[0].supply = "vdd";
> > > > + data->supplies[1].supply = "vddio";
> > > > + ret = devm_regulator_bulk_get(dev,
> > > > + ARRAY_SIZE(data->supplies),
> > > > + data->supplies);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to get regulators: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > ret = bmi160_chip_init(data, use_spi);
> > > > if (ret)
> > > > return ret;
> > > > @@ -853,6 +878,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> > > > }
> > > > EXPORT_SYMBOL_GPL(bmi160_core_probe);
> > > >
> > > > -MODULE_AUTHOR("Daniel Baluta <[email protected]");
> > > > +MODULE_AUTHOR("Daniel Baluta <[email protected]>");
> > >
> > > Good fix but shouldn't be in this patch. Put it a separate patch on it's own.
> > >
> >
> > Ok will separate this fix into another patch, thank you!
> >
> > > > MODULE_DESCRIPTION("Bosch BMI160 driver");
> > > > MODULE_LICENSE("GPL v2");
> > >
> > >
> >
> > Best regards,
> > Jonathan Albrieux
>
Best regards,
Jonathan Albrieux