Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
able to calculate the processed value with standard userspace IIO
tools. Can be used for triggered buffers as well.
Signed-off-by: Vasileios Amoiridis <[email protected]>
---
drivers/iio/pressure/bmp280-core.c | 58 ++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index dfd845acfa22..6d7734f867bc 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -138,16 +138,22 @@ static const struct iio_chan_spec bmp280_channels[] = {
{
.type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
{
.type = IIO_HUMIDITYRELATIVE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
},
};
@@ -156,6 +162,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_PRESSURE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
@@ -163,6 +171,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
@@ -170,6 +180,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
{
.type = IIO_HUMIDITYRELATIVE,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
@@ -485,6 +497,52 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
break;
}
break;
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_HUMIDITYRELATIVE:
+ *val = data->chip_info->read_humid(data);
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_PRESSURE:
+ *val = data->chip_info->read_press(data);
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_TEMP:
+ *val = data->chip_info->read_temp(data);
+ ret = IIO_VAL_INT;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_HUMIDITYRELATIVE:
+ *val = data->chip_info->humid_coeffs[0];
+ *val2 = data->chip_info->humid_coeffs[1];
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+ case IIO_PRESSURE:
+ *val = data->chip_info->press_coeffs[0];
+ *val2 = data->chip_info->press_coeffs[1];
+ ret = IIO_VAL_FRACTIONAL;
+ break;
+ case IIO_TEMP:
+ *val = data->chip_info->temp_coeffs[0];
+ *val2 = data->chip_info->temp_coeffs[1];
+
+ if (!strcmp(indio_dev->name, "bmp580"))
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ else
+ ret = IIO_VAL_FRACTIONAL;
+
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
switch (chan->type) {
case IIO_HUMIDITYRELATIVE:
--
2.25.1
On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> able to calculate the processed value with standard userspace IIO
> tools. Can be used for triggered buffers as well.
..
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_HUMIDITYRELATIVE:
> + *val = data->chip_info->read_humid(data);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_PRESSURE:
> + *val = data->chip_info->read_press(data);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_TEMP:
> + *val = data->chip_info->read_temp(data);
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
Is it mutex that prevents us from returning here?
If so, perhaps switching to use cleanup.h first?
> + }
> + break;
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > able to calculate the processed value with standard userspace IIO
> > tools. Can be used for triggered buffers as well.
>
> ...
>
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_HUMIDITYRELATIVE:
> > + *val = data->chip_info->read_humid(data);
> > + ret = IIO_VAL_INT;
> > + break;
> > + case IIO_PRESSURE:
> > + *val = data->chip_info->read_press(data);
> > + ret = IIO_VAL_INT;
> > + break;
> > + case IIO_TEMP:
> > + *val = data->chip_info->read_temp(data);
> > + ret = IIO_VAL_INT;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
>
> Is it mutex that prevents us from returning here?
> If so, perhaps switching to use cleanup.h first?
>
I haven't seen cleanup.h used in any file and now that I searched,
only 5-6 are including it. I am currently thinking if the mutex
that already exists is really needed since most of the drivers
don't have it + I feel like this is something that should be done
by IIO, thus maybe it's not even needed here.
> > + }
> > + break;
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Best regards,
Vasileios Amoiridis
On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > able to calculate the processed value with standard userspace IIO
> > > tools. Can be used for triggered buffers as well.
..
> > > + case IIO_CHAN_INFO_RAW:
> > > + switch (chan->type) {
> > > + case IIO_HUMIDITYRELATIVE:
> > > + *val = data->chip_info->read_humid(data);
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > + case IIO_PRESSURE:
> > > + *val = data->chip_info->read_press(data);
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > + case IIO_TEMP:
> > > + *val = data->chip_info->read_temp(data);
> > > + ret = IIO_VAL_INT;
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + break;
> >
> > Is it mutex that prevents us from returning here?
> > If so, perhaps switching to use cleanup.h first?
>
> I haven't seen cleanup.h used in any file and now that I searched,
> only 5-6 are including it.
Hmm... Which repository you are checking with?
$ git grep -lw cleanup.h -- drivers/ | wc -l
47
(Today's Linux Next)
> I am currently thinking if the mutex
> that already exists is really needed since most of the drivers
> don't have it + I feel like this is something that should be done
> by IIO, thus maybe it's not even needed here.
> > > + }
> > > + break;
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > able to calculate the processed value with standard userspace IIO
> > > > tools. Can be used for triggered buffers as well.
>
> ...
>
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + switch (chan->type) {
> > > > + case IIO_HUMIDITYRELATIVE:
> > > > + *val = data->chip_info->read_humid(data);
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > + case IIO_PRESSURE:
> > > > + *val = data->chip_info->read_press(data);
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > + case IIO_TEMP:
> > > > + *val = data->chip_info->read_temp(data);
> > > > + ret = IIO_VAL_INT;
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + break;
> > >
> > > Is it mutex that prevents us from returning here?
> > > If so, perhaps switching to use cleanup.h first?
> >
> > I haven't seen cleanup.h used in any file and now that I searched,
> > only 5-6 are including it.
>
> Hmm... Which repository you are checking with?
>
> $ git grep -lw cleanup.h -- drivers/ | wc -l
> 47
>
> (Today's Linux Next)
>
I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
drivers that use it.
> > I am currently thinking if the mutex
> > that already exists is really needed since most of the drivers
> > don't have it + I feel like this is something that should be done
> > by IIO, thus maybe it's not even needed here.
>
> > > > + }
> > > > + break;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > able to calculate the processed value with standard userspace IIO
> > > > > tools. Can be used for triggered buffers as well.
> >
> > ...
> >
> > > > > + case IIO_CHAN_INFO_RAW:
> > > > > + switch (chan->type) {
> > > > > + case IIO_HUMIDITYRELATIVE:
> > > > > + *val = data->chip_info->read_humid(data);
> > > > > + ret = IIO_VAL_INT;
> > > > > + break;
> > > > > + case IIO_PRESSURE:
> > > > > + *val = data->chip_info->read_press(data);
> > > > > + ret = IIO_VAL_INT;
> > > > > + break;
> > > > > + case IIO_TEMP:
> > > > > + *val = data->chip_info->read_temp(data);
> > > > > + ret = IIO_VAL_INT;
> > > > > + break;
> > > > > + default:
> > > > > + ret = -EINVAL;
> > > > > + break;
> > > >
> > > > Is it mutex that prevents us from returning here?
> > > > If so, perhaps switching to use cleanup.h first?
> > >
> > > I haven't seen cleanup.h used in any file and now that I searched,
> > > only 5-6 are including it.
> >
> > Hmm... Which repository you are checking with?
> >
> > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > 47
> >
> > (Today's Linux Next)
> >
>
> I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> drivers that use it.
>
> > > I am currently thinking if the mutex
> > > that already exists is really needed since most of the drivers
> > > don't have it + I feel like this is something that should be done
> > > by IIO, thus maybe it's not even needed here.
> >
After some researching today, I realized that all the
{read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
for channel mapping in the kernel and it looks like they are guarded by
the mutex_{un}lock(&iio_dev_opaque->info_exist_lock). So I feel that the
mutexes in the aforementioned functions can be dropped. When you have the
time please have a look, maybe the could be dropped.
In general, there is quite some cleaning that can be done in this driver
but is it wise to include it in the triggered buffer support series??? I
have noticed quite some things that could be improved but I am hesitating
to do it now in order to not "pollute" this series with many cleanups and
leave it for another cleanup series for example.
Best regards,
Vasilis Amoiridis
> > > > > + }
> > > > > + break;
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
On Thu, 14 Mar 2024 11:57:28 +0100
vamoirid <[email protected]> wrote:
> On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > > able to calculate the processed value with standard userspace IIO
> > > > > > tools. Can be used for triggered buffers as well.
> > >
> > > ...
> > >
> > > > > > + case IIO_CHAN_INFO_RAW:
> > > > > > + switch (chan->type) {
> > > > > > + case IIO_HUMIDITYRELATIVE:
> > > > > > + *val = data->chip_info->read_humid(data);
> > > > > > + ret = IIO_VAL_INT;
> > > > > > + break;
> > > > > > + case IIO_PRESSURE:
> > > > > > + *val = data->chip_info->read_press(data);
> > > > > > + ret = IIO_VAL_INT;
> > > > > > + break;
> > > > > > + case IIO_TEMP:
> > > > > > + *val = data->chip_info->read_temp(data);
> > > > > > + ret = IIO_VAL_INT;
> > > > > > + break;
> > > > > > + default:
> > > > > > + ret = -EINVAL;
> > > > > > + break;
> > > > >
> > > > > Is it mutex that prevents us from returning here?
> > > > > If so, perhaps switching to use cleanup.h first?
> > > >
> > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > only 5-6 are including it.
> > >
> > > Hmm... Which repository you are checking with?
> > >
> > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > 47
> > >
> > > (Today's Linux Next)
> > >
> >
> > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > drivers that use it.
Yes - but that's because it's new - most of the stuff in 6.8 was the proof
points for the patches originally introducing support for autocleanup (so typically
one or two cases for each type of handling) That doesn't mean we don't want it
in drivers that are being worked upon if it gives a significant advantage.
Some features we need will merge shortly, and a great deal more usage
of this autocleanup will occur.
> >
> > > > I am currently thinking if the mutex
> > > > that already exists is really needed since most of the drivers
> > > > don't have it + I feel like this is something that should be done
> > > > by IIO, thus maybe it's not even needed here.
> > >
>
> After some researching today, I realized that all the
> {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> for channel mapping in the kernel and it looks like they are guarded by
> the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
Why is that relevant to this patch which isn't using that interface at all?
Those protections are to ensure that a consumer driver doesn't access a removed
IIO device, not accesses directly from userspace.
>so I feel that the
> mutexes in the aforementioned functions can be dropped. When you have the
> time please have a look, maybe the could be dropped.
Identify what your locks are protecting. Those existence locks have
very specific purpose and should not be relied on for anything else.
If this driver is protecting state known only to itself, then it must
be responsible for appropriate locking.
>
> In general, there is quite some cleaning that can be done in this driver
> but is it wise to include it in the triggered buffer support series???
Generally if working on a driver and you see cleanup that you think should
be done, it belongs before any series adding new features, precisely because
that code can typically end up simpler as a result. This sounds like one
of those cases. Normally that only includes things that are directly related
to resulting code for new features (or applying the same cleanup across a driver)
as we don't want to make people do a full scrub of a driver before adding
anything as it will just create too much noise.
So for this case, it does look like a quick use of guard(mutex) in
a precursor patch will simplify what you add here - hence that's a reasonable
request for Andy to make.
Jonathan
> I
> have noticed quite some things that could be improved but I am hesitating
> to do it now in order to not "pollute" this series with many cleanups and
> leave it for another cleanup series for example.
>
> Best regards,
> Vasilis Amoiridis
>
> > > > > > + }
> > > > > > + break;
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
On Wed, 13 Mar 2024 18:40:04 +0100
Vasileios Amoiridis <[email protected]> wrote:
> Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> able to calculate the processed value with standard userspace IIO
> tools. Can be used for triggered buffers as well.
>
> Signed-off-by: Vasileios Amoiridis <[email protected]>
> ---
> drivers/iio/pressure/bmp280-core.c | 58 ++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index dfd845acfa22..6d7734f867bc 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -138,16 +138,22 @@ static const struct iio_chan_spec bmp280_channels[] = {
> {
> .type = IIO_PRESSURE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
Next to each (or at least the first) existing entry for PROCESSED add a
comment that says it is maintained for ABI backwards compatibility reasons.
I really don't want people copying the result of this patch into new drivers
- we've ended up here because of a less than ideal decision in the past, that
history doesn't apply to other drivers.
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> {
> .type = IIO_HUMIDITYRELATIVE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> },
> };
> @@ -156,6 +162,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_PRESSURE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> @@ -163,6 +171,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> @@ -170,6 +180,8 @@ static const struct iio_chan_spec bmp380_channels[] = {
> {
> .type = IIO_HUMIDITYRELATIVE,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> @@ -485,6 +497,52 @@ static int bmp280_read_raw(struct iio_dev *indio_dev,
> break;
> }
> break;
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_HUMIDITYRELATIVE:
> + *val = data->chip_info->read_humid(data);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_PRESSURE:
> + *val = data->chip_info->read_press(data);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_TEMP:
> + *val = data->chip_info->read_temp(data);
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_HUMIDITYRELATIVE:
> + *val = data->chip_info->humid_coeffs[0];
> + *val2 = data->chip_info->humid_coeffs[1];
> + ret = IIO_VAL_FRACTIONAL;
> + break;
> + case IIO_PRESSURE:
> + *val = data->chip_info->press_coeffs[0];
> + *val2 = data->chip_info->press_coeffs[1];
> + ret = IIO_VAL_FRACTIONAL;
> + break;
> + case IIO_TEMP:
> + *val = data->chip_info->temp_coeffs[0];
> + *val2 = data->chip_info->temp_coeffs[1];
> +
> + if (!strcmp(indio_dev->name, "bmp580"))
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + else
> + ret = IIO_VAL_FRACTIONAL;
> +
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> switch (chan->type) {
> case IIO_HUMIDITYRELATIVE:
On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 11:57:28 +0100
> vamoirid <[email protected]> wrote:
>
> > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > > > able to calculate the processed value with standard userspace IIO
> > > > > > > tools. Can be used for triggered buffers as well.
> > > >
> > > > ...
> > > >
> > > > > > > + case IIO_CHAN_INFO_RAW:
> > > > > > > + switch (chan->type) {
> > > > > > > + case IIO_HUMIDITYRELATIVE:
> > > > > > > + *val = data->chip_info->read_humid(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + case IIO_PRESSURE:
> > > > > > > + *val = data->chip_info->read_press(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + case IIO_TEMP:
> > > > > > > + *val = data->chip_info->read_temp(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + default:
> > > > > > > + ret = -EINVAL;
> > > > > > > + break;
> > > > > >
> > > > > > Is it mutex that prevents us from returning here?
> > > > > > If so, perhaps switching to use cleanup.h first?
> > > > >
> > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > only 5-6 are including it.
> > > >
> > > > Hmm... Which repository you are checking with?
> > > >
> > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > 47
> > > >
> > > > (Today's Linux Next)
> > > >
> > >
> > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > drivers that use it.
>
> Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> points for the patches originally introducing support for autocleanup (so typically
> one or two cases for each type of handling) That doesn't mean we don't want it
> in drivers that are being worked upon if it gives a significant advantage.
> Some features we need will merge shortly, and a great deal more usage
> of this autocleanup will occur.
>
> > >
> > > > > I am currently thinking if the mutex
> > > > > that already exists is really needed since most of the drivers
> > > > > don't have it + I feel like this is something that should be done
> > > > > by IIO, thus maybe it's not even needed here.
> > > >
> >
> > After some researching today, I realized that all the
> > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > for channel mapping in the kernel and it looks like they are guarded by
> > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
>
> Why is that relevant to this patch which isn't using that interface at all?
> Those protections are to ensure that a consumer driver doesn't access a removed
> IIO device, not accesses directly from userspace.
>
> >so I feel that the
> > mutexes in the aforementioned functions can be dropped. When you have the
> > time please have a look, maybe the could be dropped.
>
> Identify what your locks are protecting. Those existence locks have
> very specific purpose and should not be relied on for anything else.
>
> If this driver is protecting state known only to itself, then it must
> be responsible for appropriate locking.
>
> >
> > In general, there is quite some cleaning that can be done in this driver
> > but is it wise to include it in the triggered buffer support series???
>
> Generally if working on a driver and you see cleanup that you think should
> be done, it belongs before any series adding new features, precisely because
> that code can typically end up simpler as a result. This sounds like one
> of those cases. Normally that only includes things that are directly related
> to resulting code for new features (or applying the same cleanup across a driver)
> as we don't want to make people do a full scrub of a driver before adding
> anything as it will just create too much noise.
>
> So for this case, it does look like a quick use of guard(mutex) in
> a precursor patch will simplify what you add here - hence that's a reasonable
> request for Andy to make.
>
> Jonathan
>
Hi Jonathan.
Thank you very much for the feedback once again. I didn't know that cleanup.h
was a new thing. I also didn't understand it when Andy mentioned it. Now that
I saw it better and I read about it, it certainly looks like a very good thing
to add.
I don't know if I sounded like I didn't like that request, but just to clarify,
I see it as a very good thing all the proposals that you do because first I
get to learn and understand how to write better code and second the users will
use a better driver! So please, the more requests, the better.
So a precursor patch adding the new functionality of the guard(mutex) in this
and possibly other places in the driver will be good indeed, thank you!
Best regards,
Vasilis
>
> > I
> > have noticed quite some things that could be improved but I am hesitating
> > to do it now in order to not "pollute" this series with many cleanups and
> > leave it for another cleanup series for example.
> >
> > Best regards,
> > Vasilis Amoiridis
> >
> > > > > > > + }
> > > > > > > + break;
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
>
On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 11:57:28 +0100
> vamoirid <[email protected]> wrote:
>
> > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > > > able to calculate the processed value with standard userspace IIO
> > > > > > > tools. Can be used for triggered buffers as well.
> > > >
> > > > ...
> > > >
> > > > > > > + case IIO_CHAN_INFO_RAW:
> > > > > > > + switch (chan->type) {
> > > > > > > + case IIO_HUMIDITYRELATIVE:
> > > > > > > + *val = data->chip_info->read_humid(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + case IIO_PRESSURE:
> > > > > > > + *val = data->chip_info->read_press(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + case IIO_TEMP:
> > > > > > > + *val = data->chip_info->read_temp(data);
> > > > > > > + ret = IIO_VAL_INT;
> > > > > > > + break;
> > > > > > > + default:
> > > > > > > + ret = -EINVAL;
> > > > > > > + break;
> > > > > >
> > > > > > Is it mutex that prevents us from returning here?
> > > > > > If so, perhaps switching to use cleanup.h first?
> > > > >
> > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > only 5-6 are including it.
> > > >
> > > > Hmm... Which repository you are checking with?
> > > >
> > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > 47
> > > >
> > > > (Today's Linux Next)
> > > >
> > >
> > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > drivers that use it.
>
> Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> points for the patches originally introducing support for autocleanup (so typically
> one or two cases for each type of handling) That doesn't mean we don't want it
> in drivers that are being worked upon if it gives a significant advantage.
> Some features we need will merge shortly, and a great deal more usage
> of this autocleanup will occur.
>
> > >
> > > > > I am currently thinking if the mutex
> > > > > that already exists is really needed since most of the drivers
> > > > > don't have it + I feel like this is something that should be done
> > > > > by IIO, thus maybe it's not even needed here.
> > > >
> >
> > After some researching today, I realized that all the
> > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > for channel mapping in the kernel and it looks like they are guarded by
> > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
>
> Why is that relevant to this patch which isn't using that interface at all?
> Those protections are to ensure that a consumer driver doesn't access a removed
> IIO device, not accesses directly from userspace.
>
> >so I feel that the
> > mutexes in the aforementioned functions can be dropped. When you have the
> > time please have a look, maybe the could be dropped.
>
> Identify what your locks are protecting. Those existence locks have
> very specific purpose and should not be relied on for anything else.
>
> If this driver is protecting state known only to itself, then it must
> be responsible for appropriate locking.
>
> >
> > In general, there is quite some cleaning that can be done in this driver
> > but is it wise to include it in the triggered buffer support series???
>
> Generally if working on a driver and you see cleanup that you think should
> be done, it belongs before any series adding new features, precisely because
> that code can typically end up simpler as a result. This sounds like one
> of those cases. Normally that only includes things that are directly related
> to resulting code for new features (or applying the same cleanup across a driver)
> as we don't want to make people do a full scrub of a driver before adding
> anything as it will just create too much noise.
>
> So for this case, it does look like a quick use of guard(mutex) in
> a precursor patch will simplify what you add here - hence that's a reasonable
> request for Andy to make.
>
> Jonathan
>
After looking into how to change the code to introduce the new guard(mutex)
I encountered the following situation.
In general, with the new guard(mutex) functinality you can remove most of the
goto statements and return immediately without doing the cleanup yourself.
In the case of this driver, in the read_raw() call, apart from the mutex,
the power management functions are also used. This means that in each case,
before returning, the pm functions will need to be called, which I don't
know if it will actually make the code cleaner. Have a look below with
an example.
----- Current Implementation -----
static int bmp280_read_raw( ... )
{
...
pm_runtime_get_sync_data(data->dev);
mutex_lock(&data->lock);
switch (mask) {
case 1:
switch (channel) {
case TEMP:
ret = read_temp();
break;
case PRESS:
ret = read_press();
break;
...
case 2:
switch (channel) {
...
case 3:
...
default:
ret = -EINVAL;
break;
}
mutex_unlock(&data->lock);
pm_runtime_mark_last_busy(data->dev);
pm_runtime_put_autosuspend(data->dev);
return ret;
}
----- End of Current Implementation -----
With the use of the guard(mutex)(&data->lock) you could immediately
return without all the break statements. But we still need to call
the pm functions. So the code, as far as I can understand will look
like this:
----- Guard Mutex Implementation -----
static int bmp280_read_raw( ... )
{
...
pm_runtime_get_sync_data(data->dev);
guard(mutex)(&data->lock);
switch (mask) {
case 1:
switch (channel {
case TEMP:
ret = read_temp();
pm_runtime_mark_last_busy(data->dev);
pm_runtime_put_autosuspend(data->dev);
return ret;
case PRESS:
ret = read_press();
pm_runtime_mark_last_busy(data->dev);
pm_runtime_put_autosuspend(data->dev);
return ret;
...
case 2:
switch (channel) {
...
case 3:
...
default:
return -EINVAL;
}
return 0;
}
----- End of Guard Mutex Implementation -----
Have I completely misunderstood something? If what I explain
above is correct, you think that this is a better implementation
and I should move forward becasue we want to use the guard(mutex)
functionality?
Maybe it is necessary to create some new type of guard to call
also the pm functions before exiting?
Cheers,
Vasilis
>
> > I
> > have noticed quite some things that could be improved but I am hesitating
> > to do it now in order to not "pollute" this series with many cleanups and
> > leave it for another cleanup series for example.
> >
> > Best regards,
> > Vasilis Amoiridis
> >
> > > > > > > + }
> > > > > > > + break;
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
>
On Fri, 15 Mar 2024 14:11:06 +0100
Vasileios Amoiridis <[email protected]> wrote:
> On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> > On Thu, 14 Mar 2024 11:57:28 +0100
> > vamoirid <[email protected]> wrote:
> >
> > > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:
> > > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:
> > > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:
> > > > > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > > > > able to calculate the processed value with standard userspace IIO
> > > > > > > > tools. Can be used for triggered buffers as well.
> > > > >
> > > > > ...
> > > > >
> > > > > > > > + case IIO_CHAN_INFO_RAW:
> > > > > > > > + switch (chan->type) {
> > > > > > > > + case IIO_HUMIDITYRELATIVE:
> > > > > > > > + *val = data->chip_info->read_humid(data);
> > > > > > > > + ret = IIO_VAL_INT;
> > > > > > > > + break;
> > > > > > > > + case IIO_PRESSURE:
> > > > > > > > + *val = data->chip_info->read_press(data);
> > > > > > > > + ret = IIO_VAL_INT;
> > > > > > > > + break;
> > > > > > > > + case IIO_TEMP:
> > > > > > > > + *val = data->chip_info->read_temp(data);
> > > > > > > > + ret = IIO_VAL_INT;
> > > > > > > > + break;
> > > > > > > > + default:
> > > > > > > > + ret = -EINVAL;
> > > > > > > > + break;
> > > > > > >
> > > > > > > Is it mutex that prevents us from returning here?
> > > > > > > If so, perhaps switching to use cleanup.h first?
> > > > > >
> > > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > > only 5-6 are including it.
> > > > >
> > > > > Hmm... Which repository you are checking with?
> > > > >
> > > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > > 47
> > > > >
> > > > > (Today's Linux Next)
> > > > >
> > > >
> > > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > > drivers that use it.
> >
> > Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> > points for the patches originally introducing support for autocleanup (so typically
> > one or two cases for each type of handling) That doesn't mean we don't want it
> > in drivers that are being worked upon if it gives a significant advantage.
> > Some features we need will merge shortly, and a great deal more usage
> > of this autocleanup will occur.
> >
> > > >
> > > > > > I am currently thinking if the mutex
> > > > > > that already exists is really needed since most of the drivers
> > > > > > don't have it + I feel like this is something that should be done
> > > > > > by IIO, thus maybe it's not even needed here.
> > > > >
> > >
> > > After some researching today, I realized that all the
> > > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > > for channel mapping in the kernel and it looks like they are guarded by
> > > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
> >
> > Why is that relevant to this patch which isn't using that interface at all?
> > Those protections are to ensure that a consumer driver doesn't access a removed
> > IIO device, not accesses directly from userspace.
> >
> > >so I feel that the
> > > mutexes in the aforementioned functions can be dropped. When you have the
> > > time please have a look, maybe the could be dropped.
> >
> > Identify what your locks are protecting. Those existence locks have
> > very specific purpose and should not be relied on for anything else.
> >
> > If this driver is protecting state known only to itself, then it must
> > be responsible for appropriate locking.
> >
> > >
> > > In general, there is quite some cleaning that can be done in this driver
> > > but is it wise to include it in the triggered buffer support series???
> >
> > Generally if working on a driver and you see cleanup that you think should
> > be done, it belongs before any series adding new features, precisely because
> > that code can typically end up simpler as a result. This sounds like one
> > of those cases. Normally that only includes things that are directly related
> > to resulting code for new features (or applying the same cleanup across a driver)
> > as we don't want to make people do a full scrub of a driver before adding
> > anything as it will just create too much noise.
> >
> > So for this case, it does look like a quick use of guard(mutex) in
> > a precursor patch will simplify what you add here - hence that's a reasonable
> > request for Andy to make.
> >
> > Jonathan
> >
>
> After looking into how to change the code to introduce the new guard(mutex)
> I encountered the following situation
No problem. We are all getting used to how to use this stuff. There
have been a few 'comments' from Linus Torvalds on people doing it wrong.
One of those (it was about cond_guard() proposal if you want to find it)
is applicable here (note that Linus was rather abrupt in that thread and
got called out for it, not in my view a good example of kernel process).
Make more use of helper functions to avoid gotos.
>
> In general, with the new guard(mutex) functinality you can remove most of the
> goto statements and return immediately without doing the cleanup yourself.
> In the case of this driver, in the read_raw() call, apart from the mutex,
> the power management functions are also used. This means that in each case,
> before returning, the pm functions will need to be called, which I don't
> know if it will actually make the code cleaner. Have a look below with
> an example.
>
> ----- Current Implementation -----
>
> static int bmp280_read_raw( ... )
> {
> ...
>
> pm_runtime_get_sync_data(data->dev);
Pull from here...
> mutex_lock(&data->lock);
>
> switch (mask) {
> case 1:
> switch (channel) {
> case TEMP:
> ret = read_temp();
> break;
> case PRESS:
> ret = read_press();
> break;
> ...
> case 2:
> switch (channel) {
> ...
>
> case 3:
> ...
> default:
> ret = -EINVAL;
> break;
> }
>
> mutex_unlock(&data->lock);
.. to here out as a separate little function - somethimg like
bmp280_read_raw_impl() which can use guard() and direct returns
internally.
Then this block will be
pm_runtime_get_sync_data(data->dev);
ret = bmp280_read_raw_impl(...);
pm_runtime_mark_last_busy(data->dev);
pm_runtime_put_autosuspend(data->dev);
return ret;
> pm_runtime_mark_last_busy(data->dev);
> pm_runtime_put_autosuspend(data->dev);
>
> return ret;
> }
>
> ----- End of Current Implementation -----
>
> With the use of the guard(mutex)(&data->lock) you could immediately
> return without all the break statements. But we still need to call
> the pm functions. So the code, as far as I can understand will look
> like this:
>
> ----- Guard Mutex Implementation -----
>
> static int bmp280_read_raw( ... )
> {
> ...
>
> pm_runtime_get_sync_data(data->dev);
> guard(mutex)(&data->lock);
>
> switch (mask) {
> case 1:
> switch (channel {
> case TEMP:
> ret = read_temp();
This inverts ordering of pm and the guard mutex, so not a good idea.
> pm_runtime_mark_last_busy(data->dev);
> pm_runtime_put_autosuspend(data->dev);
> return ret;
> case PRESS:
> ret = read_press();
> pm_runtime_mark_last_busy(data->dev);
> pm_runtime_put_autosuspend(data->dev);
> return ret;
> ...
> case 2:
> switch (channel) {
> ...
> case 3:
> ...
> default:
> return -EINVAL;
> }
>
> return 0;
> }
>
> ----- End of Guard Mutex Implementation -----
>
> Have I completely misunderstood something? If what I explain
> above is correct, you think that this is a better implementation
> and I should move forward becasue we want to use the guard(mutex)
> functionality?
>
> Maybe it is necessary to create some new type of guard to call
> also the pm functions before exiting?
Don't try that approach - it's complexity that will get a response
you don't want from Linus. Helper functions solve this one
for us nicely.
At somepoint maybe generic infrastructure for runtime pm handling
will be added, but that stuff is complex enough already so I suspect
not or not until people are in general much more confident with the
cleanup.h infrastructure and where it is appropriate.
What you are doing here should all be standard usage at the simpler
end of the scale, so not a risky as a few things I'm trying to get in :)
Jonathan
>
> Cheers,
> Vasilis
>
> >
> > > I
> > > have noticed quite some things that could be improved but I am hesitating
> > > to do it now in order to not "pollute" this series with many cleanups and
> > > leave it for another cleanup series for example.
> > >
> > > Best regards,
> > > Vasilis Amoiridis
> > >
> > > > > > > > + }
> > > > > > > > + break;
> > > > >
> > > > > --
> > > > > With Best Regards,
> > > > > Andy Shevchenko
> > > > >
> > > > >
> >
>