Message-ID: <20160715132835.GC19840@leverpostej>
Date:	Fri, 15 Jul 2016 14:28:36 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Anup Patel <anup.patel@...adcom.com>
Cc:	"Hans J. Koch" <hjk@...sjkoch.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jonathan Corbet <corbet@....net>,
	Scott Branden <sbranden@...adcom.com>,
	linux-doc@...r.kernel.org, Ray Jui <rjui@...adcom.com>,
	linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	bcm-kernel-feedback-list@...adcom.com,
	Ankit Jindal <thatsjindal@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Jan Viktorin <viktorin@...ivetech.com>,
	devicetree@...r.kernel.org
Subject: Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

[adding devicetree list]

On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> From: Jan Viktorin <viktorin@...ivetech.com>
> 
> The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.
> 
> It accepts the of_id module parameter to specify UIO compatible
> string as module parameter. There are few other module parameters
> to specify DT property name for number bits in DMA mask and details
> about dynamic regions.
> 
> Following are the newly added module parameters:
> 1) of_id: The UIO compatible string to be used for DT probing
> 2) of_dma_bits_prot: This is name of OF property which will be
> used to specify number of DMA mask bits in UIO DT node.
> 3) of_dmem_count_prop: This is name of OF property which will be
> used to specify number of dynamic regions in UIO DT node.
> 4) of_dmem_sizes_prop: This is name of OF property which will be
> used to specify sizes of each dynamic regions in UIO DT node.
> 
> Signed-off-by: Jan Viktorin <viktorin@...ivetech.com>
> Signed-off-by: Anup Patel <anup.patel@...adcom.com>
> ---
>  drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> index a4d6d81..0a0cc19 100644
> --- a/drivers/uio/uio_dmem_genirq.c
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  	return 0;
>  }
>  
> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";

What are these proeprties? What is a "dynamic region" in hardware terms?

Why must they be in the DT, and why are the *property names* arbitrarily
overridable as module parameters? What exactly do you expect there to be
in a DT?

DT bindings need documentation, but there was none as part of this series.

I do not think this makes sense from a DT perspective.

Thanks,
Mark.

> +
> +static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
> +{
> +	struct uio_dmem_genirq_pdata pdata;
> +	u32 dma_bits, regions;
> +	u32 sizes[MAX_UIO_MAPS];
> +	int ret;
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dma_bits_prop, &dma_bits);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dma_bits_prop);
> +		return ret;
> +	}
> +	if (dma_bits > 64)
> +		dma_bits = 64;
> +
> +	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dmem_count_prop, &regions);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dmem_count_prop);
> +		return ret;
> +	}
> +
> +	regions = min_t(u32, regions, MAX_UIO_MAPS);
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node,
> +					 uio_of_dmem_sizes_prop, sizes,
> +					 regions);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Missing or invalid property %s\n",
> +			uio_of_dmem_sizes_prop);
> +		return ret;
> +	}
> +
> +	pdata.dynamic_region_sizes = devm_kzalloc(&pdev->dev,
> +			sizeof(*pdata.dynamic_region_sizes) *
> +			pdata.num_dynamic_regions, GFP_KERNEL);
> +	if (!pdata.dynamic_region_sizes) {
> +		dev_err(&pdev->dev, "Unable to alloc dynamic_region_sizes\n");
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	pdata.num_dynamic_regions = regions;
> +	while (regions--)
> +		pdata.dynamic_region_sizes[regions] = sizes[regions];
> +
> +	pdata.uioinfo.name = pdev->dev.of_node->name;
> +	pdata.uioinfo.version = "devicetree";
> +
> +	return platform_device_add_data(pdev, &pdata, sizeof(pdata));
> +}
> +
>  static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  {
> -	struct uio_dmem_genirq_pdata *pdata = dev_get_platdata(&pdev->dev);
> -	struct uio_info *uioinfo = &pdata->uioinfo;
> +	struct uio_dmem_genirq_pdata *pdata;
> +	struct uio_info *uioinfo;
>  	struct uio_dmem_genirq_platdata *priv;
>  	struct uio_mem *uiomem;
>  	int ret = -EINVAL;
>  	int i;
>  
>  	if (pdev->dev.of_node) {
> -		/* alloc uioinfo for one device */
> -		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
> -		if (!uioinfo) {
> -			ret = -ENOMEM;
> -			dev_err(&pdev->dev, "unable to kmalloc\n");
> +		ret = uio_dmem_genirq_alloc_platdata(pdev);
> +		if (ret)
>  			goto bad2;
> -		}
> -		uioinfo->name = pdev->dev.of_node->name;
> -		uioinfo->version = "devicetree";
>  	}
>  
> +	pdata = dev_get_platdata(&pdev->dev);
> +	uioinfo = &pdata->uioinfo;
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	if (uioinfo->handler || uioinfo->irqcontrol ||
>  	    uioinfo->irq_flags & IRQF_SHARED) {
>  		dev_err(&pdev->dev, "interrupt configuration error\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
>  		ret = -ENOMEM;
>  		dev_err(&pdev->dev, "unable to kmalloc\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
> -	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (!pdev->dev.of_node)
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>  
>  	priv->uioinfo = uioinfo;
>  	spin_lock_init(&priv->lock);
> @@ -278,10 +339,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  	return 0;
>   bad1:
>  	kfree(priv);
> - bad0:
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(uioinfo);
>   bad2:
>  	return ret;
>  }
> @@ -296,10 +353,6 @@ static int uio_dmem_genirq_remove(struct platform_device *pdev)
>  	priv->uioinfo->handler = NULL;
>  	priv->uioinfo->irqcontrol = NULL;
>  
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(priv->uioinfo);
> -
>  	kfree(priv);
>  	return 0;
>  }
> @@ -327,10 +380,22 @@ static const struct dev_pm_ops uio_dmem_genirq_dev_pm_ops = {
>  };
>  
>  #ifdef CONFIG_OF
> -static const struct of_device_id uio_of_genirq_match[] = {
> -	{ /* empty for now */ },
> +static struct of_device_id uio_of_genirq_match[] = {
> +	{ /* This is filled with module_parm */ },
> +	{ /* end of list */ },
>  };
>  MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
> +MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");
> +module_param_string(of_dma_bits_prop, uio_of_dma_bits_prop, 128, 0);
> +MODULE_PARM_DESC(of_dma_bits_prop,
> +		 "Openfirmware property for number of bits in DMA mask");
> +module_param_string(of_dmem_count_prop, uio_of_dmem_count_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_count_prop,
> +		 "Openfirmware property for dynamic region count");
> +module_param_string(of_dmem_sizes_prop, uio_of_dmem_sizes_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_sizes_prop,
> +		 "Openfirmware property for dynamic region sizes");
>  #endif
>  
>  static struct platform_driver uio_dmem_genirq = {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Powered by blists - more mailing lists