Date:   Sat, 27 Nov 2021 17:28:09 +0000
From:   Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
To:     Antoniu Miclaus <antoniu.miclaus@analog.com>
Cc:     <robh+dt@kernel.org>, <linux-iio@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 1/3] iio: frequency: admv1013: add support for
 ADMV1013
Message-ID: <20211127172646.22848575@jic23-huawei>
In-Reply-To: <20211123115336.65827-1-antoniu.miclaus@analog.com>
References: <20211123115336.65827-1-antoniu.miclaus@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 23 Nov 2021 13:53:34 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> The ADMV1013 is a wideband, microwave upconverter optimized
> for point to point microwave radio designs operating in the
> 24 GHz to 44 GHz radio frequency (RF) range.
> 
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADMV1013.pdf
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu,

Clearly you are exploring new territory here so thanks for persisting and
I think we are getting close to where we want to be with this.


Other than one question over whether the generated ABI using
extinfo matches what you have in the docs, the remaining changes are all ABI
/ dt binding review related which will have impacts in here as well.

hmm. Side note, my kernel.org email is blocking for some reason so sending
using a different account. Hopefully it will make it!

> +
> +#define _ADMV1013_EXT_INFO(_name, _shared, _ident) { \
> +		.name = _name, \
> +		.read = admv1013_read, \
> +		.write = admv1013_write, \
> +		.private = _ident, \
> +		.shared = _shared, \
> +}
> +
> +static const struct iio_enum admv1013_mode_enum = {
> +	.items = admv1013_modes,
> +	.num_items = ARRAY_SIZE(admv1013_modes),
> +	.get = admv1013_get_mode,
> +	.set = admv1013_set_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info admv1013_ext_info[] = {
> +	_ADMV1013_EXT_INFO("i", IIO_SEPARATE, ADMV1013_RFMOD_I),
> +	_ADMV1013_EXT_INFO("q", IIO_SEPARATE, ADMV1013_RFMOD_Q),
> +	IIO_ENUM("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
> +	IIO_ENUM_AVAILABLE("freq_mode", IIO_SHARED_BY_ALL, &admv1013_mode_enum),
> +	{ },
> +};
> +
> +#define ADMV1013_CHAN_PHASE(_channel, _channel2, _admv1013_ext_info) {		\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.output = 0,						\
> +	.indexed = 1,						\
> +	.channel2 = _channel2,					\
> +	.channel = _channel,					\
> +	.differential = 1,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\
> +	.ext_info = _admv1013_ext_info,				\
> +	}
> +
> +#define ADMV1013_CHAN_CALIB(_channel, _admv1013_ext_info) {\
> +	.type = IIO_ALTVOLTAGE,					\
> +	.output = 0,						\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\

This has me a little confused.  How do these map to the
0-1_phase_i etc that you have in the ABI docs?
Unless I'm completely forgetting how this works this will give us
one attribute for phase and another one for _i

Perhaps it's worth providing a sysfs directory listing for this one
given it's rather unusual.

> +	.ext_info = _admv1013_ext_info,				\
> +	}
> +
> +static const struct iio_chan_spec admv1013_channels[] = {
> +	ADMV1013_CHAN_PHASE(0, 1, admv1013_ext_info),
> +	ADMV1013_CHAN_CALIB(0, admv1013_ext_info),
> +	ADMV1013_CHAN_CALIB(1, admv1013_ext_info),
> +};
> +