Re: [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support

[Dan Robertson <dan@dlrobertson.com>]

[-- Attachment #1: Type: text/plain, Size: 6481 bytes --]

On Mon, Jul 20, 2020 at 12:23:52PM +0100, Jonathan Cameron wrote:
> On Wed, 15 Jul 2020 01:02:26 -0400
> Dan Robertson <dan@dlrobertson.com> wrote:
> 
> >  - Add system sleep ops if CONFIG_PM_SLEEP is set.
> >  - Add attribute for setting the power mode of the
> >    device.
> 
> This second part is a problem because power modes tend to be extremely
> device specific.  Hence generic userspace is near impossible.
> 
> So what we try to do is to map these as much as possible to
> things that the driver can figure out for itself, such as switching
> parameters of how things are captured (sampling frequency etc) or
> runtime pm.
> 
> Whilst this may not cover 'all' usecases, it will cover a lot more
> than implementing a custom attribute.

That makes sense. I'll drop this for now and think about it a bit, but
I think runtime pm would make sense.

> 
> As a side note, a custom attribute also need Docs
> in Documentation/ABI/testing/sysfs-bus-iio-*
> 
> Thanks,
> 
> Jonathan
> 
> 
> > 
> > Signed-off-by: Dan Robertson <dan@dlrobertson.com>
> > ---
> >  drivers/iio/accel/bma400.h      |   3 +
> >  drivers/iio/accel/bma400_core.c | 132 ++++++++++++++++++++++++--------
> >  drivers/iio/accel/bma400_i2c.c  |   1 +
> >  drivers/iio/accel/bma400_spi.c  |   1 +
> >  4 files changed, 107 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index 5ad10db9819f..e9dd9e918aac 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -10,6 +10,7 @@
> >  #define _BMA400_H_
> >  
> >  #include <linux/bits.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >  
> >  /*
> > @@ -96,4 +97,6 @@ int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
> >  
> >  int bma400_remove(struct device *dev);
> >  
> > +extern const struct dev_pm_ops bma400_pm_ops;
> > +
> >  #endif
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index cc77f89c048b..5af57b8e1fd7 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -147,36 +147,6 @@ bma400_accel_get_mount_matrix(const struct iio_dev *indio_dev,
> >  	return &data->orientation;
> >  }
> >  
> > -static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > -	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
> > -	{ }
> > -};
> 
> It would be helpful to do this reorganizing as a precursor patch where
> nothing functional changes.   Then we can see very quickly what is new
> in the functional changes.

I'll drop it for now. It isn't needed without the custom attribute.

> > -
> > -#define BMA400_ACC_CHANNEL(_axis) { \
> > -	.type = IIO_ACCEL, \
> > -	.modified = 1, \
> > -	.channel2 = IIO_MOD_##_axis, \
> > -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > -		BIT(IIO_CHAN_INFO_SCALE) | \
> > -		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > -	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > -		BIT(IIO_CHAN_INFO_SCALE) | \
> > -		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > -	.ext_info = bma400_ext_info, \
> > -}
> > -
> > -static const struct iio_chan_spec bma400_channels[] = {
> > -	BMA400_ACC_CHANNEL(X),
> > -	BMA400_ACC_CHANNEL(Y),
> > -	BMA400_ACC_CHANNEL(Z),
> > -	{
> > -		.type = IIO_TEMP,
> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > -	},
> > -};
> > -
> >  static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> >  {
> >  	unsigned int raw_temp;
> > @@ -542,6 +512,73 @@ static int bma400_set_power_mode(struct bma400_data *data,
> >  	return 0;
> >  }
> >  
> > +static const char * const bma400_power_modes[] = {
> > +	"sleep",
> > +	"low-power",
> > +	"normal"
> > +};
> > +
> > +int bma400_power_mode_enum_get(struct iio_dev *dev,
> > +			       const struct iio_chan_spec *chan)
> > +{
> > +	struct bma400_data *data = iio_priv(dev);
> > +
> > +	return data->power_mode;
> > +}
> > +
> > +int bma400_power_mode_enum_set(struct iio_dev *dev,
> > +			       const struct iio_chan_spec *chan,
> > +			       unsigned int mode)
> > +{
> > +	struct bma400_data *data = iio_priv(dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	ret = bma400_set_power_mode(data, mode);
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_enum bma400_power_mode_enum = {
> > +	.items = bma400_power_modes,
> > +	.num_items = ARRAY_SIZE(bma400_power_modes),
> > +	.get = bma400_power_mode_enum_get,
> > +	.set = bma400_power_mode_enum_set,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > +	IIO_ENUM("power_mode", true, &bma400_power_mode_enum),
> > +	IIO_ENUM_AVAILABLE("power_mode", &bma400_power_mode_enum),
> > +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
> > +	{ }
> > +};
> > +
> > +#define BMA400_ACC_CHANNEL(_axis) { \
> > +	.type = IIO_ACCEL, \
> > +	.modified = 1, \
> > +	.channel2 = IIO_MOD_##_axis, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > +		BIT(IIO_CHAN_INFO_SCALE) | \
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > +		BIT(IIO_CHAN_INFO_SCALE) | \
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > +	.ext_info = bma400_ext_info, \
> > +}
> > +
> > +static const struct iio_chan_spec bma400_channels[] = {
> > +	BMA400_ACC_CHANNEL(X),
> > +	BMA400_ACC_CHANNEL(Y),
> > +	BMA400_ACC_CHANNEL(Z),
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > +	},
> > +};
> > +
> >  static void bma400_init_tables(void)
> >  {
> >  	int raw;
> > @@ -848,6 +885,41 @@ int bma400_remove(struct device *dev)
> >  }
> >  EXPORT_SYMBOL(bma400_remove);
> >  
> > +#ifdef CONFIG_PM_SLEEP
> 
> Ifdef protections around PM functions tend to go wrong so usually
> better to just mark them __maybe_unused and rely on the
> linker removing them if they aren't.

Sounds good.

Cheers,

 - Dan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]