| Message ID | cover.1679474247.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support ROHM BU27034 ALS sensor | expand |
On Wed, Mar 22, 2023 at 11:05:23AM +0200, Matti Vaittinen wrote: > Revision history: > v4 => v5: Mostly fixes to review comments from Andy and Jonathan. > - more accurate change-log in individual patches > - copy code from DRM test helper instead of moving it to simplify > merging 1) Why do you think this is a problem? 2) How would we avoid spreading more copies of the same code in the future? 1) Merge conflicts is not a bad thing. It shows that people tested their code in isolation and stabilized it before submitting to the upper maintainer. https://yarchive.net/comp/linux/git_merges_from_upstream.html 2) Spreading the same code when we _know_ this, should be very well justified. Merge conflict is an administrative point, and not a technical obstacle to avoid. > - document all exported GTS helpers. > - inline a few GTS helpers > - use again Milli lux for the bu27034 with RAW IIO_LIGHT channel and scale > - Fix bug from added in v4 bu27034 time setting.
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: Hello Andy, > On Wed, Mar 22, 2023 at 11:05:23AM +0200, Matti Vaittinen wrote: > >> Revision history: >> v4 => v5: Mostly fixes to review comments from Andy and Jonathan. >> - more accurate change-log in individual patches > >> - copy code from DRM test helper instead of moving it to simplify >> merging > > 1) Why do you think this is a problem? > 2) How would we avoid spreading more copies of the same code in the future? > > > 1) Merge conflicts is not a bad thing. It shows that people tested their code > in isolation and stabilized it before submitting to the upper maintainer. > > https://yarchive.net/comp/linux/git_merges_from_upstream.html > > 2) Spreading the same code when we _know_ this, should be very well justified. > Merge conflict is an administrative point, and not a technical obstacle to > avoid. > I believe this was suggested by Maxime and the rationale is that by just copying the helpers for now, that would make it easier to land instead of requiring coordination between different subystems. Otherwise the IIO tree will need to provide an inmutable branch for the DRM tree to merge and so on. I agree with Maxime that a little bit of duplication (that can be cleaned up by each subsystem at their own pace) is the path of least resistance.
On 3/22/23 12:34, Javier Martinez Canillas wrote: > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: > > Hello Andy, > >> On Wed, Mar 22, 2023 at 11:05:23AM +0200, Matti Vaittinen wrote: >> >>> Revision history: >>> v4 => v5: Mostly fixes to review comments from Andy and Jonathan. >>> - more accurate change-log in individual patches >> >>> - copy code from DRM test helper instead of moving it to simplify >>> merging >> >> 1) Why do you think this is a problem? >> 2) How would we avoid spreading more copies of the same code in the future? >> >> >> 1) Merge conflicts is not a bad thing. It shows that people tested their code >> in isolation and stabilized it before submitting to the upper maintainer. >> >> https://yarchive.net/comp/linux/git_merges_from_upstream.html >> >> 2) Spreading the same code when we _know_ this, should be very well justified. >> Merge conflict is an administrative point, and not a technical obstacle to >> avoid. I definitely agree. This is also why I did the renaming and not copying in the first version. In this version I did still add the subsequent patch 2/8 - which drops the duplicates from DRM tree. > I believe this was suggested by Maxime and the rationale is that by just > copying the helpers for now, that would make it easier to land instead of > requiring coordination between different subystems. This is correct. > Otherwise the IIO tree will need to provide an inmutable branch for the > DRM tree to merge and so on. Or, if we carry the patch 1/8 via self-test tree, then we get even more players here. Still, I am not opposing immutable branch because that would allow fast applying of the patch 2/8 as well. Longer that is delayed, more likely we will see more users of the DRM helpers and harder it gets to remove the duplicates later. > I agree with Maxime that a little bit of duplication (that can be cleaned > up by each subsystem at their own pace) is the path of least resistance. I'd say this depends. It probably is the path of least resistance for people maintaining the trees. It can also be the path of least resistance in general - but it depends on if there will be no new users for those DRM helpers while waiting the new APIs being merged in DRM tree. More users we see in DRM, more effort the clean-up requires. I have no strong opinion on this specific case. I'd just be happy to see the IIO tests getting in preferably sooner than later - although 'soon' and 'late' does also depend on other factors besides these helpers... Yours, -- Matti
On Wed, Mar 22, 2023 at 12:59:33PM +0200, Matti Vaittinen wrote: > On 3/22/23 12:34, Javier Martinez Canillas wrote: > > > On Wed, Mar 22, 2023 at 11:05:23AM +0200, Matti Vaittinen wrote: ... > > > > - copy code from DRM test helper instead of moving it to simplify > > > > merging > > > > > > 1) Why do you think this is a problem? > > > 2) How would we avoid spreading more copies of the same code in the future? > > > > > > > > > 1) Merge conflicts is not a bad thing. It shows that people tested their code > > > in isolation and stabilized it before submitting to the upper maintainer. > > > > > > https://yarchive.net/comp/linux/git_merges_from_upstream.html > > > > > > 2) Spreading the same code when we _know_ this, should be very well justified. > > > Merge conflict is an administrative point, and not a technical obstacle to > > > avoid. > > I definitely agree. This is also why I did the renaming and not copying in > the first version. In this version I did still add the subsequent patch 2/8 > - which drops the duplicates from DRM tree. > > > I believe this was suggested by Maxime and the rationale is that by just > > copying the helpers for now, that would make it easier to land instead of > > requiring coordination between different subystems. > > This is correct. > > > Otherwise the IIO tree will need to provide an inmutable branch for the > > DRM tree to merge and so on. > > Or, if we carry the patch 1/8 via self-test tree, then we get even more > players here. > > Still, I am not opposing immutable branch because that would allow fast > applying of the patch 2/8 as well. Longer that is delayed, more likely we > will see more users of the DRM helpers and harder it gets to remove the > duplicates later. > > > I agree with Maxime that a little bit of duplication (that can be cleaned > > up by each subsystem at their own pace) is the path of least resistance. > > I'd say this depends. It probably is the path of least resistance for people > maintaining the trees. It can also be the path of least resistance in > general - but it depends on if there will be no new users for those DRM > helpers while waiting the new APIs being merged in DRM tree. More users we > see in DRM, more effort the clean-up requires. > > I have no strong opinion on this specific case. I'd just be happy to see the > IIO tests getting in preferably sooner than later - although 'soon' and > 'late' does also depend on other factors besides these helpers... Since I'm not a maintainer of either, and one of them requires something, I can't oppose.
On Wed, Mar 22, 2023 at 12:59:33PM +0200, Matti Vaittinen wrote: > > I agree with Maxime that a little bit of duplication (that can be cleaned > > up by each subsystem at their own pace) is the path of least resistance. > > I'd say this depends. It probably is the path of least resistance for people > maintaining the trees. It can also be the path of least resistance in > general - but it depends on if there will be no new users for those DRM > helpers while waiting the new APIs being merged in DRM tree. More users we > see in DRM, more effort the clean-up requires. So far there's one user in DRM, and I'm not aware of any current work using it at the moment. Even if some show up in the short-term future, it's not going to be overwhelming. Maxime
Support ROHM BU27034 ALS sensor This series adds support for ROHM BU27034 Ambient Light Sensor. The BU27034 has configurable gain and measurement (integration) time settings. Both of these have inversely proportional relation to the sensor's intensity channel scale. Many users only set the scale, which means that many drivers attempt to 'guess' the best gain+time combination to meet the scale. Usually this is the biggest integration time which allows setting the requested scale. Typically, increasing the integration time has better accuracy than increasing the gain, which often amplifies the noise as well as the real signal. However, there may be cases where more responsive sensors are needed. So, in some cases the longest integration times may not be what the user prefers. The driver has no way of knowing this. Hence, the approach taken by this series is to allow user to set both the scale and the integration time with following logic: 1. When scale is set, the existing integration time is tried to be maintained as a first priority. 1a) If the requested scale can't be met by current time, then also other time + gain combinations are searched. If scale can be met by some other integration time, then the new time may be applied. If the time setting is common for all channels, then also other channels must be able to maintain their scale with this new time (by changing their gain). The new times are scanned in the order of preference (typically the longest times first). 1b) If the requested scale can be met using current time, then only the gain for the channel is changed. 2. When the integration time change - scale is tried to be maintained. When integration time change is requested also gain for all impacted channels is adjusted so that the scale is not changed, or is chaned as little as possible. This is different from the RFCv1 where the request was rejected if suitable gain couldn't be found for some channel(s). This logic is simple. When total gain (either caused by time or hw-gain) is doubled, the scale gets halved. Also, the supported times are given a 'multiplier' value which tells how much they increase the total gain. However, when I wrote this logic in bu27034 driver, I made quite a few errors on the way - and driver got pretty big. As I am writing drivers for two other sensors (RGB C/IR + flicker BU27010 and RGB C/IR BU27008) with similar gain-time-scale logic I thought that adding common helpers for these computations might be wise. I hope this way all the bugs will be concentrated in one place and not in every individual driver ;) Hence, this series also intriduces IIO gain-time-scale helpers (abbreviated as gts-helpers) + a couple of KUnit tests for the most hairy parts. Speaking of which - testing the devm interfaces requires a 'dummy device'. There were neat helpers in DRM tests for creating and freeing such a device. This series moves those helpers to more generic location. What is worth noting is that there is something similar ongoing in the CCF territory: https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/ These efforts should be somehow coordinated in order to avoid any avoid conflicts. Finally, these added helpers do provide some value also for drivers which only: a) allow gain change or b) allow changing both the time and gain while trying to maintain the scale. For a) we provide the gain - selector (register value) table format + selector to gain look-ups, gain <-> scale conversions and the available scales helpers. For latter case we also provide the time-tables, and actually all the APIs should be usable by setting the time multiplier to 1. (not testeted thoroughly though). The patch 1/8 introduces the helpers for creating/dropping a test device for devm-tests. It can be applied alone. The patches 2/8 (convert DRM tests to use new helper) depends on patch 1/8 but is othervice not part of this series. It can be applied to DRM tree after the dependency to 1/8 is handled. The patch 5/8 (IIO GTS tests) also depends on the patch 1/8 (and also other patches in the series). Rest of the series should be Ok to be applied with/without the patches 1/8, 2/8, 5/8 - although the 5/8 would be "nice to have" together with the rest of the series for the testability reasons. Revision history: v4 => v5: Mostly fixes to review comments from Andy and Jonathan. - more accurate change-log in individual patches - copy code from DRM test helper instead of moving it to simplify merging - document all exported GTS helpers. - inline a few GTS helpers - use again Milli lux for the bu27034 with RAW IIO_LIGHT channel and scale - Fix bug from added in v4 bu27034 time setting. v3 => v4: (Still mostly fixes to review comments from Andy and Jonathan) - more accurate change-log in individual patches - dt-binding and maintainer patches unchanged. - dropped unused helpers and converted ones currently used only internally to static. - extracted "dummy device" creation helpers from DRM tests. - added tests for devm APIs - dropped scale for PROCESSED channel in BU27034 and converted mLux values to luxes - dropped channel 2 GAIN setting which can't be done due to HW limitations. v2 => v3: (Mostly fixes to review comments from Andy and Jonathan) - dt-binding and maintainer patches unchanged. - iio-gts-helper tests: Use namespaces - iio-gts-helpers + bu27034 plenty of changes. See more comprehensive changelog in individual patches. RFCv1 => v2: dt-bindings: - Fix binding file name and id by using comma instead of a hyphen to separate the vendor and part names. gts-helpers: - fix include guardian - Improve kernel doc for iio_init_iio_gts. - Add iio_gts_scale_to_total_gain - Add iio_gts_total_gain_to_scale - Fix review comments from Jonathan - add documentation to few functions - replace 0xffffffffffffffffLLU by U64_MAX - some styling fixes - drop unnecessary NULL checks - order function arguments by in / out purpose - drop GAIN_SCALE_ITIME_MS() - Add helpers for available scales and times - Rename to iio-gts-helpers gts-tests: - add tests for available scales/times helpers - adapt to renamed iio-gts-helpers.h header bu27034-driver: - (really) protect read-only registers - fix get and set gain - buffered mode - Protect the whole sequences including meas_en/meas_dis to avoid messing up the enable / disable order - typofixes / doc improvements - change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US() - use more accurate scale for lux channel (milli lux) - provide available scales / integration times (using helpers). - adapt to renamed iio-gts-helpers.h file - bu27034 - longer lines in Kconfig - Drop bu27034_meas_en and bu27034_meas_dis wrappers. - Change device-name from bu27034-als to bu27034 MAINTAINERS: - Add iio-list --- Matti Vaittinen (8): drivers: kunit: Generic helpers for test device creation drm/tests: helpers: Use generic helpers dt-bindings: iio: light: Support ROHM BU27034 iio: light: Add gain-time-scale helpers iio: test: test gain-time-scale helpers MAINTAINERS: Add IIO gain-time-scale helpers iio: light: ROHM BU27034 Ambient Light Sensor MAINTAINERS: Add ROHM BU27034 .../bindings/iio/light/rohm,bu27034.yaml | 46 + MAINTAINERS | 14 + drivers/base/test/Kconfig | 5 + drivers/base/test/Makefile | 2 + drivers/base/test/test_kunit_device.c | 83 + drivers/gpu/drm/Kconfig | 2 + .../gpu/drm/tests/drm_client_modeset_test.c | 5 +- drivers/gpu/drm/tests/drm_kunit_helpers.c | 69 - drivers/gpu/drm/tests/drm_managed_test.c | 5 +- drivers/gpu/drm/tests/drm_modes_test.c | 5 +- drivers/gpu/drm/tests/drm_probe_helper_test.c | 5 +- drivers/gpu/drm/vc4/Kconfig | 1 + drivers/gpu/drm/vc4/tests/vc4_mock.c | 3 +- .../gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 9 +- drivers/iio/Kconfig | 3 + drivers/iio/Makefile | 1 + drivers/iio/industrialio-gts-helper.c | 1064 ++++++++++++ drivers/iio/light/Kconfig | 14 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27034.c | 1482 +++++++++++++++++ drivers/iio/test/Kconfig | 14 + drivers/iio/test/Makefile | 1 + drivers/iio/test/iio-test-gts.c | 542 ++++++ include/drm/drm_kunit_helpers.h | 7 +- include/kunit/platform_device.h | 13 + include/linux/iio/iio-gts-helper.h | 206 +++ 26 files changed, 3515 insertions(+), 87 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27034.yaml create mode 100644 drivers/base/test/test_kunit_device.c create mode 100644 drivers/iio/industrialio-gts-helper.c create mode 100644 drivers/iio/light/rohm-bu27034.c create mode 100644 drivers/iio/test/iio-test-gts.c create mode 100644 include/kunit/platform_device.h create mode 100644 include/linux/iio/iio-gts-helper.h base-commit: eeac8ede17557680855031c6f305ece2378af326