Received-SPF: pass (google.com: domain of linux-kernel+bounces-194324-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: "Nechita, Ramona" <Ramona.Nechita@analog.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        Jonathan Cameron
	<jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Hennerich,
 Michael" <Michael.Hennerich@analog.com>,
        Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
        "Sa, Nuno" <Nuno.Sa@analog.com>,
        Marius
 Cristea <marius.cristea@microchip.com>,
        "Schmitt, Marcelo"
	<Marcelo.Schmitt@analog.com>,
        Maksim Kiselev <bigunclemax@gmail.com>,
        Ivan
 Mikhaylov <fr0st61te@gmail.com>,
        Marcus Folkesson
	<marcus.folkesson@gmail.com>,
        Liam Beguin <liambeguin@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] drivers: iio: adc: add support for ad777x family
Thread-Topic: [PATCH] drivers: iio: adc: add support for ad777x family
Thread-Index: AQHarD+43ERipWM1gkKh9ZW8mJCjp7GjTUqAgAsEBnA=
Date: Wed, 29 May 2024 15:01:06 +0000
Message-ID: 
 <SN6PR03MB432071701D6A19C3B4C0C33FF3F22@SN6PR03MB4320.namprd03.prod.outlook.com>
References: <20240522120005.18197-1-ramona.nechita@analog.com>
 <Zk3-qxCb0zfR440Q@smile.fi.intel.com>
