On 14 October 2018 at 12:33, Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> wrote:
> This driver provides support for Alcor Micro AU6601 and AU6621
> card readers.
>
> This is single LUN HW and it is expected to work with following standards:
> - Support SDR104 / SDR50
> - MultiMedia Card (MMC)
> - Memory Stick (MS)
> - Memory Stick PRO (MS_Pro)
>
> Since it is a PCIe controller, it should work on any architecture
> supporting PCIe. For now, it was developed and tested only on x86_64.
>
> This driver is a result of RE work and was created without any
> documentation or real knowledge of HW internals.
>
> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
> ---
> changes
> 2018.10.09 v5:
> - fix "depends *on*" from v4 and retest results on au6601 and au6621.
>
> 2018.10.09 v4:
> - use depends MISC_ALCOR_PCI instead of select.
>
> 2018.10.08 v3:
> - rework driver and split it to PCI MFD + SDMMC parts
> - remove module parameters
> - remove most of debug outputs
> - remove some comments
>
> drivers/misc/Makefile | 2 +-
> drivers/misc/cardreader/Kconfig | 11 +
> drivers/misc/cardreader/Makefile | 4 +-
> drivers/misc/cardreader/alcor_pci.c | 377 +++++++++
> drivers/mmc/host/Kconfig | 7 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/alcor.c | 1167 +++++++++++++++++++++++++++
> include/linux/alcor_pci.h | 287 +++++++
> 8 files changed, 1853 insertions(+), 3 deletions(-)
> create mode 100644 drivers/misc/cardreader/alcor_pci.c
> create mode 100644 drivers/mmc/host/alcor.c
> create mode 100644 include/linux/alcor_pci.h
>
Would you mind splitting this into one patch for the card reader
driver and one for the mmc driver.
Also please make sure to add maintainers for pci and card readers.
[...]
> +++ b/drivers/misc/cardreader/alcor_pci.c
[...]
> +static int alcor_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct alcor_dev_cfg *cfg;
> + struct alcor_pci_priv *priv;
> + int ret, i, bar = 0;
> +
> + cfg = (void *)ent->driver_data;
> +
> + ret = pcim_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + idr_preload(GFP_KERNEL);
> + spin_lock(&alcor_pci_lock);
> + ret = idr_alloc(&alcor_pci_idr, priv, 0, 0, GFP_NOWAIT);
> + if (ret >= 0)
> + priv->id = ret;
> + spin_unlock(&alcor_pci_lock);
> + idr_preload_end();
> + if (ret < 0)
> + return ret;
This can be simplified by using ida_simple_get|remove(), please convert to that.
> +
> + priv->pdev = pdev;
> + priv->parent_pdev = pdev->bus->self;
> + priv->dev = &pdev->dev;
> + priv->cfg = cfg;
> + priv->irq = pdev->irq;
> +
> + ret = pci_request_regions(pdev, DRV_NAME_ALCOR_PCI);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot request region\n");
> + return -ENOMEM;
> + }
> +
> + if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
> + dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
> + ret = -ENODEV;
> + goto error_release_regions;
> + }
> +
> + priv->iobase = pcim_iomap(pdev, bar, 0);
> + if (!priv->iobase) {
> + ret = -ENOMEM;
> + goto error_release_regions;
> + }
> +
> + /* make sure irqs are disabled */
> + alcor_write32(priv, 0, AU6601_REG_INT_ENABLE);
> + alcor_write32(priv, 0, AU6601_MS_INT_ENABLE);
> +
> + ret = dma_set_mask_and_coherent(priv->dev, AU6601_SDMA_MASK);
> + if (ret) {
> + dev_err(priv->dev, "Failed to set DMA mask\n");
> + goto error_release_regions;
> + }
> +
> + pci_set_master(pdev);
> + pci_set_drvdata(pdev, priv);
> + alcor_pci_init_check_aspm(priv);
> +
> + for (i = 0; i < ARRAY_SIZE(alcor_pci_cells); i++) {
> + alcor_pci_cells[i].platform_data = priv;
> + alcor_pci_cells[i].pdata_size = sizeof(*priv);
> + }
> + ret = mfd_add_devices(&pdev->dev, priv->id, alcor_pci_cells,
> + ARRAY_SIZE(alcor_pci_cells), NULL, 0, NULL);
> + if (ret < 0)
> + goto error_release_regions;
> +
> + alcor_pci_aspm_ctrl(priv, 0);
> +
> + return 0;
> +
> +error_release_regions:
> + pci_release_regions(pdev);
> + return ret;
> +}
[...]
> +++ b/drivers/mmc/host/alcor.c
[...]
> +
> +static void alcor_request_complete(struct alcor_sdmmc_host *host,
> + bool cancel_timeout)
> +{
> + struct mmc_request *mrq;
> +
> + /*
> + * If this tasklet gets rescheduled while running, it will
/s/tasklet/work
> + * be run again afterwards but without any active request.
> + */
> + if (!host->mrq)
> + return;
> +
> + if (cancel_timeout)
> + cancel_delayed_work(&host->timeout_work);
> +
> + mrq = host->mrq;
> +
> + host->mrq = NULL;
> + host->cmd = NULL;
> + host->data = NULL;
> + host->dma_on = 0;
> +
> + mmc_request_done(host->mmc, mrq);
> +}
> +
[...]
> +#ifdef CONFIG_PM
Change to CONFIG_PM_SLEEP
> +static int alcor_pci_sdmmc_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct alcor_sdmmc_host *host = platform_get_drvdata(pdev);
> +
> + if (cancel_delayed_work_sync(&host->timeout_work))
> + alcor_request_complete(host, 0);
> +
> + alcor_hw_uninit(host);
> +
> + return 0;
> +}
> +
> +static int alcor_pci_sdmmc_resume(struct platform_device *pdev)
> +{
> + struct alcor_sdmmc_host *host = platform_get_drvdata(pdev);
> +
> + alcor_hw_init(host);
> +
> + return 0;
> +}
> +#else
> +#define alcor_pci_sdmmc_suspend NULL
> +#define alcor_pci_sdmmc_resume NULL
Drop this "#else" and thus these definitions as well.
Instead use "static SIMPLE_DEV_PM_OPS()", similar to how you did it
for the card reader driver.
[...]
> diff --git a/include/linux/alcor_pci.h b/include/linux/alcor_pci.h
[...]
> +
> +#define DRV_NAME_ALCOR_PCI "alcor_pci"
> +#define DRV_NAME_ALCOR_PCI_SDMMC "alcor_sdmmc"
> +#define DRV_NAME_ALCOR_PCI_MS "alcor_ms"
Please move these three to the c-files instead.
> +
> +#define PCI_ID_ALCOR_MICRO 0x1AEA
> +#define PCI_ID_AU6601 0x6601
> +#define PCI_ID_AU6621 0x6621
> +
> +#define MHZ_TO_HZ(freq) ((freq) * 1000 * 1000)
This looks like something that should be made generic.
You may either do that or perhaps start simple by just putting the
values for the below defines without using the macro above.
> +
> +#define AU6601_BASE_CLOCK MHZ_TO_HZ(31)
> +#define AU6601_MIN_CLOCK (150 * 1000)
> +#define AU6601_MAX_CLOCK MHZ_TO_HZ(208)
> +#define AU6601_MAX_DMA_SEGMENTS 1
> +#define AU6601_MAX_PIO_SEGMENTS 1
> +#define AU6601_MAX_DMA_BLOCK_SIZE 0x1000
> +#define AU6601_MAX_PIO_BLOCK_SIZE 0x200
> +#define AU6601_MAX_DMA_BLOCKS 1
> +#define AU6601_DMA_LOCAL_SEGMENTS 1
[...]
Overall this looks to me, besides the rather minor comments above.
Kind regards
Uffe
