Hi Greg,
Please find my inline comments below. > -----Original Message----- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Thursday 25 April 2019 21:26 > To: Dragan Cvetic <drag...@xilinx.com> > Cc: a...@arndb.de; Michal Simek <mich...@xilinx.com>; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Derek > Kiernan <dkier...@xilinx.com> > Subject: Re: [PATCH V2 02/12] misc: xilinx-sdfec: add core driver > > On Tue, Apr 09, 2019 at 11:06:44AM +0100, Dragan Cvetic wrote: > > +++ b/include/uapi/misc/xilinx_sdfec.h > > @@ -0,0 +1,42 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > That is not the proper SPDX license for a UAPI kernel header file :( > > Please fix this up. I will put /* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ Thanks. > > Also, why do you need a uapi file at all? Who is using these values? > These values are descriptors of the SD-FEC HW and its state, they will be used by the user space API. Please advise. > > +/* > > + * Xilinx SD-FEC > > + * > > + * Copyright (C) 2016 - 2017 Xilinx, Inc. > > + * > > + * Description: > > + * This driver is developed for SDFEC16 IP. It provides a char device > > + * in sysfs and supports file operations like open(), close() and ioctl(). > > + */ > > +#ifndef __XILINX_SDFEC_H__ > > +#define __XILINX_SDFEC_H__ > > + > > +/** > > + * enum xsdfec_state - State. > > + * @XSDFEC_INIT: Driver is initialized. > > + * @XSDFEC_STARTED: Driver is started. > > + * @XSDFEC_STOPPED: Driver is stopped. > > + * @XSDFEC_NEEDS_RESET: Driver needs to be reset. > > + * @XSDFEC_PL_RECONFIGURE: Programmable Logic needs to be recofigured. > > + * > > + * This enum is used to indicate the state of the driver. > > + */ > > +enum xsdfec_state { > > + XSDFEC_INIT = 0, > > + XSDFEC_STARTED, > > + XSDFEC_STOPPED, > > + XSDFEC_NEEDS_RESET, > > + XSDFEC_PL_RECONFIGURE, > > +}; > > + > > +/** > > + * struct xsdfec_config - Configuration of SD-FEC core. > > + * @fec_id: ID of SD-FEC instance. ID is limited to the number of active > > + * SD-FEC's in the FPGA and is related to the driver instance > > + * Minor number. > > + */ > > +struct xsdfec_config { > > + s32 fec_id; > > What is this structure for and why are you telling userspace about it? > If you are telling userspace about it, you are using the wrong data > type :( My mistake, I should use __s32, I'll fix all of them. > > thanks, > > greg k-h Thank you Dragan
