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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
