Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly

From: Jonas Gorski <jonas.gorski@gmail.com>
To: Kursad Oney <kursad.oney@broadcom.com>
Cc: William Zhang <william.zhang@broadcom.com>,
	Linux SPI List <linux-spi@vger.kernel.org>,
	Broadcom Kernel List <bcm-kernel-feedback-list@broadcom.com>,
	tomer.yacoby@broadcom.com, dregan@mail.com, f.fainelli@gmail.com,
	anand.gore@broadcom.com, dan.beygelman@broadcom.com,
	joel.peshkin@broadcom.com, Mark Brown <broonie@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly
Date: Thu, 26 Jan 2023 18:33:33 +0100	[thread overview]
Message-ID: <CAOiHx=nZBot4xuZ4aG6-4Ch18wNOK0Ud2+z7B2W7+YEc+U3DHg@mail.gmail.com> (raw)
In-Reply-To: <CAMm8Nh0Lh+oUXZGCTBC-zQPQeg9-1dPUyoq34BP2ZP_vJqWX-A@mail.gmail.com>

Hi Kursad,

On Thu, 26 Jan 2023 at 17:22, Kursad Oney <kursad.oney@broadcom.com> wrote:
> >
> > While there, you might also want to check the cs_off value(s) as well.
>
> Can you explain this please?

I'm talking about the transfer property cs_off:

" @cs_off: performs the transfer with chipselect off."

See how it is handled in the generic spi_transfer_one_message():

        spi_set_cs(msg->spi, !xfer->cs_off, false);
        ...
        list_for_each_entry(xfer, &msg->transfers, transfer_list) {
                ...
                if (xfer->cs_change) {
                        if (list_is_last(&xfer->transfer_list,
                                         &msg->transfers)) {
                                keep_cs = true;
                        } else {
                                if (!xfer->cs_off)
                                        spi_set_cs(msg->spi, false, false);
                                _spi_transfer_cs_change_delay(msg, xfer);
                                if (!list_next_entry(xfer,
transfer_list)->cs_off)
                                        spi_set_cs(msg->spi, true, false);
                        }
                } else if (!list_is_last(&xfer->transfer_list,
&msg->transfers) &&
                           xfer->cs_off != list_next_entry(xfer,
transfer_list)->cs_off) {
                        spi_set_cs(msg->spi, xfer->cs_off, false);
                }
                ...
      }

if we fix the cs_change handling, we might as well bring it up to state.

In theory I would suggest to switch to implementing the set_cs() /
transfer_one() so you could let the core take care of all of that, but
that wouldn't work with dynamically switching to prepend mode. Might
be something for v1.1 though.

Regards
Jonas