RE: [PATCH] [linux][master][v1] misc: Add Xilinx ZYNQMP VCU logicoreIP init driver

[Dhaval Rajeshbhai Shah]

Hi Greg k-h,

Thanks a lot for the review.

Replies inline.

-----Original Message-----
From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] 
Sent: Tuesday, December 05, 2017 5:30 AM
To: Dhaval Rajeshbhai Shah <DSHAH@xxxxxxxxxx>
Cc: arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx; Hyun Kwon <hyunk@xxxxxxxxxx>; Dhaval Rajeshbhai Shah <DSHAH@xxxxxxxxxx>
Subject: Re: [PATCH] [linux][master][v1] misc: Add Xilinx ZYNQMP VCU logicoreIP init driver

On Tue, Dec 05, 2017 at 03:43:32AM -0800, Dhaval Shah wrote:
> Xilinx ZYNQMP VCU Init driver is based on the new LogiCoreIP design 
> created. This driver will provide the api which can be used by the 
> encoder and decoder driver to get the configured value.

Your subject has a lot of [] in it, none of that is needed except the [PATCH] one :)
[Dhaval ] : I will take care from next version. 

> 
> Signed-off-by: Dhaval Shah <dshah@xxxxxxxxxx>
> ---
>  drivers/misc/Kconfig    |   6 +
>  drivers/misc/Makefile   |   1 +
>  drivers/misc/xlnx_vcu.c | 664 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/misc/xlnx_vcu.h |  18 ++
>  4 files changed, 689 insertions(+)
>  create mode 100644 drivers/misc/xlnx_vcu.c  create mode 100644 
> include/misc/xlnx_vcu.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 
> f1a5c23..3b7c796 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -496,6 +496,12 @@ config PCI_ENDPOINT_TEST
>             Enable this configuration option to enable the host side test driver
>             for PCI Endpoint.
>  
> +config XILINX_VCU
> +       tristate "Xilinx VCU Init"
> +       default n

That's always the default, no need for this.
[Dhaval ] : I will remove that. 

> +       help
> +          Driver for the Xilinx VCU Init based on the logicoreIP.

You need a lot more help text here to explain what this driver is, what it is for, and who would need it.
[Dhaval ] : I will provide more help text to provide more help on driver. 

Also, why is this a misc driver?
[Dhaval ] :  this driver is for the logicoreIP which is created to support the Processing system and Programmable logic isolation and to provide the clock related information. So this is not a VCU driver and but just a intermediate driver which supports logicoreIP. That's why no subsystem for this.

> --- /dev/null
> +++ b/drivers/misc/xlnx_vcu.c
> @@ -0,0 +1,664 @@
> +/*
> + * Xilinx VCU Init
> + *
> + * Copyright (C) 2016 - 2017 Xilinx, Inc.
> + *
> + * Contacts   Dhaval Shah <dshah@xxxxxxxxxx>
> + *
> + * SPDX-License-Identifier: GPL-2.0

That line goes at the top of the file with // in front of it.
[Dhaval ] : I will update that SPDX license as you said. 


> + */
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <misc/xlnx_vcu.h>

Why do you need a .h file for a single driver?
[Dhaval ] : There are few APIs and structure which are provided in the .h file which will be used by the other driver on the Xilinx based system. I have exported few API as well because of this reason. Those API will share the information from the logicoreIP register set.

> +/**
> + * xvcu_get_color_depth - read the color depth register
> + * @xvcu:	Pointer to the xvcu_device structure
> + *
> + * Return:	Returns 32bit value
> + *
> + */
> +u32 xvcu_get_color_depth(struct xvcu_device *xvcu) {
> +	return xvcu_read(xvcu->logicore_reg_ba, VCU_ENC_COLOR_DEPTH); } 
> +EXPORT_SYMBOL_GPL(xvcu_get_color_depth);

Why is your driver exporting symbols that no one uses?

This feels very odd...
[Dhaval ] :  There are few information from the logicoreIp register set which needs to share with other driver. That's why those API are exported to use by other driver to get those information based on the requirement.

greg k-h

Regards,
Dhaval