From: Baolin Wang <baolin.wang@linaro.org> To: Mark Brown <broonie@kernel.org> Cc: Baolin Wang <baolin.wang@spreadtrum.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, linux-spi@vger.kernel.org, devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform Date: Thu, 7 Sep 2017 11:13:00 +0800 [thread overview] Message-ID: <CAMz4kuL1cDD0PTBJ9-7an3KnFyhE9iZOp4VU4MCsHcx8TG5DhQ@mail.gmail.com> (raw) In-Reply-To: <20170906150446.v7qqc24wxeykcwwk@sirena.co.uk> Hi Mark, On 6 September 2017 at 23:04, Mark Brown <broonie@kernel.org> wrote: > On Wed, Sep 06, 2017 at 02:10:44PM +0800, Baolin Wang wrote: > > This looks like a nice, clean driver. A few fairly small issues though: > >> +config SPI_SPRD_ADI >> + tristate "Spreadtrum ADI controller" >> + depends on ARCH_SPRD >> + help >> + ADI driver based on SPI for Spreadtrum SoCs. >> + > > I can't see any hard dependencies on the architecture - can you add an > || COMPILE_TEST for more coverage? Yes. > >> + ret = devm_spi_register_controller(&pdev->dev, ctlr); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register SPI controller\n"); >> + goto free_hwlock; >> + } > >> + spi_controller_put(ctlr); > > You used devm_ to register the controller, no need to free it > explicitly. Indeed. > >> +static int __init sprd_adi_init(void) >> +{ >> + return platform_driver_register(&sprd_adi_driver); >> +} >> +subsys_initcall(sprd_adi_init); > > Why is this subsys_initcall() and not module_platform_driver()? Since ADI is one very fundamental driver for our SoC, many drivers such as regulator need depend on ADI, and regulator need to regulate core voltage as earlier as possible. Very appreciated for your comments. -- Baolin.wang Best Regards
WARNING: multiple messages have this Message-ID (diff)
From: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform Date: Thu, 7 Sep 2017 11:13:00 +0800 [thread overview] Message-ID: <CAMz4kuL1cDD0PTBJ9-7an3KnFyhE9iZOp4VU4MCsHcx8TG5DhQ@mail.gmail.com> (raw) In-Reply-To: <20170906150446.v7qqc24wxeykcwwk-7j8lgAiuQgnQXOPxS62xeg@public.gmane.org> Hi Mark, On 6 September 2017 at 23:04, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > On Wed, Sep 06, 2017 at 02:10:44PM +0800, Baolin Wang wrote: > > This looks like a nice, clean driver. A few fairly small issues though: > >> +config SPI_SPRD_ADI >> + tristate "Spreadtrum ADI controller" >> + depends on ARCH_SPRD >> + help >> + ADI driver based on SPI for Spreadtrum SoCs. >> + > > I can't see any hard dependencies on the architecture - can you add an > || COMPILE_TEST for more coverage? Yes. > >> + ret = devm_spi_register_controller(&pdev->dev, ctlr); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register SPI controller\n"); >> + goto free_hwlock; >> + } > >> + spi_controller_put(ctlr); > > You used devm_ to register the controller, no need to free it > explicitly. Indeed. > >> +static int __init sprd_adi_init(void) >> +{ >> + return platform_driver_register(&sprd_adi_driver); >> +} >> +subsys_initcall(sprd_adi_init); > > Why is this subsys_initcall() and not module_platform_driver()? Since ADI is one very fundamental driver for our SoC, many drivers such as regulator need depend on ADI, and regulator need to regulate core voltage as earlier as possible. Very appreciated for your comments. -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-09-07 3:13 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-09-06 6:10 [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation Baolin Wang 2017-09-06 6:10 ` Baolin Wang 2017-09-06 6:10 ` Baolin Wang 2017-09-06 6:10 ` [PATCH 2/2] spi: Add ADI driver for Spreadtrum platform Baolin Wang 2017-09-06 6:10 ` Baolin Wang 2017-09-06 15:04 ` Mark Brown 2017-09-06 15:04 ` Mark Brown 2017-09-07 3:13 ` Baolin Wang [this message] 2017-09-07 3:13 ` Baolin Wang 2017-09-07 10:10 ` Mark Brown 2017-09-07 10:10 ` Mark Brown 2017-09-07 11:21 ` Baolin Wang 2017-09-06 14:59 ` [PATCH 1/2] dt-bindings: spi: Add Spreadtrum ADI controller documentation Mark Brown 2017-09-06 14:59 ` Mark Brown 2017-09-07 3:29 ` Baolin Wang 2017-09-07 3:29 ` Baolin Wang 2017-09-07 9:54 ` Mark Brown 2017-09-07 11:03 ` Baolin Wang 2017-09-07 11:44 ` Mark Brown 2017-09-08 1:57 ` Baolin Wang 2017-09-08 1:57 ` Baolin Wang
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=CAMz4kuL1cDD0PTBJ9-7an3KnFyhE9iZOp4VU4MCsHcx8TG5DhQ@mail.gmail.com \ --to=baolin.wang@linaro.org \ --cc=baolin.wang@spreadtrum.com \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.