Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Pop, Cristian" <Cristian.Pop@analog.com>
To:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>
CC:     "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC,
 DAC
Thread-Topic: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC,
 DAC
Thread-Index: AQHWW0JTA3aqb5CuhECQCRTQAzQghqkJ75sAgAaT34CABk8VcA==
Date:   Fri, 24 Jul 2020 15:23:21 +0000
Message-ID: <SN2PR03MB2317AC79D13C3A89882534F4E7770@SN2PR03MB2317.namprd03.prod.outlook.com>
References: <20200716072737.9535-1-cristian.pop@analog.com>
        <9bd0363c-e8fb-a36f-a107-0afd8f7851d0@metafoo.de>
 <20200720145225.4f5d552b@archlinux>
In-Reply-To: <20200720145225.4f5d552b@archlinux>
Accept-Language: ro-RO, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9bd680c8-4bd2-454d-6c4c-08d82fe57ffc
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2020 15:23:21.2861
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MM462fhb3d00cicSvgTg6N/pdrjvBOGV4cJBlk4QgJfz//aAB4vjaOdeZeQzdihmg8xdzznz8QFI6Zq/Ud9CgQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR03MB4558
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-07-24_05:2020-07-24,2020-07-24 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0
 priorityscore=1501 mlxscore=0 suspectscore=0 clxscore=1011 spamscore=0
 phishscore=0 adultscore=0 lowpriorityscore=0 bulkscore=0 mlxlogscore=999
 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2006250000 definitions=main-2007240120
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, July 20, 2020 4:52 PM
> To: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Pop, Cristian <Cristian.Pop@analog.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit =
ADC,
> DAC
>=20
> [External]
>=20
> On Thu, 16 Jul 2020 11:25:36 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>=20
> > On 7/16/20 9:27 AM, Cristian Pop wrote:
> > > Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)
> >
> > Very sneaky way of introducing a iio-gpio-proxy driver to be able to
> > access GPIOs through libiio ;). I'm not really a fan of the whole idea.
> >
> > But either way I think this needs a better description of what 1-bit
> > converters are and how they are used.
> I'll second that.  If we want to do this, I'd much rather seeing as an ex=
plicit gpio
> to IIO bridge driver.
>=20
> If there is a comparator on an ADC pin, then the analog characteristics o=
f that
> need describing.  There might be some argument in favour if that was done
> and hence we had scale etc provided for the channel.
>=20
> Given this is really just putting a new interface on gpios please cc the =
gpio
> maintainer / list for future versions.
>=20
> Thanks,
>=20
> Jonathan
>=20
>=20
> >
> > >
> > > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > > ---
> > >   drivers/iio/addac/one-bit-adc-dac.c | 229
> ++++++++++++++++++++++++++++
> > >   1 file changed, 229 insertions(+)
> > >   create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> > >
> > > diff --git a/drivers/iio/addac/one-bit-adc-dac.c
> > > b/drivers/iio/addac/one-bit-adc-dac.c
> > > new file mode 100644
> > > index 000000000000..8e2a8a09fedb
> > > --- /dev/null
> > > +++ b/drivers/iio/addac/one-bit-adc-dac.c
> > > @@ -0,0 +1,229 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices ONE_BIT_ADC_DAC
> > > + * Digital to Analog Converters driver
> > > + *
> > > + * Copyright 2019 Analog Devices Inc.
>=20
> Probably update to 2020 or 2019-2020
>=20
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/gpio/consumer.h>
> > > +
> > > +enum ch_direction {
> > > +	CH_IN,
> > > +	CH_OUT,
> > > +};
> > > +
> > > +struct one_bit_adc_dac_state {
> > > +	struct platform_device  *pdev;
> > > +	struct gpio_descs       *in_gpio_descs;
> > > +	struct gpio_descs       *out_gpio_descs;
> > > +};
> > > +
> > > + #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)
> 	\
> > > +	{								\
> > > +		.type =3D IIO_VOLTAGE,					\
> > > +		.indexed =3D 1,						\
> > > +		.channel =3D idx,						\
> > > +		.info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW),
> 	\
> > > +		.output =3D direction,					\
> > > +	}
>=20
> Macro only used in one place, I'd not bother with the macro.
>=20
> > > +
> > > +static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
> > > +	const struct iio_chan_spec *chan, int *val, int *val2, long info)
> > > +{
> > > +	struct one_bit_adc_dac_state *st =3D iio_priv(indio_dev);
> > > +	int in_num_ch =3D 0, out_num_ch =3D 0;
> > > +	int channel =3D chan->channel;
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch =3D st->in_gpio_descs->ndescs;
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch =3D st->out_gpio_descs->ndescs;
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		if (channel < in_num_ch) {
> > > +			*val =3D gpiod_get_value_cansleep(
> > > +				st->in_gpio_descs->desc[channel]);
> > > +		} else {
> > > +			channel -=3D in_num_ch;
> > > +			*val =3D gpiod_get_value_cansleep(
> > > +				st->out_gpio_descs->desc[channel]);
> > > +		}
> > > +		return IIO_VAL_INT;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan,
> > > +			    int val,
> > > +			    int val2,
> > > +			    long info)
> > > +{
> > > +	struct one_bit_adc_dac_state *st =3D iio_priv(indio_dev);
> > > +	int in_num_ch =3D 0, out_num_ch =3D 0;
> > > +	int channel =3D chan->channel;
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch =3D st->in_gpio_descs->ndescs;
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch =3D st->out_gpio_descs->ndescs;
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		if (channel < in_num_ch) {
> > > +			gpiod_set_value_cansleep(
> > > +				st->in_gpio_descs->desc[channel], val);
> >
> > How can we set a value on an input GPIO?
> >
> > > +		} else {
> > > +			channel -=3D in_num_ch;
> > > +			gpiod_set_value_cansleep(
> > > +				st->out_gpio_descs->desc[channel], val);
> > > +		}
> > > +
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static const struct iio_info one_bit_adc_dac_info =3D {
> > > +	.read_raw =3D &one_bit_adc_dac_read_raw,
> > > +	.write_raw =3D &one_bit_adc_dac_write_raw, };
> > > +
> > > +static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
> > > +					struct iio_chan_spec *channels,
> > > +					const char *propname,
> > > +					int num_ch,
> > > +					enum ch_direction direction,
> > > +					int offset)
> > > +{
> > > +	struct one_bit_adc_dac_state *st =3D iio_priv(indio_dev);
> > > +	const char **gpio_names;
> > > +	int ret, i;
> > > +
> > > +	if (num_ch <=3D 0)
> > > +		return 0;
> > > +
> > > +	gpio_names =3D devm_kcalloc(indio_dev->dev.parent,
> > > +					num_ch,
> > > +					sizeof(char *),
> > sizeof(*gpio_names). It might be better to use normal kcalloc, kfree
> > here since you only use it in this function.
>=20
> Definitely.
>=20
> > > +					GFP_KERNEL);
> > > +	if (!gpio_names)
> > > +		return -ENOMEM;
> > > +
> > > +	ret =3D device_property_read_string_array(&st->pdev->dev,
> > > +					propname,
> > > +					gpio_names,
> > > +					num_ch);
> Take advantage of the new longer acceptable line length (100 chars) to ma=
ke
> some of these more readable.
>=20
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	for (i =3D 0; i < num_ch; i++) {
> > > +		channels[i] =3D (struct
> iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +
> > > +							offset,
> > > +							direction);
> > > +		channels[i].extend_name =3D gpio_names[i];
> > I think we want to avoid using extend_name in new drivers because it
> > makes for a very clumsy ABI. We should add a label property like we
> > have for the device for channels to have a symbolic name of the channel=
.
The current dts looks like this:
one-bit-adc-dac@0 {
				in-gpios =3D <&gpio 17 0>, <&gpio 27 0>;
				in-gpio-names =3D "i_17", "i_27";
				out-gpios =3D <&gpio 23 0>, <&gpio 24 0>;
				out-gpio-names =3D "o_23", "o_24";
			};
Resulting in channels:
in_voltage0_i_17_raw
in_voltage1_i_27_raw
out_voltage2_o_23_raw
out_voltage3_o_24_raw
If we want to lose extend_name, please provide an example for using labels.
How the dts should look like, how do I use it in the driver?
>=20
> Agreed. It keeps getting talked about, but no patches yet IIRC.
>=20
> I'd expect separate indexing for input and output channels.
> The in / out distinguishes them.
>=20
> in_voltage0_raw
> out_voltage0_raw etc
>=20
>=20
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int one_bit_adc_dac_parse_dt(struct iio_dev *indio_dev) {
> > > +	struct one_bit_adc_dac_state *st =3D iio_priv(indio_dev);
> > > +	struct iio_chan_spec *channels;
> > > +	int ret, in_num_ch =3D 0, out_num_ch =3D 0;
> > > +
> > > +	st->in_gpio_descs =3D devm_gpiod_get_array_optional(&st->pdev->dev,
> > > +						"in", GPIOD_IN);
> > > +	if (IS_ERR(st->in_gpio_descs))
> > > +		return PTR_ERR(st->in_gpio_descs);
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch =3D st->in_gpio_descs->ndescs;
> > > +
> > > +	st->out_gpio_descs =3D devm_gpiod_get_array_optional(&st->pdev-
> >dev,
> > > +						"out", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(st->out_gpio_descs))
> > > +		return PTR_ERR(st->out_gpio_descs);
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch =3D st->out_gpio_descs->ndescs;
> > > +
> > > +	channels =3D devm_kcalloc(indio_dev->dev.parent, (in_num_ch +
> out_num_ch),
> > > +				sizeof(struct iio_chan_spec), GFP_KERNEL);
> >
> > sizeof(*channels) to avoid accidentally using the wrong type.
> >
> > > +	if (!channels)
> > > +		return -ENOMEM;
> > > +
> > > +	ret =3D one_bit_adc_dac_set_ch(indio_dev, &channels[0],
> > > +					"in-gpio-names", in_num_ch,
> > > +					CH_IN, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret =3D one_bit_adc_dac_set_ch(indio_dev, &channels[in_num_ch],
> > > +					"out-gpio-names", out_num_ch,
> > > +					CH_OUT, in_num_ch);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	indio_dev->channels =3D channels;
> > > +	indio_dev->num_channels =3D in_num_ch + out_num_ch;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int one_bit_adc_dac_probe(struct platform_device *pdev) {
> > > +	struct iio_dev *indio_dev;
> > > +	struct one_bit_adc_dac_state *st;
> > > +	int ret;
> > > +
> > > +	indio_dev =3D devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	st =3D iio_priv(indio_dev);
> > > +	st->pdev =3D pdev;
> > > +	indio_dev->dev.parent =3D &pdev->dev;
> > parent assignment should not be needed thanks to Alex's work.
> > > +	indio_dev->name =3D "one-bit-adc-dac";
> > > +	indio_dev->modes =3D INDIO_DIRECT_MODE;
> > > +	indio_dev->info =3D &one_bit_adc_dac_info;
> > > +
> > > +	ret =3D one_bit_adc_dac_parse_dt(indio_dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	platform_set_drvdata(pdev, indio_dev);
> >
> > There does not seem to be a matching get_drvdata() anywhere so this is
> > not needed.
> >
> > > +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > > +}
> > > +
> > > +static const struct of_device_id one_bit_adc_dac_dt_match[] =3D {
> > > +	{ .compatible =3D "adi,one-bit-adc-dac" },
>=20
> This is definitely not ADI specific.  It also currently looks like a poli=
cy thing
> rather than truely defined by the wiring.
> Hence I'd kind of expect to see it instantiated via configfs rather than =
a dt
> binding.
>=20
> Note this would definitely need a dt binding doc if done this way.
> My gut feeling is that as it stands, it would go nowhere.
>=20
>=20
>=20
> > > +	{},
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
> > > +
> > > +static struct platform_driver one_bit_adc_dac_driver =3D {
> > > +	.driver =3D {
> > > +		.name =3D "one-bit-adc-dac",
> > > +		.of_match_table =3D one_bit_adc_dac_dt_match,
> > > +	},
> > > +	.probe =3D one_bit_adc_dac_probe,
> > > +};
> > > +
> > > +module_platform_driver(one_bit_adc_dac_driver);
> > > +
> > > +MODULE_AUTHOR("Cristian Pop <cristian.pop@analog.com>");
> > > +MODULE_DESCRIPTION("Analog Devices ONE_BIT_ADC_DAC");
> > > +MODULE_LICENSE("GPL v2");
> >
> >