| Message ID | 20220903222402.3426058-1-cmo@melexis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: temperature: mlx90632: Add powermanagement | expand |
On Sun, 4 Sep 2022 00:24:02 +0200 cmo@melexis.com wrote: > From: Crt Mori <cmo@melexis.com> > > Allow users to read sensor sampling frequency to better plan the > application measurement requests. > > Signed-off-by: Crt Mori <cmo@melexis.com> Please use the read_avail() callback for the available attribute. One day I'll finish converting all the many old attrs for this over, but I don't want to introduce more work to do in the meantime. Otherwise this looks fine to me. > --- > drivers/iio/temperature/mlx90632.c | 44 ++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c > index 9acd819c76a6..37edd324d6a1 100644 > --- a/drivers/iio/temperature/mlx90632.c > +++ b/drivers/iio/temperature/mlx90632.c > @@ -80,6 +80,9 @@ > #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */ > > #define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits */ > +#define MLX90632_REFRESH_RATE(ee_val) (MLX90632_EE_RR(ee_val) >> 8) > + /* Extract Refresh Rate from ee register */ > +#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8) > > /* Measurement types */ > #define MLX90632_MTYP_MEDICAL 0 > @@ -908,6 +911,24 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) > return ret; > } > > +static int mlx90632_get_refresh_rate(struct mlx90632_data *data, > + int *refresh_rate) > +{ > + unsigned int meas1; > + int ret; > + > + ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1); > + if (ret < 0) > + return ret; > + > + *refresh_rate = MLX90632_REFRESH_RATE(meas1); > + > + return ret; > +} > + > +static const int mlx90632_freqs[][2] = { {0, 500000}, {1, 0}, {2, 0}, {4, 0}, > + {8, 0}, {16, 0}, {32, 0}, {64, 0} }; > + > static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data) > { > unsigned long now; > @@ -978,6 +999,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev, > *val = data->object_ambient_temperature; > ret = IIO_VAL_INT; > break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = mlx90632_get_refresh_rate(data, &cr); > + if (ret < 0) > + goto mlx90632_read_raw_pm; > + > + *val = mlx90632_freqs[cr][0]; > + *val2 = mlx90632_freqs[cr][1]; > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > default: > ret = -EINVAL; > break; > @@ -1012,12 +1042,24 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev, > } > } > > +static IIO_CONST_ATTR(sampling_frequency_available, "0.5 1 2 4 8 16 32 64"); > + > +static struct attribute *mlx90632_attributes[] = { > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, Please use the read_avail() callback and appropriate mask instead of a separate attribute. I'm slowly trying to kill off these, because of the lack of inkernel visibility. Obviously may never be relevant for this driver, but the fewer new instances of attrs being added for things the core handles the better. You can also reused your _freq table above to provide the data in one place rather than current repetition. > + NULL > +}; > + > +static const struct attribute_group mlx90632_attribute_group = { > + .attrs = mlx90632_attributes, > +}; > + > static const struct iio_chan_spec mlx90632_channels[] = { > { > .type = IIO_TEMP, > .modified = 1, > .channel2 = IIO_MOD_TEMP_AMBIENT, > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > }, > { > .type = IIO_TEMP, > @@ -1025,12 +1067,14 @@ static const struct iio_chan_spec mlx90632_channels[] = { > .channel2 = IIO_MOD_TEMP_OBJECT, > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > }, > }; > > static const struct iio_info mlx90632_info = { > .read_raw = mlx90632_read_raw, > .write_raw = mlx90632_write_raw, > + .attrs = &mlx90632_attribute_group, > }; > > static int mlx90632_sleep(struct mlx90632_data *data)
On Sun, 4 Sep 2022 00:24:02 +0200 cmo@melexis.com wrote: > From: Crt Mori <cmo@melexis.com> > > Allow users to read sensor sampling frequency to better plan the > application measurement requests. I didn't read this closely enough. Why is the frequency read only? We'd not normally have an available attribute in that case, because the values aren't available if we have no way to set the value. Jonathan > > Signed-off-by: Crt Mori <cmo@melexis.com> > --- > drivers/iio/temperature/mlx90632.c | 44 ++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c > index 9acd819c76a6..37edd324d6a1 100644 > --- a/drivers/iio/temperature/mlx90632.c > +++ b/drivers/iio/temperature/mlx90632.c > @@ -80,6 +80,9 @@ > #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */ > > #define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits */ > +#define MLX90632_REFRESH_RATE(ee_val) (MLX90632_EE_RR(ee_val) >> 8) > + /* Extract Refresh Rate from ee register */ > +#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8) > > /* Measurement types */ > #define MLX90632_MTYP_MEDICAL 0 > @@ -908,6 +911,24 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) > return ret; > } > > +static int mlx90632_get_refresh_rate(struct mlx90632_data *data, > + int *refresh_rate) > +{ > + unsigned int meas1; > + int ret; > + > + ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1); > + if (ret < 0) > + return ret; > + > + *refresh_rate = MLX90632_REFRESH_RATE(meas1); > + > + return ret; > +} > + > +static const int mlx90632_freqs[][2] = { {0, 500000}, {1, 0}, {2, 0}, {4, 0}, > + {8, 0}, {16, 0}, {32, 0}, {64, 0} }; > + > static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data) > { > unsigned long now; > @@ -978,6 +999,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev, > *val = data->object_ambient_temperature; > ret = IIO_VAL_INT; > break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = mlx90632_get_refresh_rate(data, &cr); > + if (ret < 0) > + goto mlx90632_read_raw_pm; > + > + *val = mlx90632_freqs[cr][0]; > + *val2 = mlx90632_freqs[cr][1]; > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > default: > ret = -EINVAL; > break; > @@ -1012,12 +1042,24 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev, > } > } > > +static IIO_CONST_ATTR(sampling_frequency_available, "0.5 1 2 4 8 16 32 64"); > + > +static struct attribute *mlx90632_attributes[] = { > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group mlx90632_attribute_group = { > + .attrs = mlx90632_attributes, > +}; > + > static const struct iio_chan_spec mlx90632_channels[] = { > { > .type = IIO_TEMP, > .modified = 1, > .channel2 = IIO_MOD_TEMP_AMBIENT, > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > }, > { > .type = IIO_TEMP, > @@ -1025,12 +1067,14 @@ static const struct iio_chan_spec mlx90632_channels[] = { > .channel2 = IIO_MOD_TEMP_OBJECT, > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > }, > }; > > static const struct iio_info mlx90632_info = { > .read_raw = mlx90632_read_raw, > .write_raw = mlx90632_write_raw, > + .attrs = &mlx90632_attribute_group, > }; > > static int mlx90632_sleep(struct mlx90632_data *data)
On Sun, 4 Sept 2022 at 17:19, Jonathan Cameron <jic23@kernel.org> wrote: > > On Sun, 4 Sep 2022 00:24:02 +0200 > cmo@melexis.com wrote: > > > From: Crt Mori <cmo@melexis.com> > > > > Allow users to read sensor sampling frequency to better plan the > > application measurement requests. > > I didn't read this closely enough. Why is the frequency read only? > We'd not normally have an available attribute in that case, because > the values aren't available if we have no way to set the value. > It is writable, but so far I did not want to include the change part, because it is writing to EEPROM and that means sensor slowly gets killed as memory has limited number of write cycles. > Jonathan > > > > > Signed-off-by: Crt Mori <cmo@melexis.com> > > --- > > drivers/iio/temperature/mlx90632.c | 44 ++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c > > index 9acd819c76a6..37edd324d6a1 100644 > > --- a/drivers/iio/temperature/mlx90632.c > > +++ b/drivers/iio/temperature/mlx90632.c > > @@ -80,6 +80,9 @@ > > #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */ > > > > #define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits */ > > +#define MLX90632_REFRESH_RATE(ee_val) (MLX90632_EE_RR(ee_val) >> 8) > > + /* Extract Refresh Rate from ee register */ > > +#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8) > > > > /* Measurement types */ > > #define MLX90632_MTYP_MEDICAL 0 > > @@ -908,6 +911,24 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) > > return ret; > > } > > > > +static int mlx90632_get_refresh_rate(struct mlx90632_data *data, > > + int *refresh_rate) > > +{ > > + unsigned int meas1; > > + int ret; > > + > > + ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1); > > + if (ret < 0) > > + return ret; > > + > > + *refresh_rate = MLX90632_REFRESH_RATE(meas1); > > + > > + return ret; > > +} > > + > > +static const int mlx90632_freqs[][2] = { {0, 500000}, {1, 0}, {2, 0}, {4, 0}, > > + {8, 0}, {16, 0}, {32, 0}, {64, 0} }; > > + > > static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data) > > { > > unsigned long now; > > @@ -978,6 +999,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev, > > *val = data->object_ambient_temperature; > > ret = IIO_VAL_INT; > > break; > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + ret = mlx90632_get_refresh_rate(data, &cr); > > + if (ret < 0) > > + goto mlx90632_read_raw_pm; > > + > > + *val = mlx90632_freqs[cr][0]; > > + *val2 = mlx90632_freqs[cr][1]; > > + ret = IIO_VAL_INT_PLUS_MICRO; > > + break; > > default: > > ret = -EINVAL; > > break; > > @@ -1012,12 +1042,24 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev, > > } > > } > > > > +static IIO_CONST_ATTR(sampling_frequency_available, "0.5 1 2 4 8 16 32 64"); > > + > > +static struct attribute *mlx90632_attributes[] = { > > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group mlx90632_attribute_group = { > > + .attrs = mlx90632_attributes, > > +}; > > + > > static const struct iio_chan_spec mlx90632_channels[] = { > > { > > .type = IIO_TEMP, > > .modified = 1, > > .channel2 = IIO_MOD_TEMP_AMBIENT, > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > > }, > > { > > .type = IIO_TEMP, > > @@ -1025,12 +1067,14 @@ static const struct iio_chan_spec mlx90632_channels[] = { > > .channel2 = IIO_MOD_TEMP_OBJECT, > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > > BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT), > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > > }, > > }; > > > > static const struct iio_info mlx90632_info = { > > .read_raw = mlx90632_read_raw, > > .write_raw = mlx90632_write_raw, > > + .attrs = &mlx90632_attribute_group, > > }; > > > > static int mlx90632_sleep(struct mlx90632_data *data) >
On Sun, 4 Sep 2022 17:23:16 +0200 Crt Mori <cmo@melexis.com> wrote: > On Sun, 4 Sept 2022 at 17:19, Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sun, 4 Sep 2022 00:24:02 +0200 > > cmo@melexis.com wrote: > > > > > From: Crt Mori <cmo@melexis.com> > > > > > > Allow users to read sensor sampling frequency to better plan the > > > application measurement requests. > > > > I didn't read this closely enough. Why is the frequency read only? > > We'd not normally have an available attribute in that case, because > > the values aren't available if we have no way to set the value. > > > > It is writable, but so far I did not want to include the change part, > because it is writing to EEPROM and that means sensor slowly gets > killed as memory has limited number of write cycles. Ah. One of those. Please add some comments to make that clear + I think the available part should wait for any patch that adds support for writing. We may want to gate that write in some fashion - perhaps with some non standard ABI so normal code can't do it. Jonathan > > > Jonathan > > > > > > > > Signed-off-by: Crt Mori <cmo@melexis.com> > > > --- > > > drivers/iio/temperature/mlx90632.c | 44 ++++++++++++++++++++++++++++++ > > > 1 file changed, 44 insertions(+) > > > > > > diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c > > > index 9acd819c76a6..37edd324d6a1 100644 > > > --- a/drivers/iio/temperature/mlx90632.c > > > +++ b/drivers/iio/temperature/mlx90632.c > > > @@ -80,6 +80,9 @@ > > > #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */ > > > > > > #define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits */ > > > +#define MLX90632_REFRESH_RATE(ee_val) (MLX90632_EE_RR(ee_val) >> 8) > > > + /* Extract Refresh Rate from ee register */ > > > +#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8) > > > > > > /* Measurement types */ > > > #define MLX90632_MTYP_MEDICAL 0 > > > @@ -908,6 +911,24 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) > > > return ret; > > > } > > > > > > +static int mlx90632_get_refresh_rate(struct mlx90632_data *data, > > > + int *refresh_rate) > > > +{ > > > + unsigned int meas1; > > > + int ret; > > > + > > > + ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1); > > > + if (ret < 0) > > > + return ret; > > > + > > > + *refresh_rate = MLX90632_REFRESH_RATE(meas1); > > > + > > > + return ret; > > > +} > > > + > > > +static const int mlx90632_freqs[][2] = { {0, 500000}, {1, 0}, {2, 0}, {4, 0}, > > > + {8, 0}, {16, 0}, {32, 0}, {64, 0} }; > > > + > > > static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data) > > > { > > > unsigned long now; > > > @@ -978,6 +999,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev, > > > *val = data->object_ambient_temperature; > > > ret = IIO_VAL_INT; > > > break; > > > + case IIO_CHAN_INFO_SAMP_FREQ: > > > + ret = mlx90632_get_refresh_rate(data, &cr); > > > + if (ret < 0) > > > + goto mlx90632_read_raw_pm; > > > + > > > + *val = mlx90632_freqs[cr][0]; > > > + *val2 = mlx90632_freqs[cr][1]; > > > + ret = IIO_VAL_INT_PLUS_MICRO; > > > + break; > > > default: > > > ret = -EINVAL; > > > break; > > > @@ -1012,12 +1042,24 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev, > > > } > > > } > > > > > > +static IIO_CONST_ATTR(sampling_frequency_available, "0.5 1 2 4 8 16 32 64"); > > > + > > > +static struct attribute *mlx90632_attributes[] = { > > > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > > > + NULL > > > +}; > > > + > > > +static const struct attribute_group mlx90632_attribute_group = { > > > + .attrs = mlx90632_attributes, > > > +}; > > > + > > > static const struct iio_chan_spec mlx90632_channels[] = { > > > { > > > .type = IIO_TEMP, > > > .modified = 1, > > > .channel2 = IIO_MOD_TEMP_AMBIENT, > > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > > > }, > > > { > > > .type = IIO_TEMP, > > > @@ -1025,12 +1067,14 @@ static const struct iio_chan_spec mlx90632_channels[] = { > > > .channel2 = IIO_MOD_TEMP_OBJECT, > > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > > > BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT), > > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), > > > }, > > > }; > > > > > > static const struct iio_info mlx90632_info = { > > > .read_raw = mlx90632_read_raw, > > > .write_raw = mlx90632_write_raw, > > > + .attrs = &mlx90632_attribute_group, > > > }; > > > > > > static int mlx90632_sleep(struct mlx90632_data *data) > >
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c index 9acd819c76a6..37edd324d6a1 100644 --- a/drivers/iio/temperature/mlx90632.c +++ b/drivers/iio/temperature/mlx90632.c @@ -80,6 +80,9 @@ #define MLX90632_PWR_STATUS_CONTINUOUS MLX90632_PWR_STATUS(3) /* continuous */ #define MLX90632_EE_RR(ee_val) (ee_val & GENMASK(10, 8)) /* Only Refresh Rate bits */ +#define MLX90632_REFRESH_RATE(ee_val) (MLX90632_EE_RR(ee_val) >> 8) + /* Extract Refresh Rate from ee register */ +#define MLX90632_REFRESH_RATE_STATUS(refresh_rate) (refresh_rate << 8) /* Measurement types */ #define MLX90632_MTYP_MEDICAL 0 @@ -908,6 +911,24 @@ static int mlx90632_calc_ambient_dsp105(struct mlx90632_data *data, int *val) return ret; } +static int mlx90632_get_refresh_rate(struct mlx90632_data *data, + int *refresh_rate) +{ + unsigned int meas1; + int ret; + + ret = regmap_read(data->regmap, MLX90632_EE_MEDICAL_MEAS1, &meas1); + if (ret < 0) + return ret; + + *refresh_rate = MLX90632_REFRESH_RATE(meas1); + + return ret; +} + +static const int mlx90632_freqs[][2] = { {0, 500000}, {1, 0}, {2, 0}, {4, 0}, + {8, 0}, {16, 0}, {32, 0}, {64, 0} }; + static int mlx90632_pm_interraction_wakeup(struct mlx90632_data *data) { unsigned long now; @@ -978,6 +999,15 @@ static int mlx90632_read_raw(struct iio_dev *indio_dev, *val = data->object_ambient_temperature; ret = IIO_VAL_INT; break; + case IIO_CHAN_INFO_SAMP_FREQ: + ret = mlx90632_get_refresh_rate(data, &cr); + if (ret < 0) + goto mlx90632_read_raw_pm; + + *val = mlx90632_freqs[cr][0]; + *val2 = mlx90632_freqs[cr][1]; + ret = IIO_VAL_INT_PLUS_MICRO; + break; default: ret = -EINVAL; break; @@ -1012,12 +1042,24 @@ static int mlx90632_write_raw(struct iio_dev *indio_dev, } } +static IIO_CONST_ATTR(sampling_frequency_available, "0.5 1 2 4 8 16 32 64"); + +static struct attribute *mlx90632_attributes[] = { + &iio_const_attr_sampling_frequency_available.dev_attr.attr, + NULL +}; + +static const struct attribute_group mlx90632_attribute_group = { + .attrs = mlx90632_attributes, +}; + static const struct iio_chan_spec mlx90632_channels[] = { { .type = IIO_TEMP, .modified = 1, .channel2 = IIO_MOD_TEMP_AMBIENT, .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), }, { .type = IIO_TEMP, @@ -1025,12 +1067,14 @@ static const struct iio_chan_spec mlx90632_channels[] = { .channel2 = IIO_MOD_TEMP_OBJECT, .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | BIT(IIO_CHAN_INFO_CALIBEMISSIVITY) | BIT(IIO_CHAN_INFO_CALIBAMBIENT), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), }, }; static const struct iio_info mlx90632_info = { .read_raw = mlx90632_read_raw, .write_raw = mlx90632_write_raw, + .attrs = &mlx90632_attribute_group, }; static int mlx90632_sleep(struct mlx90632_data *data)