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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 48116C4345F for ; Fri, 26 Apr 2024 16:19:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=9lBu2Lb8F+WdtcAmQjSSvl7Qb3NaU+FQOd2rmtBLjtE=; b=lN38G8K5ru+LjaRwnaX3JM6fA+ FQVZq9PrKua/BTIvuSgnDGM2cUpfyX5ZP1elutt+TxPHrvpd8RlYgDRQ1s+rwVauANzBIblq9CMDl YgFtuj/Ew1U7IydiKdIYny73sQYMRVCc3zycJ7m6yWVVdfPhTx67iOLQG9NVmDk7gfHI5ykuuhIcd jhSGXa8WV5Wv6AeZ5X5ztBcTGQFSSCXqqY7O9MFtRD/O3KhTXMN5i/siojjnceiYlWIvcz2vvnfrb C1DstVYON0ht/sS0eZVjwgaE6H6VZDmz7E/cK99zTUt9Y6tdYas7LvpUwfaoLHCrksh3LjjW4efi1 dKE3t2Nw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s0OID-0000000DDyt-1vof; Fri, 26 Apr 2024 16:19:21 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s0OIA-0000000DDyG-0i4D for linux-arm-kernel@lists.infradead.org; Fri, 26 Apr 2024 16:19:19 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 7BB83620CF; Fri, 26 Apr 2024 16:19:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 986D7C113CD; Fri, 26 Apr 2024 16:19:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1714148357; bh=DHuoJm0eBZR9q50qciAmM9YlONSJlsoA+mOZEcCTj/Q=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bWXjn+TSpDKyZjwX/zY5TvCtVCTlNBlrcUdVPGTSM1NYNxauM7Bo6Bvyvnhb8283x lHluR1EOZSHXQyCkJZnQu0T4fq2FnBKVj/iNEHMU25VUIbBy0LKt6vPdtHLDn8zV0i 6CVgZWGt90d9JLip6Now60Cz2H+Au1n6vn3leP7DNuFeM0ODsGlxdWk8VDYF/PevvH vMI5Ohcuvh4kUkZ6P/hSJWwBSgS2S6YoxNjNTtrhp/TnuqRhxD+ejy+0rc/8VqnRq6 nbd75YBEx7NeyqbvFQWuJ4n56jmdP1H8/nxwn4LPVw1+Z+CKEWXhQCbwXUfAwIErTs R8TWqg6RgogYw== Date: Fri, 26 Apr 2024 18:19:13 +0200 From: Lorenzo Bianconi To: Andy Shevchenko Cc: linux-spi@vger.kernel.org, conor@kernel.org, broonie@kernel.org, lorenzo.bianconi83@gmail.com, linux-arm-kernel@lists.infradead.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, devicetree@vger.kernel.org, nbd@nbd.name, john@phrozen.org, dd@embedd.com, catalin.marinas@arm.com, will@kernel.org, upstream@airoha.com, angelogioacchino.delregno@collabora.com Subject: Re: [PATCH v4 3/3] spi: airoha: add SPI-NAND Flash controller driver Message-ID: References: <2047e9c8372b51dc263178a12e194b8826f1abe7.1714119615.git.lorenzo@kernel.org> MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240426_091918_320307_BF39D471 X-CRM114-Status: GOOD ( 27.28 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============5622122370507096846==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============5622122370507096846== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="LykAaqEnqTHChxLF" Content-Disposition: inline --LykAaqEnqTHChxLF Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable > On Fri, Apr 26, 2024 at 11:31=E2=80=AFAM Lorenzo Bianconi wrote: > > > > Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface > > found on Airoha ARM SoCs. >=20 > ... >=20 > > +#include >=20 > No driver should include asm-generic, basically 99.9% of the kernel > code must not do that. I.o.w. asm-generic is very special. >=20 ack we can use instead > > +#include > > +#include >=20 > + delay.h >=20 > > +#include > > +#include >=20 > + errno.h >=20 > > +#include >=20 > Can you make it ordered (I noticed this after a while)? >=20 > + limits.h >=20 > > +#include >=20 > + minmax.h >=20 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include >=20 > + types.h >=20 > Also note, we usually place headers from more generic to less, hence > linux/* followed by asm/* and not vice versa. ack, I will fix it. >=20 > ... >=20 > > +struct airoha_snand_dev { > > + size_t buf_len; > > + > > + u8 *txrx_buf; > > + dma_addr_t dma_addr; > > + > > + u64 cur_page_num; > > + bool data_need_update; > > +}; >=20 > ... >=20 > > + /* quad io / quad out */ >=20 > io --> in ? ack, I will fix it. >=20 > ... >=20 > > + /* dual io / dual out */ >=20 > Ditto. >=20 > ... >=20 > > + case SPI_MEM_DATA_OUT: > > + /* check dummy cycle first */ > > + if (op->dummy.nbytes) > > + return false; > > + > > + /* program load quad out */ > > + if (op->addr.buswidth =3D=3D 1 && op->data.buswidth =3D= =3D 4) > > + return true; > > + > > + /* standard spi */ > > + if (op->addr.buswidth =3D=3D 1 && op->data.buswidth =3D= =3D 1) > > + return true; >=20 > > + default: > > + break; > > + } > > + > > + return false; >=20 > Why not return false directly from the default case? it is because we still need the 'return false' at the end of routine for the other cases due to SPI_MEM_DATA_IN and SPI_MEM_DATA_OUT. >=20 > ... >=20 > > + op->data.nbytes =3D min_t(size_t, op->data.nbytes, 160 = - len); >=20 > You probably wanted clamp(). It's discouraged to use min_t() for unsigned= types. do you mean doing something like: op->data.nbytes =3D clamp(op->data.nbytes, op->data.nbytes, 160 - len); maybe an 'if' condition is more readable, what do you think? >=20 > ... >=20 > > + err =3D regmap_read_poll_timeout(as_ctrl->regmap_nfi, REG_SPI_N= FI_INTR, > > + val, (val & SPI_NFI_AHB_DONE), 0, > > + USEC_PER_SEC); >=20 > Perhaps > 1 * USEC_PER_SEC > ? >=20 > Easy to read plain numbers like this to get the idea "this is 1 SEC > timeout". Also editors highlight plain integers with a different > colour. >=20 > ... >=20 > > + /* addr part */ > > + cmd =3D opcode =3D=3D SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8; > > + put_unaligned_be64(op->addr.val, data); >=20 > > + for (i =3D 0; i < op->addr.nbytes; i++) { > > + err =3D airoha_snand_write_data(as_ctrl, cmd, > > + &data[8 - op->addr.nbytes= + i], >=20 > Now you can update a for loop to make this prettier, right? >=20 > > + sizeof(data[0])); > > + if (err) > > + return err; > > + } >=20 > for (i =3D 8 - op->addr.nbytes; i < 8; i++) { > err =3D airoha_snand_write_data(as_ctrl, cmd, &data[i], > sizeof(data[0])); > ... > } >=20 > Note, 8 can be replaced by sizeof() / ARRAY_SIZE() but I'm not insisting. ack, I agree. I will fix it. >=20 > ... >=20 > > + devm_kfree(as_ctrl->dev, as_dev->txrx_buf); > > + devm_kfree(as_ctrl->dev, as_dev); >=20 > Why?! Using devm_*free() explicitly hints about either > misunderstanding of devm concept, or object's lifetime. ack, I agree, we can get rid of them. >=20 > ... >=20 > > + spi_set_ctldata(spi, NULL); >=20 > Seems there is no consensus on NULLifying this (when, if even needed), > but it's fine. >=20 > ... >=20 > > + base =3D devm_platform_get_and_ioremap_resource(pdev, 0, &res); >=20 > How is 'res' being used exactly? right, we can pass NULL here to devm_platform_get_and_ioremap_resource() >=20 > > + if (IS_ERR(base)) > > + return PTR_ERR(base); >=20 > ... >=20 > > + base =3D devm_platform_get_and_ioremap_resource(pdev, 1, &res); >=20 > Ditto. >=20 > > + if (IS_ERR(base)) > > + return PTR_ERR(base); >=20 >=20 > ... >=20 > > + ctrl->dev.of_node =3D dev->of_node; >=20 > Use device_set_node() instead. > You might need dev_fwnode() from property.h. ack, I will fix it. Regards, Lorenzo >=20 > --=20 > With Best Regards, > Andy Shevchenko --LykAaqEnqTHChxLF Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCZivUAQAKCRA6cBh0uS2t rICzAP9ek0ugGc+6vVGU6OUSIHPYCmrh6Xsx0GriI+27vMdKUgD+Jp6uEr6BzhCc wxkmky2hnY+tUvxFF8G59BVhuCrLDAY= =blsC -----END PGP SIGNATURE----- --LykAaqEnqTHChxLF-- --===============5622122370507096846== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============5622122370507096846==--