Re: [PATCH 04/11] mfd: flexcard: add clocksrc device

[Lee Jones <lee.jones@linaro.org>]

On Wed, 25 Mar 2015, Holger Dengler wrote:

> From: Benedikt Spranger <b.spranger@linutronix.de>
> 
> The Flexcard offers a Flexray network synchronized counter with a
> selectable resolution of 1us, 100ns or 10ns. Add an appropriate MFD-cell
> to use the counter.
> 
> Signed-off-by: Holger Dengler <dengler@linutronix.de>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> cc: Samuel Ortiz <sameo@linux.intel.com>
> cc: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/flexcard/core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/mfd/flexcard/core.c b/drivers/mfd/flexcard/core.c
> index 6477019..c73ae5c 100644
> --- a/drivers/mfd/flexcard/core.c
> +++ b/drivers/mfd/flexcard/core.c
> @@ -28,6 +28,29 @@ static DEFINE_IDA(flexcard_ida);
>  
>  static const char drv_name[] = "flexcard";
>  
> +static struct resource flexcard_clk_res[] = {
> +	{
> +		.name   = "flexcard-clksrc",
> +		.start  = 0x700,

We normally define these with a friendly name.

> +		.end    = 0x713,

Then use "NEW_DEFINE + 0x13" here.

> +		.flags  = IORESOURCE_MEM,
> +	}, {
> +		.name   = "flexcard-reset",
> +		.start  = 0x144,
> +		.end    = 0x147,
> +		.flags  = IORESOURCE_MEM,
> +	},
> +};
> +
> +static struct mfd_cell flexcard_clk_dev[] = {
> +	{
> +		.id = 0,

You define this here.

> +		.name = "flexcard-clksrc",
> +		.num_resources = ARRAY_SIZE(flexcard_clk_res),
> +		.resources = flexcard_clk_res,
> +	},
> +};
> +

[...]

> +static int flexcard_clk_setup(struct flexcard_device *priv)
> +{
> +	struct pci_dev *pdev = priv->pdev;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(flexcard_clk_res); i++)
> +		flexcard_clk_res[i].parent = &pdev->resource[0];

You're already providing &pdev->resource[0] as the (5th) 'mem_base'
parameter of mfd_add_devices() where the core will populate .parent
for you.  No need to also hand-roll it here.

> +	flexcard_clk_dev[0].id = priv->cardnr;

Then override it here?

> +	return mfd_add_devices(&pdev->dev, 0, flexcard_clk_dev,
> +			       ARRAY_SIZE(flexcard_clk_dev),
> +			       &pdev->resource[0], 0, NULL);

Why do you have to register the clock as a seperate mfd_add_devices()
call?  How many MFD core devices are on this h/w?

> +}
> +
>  static int flexcard_misc_open(struct inode *inode, struct file *filp)
>  {
>  	struct flexcard_device *priv =
> @@ -233,6 +271,12 @@ static int flexcard_probe(struct pci_dev *pdev,
>  		goto out_unmap;
>  	}
>  
> +	ret = flexcard_clk_setup(priv);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to register clksrc: %d\n", ret);
> +		goto out_remove;
> +	}
> +
>  	ret = misc_register(&priv->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to register miscdevice: %d\n", ret);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog