From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 116B33F4100 for ; Mon, 17 Aug 2026 11:59:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786967988; cv=none; b=TIbVVthJJ398U9OluiPAViSuw97XHef5JJN3tbF4hgk5T4vOXVdldyKHLnbfp05wdEcHfQuIgnGst3tIAvBO7cFJqoR/GwgcdXyc36qAejxOTB9yK4yBBXUX/utufPhIXhczRm0i+Q5GlH4cFB4DzsXg2kWHdO3cjfEItfcDxr0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786967988; c=relaxed/simple; bh=bfCLNI1PRMYnsjfBYgdj4X+i/++F7HwXsOs4ZT1LSGk=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=sAVHIZ8AYqiRWFXqapHzbWVBDNZZ0P4vCMmbBFkBA3Ws+l/URgNxdUriC7qQDN4i4DJCKUK9gWrx5ZMDC1IxH964HTESwYUkiJ8v6j3ftAJlzPxC+TSzfhfTgCiiOnUb+BkZsyo7IEnIlG/NgVdJ69rRc+Mi0jvQCLDDvazq0P0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=LUxQy59N; arc=none smtp.client-ip=209.85.128.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LUxQy59N" Received: by mail-wm1-f42.google.com with SMTP id 5b1f17b1804b1-4980fe6b3beso28647635e9.0 for ; Mon, 17 Aug 2026 04:59:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786967985; x=1787572785; darn=vger.kernel.org; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=UCkUaJwsiIx2/8B6UmuqctR4tL1PfcLhtviHSxv/jPY=; b=LUxQy59NcPTaS03F/TV+DFkuyFXvwbGh8PVJP1SfFSAR+Pv5kEmUDe6mDZlX+JY3BW Xv8Ui+UlacrN6oXSSgUt04WMvBJY94uq0dp4Mv+VH2eE6QTjRLPyi9V4m7OlxbPBZxaf RUp4W3QKozU/DYdPO8BN0qBjr6NBgLKUBg0OJ8Ajyxrb7uGbZVq6wvmefEUK8EXJAGo4 cu+FpcsUa2MNKUfF7p6NrzTTCr7B0Q5X7/QJkf2eJxdAtWQ9mCz3BO+Vihnk0UDumJOI gUi1TUdUis7aKQTa8daCoFec7p3uPolEHTNiFXOEBj536+VUu8br0fhPHy1HQQl9/BPt EeYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786967985; x=1787572785; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=UCkUaJwsiIx2/8B6UmuqctR4tL1PfcLhtviHSxv/jPY=; b=aScyzHzu4gGKCK89sNoC1cxQ7duwhCa1uOvCLlDRDdF4u+J+JbP6zqprnEAG8caJfT wCmfzTKDpw2/PClV63cPSeI18tGbyxR25P2vWyQL/SCXGuTqMMoyO3ty+Nv6SCiNsI25 qAe5mjfKrIZnrLAJSB26At6I0JqKLsAm5yluLwBnq9BbCUj9fP52FUuWplbiBb/Ixxgo LDTi2ipcZp+VTQyDetoSruJbdf9MW8xR5Q12oxEPII1R2z7exkv9twkgznS8ufq0XvVe 0CxWbrCK6jlVc6sMXEzdzOiphPxV0jU7SX1ibng4l7ktIGlAsWhQemuAVJ4MdXMOqxF4 4TAw== X-Forwarded-Encrypted: i=1; AHgh+RqnMudBWhAQgVPJkfJ5cgfnUfpEvKyg/1LiUK4dwqs30qDkMwgAgKAqfyYf8S4yrkvK73h1L64dv/RTGLQ=@vger.kernel.org X-Gm-Message-State: AOJu0YzNmnn9yjB249ANU8fwHVGxPq2ey+it69GQAinSVU8tnPTMY65c pIqBNYpA8SBP5cg4VIXPRs07l9o87vMwB8dpnL4tJrnlllq7gPEYsYa7 X-Gm-Gg: AR+sD10bqFxfQ+5v2U9GOGT4Q++HMr7PkNRgsf8yoETb5kTywYqpbX1KlypIxNef3jx nTLGLbhSDZQeja+TXCCo/3/o/Q6upk4MHnicfWTKRzmxEST9s2ZMH/X0W5ub3wOLR16Jv38wt5k MtevWaCYy5M69SS+6POqC1+3is0y8+7QAkpMVyYxdlITX0g5QMSmoR4itFCj62Iqd73F0pK1S0g uCJkb8LXaxy2dyexkWPziypUB2/xvrWfyNyXMOpefIaCnrfpQenp5kj9yRmMBz1hieK6VLwgslQ zHYjO1uR+iXl1xDBkctX3uKqDezc36AyFKW/cBW+rlU8c2sChs5X8EM11IN5U4QmMZybFoRAFOl n8yO/xkxltjpnyfekG0m86hO0rWRO8lpBsICUpcorvoXeoE6dm8zBKGN4NOgP0U4UQjKi2Guxh5 zvJAhG6m1fpSNhYjzfZa1xkI/Hp+dTqOlnDRNnaWHLN4nEXihC0ZIXNBxG1faCxlSnAE1Sc/8qo omSShn9cDW5qqCKQ/ymGFzFD4YHTUTBp+eyHEaNPCx2jzMVhgcN3SoxaIm2TWGgTHhuzSruz0p1 wqES90gLuqAZgAx91ZcnqvJiXTkKigu4gIpm1aM29lz7Tu4FIqMEPSjE5FEuOxMGYuRKkbv+odU selV1WFfqELfMZtaczH6HckdVcXgRoA0bLMzOXQ6FBz0u8RXXVqn/taYCGjpVTovcFek/mg== X-Received: by 2002:a05:600c:3b90:b0:499:59fd:dbfc with SMTP id 5b1f17b1804b1-49987a4f131mr325063325e9.1.1786967985143; Mon, 17 Aug 2026 04:59:45 -0700 (PDT) Received: from localhost (90-182-112-124.rcp.o2.cz. [90.182.112.124]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-499961098cdsm162556715e9.6.2026.08.17.04.59.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Aug 2026 04:59:45 -0700 (PDT) Date: Mon, 17 Aug 2026 13:59:20 +0200 From: Joshua Crofts To: Jonathan Cameron Cc: Andy Shevchenko , Petar Stepanovic , Akhila Kavi , Prasad Bolisetty , David Lechner , Nuno =?ISO-8859-1?Q?S=E1?= , Andy Shevchenko , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Harshit Shah , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 2/2] iio: adc: add Axiado SARADC driver Message-ID: <20260817135920.0000461d@gmail.com> In-Reply-To: <20260817034541.053dac28@jic23-huawei> References: <20260810-axiado-ax3000-ax3005-saradc-v5-0-bb0df3ecedaa@axiado.com> <20260810-axiado-ax3000-ax3005-saradc-v5-2-bb0df3ecedaa@axiado.com> <20260817034541.053dac28@jic23-huawei> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.51; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 17 Aug 2026 03:45:41 +0100 Jonathan Cameron wrote: > On Mon, 10 Aug 2026 21:27:26 +0300 > Andy Shevchenko wrote: > > > On Mon, Aug 10, 2026 at 07:49:36AM -0700, Petar Stepanovic wrote: > > > Add support for the SARADC controller found on Axiado AX3000 and > > > AX3005 SoCs. > > > > > > The driver supports single-shot voltage reads through the IIO > > > subsystem. The number of available input channels is selected from > > > the SoC match data, allowing AX3000 and AX3005 variants to use the > > > same driver. > > > > ... > > > > > +/* MANUAL_CTRL register fields */ > > > > ^^^ > > > > > +#define AX_SARADC_MANUAL_CTRL_ENABLE BIT(0) > > > +#define AX_SARADC_MANUAL_CTRL_CH_SEL_MASK GENMASK(4, 1) > > > + > > > +#define AX_RESOLUTION_BITS 10 > > > +#define AX_SARADC_CONV_CYCLES 13 > > > +#define AX_SARADC_CONV_DELAY_MARGIN_US 10 > > > + > > > +struct axiado_saradc { > > > + struct regmap *regmap; > > > + struct mutex lock; /* Serializes ADC conversions. */ > > > > Choose the same style for all single-line comments (here is a period present > > while in the above, for instance, there is none). > > Hmm. It is inconsistent in other places, but those two are arguably > correct. The second is a sentence, the first is not (no verb) > > Anyhow, I did a sweep for other sentences and not all of them have > periods. Given the rest of the driver looked fine to me and the other > suggestions Andy made are easy to apply. > > Applied with the following diff to the testing branch of iio.git > Note I'm fine with rebasing that (and will do on rc1 once that's available) > so extra tags or review comments are easy to add for a few weeks at least. > > Mostly I did this just to reduce how many patch sets were undergoing > revisions... I'm still over a 100 emails to read and run out of > time for today. > > Jonathan > > > diff --git a/drivers/iio/adc/axiado_saradc.c b/drivers/iio/adc/axiado_saradc.c > index daa98dd2f176..699ee31616fc 100644 > --- a/drivers/iio/adc/axiado_saradc.c > +++ b/drivers/iio/adc/axiado_saradc.c > @@ -34,10 +34,10 @@ > #define AX_SARADC_GLOBAL_CTRL_PD BIT(2) > #define AX_SARADC_GLOBAL_CTRL_ENABLE BIT(0) > > -/* GLOBAL_CTRL SAMPLE_MASK field value: 0 selects 16 samples */ > +/* GLOBAL_CTRL SAMPLE_MASK field value: 0 selects 16 samples. */ > #define AX_SARADC_GLOBAL_CTRL_SAMPLE_16 0 > > -/* GLOBAL_CTRL MODE_MASK field value: 1 selects manual mode */ > +/* GLOBAL_CTRL MODE_MASK field value: 1 selects manual mode. */ > #define AX_SARADC_GLOBAL_CTRL_MODE_MANUAL 1 > > /* MANUAL_CTRL register fields */ > @@ -76,16 +76,15 @@ static int axiado_saradc_conversion(struct axiado_saradc *info, > > guard(mutex)(&info->lock); > > - /* Select the channel to be used and trigger conversion */ > + /* Select the channel to be used and trigger conversion. */ > ret = regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG, > AX_SARADC_MANUAL_CTRL_ENABLE | > FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, chan->channel)); > if (ret) > return ret; > > - /* Hardware requires 13 conversion cycles at clk_rate */ > - usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC, > - info->clk_rate); > + /* Hardware requires 13 conversion cycles at clk_rate. */ > + usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC, info->clk_rate); > fsleep(usecs + AX_SARADC_CONV_DELAY_MARGIN_US); > > ret = regmap_read(info->regmap, AX_SARADC_DOUT_REG, ®val); > @@ -220,7 +219,7 @@ static int axiado_saradc_probe(struct platform_device *pdev) > > soc_data = device_get_match_data(dev); > if (!soc_data) > - return dev_err_probe(dev, -EINVAL, "failed to get match data\n"); > + return dev_err_probe(dev, -ENODATA, "failed to get match data\n"); > > > > > + unsigned long clk_rate; > > > + int vref_uV; > > > +}; > > > > ... > > > > > +static int axiado_saradc_conversion(struct axiado_saradc *info, > > > + struct iio_chan_spec const *chan, int *val) > > > +{ > > > + unsigned long usecs; > > > + unsigned int regval; > > > + int ret; > > > + > > > + guard(mutex)(&info->lock); > > > + > > > + /* Select the channel to be used and trigger conversion */ > > > + ret = regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG, > > > + AX_SARADC_MANUAL_CTRL_ENABLE | > > > + FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, chan->channel)); > > > + if (ret) > > > + return ret; > > > + > > > + /* Hardware requires 13 conversion cycles at clk_rate */ > > > > > + usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC, > > > + info->clk_rate); > > > > I think it's okay to have this on a single line (83 characters). > > > > > + fsleep(usecs + AX_SARADC_CONV_DELAY_MARGIN_US); > > > + > > > + ret = regmap_read(info->regmap, AX_SARADC_DOUT_REG, ®val); > > > + > > > + /* Best effort to stop manual conversion. */ > > > + regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG, 0); > > > + > > > + if (ret) > > > + return ret; > > > + > > > + *val = regval & GENMASK(AX_RESOLUTION_BITS - 1, 0); > > > + > > > + return 0; > > > +} > > > > ... > > > > > + soc_data = device_get_match_data(dev); > > > + if (!soc_data) > > > + return dev_err_probe(dev, -EINVAL, "failed to get match data\n"); > > > > -ENODATA > > > > With the diff Jonathan sent in mind, Reviewed-by: Joshua Crofts -- Kind regards, Joshua Crofts