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
