Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752883AbbKZJAo (ORCPT ); Thu, 26 Nov 2015 04:00:44 -0500 Received: from mail-wm0-f45.google.com ([74.125.82.45]:34803 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752413AbbKZJAm (ORCPT ); Thu, 26 Nov 2015 04:00:42 -0500 Subject: Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors To: Peter Meerwald-Stadler References: <1448450896-24387-1-git-send-email-mtitinger@baylibre.com> <1448450896-24387-2-git-send-email-mtitinger@baylibre.com> Cc: jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org From: Marc Titinger Message-ID: <5656CA36.9000908@baylibre.com> Date: Thu, 26 Nov 2015 10:00:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2955 Lines: 168 On 25/11/2015 13:20, Peter Meerwald-Stadler wrote: > >> in SOFTWARE buffer mode, a kthread will capture the active scan_elements >> into a kfifo, then compute the remaining time until the next capture tick >> and do an active wait (udelay). > > minor comments below Thanks Peter! All fixed in next iteration. M. ... >> +config INA2XX_IIO >> + tristate "Texas Instruments INA2xx Power Monitors IIO driver" >> + depends on I2C >> + select REGMAP_I2C >> + select IIO_BUFFER >> + help >> + Say yes here to build support for TI INA2xx familly Power Monitors. > > family > >> + >> + Note that this is not the existing hwmon interface for this device. > > this message not very clear removed. This was fuel to discuss the RFC. ... >> + >> +/* >> + * INA2XX registers definition >> + */ >> +/* common register definitions */ > > comment style; do we need both? removed one. >> + >> +/*Integration Time for VShunt */ > > time > ok >> + int itb; /* Bus voltage integration time uS */ >> + int its; /* Shunt voltage integration tim uS */ > > time > ok ge_shift) >> + * chip->config->bus_voltage_lsb; >> + *val = *uval/1000000; > > whitespace around / please ok >> + >> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip, > > retval should have type int indeed! >> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100, >> + 2116, 4156, 8244}; > > maybe whitespace before } ok >> +} >> + >> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip, > > retval should have type int ok >> + return 0; >> +} >> + > > drop dup newline > ok >> + >> +/*Sampling Freq is a consequence of the integration times of the V channels.*/ > > whitespace after /* and before */ please > ok >> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \ >> + .type = IIO_VOLTAGE, \ >> + .address = _address, \ >> + .indexed = 1, \ >> + .channel = (_index), \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >> + BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .scan_index = (_index), \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 16, \ >> + .storagebits = 16, \ >> + .endianness = IIO_BE, \ >> + } \ >> +} >> + > > drop dup newline > ok >> +} >> + >> +/* frequencies matching the cummulated integration times for vshunt and vbus */ > > cumulated wrong comment anyway, fixed. >> + * Set current LSB to 1mA, shunt is in uOhms >> + * (equation 13 in datasheet). We hardcode a Current_LSB >> + * of 1.0 x10-6. The only remaining parameter is RShunt > > full stop ok >> + mutex_destroy(&chip->state_lock); > > needed? removed. >> + >> + iio_device_unregister(indio_dev); > > not needed since devm_iio_device_register() is used ok -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/