Re: [PATCH v2 3/4] iio: imu: bmi160: added regulator support

[Jonathan Albrieux <jonathan.albrieux@gmail.com>]

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@gmail.com> 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@gmail.com> 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@gmail.com>
> > > > ---  
> > > 
> > > 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@intel.com");
> > > > +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");  
> > > 
> > > 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