Re: [PATCH v3 3/3] spi: airoha: add SPI-NAND Flash controller driver

From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
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 v3 3/3] spi: airoha: add SPI-NAND Flash controller driver
Date: Wed, 24 Apr 2024 16:38:02 +0200	[thread overview]
Message-ID: <ZikZSkxKh0OYCjqY@lore-desk> (raw)
In-Reply-To: <CAHp75Vdf9_-KDLkm2jmkpM=pw=U4nnwYf4dKn8xg=N+DpFYXTQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

> On Wed, Apr 24, 2024 at 1:07 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:
> 
> ...
> 
> > > > +   /* addr part */
> > > > +   for (i = 0; i < op->addr.nbytes; i++) {
> > > > +           u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
> > > > +
> > > > +           data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);
> > >
> > > Seems like you wanted to have always the same endianess and hence can be done
> > > outside the loop via cpu_to_xxx()?
> >
> > sorry, I did not get what you mean here, data value relies on the loop
> > iteration.
> 
>   u8 byte_stream[8];
>   u8 cmd;
> 
>   cmd = ...;
> 
>   // find what suits your case(s)
>   put_unaligned_be64(op->addr.val, byte_stream);
>   for-loop {
>      err = _write_data();
>      ...
>   }

ack, I will fix it in v4.

Rregards,
Lorenzo

> 
> 
> > > > +           err = airoha_snand_write_data(as_ctrl, cmd, &data,
> > > > +                                         sizeof(data));
> > > > +           if (err)
> > > > +                   return err;
> > > > +   }
> 
> ...
> 
> > > Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)
> >
> > I guess we can just allocate as_dev and as_dev->txrx_buf with devm_kzalloc()
> > here, agree?
> 
> If this is a solely part of ->probe() stage, yes.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]