In-Reply-To: <Zk3-qxCb0zfR440Q@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?gV1CUSwNN6z3xLCbQJxC2WkqWTxhCBvcMAjpKf3Ht5dFj4XcF/u4mUW+2ZRH?=
 =?us-ascii?Q?qUlAjbT6RRkMTqs46HapZOr9c75oudqyQCxEmzc4JimCGxYAmMbCv7vt7lPb?=
 =?us-ascii?Q?zONxym+cdZXy2PBozjzWbhYKpS9Ch0Q2j1MHTQgOHV5A3OK87lOIzyuUquhf?=
 =?us-ascii?Q?uCrXkQkC+OJ23Z0kFxepAB0I7yt8IHnY+FBlpwNjaHfDvJYgESLmWuoPV+7s?=
 =?us-ascii?Q?71+KCRBqXpFy2UnQjN/Uj4nzy8kYPBtt7aKaLtB2Lf270qALZhRMvjA3RzPa?=
 =?us-ascii?Q?Y/MJ18wR3QC0pnv15y3EkXvtRpg7apBuzbDB4RfAj9CX01L/0OGKGfd/Usdw?=
 =?us-ascii?Q?qECEbmKPxYS+UnuEo1F7UbGCopG/BPOKE/bK4QpOg956RiqNXuCM+qA+9pge?=
 =?us-ascii?Q?jOycPyg13Dd4GmqSYj84Ag88raqz2i2BIxHGZ5ARhSysHVn0nCpw/Nt1VD9O?=
 =?us-ascii?Q?Vh051nr42zSA4mbiF46BWhHBo7megxSZDmkVDitwcxWMHJV3uL2DZVO2iBNT?=
 =?us-ascii?Q?VV7ljaOIHgfE22arP9/L9ptiikL3pxQJo45mw6dfFz9oGLNxuU4MXmH2RTa9?=
 =?us-ascii?Q?MWFTpL9emwt5pvbxVFAlazZQIFOUqXggM4IrvCsdQcV0bbkCJvsbDgSaTD6V?=
 =?us-ascii?Q?qKefULvKkKLJZWqtUYmryOK3NV8QaFvFsf+ykn2vDmZ7B8IwYBzEWnRN0nQP?=
 =?us-ascii?Q?OmGTvqO3Usar23OwlRDmUGbAGJmX8s1Q/DbHURS+CK1T2JoaDi2lZsvYCLe/?=
 =?us-ascii?Q?7q64h5kMTy90QNr94x3oZzzZ5+k7SXjMUDSgJ49LuAhqDoT5c7kZ9kosurRG?=
 =?us-ascii?Q?FF9h1h4fA+lv58YK1dQMPP/QTkafDhkZCwrskIRIUTkNcA6q33XHzKsl2Kj5?=
 =?us-ascii?Q?SQFoLpKMOiTOWr2QOk5FNV6jskiOCdElkeuu07FIRes0NbMO/7sQN4XkeaYq?=
 =?us-ascii?Q?sCQC+D7HMfw/25rgpLOBul+yP0ADx58mBbN02tBZNraZtnwkY/WsfgtYSYSe?=
 =?us-ascii?Q?L7kwcTyXoUZrB3+T7n/kz5yD5xS4NBc0n03tW4JeHZv6dVvpR1uNcW4g+8wl?=
 =?us-ascii?Q?lMNsgLyLvo/cbGkvIgbQE3iVpykuQ089DxWGAlP83gGphd2yQfuK/eKbLrJi?=
 =?us-ascii?Q?fwJquj2VAwgXYf2uLdkUNJHF2l7LE5wxNUEdJxgoqAe5dgsfpsSXzoZihGmx?=
 =?us-ascii?Q?JgJw+MrjbcLmoQVCu68hg7ImZNTbmKtNshpl5eqvJ4Rp3Ugzn+fJyNDLzFZ8?=
 =?us-ascii?Q?M3dEfzHMoQjXr5jta4psci7C+x+4RX1XKkTfzXEqoABE+HbCZGSDo6PXHeWd?=
 =?us-ascii?Q?cEI76Mp1F2OAbym9fdLkAUhqYRkDtR3A0Bw/CE2boeGmNp7DRMTazdrn81bq?=
 =?us-ascii?Q?RBhY2XA6KketlTAw5r6JEolArsn1YBIHGqTwROIaf+4WUaeIZTB5yNp7+SXh?=
 =?us-ascii?Q?ytZZ7m4lzovje1LT7dvKIuj0RAPwnvNgMFtJzhcs0cqCWwI8vQA77omrfA2b?=
 =?us-ascii?Q?rEHswhz5H7Btvg/kyx9jt3dFY7/kLaA+Odyx+FE7TtCXQ3yvd1NESoL0C2/k?=
 =?us-ascii?Q?irVybltmNmMlvMxC8AF8PZrK0Hk+/eIJcvy0Agf3?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: analog.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR03MB4320.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c8f38b10-4293-4ba3-43df-08dc7ff02abd
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 May 2024 15:01:06.5176
 (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: pA1eQwo0jno8HbPd8ZkpZknhQO72t/mpr2ywYeLtOQvlzQYUeJjvhqKjOjxu8tTV8NKrCbeRkMq61X4AJzx4YyUMfQwzakczntRx2Q77OV4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR03MB7086
X-Proofpoint-GUID: 2OwRLKIVvF017WC59Bnsj78Dj_JpLaSE
X-Proofpoint-ORIG-GUID: 2OwRLKIVvF017WC59Bnsj78Dj_JpLaSE
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-29_11,2024-05-28_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0
 lowpriorityscore=0 bulkscore=0 priorityscore=1501 impostorscore=0
 phishscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 spamscore=0
 clxscore=1015 suspectscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.19.0-2405170001 definitions=main-2405290103

Hello,=20

Thank you for the review. I implemented most of your comments, excepting so=
me things that were slightly unclear to me or I had a different explanation=
 I left a couple of comments below, and I will send a new patch in a separ=
ate email.

>On Wed, May 22, 2024 at 02:59:53PM +0300, ranechita wrote:
>> Added support for ad7770,ad7771,ad7779 ADCs. The data is streamed only=20
>> on the spi-mode, without using the data lines.
>
>> ---
>
>Please, explain here, in the comment area, why any existing driver can not=
 be reused (extended) for these ADCs.
>
>...
>
>> +#include <linux/gpio.h>
>
>This header must not be in the new code.
>
>...
>
>> +#define AD777X_SINC3_MAXFREQ			16000
>> +#define AD777X_SINC5_MAXFREQ			128000
>
>HZ_PER_KHZ ? You will need units.h.
>
>...
>
>> +#define DEC3					1000
>> +#define DEC6					1000000
>
>NIH some of units.h constants. Use them.
>
>...
>
>
>> +	/*
>> +	 * DMA (thus cache coherency maintenance) requires the
>> +	 * transfer buffers to live in their own cache lines.
>> +	 */
>> +	u8			reg_rx_buf[3] ____cacheline_aligned;
>> +	u8			reg_tx_buf[3];
>
>> +	u8			spidata_rx[32];
>> +	u8			spidata_tx[32];
>
>These will not be cache aligned. Is it a problem?

No, it should be fine without the alignment.

> ------
>
>> +{
>> +	int ret;
>> +	u8 regval, data;
>> +
>> +	ret =3D ad777x_spi_read(st, reg, &data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regval =3D data;
>> +	regval &=3D ~mask;
>> +	regval |=3D val;
>> +
>> +	if (regval !=3D data) {
>> +		ret =3D ad777x_spi_write(st, reg, regval);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>
>This all can be written as
>
>	if (regval !=3D data)
>		return ad777x_spi_write(st, reg, regval);
>
>	return 0;
>
>...or...
>
>	if (regval =3D=3D data)
>		return 0;
>
>	return ad777x_spi_write(st, reg, regval);
>
>(I prefer the latter as it shows better the flow)
>
>> +}
>
>No mutex no nothing for RMW op like this?
>
>Btw, can't you use regmap for IO?

Unfortunately, I don't think regmap could be used, because of the crc and t=
he fact that data is shifted out on the SPI SDO line in the interrupt. I co=
nsider perhaps adding regmap to the mix might complicate things a bit.=20

>
>...
>
>> +	if (st->filter_enabled =3D=3D AD777X_SINC3 &&
>> +	    sampling_freq > AD777X_SINC3_MAXFREQ) {
>> +		return -EINVAL;
>> +	} else if (st->filter_enabled =3D=3D AD777X_SINC5 &&
>
>Redundant 'else'
>
>> +		   sampling_freq > AD777X_SINC5_MAXFREQ) {
>
>Broken indentation.
>
>> +		return -EINVAL;
>> +	}
>
>Unneeded {}.
>
>...
>
>> +	ret =3D ad777x_spi_write(st, AD777X_REG_SRC_N_LSB, lsb);
>> +	if (ret)
>> +		return ret;
>> +	ret =3D ad777x_spi_write(st, AD777X_REG_SRC_N_MSB, msb);
>> +	if (ret)
>> +		return ret;
>
>Can you use 16-bit writes?
>Same Q to all similar LSB/MSB write groups.

I cannot do 16-bit writes due to how the spi functions on the chip and beca=
use the registers for MSB/LSB are at different addresses.

>--------
>> +static void ad777x_clk_disable(void *data) {
>> +	clk_disable_unprepare(data);
>> +}
>
>See below.
>
>...
>
>> +	st->mclk =3D devm_clk_get(&spi->dev, "mclk");
>> +	if (IS_ERR(st->mclk))
>> +		return PTR_ERR(st->mclk);
>> +
>> +	ret =3D clk_prepare_enable(st->mclk);
>> +	if (ret < 0)
>> +		return ret;
>
>> +	ret =3D devm_add_action_or_reset(&spi->dev,
>> +				       ad777x_clk_disable,
>> +				       st->mclk);
>> +	if (ret)
>> +		return ret;
>
>So, what's wrong with the _enabled() API?

Sorry, I am not sure what you mean here by _enabled() API, is there a diffe=
rent mechanism that can be used for this type of operations?

Best Regards,
Ramona=20