Date:   Fri, 07 Sep 2018 15:30:44 +0530
From:   dkota@...eaurora.org
To:     Doug Anderson <dianders@...omium.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        Mark Brown <broonie@...nel.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        devicetree@...r.kernel.org,
        Girish Mahadevan <girishm@...eaurora.org>
Subject: Re: [PATCH V3] spi: spi-geni-qcom: Add SPI driver support for GENI
 based QUP

On 2018-09-02 10:36, Doug Anderson wrote:
> Hi,
> 
> On Fri, Aug 24, 2018 at 3:42 AM, Dilip Kota <dkota@...eaurora.org> 
> wrote:
>> From: Girish Mahadevan <girishm@...eaurora.org>
>> 
>> This driver supports GENI based SPI Controller in the Qualcomm SOCs. 
>> The
>> Qualcomm Generic Interface (GENI) is a programmable module supporting 
>> a
>> wide range of serial interfaces including SPI. This driver supports 
>> SPI
>> operations using FIFO mode of transfer.
>> 
>> Signed-off-by: Girish Mahadevan <girishm@...eaurora.org>
>> Signed-off-by: Dilip Kota <dkota@...eaurora.org>
>> ---
>> Addressing all the reviewer commets given in Patchset1.
>> Summerizing all the comments below:
>> 
>>         MAKEFILE: Arrange SPI-GENI driver in alphabetical order
>>         Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
>>         Enable SPI core auto runtime pm, and remove runtime pm calls.
>>         Remove spi_geni_unprepare_message(), 
>> spi_geni_unprepare_transfer_hardware()
>>         Remove likely/unlikely keywords.
>>         Remove get_spi_master() and use dev_get_drvdata()
>>         Move request_irq to probe()
>>         Mark bus number assignment to -1 as SPI core framework will 
>> assign dynamically
>>         Use devm_spi_register_master()
>>         Include platform_device.h instead of of_platform.h
>>         Removing macros which are used only once:
>>                 #define SPI_NUM_CHIPSELECT     4
>>                 #define SPI_XFER_TIMEOUT_MS    250
>>         Place Register field definitions next to respective Register 
>> definitions.
>>         Replace int and u32 declerations to unsigned int.
>>         Remove Hex numbers in debug prints.
>>         Declare mode as u16 in spi_setup_word_len()
>>         Remove the labels: setup_fifo_params_exit: 
>> exit_prepare_transfer_hardware:
>>         Declaring struct spi_master as spi everywhere in the file.
>>         Calling spi_finalize_current_transfer() for end of transfer.
>>         Hard code the SPI controller max frequency instead of reading 
>> from DTSI node.
>>         Spinlock not required, removed it.
>>         Removed unrequired error prints.
>>         Fix KASAN error in geni_spi_isr().
>>         Remove spi-geni-qcom.h
>>         Remove inter words delay and CS to Clock toggle delay logic in 
>> the driver, as of now no clients are using it.
>>         Will submit this logic in the next patchset.
>>         Use major, minor and step macros to read from hardware version 
>> register.
>> 
>>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -
>>  drivers/spi/Kconfig                                |  12 +
>>  drivers/spi/Makefile                               |   1 +
>>  drivers/spi/spi-geni-qcom.c                        | 678 
>> +++++++++++++++++++++
>>  4 files changed, 691 insertions(+), 2 deletions(-)
> 
> See below for comments.  In general I've tried to post patches to
> address my own comments.  See the series ending at
> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142/1>.
> From there you can download patch files by using the "DOWNLOAD" link
> at the bottom.  Yell if you have problems.  Hopefully that's useful.
> I expect that you can squash many of these into your patch to give you
> a leg up on v3.
> 
> NOTE: I won't promise that I made no mistakes on my fixup patches nor
> that I caught everything or did everything right.  I'll plan to take a
> fresh look at the whole patch when I see your v3.
> 
> 
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -60,7 +60,6 @@ Required properties:
>>  - interrupts:          Must contain SPI controller interrupts.
>>  - clock-names:         Must contain "se".
>>  - clocks:              Serial engine core clock needed by the device.
>> -- spi-max-frequency:   Specifies maximum SPI clock frequency, units - 
>> Hz.
> 
> As per Rob's feedback, please split the device tree change into a
> separate patch and justify it.  Perhaps the commit message could be
> something like:
> 
> ---
Ok, will submit as separate patch.
> 
> dt-bindings: spi: Remove spi-max-frequency from qcom,geni-se controller
> 
> No other SPI controllers have a "spi-max-frequency" at the controller
> level.  The normal "spi-max-frequency" property is something that is
> used when defining the nodes for SPI slaves.  While it is possible
> that someone might want to define a controller-level max frequency it
> should be done in other ways (perhaps by keying off a compatible
> string?)
> 
> ---
> 
> I think in the past Mark Brown has also requested that the bindings
> actually live under "Documentation/devicetree/bindings/spi/", so
> perhaps you should also add a patch to your series that moves this
> documentation there and changes the "soc/qcom/qcom,geni-se.txt" to
> reference that.
> 
> 
>> +static irqreturn_t geni_spi_isr(int irq, void *data);
>> +
>> +struct spi_geni_master {
>> +       struct geni_se se;
>> +       unsigned int irq;
> 
> In v1 Stephen requested that many things in this struct become
> "unsigned int", but he didn't mean the "irq".  Please change this back
> to an int.  As you have things right now the code "if (spi_geni->irq <
> 0)" you have below is a no-op.  :(
> 
> ...oh, and as Stephen pointed out to me offline you don't even need to
> store irq.
> 
Ok
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201065
> 
> 
>> +       struct device *dev;
>> +       unsigned int rx_fifo_depth;
>> +       unsigned int tx_fifo_depth;
>> +       unsigned int tx_fifo_width;
>> +       unsigned int tx_wm;
> 
> Anything that is effectively the contents of a hardware register
> should be of type 'u32'.
Ok, will update it
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201066
> 
> 
>> +static int get_spi_clk_cfg(unsigned int speed_hz,
>> +                       struct spi_geni_master *mas,
>> +                       unsigned int *clk_idx,
>> +                       unsigned int *clk_div)
>> +{
>> +       unsigned long sclk_freq;
>> +       struct geni_se *se = &mas->se;
>> +       int ret;
>> +
>> +       ret = geni_se_clk_freq_match(&mas->se,
>> +                               (speed_hz * mas->oversampling), 
>> clk_idx,
>> +                               &sclk_freq, true);
> 
> Pass "false" as the last parameter after picking my geni patch to make
> this work right.  For my geni patch see
> <https://lkml.kernel.org/r/20180830183612.169572-2-dianders@chromium.org>.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
> Agree. Will mark it to False.
> 
>> +       if (ret) {
>> +               dev_err(mas->dev, "%s: Failed(%d) to find src clk for 
>> %dHz\n",
>> +                                               __func__, ret, 
>> speed_hz);
> 
> As I said in previous reviews: printing __func__ is generally
> discouraged for dev_xx printouts.  They've already got the device name
> and that together with the string should be enough.  Remove it here
> and elsewhere in this file.
> 
Ok, will traverse the driver and remove __func__
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201067
> 
> 
>> +               return ret;
>> +       }
>> +
>> +       *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);
> 
> After changing exact to false, you'll want:
> 
>   *clk_div = DIV_ROUND_UP(sclk_freq, mas->oversampling * speed_hz);
>   actual_hz = sclk_freq / (mas->oversampling * *clk_div);
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
> 
Ok. I see actual_hz variable for debug prints itself.
> 
>> +       if (!(*clk_div)) {
>> +               dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d 
>> speed:%u\n",
>> +                       __func__, sclk_freq, mas->oversampling, 
>> speed_hz);
>> +               return -EINVAL;
>> +       }
> 
> After change above to use DIV_ROUND_UP() you no longer need to check
> for a zero clk_div.
Ok
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
> 
> 
>> +static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
>> +                                       unsigned int bits_per_word)
>> +{
>> +       unsigned int word_len, pack_words;
>> +       bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);
> 
> Gratuitously using the __relaxed versions of readl() and writel() is
> discouraged in Linux because it gives very little performance gain in
> 99.9% of the cases and it makes the code significantly harder to
> reason about it.  Remove it in spi-geni-qcom.  If it is shown that a
> particular read or write is in a performance critical section then
> that one instance can be added back in after carefully analysis and
> documentation.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201068
Ok
> 
> 
>> +static int setup_fifo_params(struct spi_device *spi_slv,
>> +                                       struct spi_master *spi)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +       unsigned int mode = spi_slv->mode;
>> +       unsigned int loopback_cfg = readl_relaxed(se->base + 
>> SE_SPI_LOOPBACK);
>> +       unsigned int cpol = readl_relaxed(se->base + SE_SPI_CPOL);
>> +       unsigned int cpha = readl_relaxed(se->base + SE_SPI_CPHA);
>> +       unsigned int demux_sel, clk_sel, m_clk_cfg, idx, div;
>> +       int ret = 0;
> 
> In v1 Stephen said:
> 
>> Don't initialize variables and then overwrite them.
> 
> This is in lots of places in your code.  I've found and fixed them for 
> you:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201070
Ok
> 
> 
>> +       unsigned int demux_output_inv = 0;
>> +
>> +       loopback_cfg &= ~LOOPBACK_MSK;
>> +       cpol &= ~CPOL;
>> +       cpha &= ~CPHA;
>> +
>> +       if (mode & SPI_LOOP)
>> +               loopback_cfg |= LOOPBACK_ENABLE;
>> +
>> +       if (mode & SPI_CPOL)
>> +               cpol |= CPOL;
>> +
>> +       if (mode & SPI_CPHA)
>> +               cpha |= CPHA;
>> +
>> +       if (spi_slv->mode & SPI_CS_HIGH)
>> +               demux_output_inv = BIT(spi_slv->chip_select);
>> +
>> +       demux_sel = spi_slv->chip_select;
>> +       mas->cur_speed_hz = spi_slv->max_speed_hz;
>> +       mas->cur_word_len = spi_slv->bits_per_word;
>> +
>> +       ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div);
>> +       if (ret) {
>> +               dev_err(mas->dev, "Err setting clks ret(%d) for %d\n",
>> +                                                       ret, 
>> mas->cur_speed_hz);
>> +               return ret;
>> +       }
>> +
>> +       clk_sel = (idx & CLK_SEL_MSK);
>> +       m_clk_cfg = ((div << CLK_DIV_SHFT) | SER_CLK_EN);
> 
> Please avoid useless parenthesis everywhere in this file.  Stephen
> pointed out many of these in v1 but the correct thing to do would have
> been to search for all instances and fix them, not just fix the ones
> that Stephen pointed out.  I've now done it for you:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201071
> 
> 
Ok
>> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
>> +{
>> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +       struct geni_se *se = &mas->se;
>> +
>> +       if (!mas->setup) {
>> +               unsigned int proto = geni_se_read_proto(se);
>> +               unsigned int major, minor, step, ver;
>> +
>> +               if (proto != GENI_SE_SPI) {
>> +                       dev_err(mas->dev, "Invalid proto %d\n", 
>> proto);
>> +                       return -ENXIO;
>> +               }
>> +               mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
>> +               mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
>> +               mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
>> +
>> +               /*
>> +                * Hardware programming guide suggests to configure
>> +                * RX FIFO RFR level to fifo_depth-2.
>> +                */
>> +               geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
>> +               mas->oversampling = 1;
>> +               /* Transmit an entire FIFO worth of data per IRQ */
>> +               mas->tx_wm = 1;
>> +               ver = geni_se_get_qup_hw_version(se);
>> +               major = GENI_SE_VERSION_MAJOR(ver);
>> +               minor = GENI_SE_VERSION_MINOR(ver);
>> +               step = GENI_SE_VERSION_STEP(ver);
> 
> You don't use "step".  Kill it.
> 
Ok
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201069
> 
> 
>> +       /*
>> +        * If CS change flag is set, then toggle the CS line in 
>> between
>> +        * transfers and keep CS asserted after the last transfer.
>> +        * Else if keep CS flag asserted in between transfers and 
>> de-assert
>> +        * CS after the last message.
>> +        */
>> +       if (xfer->cs_change) {
>> +               if (list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
>> +                       m_param |= FRAGMENTATION;
>> +       } else {
>> +               if (!list_is_last(&xfer->transfer_list,
>> +                               &spi->cur_msg->transfers))
> 
> In v1 Stephen said:
> 
>> Combine else and if here into an else if.
> 
> ...but really I think you could just do:
> 
> /*
>  * Keep the CS asserted in either of the two cases:
>  * - It's not the last transfer (and cs_change is not set)
>  * - It is the last transfer (and cs_change is set)
>  *
>  * ...AKA keep it asserted whenever the "last transfer" and cs_change
>  * booleans agree.
>  */
> if (!!xfer->cs_change ==
>     !!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
>         m_param |= FRAGMENTATION;
> 
This complete logic will be removed with set_cs() change, because 
m_param |= FRAGMENTATION; should set in all the cases.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201072
> 
> 
>> +       if (m_cmd & SPI_TX_ONLY) {
>> +               mas->tx_rem_bytes = xfer->len;
>> +               writel_relaxed(trans_len, se->base + 
>> SE_SPI_TX_TRANS_LEN);
>> +       }
>> +
>> +       if (m_cmd & SPI_RX_ONLY) {
>> +               writel_relaxed(trans_len, se->base + 
>> SE_SPI_RX_TRANS_LEN);
>> +               mas->rx_rem_bytes = xfer->len;
>> +       }
>> +       writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>> +       geni_se_setup_m_cmd(se, m_cmd, m_param);
>> +       if (m_cmd & SPI_TX_ONLY)
>> +               writel_relaxed(mas->tx_wm, se->base + 
>> SE_GENI_TX_WATERMARK_REG);
> 
> In v1 Stephen said:
> 
>> This can't be combined with above m_cmd & SPI_TX_ONLY statement?
> 
> In v2 I said:
> 
>> Girish said it can't.  ...but IMO if it really can't then please add a
>> comment in the code so someone doesn't later go back and try to
>> combine.
> 
> I don't see a comment.  An example of an appropriate comment (assuming
> that the geni hardware really does need the services of a dark wizard)
> would be:
> 
I will add the detailed comments.
> /*
>  * NOTE: we have to write the SE_GENI_TX_WATERMARK_REG _after_ the
>  * write to SE_SPI_TRANS_CFG and the call to geni_se_setup_m_cmd()
>  * because these two things perform a special incantation to summon a
>  * dark wizard and we're not allowed to set the watermark until the
>  * dark wizard arrives.
>  */
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201073
> 
> Presumably writing to the watermark kicks off the interrupt happening
> so it has to be last.  That would be something to say instead of
> talking about magic.
> 
> 
>> +static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
>> +{
>> +       unsigned int i = 0;
>> +       unsigned int tx_fifo_width = mas->tx_fifo_width / 
>> BITS_PER_BYTE;
> 
> This is super confusing.  You have two things with the same name
> "tx_fifo_width" one that's in terms of bits and one that's in terms of
> bytes.  ...in general it's really hard to figure out the units of all
> your variables here.  For things that it's not clear you should rename
> variables.
> 
Ok, will rename it.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075
> 
> 
>> +       unsigned int max_bytes;
>> +       const u8 *tx_buf;
>> +       struct geni_se *se = &mas->se;
>> +
>> +       if (!mas->cur_xfer)
>> +               return IRQ_NONE;
>> +
>> +       /*
>> +        * For non-byte aligned bits-per-word values(e.g 9).
>> +        * The FIFO packing is set to 1 SPI word per FIFO word.
> 
> In v1 Stephen said:
> 
>> Decapitalize "The"
> 
> You added a "." at the end of the first line so I guess it's not quite
> as wrong as it was but it's still better grammar to just have one
> sentence.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201074
> 
> 
Ok
>> +        * Assumption is that each SPI word will be accomodated in
>> +        * ceil (bits_per_word / bits_per_byte)
>> +        * and the next SPI word starts at the next byte.
>> +        * In such cases, we can fit 1 SPI word per FIFO word so 
>> adjust the
>> +        * max byte that can be sent per IRQ accordingly.
>> +        */
>> +       max_bytes = (mas->tx_fifo_depth - mas->tx_wm);
>> +       if (mas->tx_fifo_width % mas->cur_word_len)
>> +               max_bytes *= ((mas->cur_word_len / BITS_PER_BYTE) + 
>> 1);
> 
> Even though the math should be the same (since you should you'll never
> divide cur_word_len evenly), the above is clearer as:
> 
>   max_bytes *= DIV_ROUND_UP(mas->cur_word_len, BITS_PER_BYTE);
> 
> ...oh, but even that is wrong.  SPI framework documents that we need
> the nearest power of 2, so a 20-bit transfer will not take up 3 bytes
> but will actually take up 4.
> 
> ...and of course this same assumption is replicated twice in both the
> tx and rx functions.  I'm just going to rewrite these two functions.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201076
> 
> 
>> +       while (i < max_bytes) {
>> +               unsigned int j;
>> +               unsigned int fifo_word = 0;
>> +               u8 *fifo_byte;
>> +               unsigned int bytes_per_fifo = tx_fifo_width;
>> +               unsigned int bytes_to_write = 0;
>> +
>> +               if (mas->tx_fifo_width % mas->cur_word_len)
>> +                       bytes_per_fifo =
>> +                               (mas->cur_word_len / BITS_PER_BYTE) + 
>> 1;
>> +
>> +               if (bytes_per_fifo < (max_bytes - i))
>> +                       bytes_to_write = bytes_per_fifo;
>> +               else
>> +                       bytes_to_write = max_bytes - i;
>> +
>> +               fifo_byte = (u8 *)&fifo_word;
>> +               for (j = 0; j < bytes_to_write; j++)
>> +                       fifo_byte[j] = tx_buf[i++];
> 
> In previous reviews Stephen pointed out that he's not super keen on
> this manual copy of bytes, but IMO it's not a huge deal and we can
> find a better way to do it later.  ...but if someone wants to propose
> a patch that makes it nicer then I won't object.
> 
> 
>> +static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
>> +{
>> +       unsigned int i = 0;
>> +       struct geni_se *se = &mas->se;
>> +       unsigned int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
> 
> You're assuming TX and RX FIFO widths are the same.  Let's make that 
> explicit.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075
> 
Agree
> 
>> +static int spi_geni_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct spi_master *spi;
>> +       struct spi_geni_master *spi_geni;
>> +       struct resource *res;
>> +       struct geni_se *se;
>> +
>> +       spi = spi_alloc_master(&pdev->dev, sizeof(struct 
>> spi_geni_master));
>> +       if (!spi) {
>> +               ret = -ENOMEM;
>> +               goto spi_geni_probe_err;
> 
> When spi_alloc_master() fails you don't want spi_master_put().
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
> 
> 
>> +       spi->prepare_transfer_hardware = 
>> spi_geni_prepare_transfer_hardware;
> 
> IMO this is wrong.  You're using prepare_transfer_hardware() to do
> one-time init.  Just do it at probe.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201079
> 
Agree, will move it to probe.
> 
>> +       init_completion(&spi_geni->xfer_done);
>> +       pm_runtime_enable(&pdev->dev);
>> +       ret = devm_spi_register_master(&pdev->dev, spi);
>> +       if (ret)
>> +               goto spi_geni_probe_unmap;
> 
> In this error case I believe you need pm_runtime_disable().
> 
Yes, I will update it.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
> 
> 
>> +       return ret;
>> +spi_geni_probe_unmap:
>> +       devm_iounmap(&pdev->dev, se->base);
> 
> The whole point of devm is that you don't need this kind of the error 
> handling.
> 
Agree
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078
> 

> 
> 
>> +static int spi_geni_remove(struct platform_device *pdev)
>> +{
>> +       struct spi_master *spi = platform_get_drvdata(pdev);
>> +       struct spi_geni_master *spi_geni = 
>> spi_master_get_devdata(spi);
>> +
>> +       geni_se_resources_off(&spi_geni->se);
> 
> In v2 I said:
> 
>> Why would you need to call geni_se_resources_off()?  Isn't it handled
>> by pm_runtime?
IMO, runtime_pm may not be called during unbind operation. Anyhow I will 
run the test and check it.
> 
> 
>> +       pm_runtime_put_noidle(&pdev->dev);
> 
> In v2 I said:
> 
>> I'm curious: why would you have a "put" here?  Won't that result in an
>> imbalance since you don't exit probe with "get"?  ...or did pm_runtime
>> throw me for a loop again?
> 
>> +       pm_runtime_disable(&pdev->dev);
>> +       return 0;
> 
> So testing:
> 
>       cd /sys/bus/platform/drivers/geni_spi/
>       echo 880000.spi > unbind
>       sleep 2
>       echo 880000.spi > bind
> 
> ...in fact it yells a lot.  To me this means that you didn't test
> spi_geni_remove() at all.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201077
> 
I will test and update it.
> 
>> +static int __maybe_unused spi_geni_suspend(struct device *dev)
>> +{
>> +       if (!pm_runtime_status_suspended(dev))
>> +               return -EBUSY;
> 
> I didn't have time to dig into Stephen Boyd's feedback, but please
> make sure you address it here.  I agree that something seems fishy
> here.
> 
I will check it.
> ---
> 
> In v2, I said:
> 
>> I'm not sure where to comment about this, so adding it to the end:
>> 
>> Between v1 and v2 you totally removed all the locking.  Presumably
>> this is because you didn't want to hold the lock in
>> handle_fifo_timeout() while waiting for the completion.  IMO taking
>> the lock out was the wrong thing to do.  You should keep it, but just
>> drop the lock before wait_for_completion_timeout() and add it back
>> afterwards.  Specifically you _don't_ want the IRQ and timeout code
>> stomping on each other.
> 
> ...but still no spinlock?
I see there is no need of taking the spinlock as timeout will be handled 
after the calculated time as per data size and speed.
There is 99.9% less chances of interrupt during the timeout handler.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081
> 
> ---
> 
> As we've discussed offline, your current driver doesn't deal with chip
> select in a correct enough way to make cros_ec_spi work.  I've applied
> and fixed up your WIP patch to fix this.
You are correct, runtime_pm wont be called by spi framework during the 
setup call. And, in ISR flag is required to differentiate between set_cs 
and actual transfer.


> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142
> 
> 
> -Doug

Powered by blists - more mailing lists