lkml.org 
[lkml]   [2018]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH V5 3/3] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
    Date
    Quoting Doug Anderson (2018-10-09 10:48:55)
    >
    > Ah, you're suggesting separating the platform_get_irq() and the
    > request_irq() so that we call platform_get_irq() as the first thing in
    > the function but don't do the request_irq() until later. Yeah, that
    > could be done and I guess if you feel strongly about it I wouldn't
    > object to the change, but I don't feel it buys us a lot and I kind of
    > like keeping the two next to each other. Specifically why I don't
    > think it buys us a lot:
    >
    > 1. You still need the "dev_err" print, right? platform_get_irq()
    > doesn't automatically print errors for you I think.

    I usually leave it out. Who cares? Maybe we should throw a dev_err()
    into platform_get_irq() on failure so we can keep drivers cleaner and
    reduce a bunch of "can't find my IRQ" messages throughout the kernel.

    >
    > 2. You now need a local variable "irq". By putting the
    > platform_get_irq() before the memory allocation you now can't store it
    > directly in mas->irq. We could try using "ret" as a temporary
    > variable but that seems worse in this case since it'd be a bit
    > fragile.
    >
    > 3. You don't get rid of any error labels / error handling so we don't
    > really save any code
    >
    > When I tried this my diffstat says 8 lines added and 7 removed, so a
    > net increase in LOC FWIW. I'm relying in gmail so my patch will be
    > whitespace-damaged (sigh), but you can find a clean one at:
    >
    > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e0325d618e209c22379e3a4269c14627b19243a8%5E%21/#F0
    >
    > ...the basic idea is this though:
    >

    Thanks! Here's an updated patch that I haven't compile tested in any way
    that hoists up the IO mapping part too, which shows that the 'se' local
    variable is almost entirely useless.

    drivers/spi/spi-geni-qcom.c | 49 ++++++++++++++++++++++-----------------------
    1 file changed, 24 insertions(+), 25 deletions(-)

    diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
    index 6432ecc4e2ca..917707448578 100644
    --- a/drivers/spi/spi-geni-qcom.c
    +++ b/drivers/spi/spi-geni-qcom.c
    @@ -538,11 +538,30 @@ static irqreturn_t geni_spi_isr(int irq, void *data)

    static int spi_geni_probe(struct platform_device *pdev)
    {
    - int ret;
    + int ret, irq;
    struct spi_master *spi;
    struct spi_geni_master *mas;
    struct resource *res;
    - struct geni_se *se;
    + void __iomem *base;
    + struct clk *clk;
    +
    + irq = platform_get_irq(pdev, 0);
    + if (irq < 0) {
    + dev_err(&pdev->dev, "Err getting IRQ %d\n", irq);
    + return irq;
    + }
    +
    + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    + base = devm_ioremap_resource(&pdev->dev, res);
    + if (IS_ERR(base))
    + return PTR_ERR(base);
    +
    + clk = devm_clk_get(&pdev->dev, "se");
    + if (IS_ERR(clk)) {
    + ret = PTR_ERR(mas->se.clk);
    + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
    + return ret;
    + }

    spi = spi_alloc_master(&pdev->dev, sizeof(*mas));
    if (!spi)
    @@ -550,27 +569,15 @@ static int spi_geni_probe(struct platform_device *pdev)

    platform_set_drvdata(pdev, spi);
    mas = spi_master_get_devdata(spi);
    + mas->irq = irq;
    mas->dev = &pdev->dev;
    mas->se.dev = &pdev->dev;
    mas->se.wrapper = dev_get_drvdata(pdev->dev.parent);
    - se = &mas->se;
    + mas->se.base = base;
    + mas->se.clk = clk;

    spi->bus_num = -1;
    spi->dev.of_node = pdev->dev.of_node;
    - mas->se.clk = devm_clk_get(&pdev->dev, "se");
    - if (IS_ERR(mas->se.clk)) {
    - ret = PTR_ERR(mas->se.clk);
    - dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
    - goto spi_geni_probe_err;
    - }
    -
    - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    - se->base = devm_ioremap_resource(&pdev->dev, res);
    - if (IS_ERR(se->base)) {
    - ret = PTR_ERR(se->base);
    - goto spi_geni_probe_err;
    - }
    -
    spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
    spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
    spi->num_chipselect = 4;
    @@ -589,13 +596,6 @@ static int spi_geni_probe(struct platform_device *pdev)
    if (ret)
    goto spi_geni_probe_runtime_disable;

    - mas->irq = platform_get_irq(pdev, 0);
    - if (mas->irq < 0) {
    - ret = mas->irq;
    - dev_err(&pdev->dev, "Err getting IRQ %d\n", ret);
    - goto spi_geni_probe_runtime_disable;
    - }
    -
    ret = request_irq(mas->irq, geni_spi_isr,
    IRQF_TRIGGER_HIGH, "spi_geni", spi);
    if (ret)
    @@ -610,7 +610,6 @@ static int spi_geni_probe(struct platform_device *pdev)
    free_irq(mas->irq, spi);
    spi_geni_probe_runtime_disable:
    pm_runtime_disable(&pdev->dev);
    -spi_geni_probe_err:
    spi_master_put(spi);
    return ret;
    }
    \
     
     \ /
      Last update: 2018-10-09 21:47    [W:2.228 / U:0.808 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site