| Message ID | 20220626122938.582107-1-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Jun 26, 2022 at 2:20 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Changes since v2: Thanks to Andy Shevchenko for review > - Fix the horrible mess I made of a rebase that meant v2 didn't build > mid patch set. > - Fix various build issues I'd missed due to wrong make file (fails to > update variables after moving them, missing unaligned include, > failure to cleanup staging driver Kconfig / Makefile) > - Change new ABI to _zeropoint and add more the description to hopefully > add clarify to what this is. I don't think it's going to be a particularly > common bit of ABI, but still good to get it right. For non-commented patches (except 17th): Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> For commented, in case you address the comments I suggested, you may also add a tag. > Note that the --no-renames was used to preserve full code listing > for the actual move out of staging to aid review. The ad7746 file is > moved with no changes. > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/iio/cdc/ad7746.c > similarity index 100% > rename from drivers/staging/iio/cdc/ad7746.c > rename to drivers/iio/cdc/ad7746.c > > This started out as an experiment with Vincent Whitchurch's roadtest > testing framework [1] and that worked well so I carried on cleaning up the > driver. Note that if you are using roadtest rebased to current > kernel, add CONFIG_UML_RANDOM as suggested by Vincent in reply > to v2 of this series. I still saw one freeze but with that config > element added roadtest runs the tests successfully the vast majority > of the time. > > Mostly this is standard tidy up, move to new interfaces, then move the driver > out of staging, but there are a few other things in here. > > 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. > The ad7746 is a 24 bit sensor and this highlighted that 9 decimal > places wasn't enough to keep the error introduced by > _raw * _scale from growing too large over the whole range (I couldn't > write a sensible test for it). So I've proposed increasing the precision > of the calculation used to provide the sysfs attributes with this value > type and printing a few more decimal places (12), but only if overflow > will not occur due to val[0] > ULLONG_MAX / PICO > 2) _zeropoint ABI addition. This driver had an odd use of _offset for > the case of differential channels that applied the offset to both > of the differential pair (hence userspace shouldn't not apply it when > converting to the base units. That isn't inline with the existing > documentation for _offset and it wasn't clear to me that it made sense > at all. To avoid confusion I've added this new ABI (_zeropoint) for this. > 3) roadtest file - note this is not a complete test set for the driver and > mainly focused on the main channel reads and places I thought I might > have broken things whilst working on the driver. Given roadtest isn't > upstream yet this test will have to wait. That doesn't block merging > the rest of the series. > > My conclusion on roadtest - Very useful indeed. I'd encourage others to > consider developing some basic sanity tests for drivers they are working on. > Hopefully my python code isn't too hideous to understand at least! > Vincent, it might be worth thinking about some generic code to handle the > 'variants' on correct ABI like I introduce here because I switched from > a shared by type scale to an individual one per channel for the voltages. > Both were ABI compliant so that sort of change is fine most of the time > though we have to be careful with it. > > All comments welcome. Note there may be changes that make more sense > to do after moving this out of staging as long as there are no ABI changes involved > etc. Feel free to highlight those sorts of changes as well as anything more > significant. > > [1] https://lore.kernel.org/all/20220311162445.346685-9-vincent.whitchurch@axis.com/ > > Jonathan Cameron (17): > iio: core: Increase precision of IIO_VAL_FRACTIONAL_LOG2 when possible > iio: ABI: Fix wrong format of differential capacitance channel ABI. > staging: iio: cdc: ad7746: Use explicit be24 handling. > staging: iio: cdc: ad7746: Push handling of supply voltage scale to > userspace. > staging: iio: cdc: ad7746: Use local buffer for multi byte reads. > staging: iio: cdc: ad7746: Factor out ad7746_read_channel() > staging: iio: cdc: ad7764: Push locking down into case statements in > read/write_raw > staging: iio: cdc: ad7746: Break up use of chan->address and use > FIELD_PREP etc > staging: iio: cdc: ad7746: Drop unused i2c_set_clientdata() > staging: iio: cdc: ad7746: Use _raw and _scale for temperature > channels. > iio: core: Introduce _zeropoint for differential channels > staging: iio: cdc: ad7746: Switch from _offset to _zeropoint for > differential channels. > staging: iio: cdc: ad7746: Use read_avail() rather than opencoding. > staging: iio: ad7746: White space cleanup > iio: cdc: ad7746: Add device specific ABI documentation. > iio: cdc: ad7746: Move driver out of staging. > RFC: iio: cdc: ad7746: Add roadtest > > Documentation/ABI/testing/sysfs-bus-iio | 21 +- > .../ABI/testing/sysfs-bus-iio-cdc-ad7746 | 11 + > drivers/iio/cdc/Kconfig | 10 + > drivers/iio/cdc/Makefile | 1 + > drivers/iio/cdc/ad7746.c | 820 ++++++++++++++++++ > drivers/iio/industrialio-core.c | 32 +- > drivers/staging/iio/Kconfig | 1 - > drivers/staging/iio/Makefile | 1 - > drivers/staging/iio/cdc/Kconfig | 17 - > drivers/staging/iio/cdc/Makefile | 6 - > drivers/staging/iio/cdc/ad7746.c | 767 ---------------- > include/linux/iio/types.h | 1 + > .../roadtest/tests/iio/cdc/__init__.py | 0 > .../roadtest/roadtest/tests/iio/cdc/config | 1 + > .../roadtest/tests/iio/cdc/test_ad7746.py | 323 +++++++ > 15 files changed, 1211 insertions(+), 801 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-cdc-ad7746 > create mode 100644 drivers/iio/cdc/ad7746.c > delete mode 100644 drivers/staging/iio/cdc/Kconfig > delete mode 100644 drivers/staging/iio/cdc/Makefile > delete mode 100644 drivers/staging/iio/cdc/ad7746.c > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/__init__.py > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/config > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/test_ad7746.py > > -- > 2.36.1 >
On Tue, 28 Jun 2022 14:21:06 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jun 26, 2022 at 2:20 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Changes since v2: Thanks to Andy Shevchenko for review > > - Fix the horrible mess I made of a rebase that meant v2 didn't build > > mid patch set. > > - Fix various build issues I'd missed due to wrong make file (fails to > > update variables after moving them, missing unaligned include, > > failure to cleanup staging driver Kconfig / Makefile) > > - Change new ABI to _zeropoint and add more the description to hopefully > > add clarify to what this is. I don't think it's going to be a particularly > > common bit of ABI, but still good to get it right. > > > For non-commented patches (except 17th): > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > For commented, in case you address the comments I suggested, you may > also add a tag. Thanks Andy! Applied, patches 2-16 with suggested modifications (except the one where I disagreed on make vs makes). I'll pick up patch 1 in a few weeks if no feedback to suggest otherwise. Patch 17 needs the roadtest framework to go upstream. Given how useful roadtest was I'm hopeful that will progress! Thanks, Jonathan > > > Note that the --no-renames was used to preserve full code listing > > for the actual move out of staging to aid review. The ad7746 file is > > moved with no changes. > > > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/iio/cdc/ad7746.c > > similarity index 100% > > rename from drivers/staging/iio/cdc/ad7746.c > > rename to drivers/iio/cdc/ad7746.c > > > > This started out as an experiment with Vincent Whitchurch's roadtest > > testing framework [1] and that worked well so I carried on cleaning up the > > driver. Note that if you are using roadtest rebased to current > > kernel, add CONFIG_UML_RANDOM as suggested by Vincent in reply > > to v2 of this series. I still saw one freeze but with that config > > element added roadtest runs the tests successfully the vast majority > > of the time. > > > > Mostly this is standard tidy up, move to new interfaces, then move the driver > > out of staging, but there are a few other things in here. > > > > 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. > > The ad7746 is a 24 bit sensor and this highlighted that 9 decimal > > places wasn't enough to keep the error introduced by > > _raw * _scale from growing too large over the whole range (I couldn't > > write a sensible test for it). So I've proposed increasing the precision > > of the calculation used to provide the sysfs attributes with this value > > type and printing a few more decimal places (12), but only if overflow > > will not occur due to val[0] > ULLONG_MAX / PICO > > 2) _zeropoint ABI addition. This driver had an odd use of _offset for > > the case of differential channels that applied the offset to both > > of the differential pair (hence userspace shouldn't not apply it when > > converting to the base units. That isn't inline with the existing > > documentation for _offset and it wasn't clear to me that it made sense > > at all. To avoid confusion I've added this new ABI (_zeropoint) for this. > > 3) roadtest file - note this is not a complete test set for the driver and > > mainly focused on the main channel reads and places I thought I might > > have broken things whilst working on the driver. Given roadtest isn't > > upstream yet this test will have to wait. That doesn't block merging > > the rest of the series. > > > > My conclusion on roadtest - Very useful indeed. I'd encourage others to > > consider developing some basic sanity tests for drivers they are working on. > > Hopefully my python code isn't too hideous to understand at least! > > Vincent, it might be worth thinking about some generic code to handle the > > 'variants' on correct ABI like I introduce here because I switched from > > a shared by type scale to an individual one per channel for the voltages. > > Both were ABI compliant so that sort of change is fine most of the time > > though we have to be careful with it. > > > > All comments welcome. Note there may be changes that make more sense > > to do after moving this out of staging as long as there are no ABI changes involved > > etc. Feel free to highlight those sorts of changes as well as anything more > > significant. > > > > [1] https://lore.kernel.org/all/20220311162445.346685-9-vincent.whitchurch@axis.com/ > > > > Jonathan Cameron (17): > > iio: core: Increase precision of IIO_VAL_FRACTIONAL_LOG2 when possible > > iio: ABI: Fix wrong format of differential capacitance channel ABI. > > staging: iio: cdc: ad7746: Use explicit be24 handling. > > staging: iio: cdc: ad7746: Push handling of supply voltage scale to > > userspace. > > staging: iio: cdc: ad7746: Use local buffer for multi byte reads. > > staging: iio: cdc: ad7746: Factor out ad7746_read_channel() > > staging: iio: cdc: ad7764: Push locking down into case statements in > > read/write_raw > > staging: iio: cdc: ad7746: Break up use of chan->address and use > > FIELD_PREP etc > > staging: iio: cdc: ad7746: Drop unused i2c_set_clientdata() > > staging: iio: cdc: ad7746: Use _raw and _scale for temperature > > channels. > > iio: core: Introduce _zeropoint for differential channels > > staging: iio: cdc: ad7746: Switch from _offset to _zeropoint for > > differential channels. > > staging: iio: cdc: ad7746: Use read_avail() rather than opencoding. > > staging: iio: ad7746: White space cleanup > > iio: cdc: ad7746: Add device specific ABI documentation. > > iio: cdc: ad7746: Move driver out of staging. > > RFC: iio: cdc: ad7746: Add roadtest > > > > Documentation/ABI/testing/sysfs-bus-iio | 21 +- > > .../ABI/testing/sysfs-bus-iio-cdc-ad7746 | 11 + > > drivers/iio/cdc/Kconfig | 10 + > > drivers/iio/cdc/Makefile | 1 + > > drivers/iio/cdc/ad7746.c | 820 ++++++++++++++++++ > > drivers/iio/industrialio-core.c | 32 +- > > drivers/staging/iio/Kconfig | 1 - > > drivers/staging/iio/Makefile | 1 - > > drivers/staging/iio/cdc/Kconfig | 17 - > > drivers/staging/iio/cdc/Makefile | 6 - > > drivers/staging/iio/cdc/ad7746.c | 767 ---------------- > > include/linux/iio/types.h | 1 + > > .../roadtest/tests/iio/cdc/__init__.py | 0 > > .../roadtest/roadtest/tests/iio/cdc/config | 1 + > > .../roadtest/tests/iio/cdc/test_ad7746.py | 323 +++++++ > > 15 files changed, 1211 insertions(+), 801 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-cdc-ad7746 > > create mode 100644 drivers/iio/cdc/ad7746.c > > delete mode 100644 drivers/staging/iio/cdc/Kconfig > > delete mode 100644 drivers/staging/iio/cdc/Makefile > > delete mode 100644 drivers/staging/iio/cdc/ad7746.c > > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/__init__.py > > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/config > > create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/test_ad7746.py > > > > -- > > 2.36.1 > > > >
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/iio/cdc/ad7746.c
similarity index 100%
rename from drivers/staging/iio/cdc/ad7746.c
rename to drivers/iio/cdc/ad7746.c
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Changes since v2: Thanks to Andy Shevchenko for review - Fix the horrible mess I made of a rebase that meant v2 didn't build mid patch set. - Fix various build issues I'd missed due to wrong make file (fails to update variables after moving them, missing unaligned include, failure to cleanup staging driver Kconfig / Makefile) - Change new ABI to _zeropoint and add more the description to hopefully add clarify to what this is. I don't think it's going to be a particularly common bit of ABI, but still good to get it right. Note that the --no-renames was used to preserve full code listing for the actual move out of staging to aid review. The ad7746 file is moved with no changes. This started out as an experiment with Vincent Whitchurch's roadtest testing framework [1] and that worked well so I carried on cleaning up the driver. Note that if you are using roadtest rebased to current kernel, add CONFIG_UML_RANDOM as suggested by Vincent in reply to v2 of this series. I still saw one freeze but with that config element added roadtest runs the tests successfully the vast majority of the time. Mostly this is standard tidy up, move to new interfaces, then move the driver out of staging, but there are a few other things in here. 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. The ad7746 is a 24 bit sensor and this highlighted that 9 decimal places wasn't enough to keep the error introduced by _raw * _scale from growing too large over the whole range (I couldn't write a sensible test for it). So I've proposed increasing the precision of the calculation used to provide the sysfs attributes with this value type and printing a few more decimal places (12), but only if overflow will not occur due to val[0] > ULLONG_MAX / PICO 2) _zeropoint ABI addition. This driver had an odd use of _offset for the case of differential channels that applied the offset to both of the differential pair (hence userspace shouldn't not apply it when converting to the base units. That isn't inline with the existing documentation for _offset and it wasn't clear to me that it made sense at all. To avoid confusion I've added this new ABI (_zeropoint) for this. 3) roadtest file - note this is not a complete test set for the driver and mainly focused on the main channel reads and places I thought I might have broken things whilst working on the driver. Given roadtest isn't upstream yet this test will have to wait. That doesn't block merging the rest of the series. My conclusion on roadtest - Very useful indeed. I'd encourage others to consider developing some basic sanity tests for drivers they are working on. Hopefully my python code isn't too hideous to understand at least! Vincent, it might be worth thinking about some generic code to handle the 'variants' on correct ABI like I introduce here because I switched from a shared by type scale to an individual one per channel for the voltages. Both were ABI compliant so that sort of change is fine most of the time though we have to be careful with it. All comments welcome. Note there may be changes that make more sense to do after moving this out of staging as long as there are no ABI changes involved etc. Feel free to highlight those sorts of changes as well as anything more significant. [1] https://lore.kernel.org/all/20220311162445.346685-9-vincent.whitchurch@axis.com/ Jonathan Cameron (17): iio: core: Increase precision of IIO_VAL_FRACTIONAL_LOG2 when possible iio: ABI: Fix wrong format of differential capacitance channel ABI. staging: iio: cdc: ad7746: Use explicit be24 handling. staging: iio: cdc: ad7746: Push handling of supply voltage scale to userspace. staging: iio: cdc: ad7746: Use local buffer for multi byte reads. staging: iio: cdc: ad7746: Factor out ad7746_read_channel() staging: iio: cdc: ad7764: Push locking down into case statements in read/write_raw staging: iio: cdc: ad7746: Break up use of chan->address and use FIELD_PREP etc staging: iio: cdc: ad7746: Drop unused i2c_set_clientdata() staging: iio: cdc: ad7746: Use _raw and _scale for temperature channels. iio: core: Introduce _zeropoint for differential channels staging: iio: cdc: ad7746: Switch from _offset to _zeropoint for differential channels. staging: iio: cdc: ad7746: Use read_avail() rather than opencoding. staging: iio: ad7746: White space cleanup iio: cdc: ad7746: Add device specific ABI documentation. iio: cdc: ad7746: Move driver out of staging. RFC: iio: cdc: ad7746: Add roadtest Documentation/ABI/testing/sysfs-bus-iio | 21 +- .../ABI/testing/sysfs-bus-iio-cdc-ad7746 | 11 + drivers/iio/cdc/Kconfig | 10 + drivers/iio/cdc/Makefile | 1 + drivers/iio/cdc/ad7746.c | 820 ++++++++++++++++++ drivers/iio/industrialio-core.c | 32 +- drivers/staging/iio/Kconfig | 1 - drivers/staging/iio/Makefile | 1 - drivers/staging/iio/cdc/Kconfig | 17 - drivers/staging/iio/cdc/Makefile | 6 - drivers/staging/iio/cdc/ad7746.c | 767 ---------------- include/linux/iio/types.h | 1 + .../roadtest/tests/iio/cdc/__init__.py | 0 .../roadtest/roadtest/tests/iio/cdc/config | 1 + .../roadtest/tests/iio/cdc/test_ad7746.py | 323 +++++++ 15 files changed, 1211 insertions(+), 801 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-cdc-ad7746 create mode 100644 drivers/iio/cdc/ad7746.c delete mode 100644 drivers/staging/iio/cdc/Kconfig delete mode 100644 drivers/staging/iio/cdc/Makefile delete mode 100644 drivers/staging/iio/cdc/ad7746.c create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/__init__.py create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/config create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/test_ad7746.py