Re: [PATCH 5/6] dmaengine: pl330: provide ACPI dmaengine interface

[Andy Shevchenko]

On Mon, Jan 4, 2016 at 7:31 AM, Wang Hongcheng <annie.wang@xxxxxxx> wrote:
> register acpi_dma controller, so ACPI devices can request pl330 DMA
> channel. Request line info is get from private data.
>
> Signed-off-by: Wang Hongcheng <annie.wang@xxxxxxx>
> ---
>  drivers/acpi/acpi_apd.c    | 34 +++++++++++++++++++++++++---------
>  drivers/dma/pl330.c        | 27 +++++++++++++++++++++++++++
>  include/linux/amba/pl330.h |  4 ++++
>  3 files changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index 4d71a65..1f16cca 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -38,11 +38,23 @@ struct apd_private_data;
>
>  static u8 peri_id[2] = { 0, 1 };
>
> -static struct dma_pl330_platdata amd_pl330 = {
> -       .nr_valid_peri = 2,
> -       .peri_id = peri_id,
> -       .mcbuf_sz = 0,
> -       .flags = IRQF_SHARED,

Agreed with bot, those two looks like WIP. Please, unite them in one
clean patch.

> +static struct dma_pl330_platdata amd_pl330[] = {
> +       {
> +               .nr_valid_peri = 2,
> +               .peri_id = peri_id,
> +               .mcbuf_sz = 0,
> +               .flags = IRQF_SHARED,

> +               .base_request_line = 1,
> +               .num = 0,

(1)

> +       },

> +       {
> +               .nr_valid_peri = 2,
> +               .peri_id = peri_id,
> +               .mcbuf_sz = 0,
> +               .flags = IRQF_SHARED,

> +               .base_request_line = 2,
> +               .num = 0,

(2)

(1) and (2) seem use wrong interpretation of the parameters. num is an
amount of request lines in the range:
base_request_line..base_request_line + num - 1.

> +       }
>  };
>  /**
>   * struct apd_device_desc - a descriptor for apd device
> @@ -101,6 +113,7 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>         unsigned int irq[AMBA_NR_IRQS];
>         struct clk *clk = ERR_PTR(-ENODEV);
>         char amba_devname[100];
> +       int devnum;
>
>         resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>         if (!resource)
> @@ -131,8 +144,11 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>         if (!amba_dev)
>                 goto amba_alloc_err;
>
> +       devnum = amba_devname[strlen(dev_name(&pdev->dev)) - 1] - '0';

This looks error prone.

> +
>         amba_dev->dev.coherent_dma_mask
>                 = acpi_dma_supported(ACPI_COMPANION(&pdev->dev)) ? DMA_BIT_MASK(64) : 0;
> +       amba_dev->dev.platform_data = &amd_pl330[devnum];
>         amba_dev->dev.fwnode = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
>
>         amba_dev->dev.parent = &pdev->dev;
> @@ -162,10 +178,10 @@ static int acpi_apd_setup_quirks(struct apd_private_data *pdata)
>
>         kfree(resource);
>
> -       dma_cap_set(DMA_MEMCPY, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_SLAVE, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_CYCLIC, amd_pl330.cap_mask);
> -       dma_cap_set(DMA_PRIVATE, amd_pl330.cap_mask);
> +       dma_cap_set(DMA_MEMCPY, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_SLAVE, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_CYCLIC, amd_pl330[devnum].cap_mask);
> +       dma_cap_set(DMA_PRIVATE, amd_pl330[devnum].cap_mask);
>
>         return 0;
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 5e5fb46..e0e0b6e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2079,6 +2079,22 @@ static struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
>         return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
>  }
>
> +static struct dma_chan *acpi_dma_pl330_xlate(struct acpi_dma_spec *dma_spec,
> +                                            struct acpi_dma *adma)
> +{
> +       struct pl330_dmac *pl330 = adma->data;
> +       struct dma_pl330_platdata *pdat;
> +       unsigned int chan_id;
> +

> +       pdat = dev_get_platdata(adma->dev);

This could be part of the definition block above

> +
> +       chan_id = dma_spec->chan_id;
> +       if (chan_id >= pl330->num_peripherals)
> +               return NULL;
> +
> +       return dma_get_slave_channel(&pl330->peripherals[chan_id].chan);
> +}
> +
>  static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  {
>         struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -2918,6 +2934,17 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>                 }
>         }
>
> +       if (has_acpi_companion(&adev->dev)) {
> +               ret = acpi_dma_controller_register(&adev->dev,
> +                                                  acpi_dma_pl330_xlate,
> +                                                  pdat->base_request_line,
> +                                                  pdat->num, pl330);
> +               if (ret) {
> +                       dev_err(&adev->dev,
> +                               "unable to register DMA to the generic ACPI DMA helpers\n");
> +               }
> +       }
> +
>         adev->dev.dma_parms = &pl330->dma_parms;
>
>         /*
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index cdc80f0..1190f69 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -31,6 +31,10 @@ struct dma_pl330_platdata {
>         unsigned mcbuf_sz;
>         /*flags for irq sharing, default is non-shared*/
>         unsigned flags;
> +       /*device base request line*/

Spaces and capital letters. Keep one style.

> +       unsigned short base_request_line;
> +       /*device request line range*/

Ditto.

> +       unsigned short num;
>  };
>
>  extern bool pl330_filter(struct dma_chan *chan, void *param);
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/