| Message ID | 20210822184927.94673-1-jacopo@jmondi.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: chemical: Add Senseair Sunrise CO2 sensor | expand |
On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > The driver supports continuous reads of temperature and CO2 concentration > through two dedicated IIO channels. It also supports calibration and error > inspection through the concentration channel ext_info. > > Minor changes in v3 Not sure, I have found bigger issues. See my comments in patch 2. So, since it's obvious you haven't tested the patch and we are at rc7 I think you can take a few weeks of time to have a look and carefully address all comments and to test.
Andy, On Sun, Aug 22, 2021 at 11:11:59PM +0300, Andy Shevchenko wrote: > On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > The driver supports continuous reads of temperature and CO2 concentration > > through two dedicated IIO channels. It also supports calibration and error > > inspection through the concentration channel ext_info. > > > > Minor changes in v3 > > Not sure, I have found bigger issues. See my comments in patch 2. > So, since it's obvious you haven't tested the patch and we are at rc7 > I think you can take a few weeks of time to have a look and carefully > address all comments and to test. I'm sorry if the patches I sent out contain a leftover debug (the pr_err) and I cannot explain how the :q commands ended up in the final patch, most probably they come from me inspecting the generated patch and being sloppy with vim on a sunday night, as they are not in the git tree. They won't have even compiled otherwise, and it's obvious where they come from if you've ever used vim. Aprt from that, this is a v3, not v7, I've tested it several times, there's no need to paternalize me as the only thing to fix is to re-add back the ending commas in arrays declarations as a result of a comment from you which I interpreted as a request from removing them. To me commas at the end of an array declaration is mostly nit picking, which I accept given the context and given I don't know the subsystem rules, but please consider the last version of the patch mostly fixes minor style issue which are questionable (lines that can span over 80 cols, terminating commas, empty line at the beginning of the switch which I liked but you didn't, 'Typically' in the bindings etc)). So please consider that it took me a sunday afternoon to please your preferences. If a debug printout leftover has escaped I'm sorry, I've been sloppy. The ':q' in the final patch are another obvious stupid mistake but the assumption that I've not tested the patches it's really not appropriate. As I appreciate your effort in review and I've silently gone over all your comments, even the questionable ones, I don't think it's fair to crucify someone which has sent a fully working driver with no major issues (none that has been found so far at least) for two stupid leftovers. I'll send v4 re-adding back the commas at the end of arrays. > > -- > With Best Regards, > Andy Shevchenko
On Mon, Aug 23, 2021 at 09:16:22AM +0200, Jacopo Mondi wrote: > On Sun, Aug 22, 2021 at 11:11:59PM +0300, Andy Shevchenko wrote: > > On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > The driver supports continuous reads of temperature and CO2 concentration > > > through two dedicated IIO channels. It also supports calibration and error > > > inspection through the concentration channel ext_info. > > > > > > Minor changes in v3 > > > > Not sure, I have found bigger issues. See my comments in patch 2. > > So, since it's obvious you haven't tested the patch and we are at rc7 > > I think you can take a few weeks of time to have a look and carefully > > address all comments and to test. > > I'm sorry if the patches I sent out contain a leftover debug (the > pr_err) and I cannot explain how the :q commands ended up in the final > patch, most probably they come from me inspecting the generated patch > and being sloppy with vim on a sunday night, as they are not in the > git tree. They won't have even compiled otherwise, and it's obvious > where they come from if you've ever used vim. > > Aprt from that, this is a v3, not v7, I've tested it several times, there's > no need to paternalize me as the only thing to fix is to re-add back > the ending commas in arrays declarations as a result of a comment from > you which I interpreted as a request from removing them. To me commas > at the end of an array declaration is mostly nit picking, which I > accept given the context and given I don't know the subsystem rules, > but please consider the last version of the patch mostly fixes minor > style issue which are questionable (lines that can span over 80 cols, > terminating commas, empty line at the beginning of the switch which I > liked but you didn't, 'Typically' in the bindings etc)). > > So please consider that it took me a sunday > afternoon to please your preferences. If a debug printout leftover has > escaped I'm sorry, I've been sloppy. The ':q' in the final patch are > another obvious stupid mistake but the assumption that I've not tested > the patches it's really not appropriate. > > As I appreciate your effort in review and I've silently gone over all > your comments, even the questionable ones, I don't think it's fair to > crucify someone which has sent a fully working driver with no major issues > (none that has been found so far at least) for two stupid leftovers. > > I'll send v4 re-adding back the commas at the end of arrays. Please, do.
The driver supports continuous reads of temperature and CO2 concentration through two dedicated IIO channels. It also supports calibration and error inspection through the concentration channel ext_info. Minor changes in v3 v2->v3: - [1/3] - Fix syntax error reported by dt_binding_check The device node label in the example cannot contain '-' - Add 'Typically' to the gpios polarities description - [2/3] - As suggested by Andy: - depends on OF, SYSFS; select REGMAP_I2C - Fix style issues: - span over 80 cols where appropriate - remove , in last entries of all arrays - use for_each_set_bit in sunrise_error_status_read() - minor style issues (brakets, empty lines, wording) v1->v2: - Add ABI documentation in [3/3] - [1/3] - Address Rob's comments on missing maxItem and add device node label - Do not change the pin's polarity description as suggested by Andy due to conflicting suggestions - [2/3] - Expand Kconfig symbol name and change driver's name as suggested by Andy - Use regmap instead of raw smbus calls as suggested by Andy - Take into account minor style comments from Andy - Install channel's ext_info to support calibration triggering and enumerate calibration modes and error status - Matt suggested to use sysfs attributes, but I found the per-channel attributes more appropriate. Hope this is good as well. Thanks j Jacopo Mondi (3): dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor iio: chemical: Add Sensteair Sunrise 006-0-007 driver iio: ABI: docs: Document Senseair Sunrise ABI .../sysfs-bus-iio-chemical-sunrise-co2 | 51 ++ .../iio/chemical/senseair,sunrise.yaml | 53 +++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 6 + drivers/iio/chemical/Kconfig | 13 + drivers/iio/chemical/Makefile | 1 + drivers/iio/chemical/sunrise_co2.c | 450 ++++++++++++++++++ 7 files changed, 576 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2 create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml create mode 100644 drivers/iio/chemical/sunrise_co2.c -- 2.32.0