On Thu, May 21, 2020 at 07:30:55PM +0100, Jonathan Cameron wrote:
> On Wed, 20 May 2020 09:17:51 +0200
> Jonathan Albrieux <jonathan.albrieux@xxxxxxxxx> 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 <jonathan.albrieux@xxxxxxxxx> 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 <jonathan.albrieux@xxxxxxxxx>
> > > > ---
> > >
> > > 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 <daniel.baluta@xxxxxxxxx");
> > > > +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@xxxxxxxxx>");
> > >
> > > 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
