* [PATCH 0/1] iio: accel: bma400: add PM_SLEEP support
@ 2020-07-15 5:02 Dan Robertson
2020-07-15 5:02 ` [PATCH 1/1] " Dan Robertson
0 siblings, 1 reply; 6+ messages in thread
From: Dan Robertson @ 2020-07-15 5:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Linus Walleij,
Andy Shevchenko, linux-iio, Dan Robertson
The following patch adds support for PM_SLEEP. I also added an attribute
for manually setting the power mode similar to what the BMA180 driver
provides. Feedback on this would be appreciated.
Cheers,
- Dan
Dan Robertson (1):
iio: accel: bma400: add PM_SLEEP support
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(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support
2020-07-15 5:02 [PATCH 0/1] iio: accel: bma400: add PM_SLEEP support Dan Robertson
@ 2020-07-15 5:02 ` Dan Robertson
2020-07-15 5:44 ` Andy Shevchenko
2020-07-20 11:23 ` Jonathan Cameron
0 siblings, 2 replies; 6+ messages in thread
From: Dan Robertson @ 2020-07-15 5:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Linus Walleij,
Andy Shevchenko, linux-iio, Dan Robertson
- Add system sleep ops if CONFIG_PM_SLEEP is set.
- Add attribute for setting the power mode of the
device.
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),
- { }
-};
-
-#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
+static int bma400_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bma400_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+#endif
+
+const struct dev_pm_ops bma400_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ SET_SYSTEM_SLEEP_PM_OPS(bma400_suspend, bma400_resume)
+#endif
+};
+EXPORT_SYMBOL(bma400_pm_ops);
+
MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index 9dcb7cc9996e..52a779d53629 100644
--- a/drivers/iio/accel/bma400_i2c.c
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -48,6 +48,7 @@ static struct i2c_driver bma400_i2c_driver = {
.driver = {
.name = "bma400",
.of_match_table = bma400_of_i2c_match,
+ .pm = &bma400_pm_ops
},
.probe = bma400_i2c_probe,
.remove = bma400_i2c_remove,
diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
index 7c2825904e08..358bd26ac4cd 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -108,6 +108,7 @@ static struct spi_driver bma400_spi_driver = {
.driver = {
.name = "bma400",
.of_match_table = bma400_of_spi_match,
+ .pm = &bma400_pm_ops
},
.probe = bma400_spi_probe,
.remove = bma400_spi_remove,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support
2020-07-15 5:02 ` [PATCH 1/1] " Dan Robertson
@ 2020-07-15 5:44 ` Andy Shevchenko
2020-07-20 23:50 ` Dan Robertson
2020-07-20 11:23 ` Jonathan Cameron
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-07-15 5:44 UTC (permalink / raw)
To: Dan Robertson
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Linus Walleij, linux-iio
On Wed, Jul 15, 2020 at 8:05 AM 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.
...
> -static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> -};
> -
> -#define BMA400_ACC_CHANNEL(_axis) { \
> -}
> -
> -static const struct iio_chan_spec bma400_channels[] = {
> -};
> -
I'm not sure how this part is related.
...
> +static const char * const bma400_power_modes[] = {
> + "sleep",
> + "low-power",
> + "normal"
Missed comma.
> +};
...
> +#ifdef CONFIG_PM_SLEEP
__maybe_unused looks better.
> +static int bma400_suspend(struct device *dev)
> +{
> +}
> +
> +static int bma400_resume(struct device *dev)
> +{
> +}
> +#endif
> +
> +const struct dev_pm_ops bma400_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
Why?
> + SET_SYSTEM_SLEEP_PM_OPS(bma400_suspend, bma400_resume)
> +#endif
> +};
> +EXPORT_SYMBOL(bma400_pm_ops);
...
> .driver = {
> .name = "bma400",
> .of_match_table = bma400_of_i2c_match,
> + .pm = &bma400_pm_ops
Missed comma.
> },
...
> .driver = {
> .name = "bma400",
> .of_match_table = bma400_of_spi_match,
> + .pm = &bma400_pm_ops
Ditto.
> },
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support
2020-07-15 5:02 ` [PATCH 1/1] " Dan Robertson
2020-07-15 5:44 ` Andy Shevchenko
@ 2020-07-20 11:23 ` Jonathan Cameron
2020-07-20 23:53 ` Dan Robertson
1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2020-07-20 11:23 UTC (permalink / raw)
To: Dan Robertson
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Linus Walleij,
Andy Shevchenko, linux-iio
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.
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.
> -
> -#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.
> +static int bma400_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int bma400_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +#endif
> +
> +const struct dev_pm_ops bma400_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> + SET_SYSTEM_SLEEP_PM_OPS(bma400_suspend, bma400_resume)
> +#endif
> +};
> +EXPORT_SYMBOL(bma400_pm_ops);
> +
> MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> index 9dcb7cc9996e..52a779d53629 100644
> --- a/drivers/iio/accel/bma400_i2c.c
> +++ b/drivers/iio/accel/bma400_i2c.c
> @@ -48,6 +48,7 @@ static struct i2c_driver bma400_i2c_driver = {
> .driver = {
> .name = "bma400",
> .of_match_table = bma400_of_i2c_match,
> + .pm = &bma400_pm_ops
> },
> .probe = bma400_i2c_probe,
> .remove = bma400_i2c_remove,
> diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
> index 7c2825904e08..358bd26ac4cd 100644
> --- a/drivers/iio/accel/bma400_spi.c
> +++ b/drivers/iio/accel/bma400_spi.c
> @@ -108,6 +108,7 @@ static struct spi_driver bma400_spi_driver = {
> .driver = {
> .name = "bma400",
> .of_match_table = bma400_of_spi_match,
> + .pm = &bma400_pm_ops
> },
> .probe = bma400_spi_probe,
> .remove = bma400_spi_remove,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support
2020-07-15 5:44 ` Andy Shevchenko
@ 2020-07-20 23:50 ` Dan Robertson
0 siblings, 0 replies; 6+ messages in thread
From: Dan Robertson @ 2020-07-20 23:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Linus Walleij, linux-iio
[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]
On Wed, Jul 15, 2020 at 08:44:59AM +0300, Andy Shevchenko wrote:
> On Wed, Jul 15, 2020 at 8:05 AM 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.
>
> ...
>
> > -static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
>
> > -};
> > -
> > -#define BMA400_ACC_CHANNEL(_axis) { \
>
> > -}
> > -
> > -static const struct iio_chan_spec bma400_channels[] = {
>
> > -};
> > -
>
> I'm not sure how this part is related.
>
> ...
Moving things around for the power mode switching endpoint.
> > +static const char * const bma400_power_modes[] = {
> > + "sleep",
> > + "low-power",
> > + "normal"
>
> Missed comma.
>
> > +};
>
> ...
>
> > +#ifdef CONFIG_PM_SLEEP
>
> __maybe_unused looks better.
Good point.
Thanks for the review! I'll address your comments in v2 of the patchset.
Cheers,
- Dan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] iio: accel: bma400: add PM_SLEEP support
2020-07-20 11:23 ` Jonathan Cameron
@ 2020-07-20 23:53 ` Dan Robertson
0 siblings, 0 replies; 6+ messages in thread
From: Dan Robertson @ 2020-07-20 23:53 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Linus Walleij,
Andy Shevchenko, linux-iio
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-21 0:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 5:02 [PATCH 0/1] iio: accel: bma400: add PM_SLEEP support Dan Robertson
2020-07-15 5:02 ` [PATCH 1/1] " Dan Robertson
2020-07-15 5:44 ` Andy Shevchenko
2020-07-20 23:50 ` Dan Robertson
2020-07-20 11:23 ` Jonathan Cameron
2020-07-20 23:53 ` Dan Robertson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.