From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vs1-f53.google.com (mail-vs1-f53.google.com [209.85.217.53]) (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 785042DECA0 for ; Sat, 7 Mar 2026 21:18:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.217.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772918289; cv=none; b=jE25o5LSqmCuVHS6WIycwrNAp9gunt6iW9wcqsQ1mpKqaqM3Uj68yMRdiJjXtf8RkY7mYDYoaU3edu2b1ImvXM8EI5HkNESSW+N9y7UWONqQ+zSCKrADRdv6B89VbfXvvDYjkdLvTDLLCNLxg6wQw7arsa1V09iXJ+gW7EDMwYw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772918289; c=relaxed/simple; bh=MhbrdTi9gVFuh6N/nDA7yix7KlLsu2/Qx2LlBF0gga4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=KuLHCiaPc0Ei5sO6NHaqD/hCqkphDbcOdCopAVIUVGbddel15REIokgGBXUz7VQH41jsUDriUGIzbxL9p/olTx93aHVIMMF8HPxKdHnT5ZLYYnc913aXdKa9sf2k47a2w9IHRvopbZTW6l2aKu7bTtGFYZQCmdewn7tUVVj1jII= 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=J+zv5dQR; arc=none smtp.client-ip=209.85.217.53 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="J+zv5dQR" Received: by mail-vs1-f53.google.com with SMTP id ada2fe7eead31-5ffea5acd19so1163378137.1 for ; Sat, 07 Mar 2026 13:18:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1772918286; x=1773523086; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=OLKpt64yKdOOJysGkN8Ulwp3Owm7MfQ5+nKTzRfPKRU=; b=J+zv5dQReoWKXE0mrja8W/CMPYYSCCi7q3GmBX8aE/SRTJIKoPAUhUWCaLsSDubIgz YEVy0yAT2XLezNaYFgrmYapg9Qdo0dQgKoXfYH/KqiVzL4mVOPDsGgwVvtaFDAqqNtB0 Ea1P+SO42G+UuWYuIt/kB4J7vt4zARbFQfOBnCcxFiGTJhwyG/rYqNWu/290nFj1SDB6 gnEsEZLKwEBw8lpglWkZjN0hCT9L5K7wnirPlOBbifiqL3ezTVkzF8JoyT4LTmWmoTyM 6yNolm5fxlG9JwrryUVnvFJ9VVNzDgQsDU+awHEuDhUBpNkTzlJpNMfTLoVujZX9q5Hw 2RNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772918286; x=1773523086; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OLKpt64yKdOOJysGkN8Ulwp3Owm7MfQ5+nKTzRfPKRU=; b=JDRu9qB114+LE8JDbgTakKehjAermy8QVxaFF0l89fiZNyMk6KUcripz8B+uJvJEkb KJT0ET4IUVAh/7ugdAtb+JRVt9nQJ5VDQqQeWxQ7mMomAlRGRqNjAyTUFk99l+qkpkCX lz/avb6rbxU/mH7w3XOcgYTOdKd1wRqn2co9flBI9hL8FRMMJScc1FXfgllmxTi5l5xZ kC7VnUlADiLXSwzNQlEW6i2AfKT5RnIu3RBx1Wn1cyAP3MTzQEiwcfW8gAB27qrTqp4q K6BMEEUWiQNjQQC9jyxrPB9dMRN6RszIRvZQ831JKgRlNX7/+5Qun820+p0LYu4qknfE XPTg== X-Forwarded-Encrypted: i=1; AJvYcCV/cLb70adtB5E/nzE1f8EXiZR/1oUqciJfXkXYNIlqEaxu1n99KqXiVXL+RudMzIlwsVxW+vvhvi5evao=@vger.kernel.org X-Gm-Message-State: AOJu0YyrFMMkW0WiydS3B//w4LzgM7WoTOIlLxDouFXuX0wSkJlxypAm w+eSQfV1UmlVIIzkeLIIaSfLUdbPNKDuLval+pEvDYyEJ8US54fMSs0g X-Gm-Gg: ATEYQzw0jKliGPTnLuCer5ErFho3Lm8w98idRhKnIA/voxi06ZhxfHnKM/mzbrnCOkI 4XaTOsfT6XPh7ickqlrmZFpB+afHYIJywc+V3pah/8x/nqhIqc/Q20E4yqnwfN0HgOB05U1gMq6 VlZnY0qKnH0JPF+vZym72bklhQLnMe0lBuV9QkL3Y1hvVwJfLaLUXwslBxSGya27qvDafdB2e2s 5FxSG8To6Iz4SDnwskra2SZ7MhoiH2bpL33vHE+j+y1zc2ZwZLt7xnVDBePZs8qlHPiqjq5IHUy cV8tyho1T4p+RR5/+AghuOLsg9F3tskj8x1GbELcWRjUS73bNwW/Qbaa5FGzE/lke77Z7Lw8enj 1FI1zMxMRaLPdL7SjMwC8k9JDsBFoM8UcIzKpqOM1B2Q714KqjSoZVswQ6OlKr75zWmHV7MGK+G Tj0SP1hEcuO+fuIY1aYbcgBpPJm/Q3OlM= X-Received: by 2002:a05:6102:26d4:b0:5ff:c64d:2283 with SMTP id ada2fe7eead31-5ffe61cf0b4mr2755999137.30.1772918286319; Sat, 07 Mar 2026 13:18:06 -0800 (PST) Received: from localhost ([2804:30c:966:7b00:fd18:61b8:43a7:4049]) by smtp.gmail.com with ESMTPSA id ada2fe7eead31-5ffe8bd6d6bsm5547413137.10.2026.03.07.13.18.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 07 Mar 2026 13:18:05 -0800 (PST) Date: Sat, 7 Mar 2026 18:18:41 -0300 From: Marcelo Schmitt To: Bhargav Joshi Cc: lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org, dlechner@baylibre.com, nuno.sa@analog.com, andy@kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, tomasborquez13@gmail.com Subject: Re: [RFC PATCH] staging: iio: ad9832: modernize ABI and remove dds.h dependency Message-ID: References: <20260305210347.120446-1-rougueprince47@gmail.com> 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-Disposition: inline In-Reply-To: +CC: tomasborquez13@gmail.com for the opportunity to comment about the proposed updates. On 03/07, Marcelo Schmitt wrote: > Hello Bhargav, > > On 03/06, Bhargav Joshi wrote: > > The AD9832 driver currently relies on legacy custom IIO macros defined > > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS) > > and, more importantly, exposes a non-standard sysfs ABI (e.g., > > frequency0, frequency1, phase0-3) directly to user space. > > > > This patch removes the custom macros and migrates the driver to standard > > IIO API mechanisms: > > - Standard attributes (frequency, phase) now use info_mask_separate. > > - Non standard specific toggles (frequencysymbol, phasesymbol, > > pincontrol) have been migrated to an ext_info array. > > - Remove dds.h header dependency. > > - Pointless frequency_scale and phase_scale attributes are dropped as > > suggested by Jonathan in > > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/ > > > > NOTE: This patch introduces an intentional ABI changes. The non-standard > > attributes (out_altvoltage0_frequency0, etc.) have been removed. They > > are replaced by standard attributes (out_altvoltage0_frequency and > > out_altvoltage0_phase). Routing to correct register while writing is > > handled by checking currently active frequencysymbol or phasesymbol. > > > > Testing: This patch has been strictly compile-tested. I do not have > > access to physical AD9832 hardware. I am submitting this as an RFC to > > see if these changes are acceptable, and to ask if someone with physical > > hardware could test thisg and provide a Tested-by tag. > > > > Signed-off-by: Bhargav Joshi > > --- > > This patch is heavily inspired from discussions in following > > thread. > > https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@gmail.com/ > > Thanks for making that clear. Still, a change log highlighting the main > differences between yours and Tomas' patches would have been appreciated. > > By the way, I'm assuming Tomas is fine with you caring on the driver update? > > More comments inline. > > > > Since this is an RFC please let me know if these changes are acceptable. > > > > drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++---------- > > 1 file changed, 80 insertions(+), 55 deletions(-) > > > > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c > > index b87ea1781b27..066a1b9ee8d5 100644 > > --- a/drivers/staging/iio/frequency/ad9832.c > > +++ b/drivers/staging/iio/frequency/ad9832.c > ... > > -/* > > - * see dds.h for further information > > - */ > > +#define AD9832_EXT_INFO(_name, _ident) { \ > > + .name = _name, \ > > + .write = ad9832_write_ext_info, \ > Why no .read procedure? I think at least for the > out_enable/out_altcurrent0_enable attribute we would want a read function. > > > + .private = _ident, \ > > + .shared = IIO_SEPARATE, \ > > +} > > > > -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM); > > -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM); > > -static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM); > > -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */ > > - > > -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H); > > -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H); > > -static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H); > > -static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H); > > -static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, > > - ad9832_write, AD9832_PHASE_SYM); > > -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/ > > - > > -static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL, > > - ad9832_write, AD9832_PINCTRL_EN); > > -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL, > > - ad9832_write, AD9832_OUTPUT_EN); > > - > > -static struct attribute *ad9832_attributes[] = { > > - &iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr, > > - &iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_phase2.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_phase3.dev_attr.attr, > I'd try to split this patch into smaller ones. > One for updating the frequency and phase channels. > > > - &iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr, > One patch for dropping the _scale attributes. > > > - &iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr, > > - &iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr, > > - NULL, > > +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = { > > + AD9832_EXT_INFO("pincontrol_en", AD9832_PINCTRL_EN), > Why did you drop the comment about making pincontrol_en a DT property? > Nevertheless, pincontrol_en is another piece I'd put in a separate patch. > > > + AD9832_EXT_INFO("frequencysymbol", AD9832_FREQ_SYM), > > + AD9832_EXT_INFO("phasesymbol", AD9832_PHASE_SYM), > Maybe I'm missing something but this doesn't seem to follow the suggested ABI. > https://lore.kernel.org/linux-iio/20251221194358.3284acb4@jic23-huawei/ > Why did you chose this particular naming for the ABI? > > > + AD9832_EXT_INFO("out_enable", AD9832_OUTPUT_EN), > I think it is okay to change the enable from device attribute to channel > attribute in this case since it's staging and this device has only one channel. > Though, I believe we should call it out_altcurrent0_enable, as proposed on > Tomas' set. I'd also set this on a separate patch. > > > + { } > > }; > > > > -static const struct attribute_group ad9832_attribute_group = { > > - .attrs = ad9832_attributes, > > +static const struct iio_chan_spec ad9832_channels[] = { > > + { > > + .type = IIO_ALTVOLTAGE, > Maybe I've missed something. Didn't the previous review reached the conclusion > that the channels should be IIO_ALTCURRENT? The datasheet it documents IOUT as > current output. > > > + .indexed = 1, > > + .channel = 0, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) | > > + BIT(IIO_CHAN_INFO_PHASE), > > + .ext_info = ad9832_ext_info, > > + }, > > }; > > > > static const struct iio_info ad9832_info = { > > - .attrs = &ad9832_attribute_group, > > + .write_raw = ad9832_write_raw, > No .read_raw ? > > > };