From: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
To: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types
Date: Wed, 03 Jun 2015 17:07:48 +0300 [thread overview]
Message-ID: <556F0A34.9010506@linux.intel.com> (raw)
In-Reply-To: <20150603121135.GH14071-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On 06/03/2015 03:11 PM, Mark Brown wrote:
> On Wed, Jun 03, 2015 at 01:49:45PM +0300, Jarkko Nikula wrote:
>
>> static bool is_lpss_ssp(const struct driver_data *drv_data)
>> {
>> - return drv_data->ssp_type == LPSS_SSP;
>> + return drv_data->ssp_type == LPSS_LPT_SSP ||
>> + drv_data->ssp_type == LPSS_BYT_SSP;
>> }
>
> switch statement please, it helps scale new types.
>
>> - switch (drv_data->ssp_type) {
>> - case QUARK_X1000_SSP:
>> + if (is_quark_x1000_ssp(drv_data)) {
>> tx_thres = TX_THRESH_QUARK_X1000_DFLT;
>> tx_hi_thres = 0;
>> rx_thres = RX_THRESH_QUARK_X1000_DFLT;
>> - break;
>> - case LPSS_SSP:
>> + } else if (is_lpss_ssp(drv_data)) {
>
> Why are we moving away from a switch statement here? Half the point of
> using them for variant selection is that it makes it easier to adjust
> the set of cases later.
>
It looked clever to reuse is_lpss_ssp() here. Onto another hand I don't
see we'll expand a lot LPSS types so I'll keep the swich-case.
>> + const struct acpi_device_id *id;
>> + int devid, type = SSP_UNDEFINED;
>
> Don't mix initialisations with other variable declarations please.
>
>> if (!ACPI_HANDLE(&pdev->dev) ||
>> acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
>> return NULL;
>>
>> + id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
>> + if (id)
>> + type = (int)id->driver_data;
>
> It'd be a bit clearer if the error case was an else here, though - on
> first read I'd expected to return an error if we couldn't identify the
> device and the initialisation is far enough away to appear in a
> different hunk.
>
Above appears to deserve a comment at least. Code gets here when device
is either probed using those ACPI IDs in pxa2xx_spi_acpi_match[] or if
it's registered as an platform device without platform data but with
ACPI companion being set.
Idea here is to set type from pxa2xx_spi_acpi_match[] only for current
ACPI probed platforms and let the acpi_match_device() return NULL for
the latter platform device case.
Upcoming platforms will integrate host controller (SPI, I2C and UART)
and a dedicated integrated DMA engine into same device with single PCI
or ACPI ID. Our plan is to register them as a MFD child devices:
http://marc.info/?l=linux-kernel&m=143317013403300&w=2
ACPI companion gets set through the MFD device registration. For
spi-pxa2xx.c it means probe enters into pxa2xx_spi_acpi_get_pdata()
because we don't set any platform data and the MMIO, clock and IRQ are
still set there.
My idea to detect these newer platforms and set the type accordingly is
to check does the device have named resource "lpss_priv" in a code snip
below
@@ -1299,6 +1319,13 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device
*pdev)
if (IS_ERR(ssp->mmio_base))
return NULL;
+ if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv")) {
+ type = LPSS_SPT_SSP;
+ pdata->tx_param = pdev->dev.parent;
+ pdata->rx_param = pdev->dev.parent;
+ pdata->dma_filter = pxa2xx_spi_idma_filter;
+ }
+
ssp->clk = devm_clk_get(&pdev->dev, NULL);
ssp->irq = platform_get_irq(pdev, 0);
ssp->type = type;
That made me thinking can I set the type to LPSS_SPT_SSP and needed DMA
filter directly when acpi_match_device() returns NULL. At quick look it
seems no any other platform device case than our new MFD registration
enters here but have to look at more carefully.
--
Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" 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:[~2015-06-03 14:07 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-03 10:49 [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types Jarkko Nikula
[not found] ` <1433328587-20797-1-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2015-06-03 10:49 ` [PATCH 2/3] spi: pxa2xx: Prepare for new Intel LPSS SPI type Jarkko Nikula
[not found] ` <1433328587-20797-2-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2015-06-03 12:14 ` Mark Brown
2015-06-03 10:49 ` [PATCH 3/3] spi: pxa2xx: Make LPSS SPI general register optional Jarkko Nikula
[not found] ` <1433328587-20797-3-git-send-email-jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2015-06-03 12:14 ` Mark Brown
2015-06-03 12:11 ` [PATCH 1/3] spi: pxa2xx: Differentiate Intel LPSS types Mark Brown
[not found] ` <20150603121135.GH14071-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-06-03 14:07 ` Jarkko Nikula [this message]
[not found] ` <556F0A34.9010506-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2015-06-03 14:39 ` Mark Brown
[not found] ` <20150603143909.GM14071-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2015-06-04 13:54 ` Jarkko Nikula
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=556F0A34.9010506@linux.intel.com \
--to=jarkko.nikula-vuqaysv1563yd54fqh9/ca@public.gmane.org \
--cc=broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.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).