From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CEA25C83F17 for ; Mon, 28 Aug 2023 16:52:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231301AbjH1Qw1 (ORCPT ); Mon, 28 Aug 2023 12:52:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41574 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230508AbjH1Qvz (ORCPT ); Mon, 28 Aug 2023 12:51:55 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8CEBF11D; Mon, 28 Aug 2023 09:51:53 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 23A246114F; Mon, 28 Aug 2023 16:51:53 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 30B80C433C8; Mon, 28 Aug 2023 16:51:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1693241512; bh=T0d80qXn16qoKb7jplMpbYYOjy7IIvigFzL4V4oJcPQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=eQ4y6VFwP1ySfyXQ5AL6hLcaeYF+VZCdxwqPZge4b4oBp8qg2JnF0C54sq8pl0L6q nq4Cpd2S25fqA4B8z8V3pW/1dVoVtNIGHeX0dqVRAjwAGdw/rXx2lmjggf7X9upB9C pphbwoUxV67wpZLj7ZTdPFuFDSx1p9x0o2h7TZxFzzS0A7tcPKj3K56GtSl8li8a92 87iKaTNPUGVCgRV7c5kwE3KQUWEbfltzygbVSiuAf0FOHlRV+oUTgbeCAlY3bGfEIj ZTdL651myMRjGIl7wpT1wOe9Z7VbOi9EMSTRtNaV3TwTw9j4rgGEyu0WaBxRSYAvfn 3JwdqdVbTaf/g== Date: Mon, 28 Aug 2023 17:52:06 +0100 From: Jonathan Cameron To: Nuno =?UTF-8?B?U8Oh?= Cc: Dumitru Ceclan , Lars-Peter Clausen , Michael Hennerich , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Andy Shevchenko , Arnd Bergmann , Cosmin Tanislav , Alexander Sverdlin , Hugo Villeneuve , Okan Sahin , Niklas Schnelle , ChiYuan Huang , Ramona Bolboaca , Ibrahim Tilki , ChiaEn Wu , William Breathitt Gray , Lee Jones , Haibo Chen , Mike Looijmans , Leonard =?UTF-8?B?R8O2aHJz?= , Ceclan Dumitru , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver Message-ID: <20230828175206.7353ffba@jic23-huawei> In-Reply-To: <34f5e2118a4714048231e6ee9a8f244248616bd0.camel@gmail.com> References: <20230810093322.593259-1-mitrutzceclan@gmail.com> <20230810093322.593259-2-mitrutzceclan@gmail.com> <34f5e2118a4714048231e6ee9a8f244248616bd0.camel@gmail.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu, 10 Aug 2023 13:57:02 +0200 Nuno S=C3=A1 wrote: > Hi Dumitru, >=20 > Thanks for taking care of this driver which is out of tree for a long tim= e... Some > comments below. Hi. A few follow ups... > > +static int ad717x_setup(struct iio_dev *indio_dev) > > +{ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct ad717x_state *st =3D = iio_priv(indio_dev); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int id; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u8 *buf; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* reset the serial interfac= e */ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0buf =3D kcalloc(8, sizeof(*b= uf), GFP_KERNEL); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!buf) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0return -ENOMEM; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0memset(buf, 0xff, 8); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D spi_write(st->sd.spi= , buf, 8); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0kfree(buf); =20 >=20 > Hmm, why allocating the buffer? I guess one could argue that we'll get DM= A safe > alignment but then maybe use some define instead of the magic 8. >=20 Just use spi_write_then_read() without an read buffer as then it will use magic bounce buffers for you within the spi subsystem. > > +static struct spi_driver ad717x_driver =3D { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0.driver =3D { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0.name=C2=A0=C2=A0=C2=A0=3D "ad717x", =20 >=20 > I would keep the name as we have out of tree which is ad7173.c. I'm not s= ure if > there's any policy in here but I think typically you just name your drive= r from the > first supported device and then extend it from there. Since here you are = just adding > more than one device at once, it would be nice if you could keep the name= of the > driver Lars originally developed... In this case, indeed the one originally developed is a good choice. Otherwise, pick a supported part. Wild cards have bitten us far too many times as manufacturers love to 'use up' gaps in their ID space with parts that are either very different or even worse look the same but have totally different interfaces... >=20