From: "Mahadevan, Girish" <girishm@codeaurora.org>
To: Mark Brown <broonie@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
sdharia@codeaurora.org, kramasub@codeaurora.org,
dianders@chromium.org, linux-arm-msm@vger.kernel.org,
swboyd@chromium.org
Subject: Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
Date: Mon, 7 May 2018 15:40:46 -0600 [thread overview]
Message-ID: <a9146e14-19d6-c718-a5d8-64aba6db5465@codeaurora.org> (raw)
In-Reply-To: <20180503233849.GF13402@sirena.org.uk>
Hi Mark
On 5/3/2018 5:38 PM, Mark Brown wrote:
> On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
>> 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.
>
> This is a DT based driver but there is no binding documentation.
> Binding documentation is required for any new DT stuff.
>
The DT documentation for the SPI driver was done as part of this patch
series
https://patchwork.kernel.org/patch/10318125/
>> + depends on ARCH_QCOM || (ARM && COMPILE_TEST)
> > Why the ARM dependency? There's no architecture specific headers
> included...
Agree, I will remove it. I will add the dependency on QCOM_GENI_SE(to be
consistent with the other GENI_QUP protocol drivers (I2C and UART))
>> obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
>> obj-$(CONFIG_SPI_QUP) += spi-qup.o
>> +obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
>> obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
>
> Please keep Kconfig and Makefile alphabetically sorted to reduce
> conflicts.
>
Ok.
>> +static struct spi_master *get_spi_master(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct spi_master *spi = platform_get_drvdata(pdev);
>> +
>> + return spi;
>> +}
>
> This doesn't look at all driver specific with the current naming but it
> actually is given that other drivers may use other driver data so it
> should be renamed. I'm also not clear why it's bouncing through the
> platform device, dev_get_drvdata() exists.
>
Agree, this function isn't needed, dev_get_drvdata() should be sufficient.
>> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
>> + struct spi_message *spi_msg)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
>> +
>> + mas->cur_speed_hz = 0;
>> + mas->cur_word_len = 0;
>> + return 0;
>> +}
>
> Is this really useful? If the driver needs to reconfigure for every
> message then it should just do that and not care about the state. If it
> might end up caring about the state anyway that suggests there's some
> kind of bug somewhere that's being masked.
>
Agree, it can be removed.
>> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> + int ret = 0;
>> + struct geni_se *se = &mas->se;
>> +
>> + ret = pm_runtime_get_sync(mas->dev);
>> + if (ret < 0) {
>
> Use auto_runtime_pm.
>
Ok
>> + if (unlikely(!mas->setup)) {
>> + int proto = geni_se_read_proto(se);
>
> Does this really need a likely/unlikely annotation - it shouldn't be any
> kind of hot path... There's a lot of these annotations in the code.
>
Ok
>> + ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
>> + IRQF_TRIGGER_HIGH, "spi_geni", mas);
>> + if (ret) {
>> + dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
>
> Why are we dynamically requesting the IRQ outside of probe? Normally an
> interrupt is requested on startup and held through the life of the
> device. I'm also not seeing any sign that it's freed except via devm...
>
Ok, will move this to probe.
>> + spi->bus_num = of_alias_get_id(pdev->dev.of_node, "spi");
>
> Don't do this, just set bus_num to -1 and let the core assign an ID.
>
Ok.
>> + spi->dev.of_node = pdev->dev.of_node;
>
> This is broken, the virtual SPI device does not exist in DT and this
> might break things.
>
I don't follow, if I don't do this the framework won't be able to probe
the slave devices of the controller.
>> + pm_runtime_enable(&pdev->dev);
>> + ret = spi_register_master(spi);
>
> No devm?
>
Agree, I will change this to use devm_spi_register_master()
Best Regards
Girish
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.
next prev parent reply other threads:[~2018-05-07 21:40 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
2018-05-07 21:40 ` Mahadevan, Girish [this message]
[not found] ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
2018-05-17 7:21 ` Mark Brown
2018-05-21 21:45 ` Mahadevan, Girish
2018-05-22 17:32 ` Mark Brown
2018-05-11 22:30 ` Stephen Boyd
2018-05-21 15:52 ` Mahadevan, Girish
2018-05-22 16:46 ` Stephen Boyd
2018-05-22 17:30 ` Mark Brown
2018-05-24 16:25 ` Mahadevan, Girish
2018-05-24 16:29 ` Mark Brown
[not found] ` <28d8ab5fdeb34e52eba7ca771a17bc06@codeaurora.org>
2018-08-03 12:18 ` dkota
2018-08-09 18:03 ` Doug Anderson
2018-08-09 18:24 ` Trent Piepho
2018-08-09 19:37 ` Doug Anderson
2018-08-10 18:43 ` Trent Piepho
2018-08-10 10:52 ` Mark Brown
2018-08-10 15:40 ` Doug Anderson
2018-08-10 16:13 ` Mark Brown
2018-08-10 16:27 ` Doug Anderson
2018-08-10 16:43 ` Mark Brown
2018-08-10 16:47 ` Doug Anderson
2018-08-10 16:29 ` dkota
2018-08-10 16:46 ` Mark Brown
2018-08-14 9:00 ` dkota
2018-08-14 15:03 ` Mark Brown
2018-08-17 10:36 ` dkota
2018-08-17 14:59 ` Mark Brown
2018-08-24 11:00 ` dkota
2018-08-10 16:49 ` Doug Anderson
2018-08-10 17:55 ` Trent Piepho
2018-06-08 23:34 ` Matthias Kaehlcke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a9146e14-19d6-c718-a5d8-64aba6db5465@codeaurora.org \
--to=girishm@codeaurora.org \
--cc=broonie@kernel.org \
--cc=dianders@chromium.org \
--cc=kramasub@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=sdharia@codeaurora.org \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).