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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 780E8C19F32 for ; Sun, 2 Mar 2025 04:12:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YH5iUDM3RoF9s0C8PkX4Jj7kEvk4gKTGiTMYdAvFXNw=; b=g6pTYaJ8c7DeEw9LCubo0J0Ur+ omV4GCoDrQ1hpTpcF0P0lJBPNDbbHqDbqr/TtT2h9EjOKBQweY/j/uyhGay4lSuHQ38K0TZuKvUzr JCA4ueRML//RwjYfDndSbEQ1TtRQ7KdRKVBHqCcaremLDgyRG47RHXot74BwfdJYM1Pk6BW93Ql7v WnHgyN0cH5Dzt8a9g1cqT6RYSqm90cTJhFCw7HFJln2b9+TloS3QL3USR4k8ys+EVz/zgpRYIxfNz WrSWQ9boJqJSLXfnQxsP6iAgtGVTwX5J1++gHszH93og5pzajIZFRt+DvPQdJCHJUce1BjWUMyzFx uvM6KiFg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1toah4-0000000FLov-3Thm; Sun, 02 Mar 2025 04:12:46 +0000 Received: from tor.source.kernel.org ([2600:3c04::f03c:95ff:fe5e:7468]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1toafX-0000000FLkf-3iA8 for linux-arm-kernel@lists.infradead.org; Sun, 02 Mar 2025 04:11:12 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 333DB61162; Sun, 2 Mar 2025 04:11:01 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1D712C4CED6; Sun, 2 Mar 2025 04:10:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1740888670; bh=mhZ4GEcprVucdPj/c25y003/+abJiOOGAR1e9Cvo/NY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=uRAQ/BprkT/fcETMQ/nvto5l6d/mJi22o7WvOY6UsRwn/nB2zaUFub9c4e4HutEhR FxSc2yd6nJX6ms2tv+vIeelPkGLRoo997BrggpXhZINTGueJF4UWmioOzWaawkqEB2 pTkMX3nkEoGq8HGmAaqik5NYczg2WLPMmBOgR9o6jw4SdLBzJRFCFUGq7yuxo4HtZn sf2e9Miao8Q7EQkyrF8dkhDWkuN5/64M/VSrdAfByWm15HcEcKb77ZsQsl96Xg2LTV wvcCxNrLi9itzebjiUPrMZKaWvF1pi8/VqyLlkyZKbM70Q8ddZou8wJ54NLQpP8xPv dje10SlihUoLg== Date: Sun, 2 Mar 2025 04:10:46 +0000 From: Jonathan Cameron To: Matti Vaittinen Cc: Matti Vaittinen , Lars-Peter Clausen , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Andy Shevchenko , Daniel Scally , Heikki Krogerus , Sakari Ailus , Greg Kroah-Hartman , "Rafael J. Wysocki" , Danilo Krummrich , Lad Prabhakar , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Hugo Villeneuve , Nuno Sa , David Lechner , Javier Carrasco , Guillaume Stols , Olivier Moysan , Dumitru Ceclan , Trevor Gamblin , Matteo Martelli , Alisa-Dariana Roman , Ramona Alexandra Nechita , AngeloGioacchino Del Regno , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, Linus Walleij Subject: Re: [PATCH v4 08/10] iio: adc: Support ROHM BD79124 ADC Message-ID: <20250302041046.7fde3c68@jic23-huawei> In-Reply-To: <4d1bf5df8f3b0f760422b6b67fcc8245ebf520e0.1740421249.git.mazziesaccount@gmail.com> References: <4d1bf5df8f3b0f760422b6b67fcc8245ebf520e0.1740421249.git.mazziesaccount@gmail.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.48; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 24 Feb 2025 20:34:30 +0200 Matti Vaittinen wrote: > The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports > an automatic measurement mode, with an alarm interrupt for out-of-window > measurements. The window is configurable for each channel. > > The I2C protocol for manual start of the measurement and data reading is > somewhat peculiar. It requires the master to do clock stretching after > sending the I2C slave-address until the slave has captured the data. > Needless to say this is not well suopported by the I2C controllers. > > Thus the driver does not support the BD79124's manual measurement mode > but implements the measurements using automatic measurement mode relying > on the BD79124's ability of storing latest measurements into register. > > The driver does also support configuring the threshold events for > detecting the out-of-window events. > > The BD79124 keeps asserting IRQ for as long as the measured voltage is > out of the configured window. Thus the driver masks the received event > for a fixed duration (1 second) when an event is handled. This prevents > the user-space from choking on the events > > The ADC input pins can be also configured as general purpose outputs. > Those pins which don't have corresponding ADC channel node in the > device-tree will be controllable as GPO. > > Signed-off-by: Matti Vaittinen Some minor stuff inline. Thanks, Jonathan > + > +#define BD79124_REG_SYSTEM_STATUS 0x0 > +#define BD79124_REG_GEN_CFG 0x01 > +#define BD79124_REG_OPMODE_CFG 0x04 > +#define BD79124_REG_PINCFG 0x05 > +#define BD79124_REG_GPO_VAL 0x0B > +#define BD79124_REG_SEQUENCE_CFG 0x10 > +#define BD79124_REG_MANUAL_CHANNELS 0x11 > +#define BD79124_REG_AUTO_CHANNELS 0x12 > +#define BD79124_REG_ALERT_CH_SEL 0x14 > +#define BD79124_REG_EVENT_FLAG 0x18 > +#define BD79124_REG_EVENT_FLAG_HI 0x1a > +#define BD79124_REG_EVENT_FLAG_LO 0x1c > +#define BD79124_REG_HYSTERESIS_CH0 0x20 > +#define BD79124_REG_EVENTCOUNT_CH0 0x22 > +#define BD79124_REG_RECENT_CH0_LSB 0xa0 > +#define BD79124_REG_RECENT_CH7_MSB 0xaf > + > +#define BD79124_ADC_BITS 12 > +#define BD79124_MASK_CONV_MODE GENMASK(6, 5) > +#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0) > +#define BD79124_CONV_MODE_MANSEQ 0 > +#define BD79124_CONV_MODE_AUTO 1 > +#define BD79124_INTERVAL_075 0 Can we make these units in these explicit? #define BD79124_INTERVAL_MS_0_75 maybe? Nice to avoid need for comments on what the units are where you use these. > +#define BD79124_INTERVAL_150 1 > +#define BD79124_INTERVAL_300 2 > +#define BD79124_INTERVAL_600 3 > + > +static int bd79124_enable_event(struct bd79124_data *data, > + enum iio_event_direction dir, unsigned int channel) > +{ > + int dir_bit = BIT(dir); > + int reg; > + u16 *limit; > + int ret; > + > + guard(mutex)(&data->mutex); > + /* Set channel to be measured */ > + ret = bd79124_start_measurement(data, channel); > + if (ret) > + return ret; > + > + data->alarm_monitored[channel] |= dir_bit; > + > + /* Add the channel to the list of monitored channels */ > + ret = regmap_set_bits(data->map, BD79124_REG_ALERT_CH_SEL, > + BIT(channel)); > + if (ret) > + return ret; > + > + if (dir == IIO_EV_DIR_RISING) { > + limit = &data->alarm_f_limit[channel]; > + reg = BD79124_GET_HIGH_LIMIT_REG(channel); > + } else { > + limit = &data->alarm_f_limit[channel]; > + reg = BD79124_GET_LOW_LIMIT_REG(channel); > + } > + /* Don't write the new limit to the hardware if we are in the > + * rate-limit period. The timer which re-enables the event will set > + * the limit. > + */ /* * Don't Check for other cases of this... > +static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel) > +{ > + int ret, evbit = BIT(IIO_EV_DIR_RISING); > + > + if (!(data->alarm_suppressed[channel] & evbit)) > + return; > + > + data->alarm_suppressed[channel] &= (~evbit); No brackets around the ~evbit. Check for other cases of this. Otherwise we'll get some script written 'cleanup'. > + > + if (!(data->alarm_monitored[channel] & evbit)) > + return; > + > + ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel), > + data->alarm_r_limit[channel]); > + if (ret) > + dev_warn(data->dev, "High limit enabling failed for channel%d\n", > + channel); > +} > + > +static void bd79124_alm_enable_worker(struct work_struct *work) > +{ > + int i; > + struct bd79124_data *data = container_of(work, struct bd79124_data, > + alm_enable_work.work); > + > + guard(mutex)(&data->mutex); > + /* > + * We should not re-enable the event if user has disabled it while > + * rate-limiting was enabled. > + */ Is this comment suggesting something that isn't done or referring to specific code? I think it wants to be in the function above where the decision is made. > + for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) { > + bd79124_re_enable_hi(data, i); > + bd79124_re_enable_lo(data, i); > + } > +} > + > + > +static irqreturn_t bd79124_event_handler(int irq, void *priv) > +{ > + int ret, i_hi, i_lo, i; > + struct iio_dev *iio_dev = priv; > + struct bd79124_data *data = iio_priv(iio_dev); > + > + /* > + * Return IRQ_NONE if bailing-out without acking. This allows the IRQ > + * subsystem to disable the offending IRQ line if we get a hardware > + * problem. This behaviour has saved my poor bottom a few times in the > + * past as, instead of getting unusably unresponsive, the system has > + * spilled out the magic words "...nobody cared". > + */ > + ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_HI, &i_hi); > + if (ret) > + return IRQ_NONE; > + > + ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo); > + if (ret) > + return IRQ_NONE; > + > + if (!i_lo && !i_hi) > + return IRQ_NONE; > + > + for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) { > + u64 ecode; > + > + if (BIT(i) & i_hi) { > + ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i, > + IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING); Align this to less tabs as per discussion on previous version. > + > + > +struct bd79124_reg_init { > + int reg; > + int val; > +}; Not used any more. > + > +static int bd79124_chan_init(struct bd79124_data *data, int channel) > +{ > + int ret; > + > + ret = regmap_write(data->map, BD79124_GET_HIGH_LIMIT_REG(channel), 4095); > + if (ret) > + return ret; > + > + return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0); > +} > + > +static int bd79124_init_mux(struct bd79124_data *data, int gpio_pins) > +{ > + return regmap_write(data->map, BD79124_REG_PINCFG, gpio_pins); Maybe squash this inline. Doesn't seem to add a lot to have the helper. > +} > + > +static int bd79124_hw_init(struct bd79124_data *data, int gpio_pins) > +{ ... > + > + /* Set the measurement interval to 0.75 mS */ This lead me to comment on defines. I'd rather code was fully self documenting and remove the comment if we can. Makes for less chance of it becoming out of sync over time. > + regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075); > + ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG, > + BD79124_MASK_AUTO_INTERVAL, regval); > + if (ret) > + return ret;