RE: [PATCH 4/6] spi: Add SPIBSC driver

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Geert,

Thank you for your review and suggestions!

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc) {
> > +       int t = 256 * 100000;
> > +
> > +       while (t--) {
> > +               if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
> > +                       return 0;
> > +               ndelay(1);
> > +       }
> 
> So this may busy loop for up to 25.6 ms? Oops...
> 
> Can you use the interrupt (SPIHF) instead, signalling a completion?

RZ/A1 doesn't have any interrupts. And I think the interrupts in RZ/A2 
only work for HyperFlash (not QSPI).

But, 25.6ms is way too long!
I'll assume the slowest clock to be 1MHz and the max FIFO transfer of 4 bytes.
That's 32us, so that's what I'll use.


> > +               if (t->cs_change) {
> > +                       dev_err(sbsc->dev, "cs_change not supported");
> > +                       return -EIO;
> > +               }
> > +       }
> 
> If you would do the checks above in .prepare_message() (like e.g. rspi
> does)...

OK, Thanks. :)

> > +       case 6:
> > +               dropr |= DROPR_OPD0(tx_data[5]);
> > +               opde |= (1 << 0);
> 
> Both checkpatch and gcc tell you to add fallthrough coments.

OK, fixed.


> ... can't you just use .transfer_one(), instead of duplicating the logic from
> spi_transfer_one_message()?
> Or is there some special detail that's not obvious to the casual reviewer?

I guess .transfer_one() could be used if I kept shoving more stuff into 
.prepare_message().

But, the screwy thing about this controller is that messages that are 
'Tx only' are processed differently then 'Tx then Rx' messages and not 
like a traditional SPI controller.
By implementing both conditions in .spi_transfer_one_message(), it makes
that more clear for the next person in my opinion because you can see 
that sometimes we have to work with the entire message as a whole, not 
just individual pieces.


> > +static const struct of_device_id of_spibsc_match[] = {
> > +       { .compatible = "renesas,spibsc"},
> > +       { .compatible = "renesas,r7s72100-spibsc", .data = (void
> *)HAS_SPBCR},
> > +       { .compatible = "renesas,r7s9210-spibsc"},
> 
> Do you need to match against all 3 in the driver?
> Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
> If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.

OK, I dropped "renesas,spibsc".
I like having individual SoC names anyway.

Chris