On Wed, Aug 18, 2021 at 10:02:13AM +0200, Jacopo Mondi wrote: > On Tue, Aug 17, 2021 at 07:58:38PM +0300, Andy Shevchenko wrote: > > On Tue, Aug 17, 2021 at 05:49:51PM +0200, Jacopo Mondi wrote: > > > Add support for the Senseair Sunrise 006-0-0007 driver through the > > > IIO subsystem. > > > > Datasheet tag / link? > > It's reported in the bindings document, I can copy it here Yes, please, as a tag Datasheet: ... Signed-of-by: ... ... > > > +config SUNRISE > > > > Too generic name for configuration and module, > > I got the same feeling but at the same time SUNRISE_006_0_0007 seems a > bit too long. Would you suggest to go for that one ? Should I also > rename the .c file accordingly ? I would go with something like sunrise_air_6 or whatever part of the model. I haven't read datasheet to propose better naming. Also possible to have something like sunrise_air_meter.c (and corresponding Kconfig). ... > > > + * TODO: Add support for controllable EN pin > > > + * TODO: Add support for single-shot operations by using the nDRY pin. > > > > Ditto. > > > > If it's not ready, then make it ready. > > If I place myself in the perspective of someone interested in > verifying support for this chip in linux, I would first be happy I > found a driver for it, then I would even be happier if I got a clear > notice of what features the driver supports and which ones are missing. > Even more so if I look at bindings and find mention of two GPIO lines > which I don't see handled in the driver. A TODO note would make it > clear that those features are missing. > > The "make it ready" it's a little bit a non-sense to me, can you count > how many drivers support -all- the features a device provides ? In the > test board I'm using those lines are not even wired, should I throw > around code I cannot even test ? My point is that TODO means that somebody is working or will work on this. Is it the case? Then my comment applies, if not, replace this with something more realistic, like * Feature list that driver does NOT support (yet): * 1) ... * 2) ... ... > > > + i2c_smbus_read_byte(client); > > > > Can you use regmap I²C API? > > Do you think it's worth it ? > I admit I never used regmap API so far, but I always got the > impression that for such simple devices it's a bit an overkill. What > benefit would it bring here ? Some of nice features for debugging and tracing are in the generic support for free. On top if even this will be part of multi-function device than you won;t need to rewrite the driver. I would definitely go for it. ... > > > + return ret ? ret : IIO_VAL_INT; > > > > Can be written as > > > > return ret ?: IIO_VAL_INT; > > > > but up to maintainers. > > > > You know I never saw this syntax before ? :D > I'll use it! It's the C standard :-) -- With Best Regards, Andy Shevchenko
