From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 73306C4338F for ; Wed, 18 Aug 2021 08:02:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4ADC260EB5 for ; Wed, 18 Aug 2021 08:02:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238670AbhHRICX (ORCPT ); Wed, 18 Aug 2021 04:02:23 -0400 Received: from relay8-d.mail.gandi.net ([217.70.183.201]:41321 "EHLO relay8-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237348AbhHRICD (ORCPT ); Wed, 18 Aug 2021 04:02:03 -0400 Received: (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 0BDE11BF206; Wed, 18 Aug 2021 08:01:25 +0000 (UTC) Date: Wed, 18 Aug 2021 10:02:13 +0200 From: Jacopo Mondi To: Andy Shevchenko Cc: Jonathan Cameron , Lars-Peter Clausen , Matt Ranostay , linux-iio@vger.kernel.org Subject: Re: [PATCH 2/2] iio: chemical: Add Senseair Sunrise 006-0-007 driver Message-ID: <20210818080213.d56phfmjlwbgzlca@uno.localdomain> References: <20210817154951.50208-1-jacopo@jmondi.org> <20210817154951.50208-3-jacopo@jmondi.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org Hi Andy, thanks for review 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 > > ... > > > +config SUNRISE > > + tristate "Senseair Sunrise 006-0-0007 CO2 sensor" > > + depends on I2C > > + help > > + Say yes here to build support for Senseair Sunrise 006-0-0007 CO2 > > + sensor. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called 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 ? > > ... > > > + * iio/co2/sunrise-006-0-0007.c > > + * > > Redundant noise. > > ... > > > + * 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 ? > > ... > > > > +#include > > Why? Perhaps mod_devicetable,h is what you had in mind. > Ah yes indeed. I got it wrong in so many places in the past ! > ... > > > + 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 ? > ... > > > +#define sunrise_readb_client(client, addr) \ > > +({ \ > > + u8 _val; \ > > + sunrise_read_byte(client, addr, &_val); \ > > + _val; \ > > +}) > > +#define sunrise_readb(addr) sunrise_readb_client(client, addr) > > Drop ugly macros and use read_poll_timeout() below. Ah! Thanks a lot, I missed the fact read_poll_timeout() accepts a variable number of arguments... However, the API of the driver i2c read function is: int sunrise_read_byte(client, reg, *val); while read_poll_timeout() expects int read(client, reg); I know the answer already: change your i2c accessor to comply. As it is only used by read_poll_timeout() I'll do so :) > > > +#define sunrise_poll_register(reg, val, cond) \ > > + readx_poll_timeout(sunrise_readb, reg, val, cond, 100, 0) > > > ... > > > +static int sunrise_read_raw(struct iio_dev *iio_dev, > > + const struct iio_chan_spec *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct sunrise_dev *sunrise = iio_priv(iio_dev); > > + struct i2c_client *client = sunrise->client; > > + u16 value; > > > + int ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + > > + mutex_lock(&sunrise->lock); > > + > > + switch (chan->type) { > > + case IIO_CONCENTRATION: { > > + u16 co2; > > Reuse value. > Ack > > + ret = sunrise_read_co2_filtered(client, &co2); > > + *val = co2; > > + mutex_unlock(&sunrise->lock); > > + > > + 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! > > + } > > + > > + case IIO_TEMP: { > > + u16 temp; > > Reuse value. > ack > > + ret = sunrise_read_word(client, > > + SUNRISE_CHIP_TEMPERATURE_REG, > > + &temp); > > + *val = temp; > > + mutex_unlock(&sunrise->lock); > > + > > + return ret ? ret : IIO_VAL_INT; > > + } > > + > > + default: > > + mutex_unlock(&sunrise->lock); > > + return -EINVAL; > > + } > > + > > + case IIO_CHAN_INFO_SCALE: > > + /* Chip temperature scale = 1/100 */ > > + *val = 1; > > + *val2 = 100; > > + return IIO_VAL_FRACTIONAL; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > ... > > > + return sunrise_poll_register(SUNRISE_CALIBRATION_STATUS_REG, status, > > + (status & BIT(5))); > > Too many parentheses. Right! Thanks for review! > > -- > With Best Regards, > Andy Shevchenko > >