Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
On Thu, Jan 16, 2014 at 10:42:01AM +0100, Olaf Hering wrote:
> On Tue, Jan 14, K. Y. Srinivasan wrote:
>
> > +enum hv_fcopy_op {
> > + START_FILE_COPY = 0,
> > + WRITE_TO_FILE,
> > + COMPLETE_FCOPY,
> > + CANCEL_FCOPY,
> > +};
> > +
> > +struct hv_fcopy_hdr {
> > + enum hv_fcopy_op operation;
> > + uuid_le service_id0; /* currently unused */
> > + uuid_le service_id1; /* currently unused */
> > +} __attribute__((packed));
>
> Is enum a fixed size?
No, it's not. The compiler has a lot of latitude in choosing the size
for enums.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/1] staging: fpgaboot: Xilinx FPGA firmware download driver
On Thu, Jan 16, 2014 at 11:47:41AM -0800, Insop Song wrote: > >> There is no way to detect FPGA until it is programmed. > >> This is a reason and the only reason of this driver to download the > >> program to the FPGA so that it can function. > > > > So how do you get the memory locations of where the FPGA is in the > > system in order to be able to send data to it? Surely that's in the > > device tree file somewhere, right? > > > > On the FPGA side, there are dedicated pins for programming, and > through these you cannot get meaningful information (again unless you > are JTAG capable) > Such as these on the FPGA side, PROGRAM_B, INIT_B, CCLK, D[0:7], and DONE. > On a process side, we use gpio pin to connect to the above pins. > It's GPIO pins that we do the bit banging as defined for programming > guide from Xilinx. Yes, but where do you learn about how those pins are hooked up to the CPU so that the driver can control them? Anyway, I have no objection to your driver, it does good things, and we want it, I think we are talking past each other at the moment, sorry. How about you repost the code, just using a platform device for the moment, and we can take it from there. After all, code in the staging tree is meant to be cleaned up :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/1] staging: fpgaboot: Xilinx FPGA firmware download driver
On Thu, Jan 16, 2014 at 8:09 AM, Greg KH wrote:
> On Thu, Jan 16, 2014 at 12:56:15AM -0800, Insop Song wrote:
>> On Tue, Jan 14, 2014 at 10:00 AM, Greg KH wrote:
>> > On Tue, Jan 14, 2014 at 01:37:50AM -0800, Insop Song wrote:
>> >> On Tue, Jan 14, 2014 at 1:18 AM, Insop Song wrote:
>> >> > On Mon, Jan 13, 2014 at 10:44 AM, Greg KH
>> >> > wrote:
>> >> >> On Sat, Jan 11, 2014 at 04:37:32PM -0800, Insop Song wrote:
>> >> >>> On Sat, Jan 11, 2014 at 4:10 PM, Greg KH
>> >> >>> wrote:
>> >> >>> > On Sat, Jan 11, 2014 at 03:56:48PM -0800, Insop Song wrote:
>> >> >>> >> On Sat, Jan 11, 2014 at 1:05 PM, Greg KH
>> >> >>> >> wrote:
>> >> >>> >> > On Sat, Jan 11, 2014 at 12:51:28PM -0800, Greg KH wrote:
>> >> >>> >> >> On Thu, Jan 09, 2014 at 11:48:04AM -0800, Insop Song wrote:
>> >> >>> >> >> > This driver downloads Xilinx FPGA firmware using gpio pins.
>> >> >>> >> >> > It loads Xilinx FPGA bitstream format firmware image and
>> >> >>> >> >> > program the Xilinx FPGA using SelectMAP (parallel) mode.
>> >> >>> >> >> >
>> >> >>> >> >> > Signed-off-by: Insop Song
>> >> >>> >> >>
>> >> >>> >> >> This patch breaks the build on my machine, please be more
>> >> >>> >> >> careful:
>> >> >>> >> >>
>> >> >>> >> >> drivers/staging/gs_fpgaboot/gs_fpgaboot.c:30:28: fatal error:
>> >> >>> >> >> include/asm/io.h: No such file or directory
>> >> >>> >> >> #include
>> >> >>> >> >>
>> >> >>> >> >> Please fix this up, test it, and resend.
>> >> >>> >> >
>> >> >>> >> > Also, while you are fixing things up, please just remove the
>> >> >>> >> > character
>> >> >>> >> > device node from the driver, you aren't doing anything with it,
>> >> >>> >> > so it's
>> >> >>> >> > not needed.
>> >> >>> >> >
>> >> >>> >>
>> >> >>> >> Thank you for the suggestion, makes a lot of sense and code
>> >> >>> >> cleaner.
>> >> >>> >>
>> >> >>> >> Could you take a quick look?
>> >> >>> >> If it is good, will send out a new patch, v5, shortly.
>> >> >>> >>
>> >> >>> >> Main change is
>> >> >>> >>
>> >> >>> >> +/* fake device for request_firmware */
>> >> >>> >> +static struct platform_device*firmware_pdev;
>> >> >>> >>
>> >> >>> >> + firmware_pdev = platform_device_register_simple("fpgaboot", -1,
>> >> >>> >> + NULL, 0);
>> >> >>> >
>> >> >>> > Why do you need a platform device? Don't you have something on the
>> >> >>> > hardware that describes the fact that this device is in the system,
>> >> >>> > or
>> >> >>> > not? Like a PCI id? DMI id? Device Tree entry?
>> >> >>> >
>> >> >>> > You must have something that you can trigger off of.
>> >> >>> >
>> >> >>>
>> >> >>> This driver is only to download fpga firmware, not intended to keep
>> >> >>> managing the fpga device after the downloading. The device we use is
>> >> >>> not part of the device tree or PCI.
>> >> >>
>> >> >> But how does the system know to load it or not, automatically?
>> >> >>
>> >> >>> It downloads the image and after that no need to be existing.
>> >> >>
>> >> >> Then why not have the module remove itself?
>> >> >>
>> >> >
>> >> > The following (insmod to load firmware and rmmod remove the module)
>> >> > commands are triggered from user space application. They are wrapped
>> >> > as a script and triggered from an application.
>> >> > Once the firmware is loaded, then this driver's job is done.
>> >
>> > Yes, but that's not the "normal" way kernel drivers interact with
>> > userspace. Please don't try to create something that is different than
>> > the thousands of other drivers out there, and rely on custom userspace
>> > scripts. Let's use the infrastructure we already have in place for
>> > doing this exact thing.
>> >
>> >> >>> Here is how we use
>> >> >>>
>> >> >>> $ insmod gs_fpga.ko file="xlinx_fpga_top_bitstream.bit"
>> >> >>> $ rmmod gs_fpga
>> >> >>
>> >> >> Linux modules should be able to be autoloaded based on hardware present
>> >> >> in the system, it's been that way for over a decade now.
>> >> >>
>> >> >
>> >> >
>> >> Greg, I think it's better to add more explanation for FPGA.
>> >
>> > I know what a FPGA is, thanks :)
>> >
>> >> FPGA (Field-programmable gate array) is not functional until the
>> >> firmware is downloaded.
>> >> That means that it is not easy, if not possible, to auto detect the
>> >> device. This is why "insmod" is manually called.
>> >
>> > That's not true, there has to be some way to detect the device is in the
>> > system, otherwise what's to prevent you from loading the module on a
>> > system that doesn't have the hardware and having bad things happen?
>> >
>> > There has to be some way to detect this device is present, right? What
>> > is it? Where is it located in the device tree description?
>> >
>>
>>
>> There is no way to detect FPGA until it is programmed.
>> This is a reason and the only reason of this driver to download the
>> program to the FPGA so that it can function.
>
> So how do you get the memory locations of where the FPGA is in the
> system in orde
[PATCH 1/1] Drivers: hv: vmbus: Don't timeout during the initial connection with host
When the guest attempts to connect with the host when there may already be a
connection with the host (as would be the case during the kdump/kexec path),
it is difficult to guarantee timely response from the host. Starting with
WS2012 R2, the host supports this ability to re-connect with the host
(explicitly to support kexec). Prior to responding to the guest, the host
needs to ensure that device states based on the previous connection to
the host have been properly torn down. This may introduce unbounded delays.
To deal with this issue, don't do a timed wait during the initial connect
with the host.
Signed-off-by: K. Y. Srinivasan
Cc:
---
drivers/hv/connection.c | 11 +--
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 855bbda..f2d7bf9 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -67,7 +67,6 @@ static int vmbus_negotiate_version(struct
vmbus_channel_msginfo *msginfo,
int ret = 0;
struct vmbus_channel_initiate_contact *msg;
unsigned long flags;
- int t;
init_completion(&msginfo->waitevent);
@@ -102,15 +101,7 @@ static int vmbus_negotiate_version(struct
vmbus_channel_msginfo *msginfo,
}
/* Wait for the connection response */
- t = wait_for_completion_timeout(&msginfo->waitevent, 5*HZ);
- if (t == 0) {
- spin_lock_irqsave(&vmbus_connection.channelmsg_lock,
- flags);
- list_del(&msginfo->msglistentry);
- spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,
- flags);
- return -ETIMEDOUT;
- }
+ wait_for_completion(&msginfo->waitevent);
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_del(&msginfo->msglistentry);
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v8] Staging: comedi: convert while loop to timeout in ni_mio_common.c
This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.
Signed-off-by: Chase Southwood
---
Okay, back to v2, basically. I fixed the checkpatch warning from v2,
and added the error checking that was from v3, but otherwise it is the
same. Of note, I have used a udelay of 1 here (Greg had suggested to
use 10, but Ian prefers 1 so I have gone with that, and I assume that
cpu_relax is no longer an option.).
2: Changed from simple clean-up to swapping a timeout in for a while loop.
3: Removed extra counter variable, and added error checking.
4: No longer using counter variable, using jiffies instead.
5: udelay for 10u, instead of 1u.
6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
in order to use cpu_relax.
7: Fix typo (msec vs msecs).
8: Revert back to v2, with some small changes (see above).
drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..10c27cb 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,22 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ static const int timeout = 1;
+ int i;
if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for
complete */
+ /* Wait for complete */
+ for (i = 0; i < timeout; i++) {
+ if (!(ni_readl(AIFIFO_Status_6143) & 0x10))
+ break;
+ udelay(1);
+ }
+ if (i == timeout) {
+ comedi_error(dev, "FIFO flush timeout.");
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
--
1.8.4.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: android: fix parentheses coding style issue in alarm-dev.c
Hello,
On 01/16/2014 12:32 AM, Michał Kwiatkowski wrote:
> This is a patch to the alarm-dev.c file that removes parentheses which
> should not appear in return statement. This error was found by the
> checkpatch.pl tool.
>
> Signed-off-by: Michał Kwiatkowski
> [...]
> +++ b/drivers/staging/android/alarm-dev.c
> @@ -68,8 +68,8 @@ static struct devalarm alarms[ANDROID_ALARM_TYPE_COUNT];
> */
> static int is_wakeup(enum android_alarm_type type)
> {
> - return (type == ANDROID_ALARM_RTC_WAKEUP ||
> - type == ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP);
> + return type == ANDROID_ALARM_RTC_WAKEUP ||
> +type == ANDROID_ALARM_ELAPSED_REALTIME_WAKEUP;
> }
This is the fourth patch that does the exact same, an other similar patch
was already applied, hence this one will not apply. Please base your patches
on -next or the staging.git tree.
--
Regards,
Levente Kurusa
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c
>On Thursday, January 16, 2014 5:31 AM, Ian Abbott wrote:
>On 2014-01-15 19:22, Chase Southwood wrote:
>> This patch for ni_mio_common.c changes out a while loop for a timeout,
>> which is preferred.
>>
>> Signed-off-by: Chase Southwood
>> ---
>>
>> Hartley,
>> I sincerely apologize for the obvious mistake, I thought I had built it
>> but clearly I made a mistake somewhere, as your observation is exactly
>> correct. It is now fixed.
>>
>> Thanks,
>> Chase Southwood
>>
>> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>>
>> 3: Removed extra counter variable, and added error checking.
>>
>> 4: No longer using counter variable, using jiffies instead.
>>
>> 5: udelay for 10u, instead of 1u.
>>
>> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
>> in order to use cpu_relax.
>>
>> 7: Fix typo (msec vs msecs).
>>
>> drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
>> >b/drivers/staging/comedi/drivers/ni_mio_common.c
>> index 457b884..ab7a74c 100644
>> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
>> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>> @@ -55,6 +55,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include "8255.h"
>> #include "mite.h"
>> #include "comedi_fc.h"
>> @@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>> {
>> const struct ni_board_struct *board = comedi_board(dev);
>> struct ni_private *devpriv = dev->private;
>> + unsigned long timeout;
>>
>> if (board->reg_type == ni_reg_6143) {
>> /* Flush the 6143 data FIFO */
>> ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
>> ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
>> - while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait for
>> complete */
>> + /* Wait for complete */
>> + timeout = jiffies + msecs_to_jiffies(500);
>> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
>> + if (time_after(jiffies, timeout)) {
>> + comedi_error(dev, "FIFO flush timeout.");
>> + break;
>> + }
>> + cpu_relax();
>> + }
>> } else {
>> devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
>> if (board->reg_type == ni_reg_625x) {
>>
>Sorry this is your worst nightmare, Chase. Your patch is great and has
>been modified in all the ways various people have suggested, but I don't
>think it is suitable.
>
>After examining the code, it turns out ni_clear_ai_fifo() is sometimes
>called from an interrupt service routine and I don't think you can wait
>for jiffies to change in that case. I think we need to go back to your
>PATCH v2 and fix up the checkpatch.pl warning that Greg mentioned.
>
>Sorry about that.
>
>--
>-=( Ian Abbott @ MEV Ltd. E-mail: )=-
>-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
It's absolutely no problem. I'll get v2 cleaned up and sent out right away!
Thanks,
Chase Southwood
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Thursday, January 16, 2014 3:27 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
>
> On Tue, Jan 14, K. Y. Srinivasan wrote:
>
> This function should return valid numbers:
>
> > +static ssize_t fcopy_write(struct file *file, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + int error = 0;
> > +
> > + if (count != sizeof(int))
> > + return 0;
>
> Should be an error.
>
> > +
> > + if (copy_from_user(&error, buf, sizeof(int)))
> > + return -EFAULT;
> > +
> > + if (in_hand_shake) {
> > + fcopy_handle_handshake();
> > + return 0;
>
> Should be sizeof(int).
>
> > + }
> > +
> > + /*
> > +* Complete the transaction by forwarding the result
> > +* to the host. But first, cancel the timeout.
> > +*/
> > + if (cancel_delayed_work_sync(&fcopy_work))
> > + fcopy_respond_to_host(error);
> > +
> > + return sizeof(int);
> > +}
Olaf,
I will make the changes in the next version of this patch.
Thank you,
K. Y
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Thursday, January 16, 2014 2:49 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
>
> On Tue, Jan 14, K. Y. Srinivasan wrote:
>
> > Implement the file copy service for Linux guests on Hyper-V. This permits
> > the
> > host to copy a file (over VMBUS) into the guest. This facility is part of
> > "guest integration services" supported on the Windows platform.
> > Here is a link that provides additional details on this functionality:
>
> The change below fixes some warnings in the daemon code.
> Compile tested only.
> I also think the newlines in some of the syslog calls should be removed.
>
> Olaf
>
>
> hv_fcopy_daemon.c: In function 'hv_start_fcopy':
> hv_fcopy_daemon.c:44:3: warning: format '%s' expects argument of type 'char
> *', but argument 3 has type '__u16 *' [-Wformat=]
>smsg->file_name);
>^
> hv_fcopy_daemon.c:44:3: warning: format '%s' expects argument of type 'char
> *', but argument 5 has type '__u16 *' [-Wformat=]
> hv_fcopy_daemon.c:57:6: warning: format '%s' expects argument of type 'char
> *', but argument 3 has type '__u16 *' [-Wformat=]
> errno, strerror(errno));
> ^
> hv_fcopy_daemon.c:61:4: warning: format '%s' expects argument of type 'char
> *', but argument 3 has type '__u16 *' [-Wformat=]
> syslog(LOG_ERR, "Invalid path: %s\n", smsg->path_name);
> ^
> hv_fcopy_daemon.c: In function 'main':
> hv_fcopy_daemon.c:117:8: warning: ignoring return value of 'daemon', declared
> with attribute warn_unused_result [-Wunused-result]
> daemon(1, 0);
> ^
> hv_fcopy_daemon.c:132:7: warning: ignoring return value of 'write', declared
> with attribute warn_unused_result [-Wunused-result]
> write(fcopy_fd, &version, sizeof(int));
>^
> hv_fcopy_daemon.c:171:9: warning: ignoring return value of 'pwrite', declared
> with attribute warn_unused_result [-Wunused-result]
>pwrite(fcopy_fd, &error, sizeof(int), 0);
> ^
>
> Signed-off-by: Olaf Hering
>
> diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
> index c0e5c90..d1fadb7 100644
> --- a/tools/hv/hv_fcopy_daemon.c
> +++ b/tools/hv/hv_fcopy_daemon.c
> @@ -35,14 +35,14 @@
> #include
>
> static int target_fd;
> -char target_fname[W_MAX_PATH];
> +static char target_fname[W_MAX_PATH];
>
> static int hv_start_fcopy(struct hv_start_fcopy *smsg)
> {
> int error = HV_E_FAIL;
>
> - sprintf(target_fname, "%s%s%s", smsg->path_name, "/",
> - smsg->file_name);
> + snprintf(target_fname, sizeof(target_fname), "%s/%s",
> + (char *)smsg->path_name, (char*)smsg->file_name);
>
> syslog(LOG_INFO, "Target file name: %s\n", target_fname);
> /*
> @@ -54,12 +54,12 @@ static int hv_start_fcopy(struct hv_start_fcopy *smsg)
> if (mkdir((char *)smsg->path_name, 0755)) {
> syslog(LOG_ERR,
> "Failed to create '%s'; error: %d %s\n",
> - smsg->path_name,
> + (char *)smsg->path_name,
> errno, strerror(errno));
> goto done;
> }
> } else {
> - syslog(LOG_ERR, "Invalid path: %s\n", smsg-
> >path_name);
> + syslog(LOG_ERR, "Invalid path: %s", (char *)smsg-
> >path_name);
> goto done;
> }
> }
> @@ -115,7 +115,8 @@ int main(void)
> char *buffer[4096 * 2];
> struct hv_fcopy_hdr *in_msg;
>
> - daemon(1, 0);
> + if (daemon(1, 0))
> + return 1;
> openlog("HV_FCOPY", 0, LOG_USER);
> syslog(LOG_INFO, "HV_FCOPY starting; pid is:%d", getpid());
>
> @@ -130,7 +131,10 @@ int main(void)
> /*
>* Register with the kernel.
>*/
> - write(fcopy_fd, &version, sizeof(int));
> + if (write(fcopy_fd, &version, sizeof(int)) != sizeof(int)) {
> + syslog(LOG_ERR, "write failed: %s",strerror(errno));
> + exit(EXIT_FAILURE);
> + }
>
> while (1) {
> /*
> @@ -169,6 +173,9 @@ int main(void)
>
> }
>
> - pwrite(fcopy_fd, &error, sizeof(int), 0);
> + if (pwrite(fcopy_fd, &error, sizeof(int), 0) != sizeof(int)) {
> + syslog(LOG_ERR, "pwrite failed: %s",strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> }
> }
Thanks Olaf; I will include these changes in the next version.
K. Y
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
> -Original Message-
> From: Olaf Hering [mailto:o...@aepfle.de]
> Sent: Thursday, January 16, 2014 1:42 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; a...@canonical.com; jasow...@redhat.com
> Subject: Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
>
> On Tue, Jan 14, K. Y. Srinivasan wrote:
>
> > +enum hv_fcopy_op {
> > + START_FILE_COPY = 0,
> > + WRITE_TO_FILE,
> > + COMPLETE_FCOPY,
> > + CANCEL_FCOPY,
> > +};
> > +
> > +struct hv_fcopy_hdr {
> > + enum hv_fcopy_op operation;
> > + uuid_le service_id0; /* currently unused */
> > + uuid_le service_id1; /* currently unused */
> > +} __attribute__((packed));
>
> Is enum a fixed size? This struct is used in other structs, so I wonder
> what will happen to the kernel/user protocol if any of that changes. Or
> with a 64bit kernel and 32bit daemon. Maybe operation should be __u32?
I will check and make the necessary changes.
Thank you,
K. Y
>
>
> Olaf
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/1] staging: fpgaboot: Xilinx FPGA firmware download driver
On Thu, Jan 16, 2014 at 12:56:15AM -0800, Insop Song wrote:
> On Tue, Jan 14, 2014 at 10:00 AM, Greg KH wrote:
> > On Tue, Jan 14, 2014 at 01:37:50AM -0800, Insop Song wrote:
> >> On Tue, Jan 14, 2014 at 1:18 AM, Insop Song wrote:
> >> > On Mon, Jan 13, 2014 at 10:44 AM, Greg KH
> >> > wrote:
> >> >> On Sat, Jan 11, 2014 at 04:37:32PM -0800, Insop Song wrote:
> >> >>> On Sat, Jan 11, 2014 at 4:10 PM, Greg KH
> >> >>> wrote:
> >> >>> > On Sat, Jan 11, 2014 at 03:56:48PM -0800, Insop Song wrote:
> >> >>> >> On Sat, Jan 11, 2014 at 1:05 PM, Greg KH
> >> >>> >> wrote:
> >> >>> >> > On Sat, Jan 11, 2014 at 12:51:28PM -0800, Greg KH wrote:
> >> >>> >> >> On Thu, Jan 09, 2014 at 11:48:04AM -0800, Insop Song wrote:
> >> >>> >> >> > This driver downloads Xilinx FPGA firmware using gpio pins.
> >> >>> >> >> > It loads Xilinx FPGA bitstream format firmware image and
> >> >>> >> >> > program the Xilinx FPGA using SelectMAP (parallel) mode.
> >> >>> >> >> >
> >> >>> >> >> > Signed-off-by: Insop Song
> >> >>> >> >>
> >> >>> >> >> This patch breaks the build on my machine, please be more
> >> >>> >> >> careful:
> >> >>> >> >>
> >> >>> >> >> drivers/staging/gs_fpgaboot/gs_fpgaboot.c:30:28: fatal error:
> >> >>> >> >> include/asm/io.h: No such file or directory
> >> >>> >> >> #include
> >> >>> >> >>
> >> >>> >> >> Please fix this up, test it, and resend.
> >> >>> >> >
> >> >>> >> > Also, while you are fixing things up, please just remove the
> >> >>> >> > character
> >> >>> >> > device node from the driver, you aren't doing anything with it,
> >> >>> >> > so it's
> >> >>> >> > not needed.
> >> >>> >> >
> >> >>> >>
> >> >>> >> Thank you for the suggestion, makes a lot of sense and code cleaner.
> >> >>> >>
> >> >>> >> Could you take a quick look?
> >> >>> >> If it is good, will send out a new patch, v5, shortly.
> >> >>> >>
> >> >>> >> Main change is
> >> >>> >>
> >> >>> >> +/* fake device for request_firmware */
> >> >>> >> +static struct platform_device*firmware_pdev;
> >> >>> >>
> >> >>> >> + firmware_pdev = platform_device_register_simple("fpgaboot", -1,
> >> >>> >> + NULL, 0);
> >> >>> >
> >> >>> > Why do you need a platform device? Don't you have something on the
> >> >>> > hardware that describes the fact that this device is in the system,
> >> >>> > or
> >> >>> > not? Like a PCI id? DMI id? Device Tree entry?
> >> >>> >
> >> >>> > You must have something that you can trigger off of.
> >> >>> >
> >> >>>
> >> >>> This driver is only to download fpga firmware, not intended to keep
> >> >>> managing the fpga device after the downloading. The device we use is
> >> >>> not part of the device tree or PCI.
> >> >>
> >> >> But how does the system know to load it or not, automatically?
> >> >>
> >> >>> It downloads the image and after that no need to be existing.
> >> >>
> >> >> Then why not have the module remove itself?
> >> >>
> >> >
> >> > The following (insmod to load firmware and rmmod remove the module)
> >> > commands are triggered from user space application. They are wrapped
> >> > as a script and triggered from an application.
> >> > Once the firmware is loaded, then this driver's job is done.
> >
> > Yes, but that's not the "normal" way kernel drivers interact with
> > userspace. Please don't try to create something that is different than
> > the thousands of other drivers out there, and rely on custom userspace
> > scripts. Let's use the infrastructure we already have in place for
> > doing this exact thing.
> >
> >> >>> Here is how we use
> >> >>>
> >> >>> $ insmod gs_fpga.ko file="xlinx_fpga_top_bitstream.bit"
> >> >>> $ rmmod gs_fpga
> >> >>
> >> >> Linux modules should be able to be autoloaded based on hardware present
> >> >> in the system, it's been that way for over a decade now.
> >> >>
> >> >
> >> >
> >> Greg, I think it's better to add more explanation for FPGA.
> >
> > I know what a FPGA is, thanks :)
> >
> >> FPGA (Field-programmable gate array) is not functional until the
> >> firmware is downloaded.
> >> That means that it is not easy, if not possible, to auto detect the
> >> device. This is why "insmod" is manually called.
> >
> > That's not true, there has to be some way to detect the device is in the
> > system, otherwise what's to prevent you from loading the module on a
> > system that doesn't have the hardware and having bad things happen?
> >
> > There has to be some way to detect this device is present, right? What
> > is it? Where is it located in the device tree description?
> >
>
>
> There is no way to detect FPGA until it is programmed.
> This is a reason and the only reason of this driver to download the
> program to the FPGA so that it can function.
So how do you get the memory locations of where the FPGA is in the
system in order to be able to send data to it? Surely that's in the
device tree file somewhere, right?
thanks,
greg k-h
___
devel mailing list
d
RE: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
> -Original Message- > From: Olaf Hering [mailto:o...@aepfle.de] > Sent: Thursday, January 16, 2014 2:17 AM > To: KY Srinivasan > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; a...@canonical.com; jasow...@redhat.com > Subject: Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service > > On Tue, Jan 14, K. Y. Srinivasan wrote: > > > +++ b/include/linux/hyperv.h > > @@ -26,6 +26,8 @@ > > #define _HYPERV_H > > > > #include > > +#include > > +#include > > limits.h is not required. Will get rid of it. Thanks, K. Y > > Olaf ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On 2014-01-15 19:22, Chase Southwood wrote:
This patch for ni_mio_common.c changes out a while loop for a timeout,
which is preferred.
Signed-off-by: Chase Southwood
---
Hartley,
I sincerely apologize for the obvious mistake, I thought I had built it
but clearly I made a mistake somewhere, as your observation is exactly
correct. It is now fixed.
Thanks,
Chase Southwood
2: Changed from simple clean-up to swapping a timeout in for a while loop.
3: Removed extra counter variable, and added error checking.
4: No longer using counter variable, using jiffies instead.
5: udelay for 10u, instead of 1u.
6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
in order to use cpu_relax.
7: Fix typo (msec vs msecs).
drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
b/drivers/staging/comedi/drivers/ni_mio_common.c
index 457b884..ab7a74c 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -55,6 +55,7 @@
#include
#include
#include
+#include
#include "8255.h"
#include "mite.h"
#include "comedi_fc.h"
@@ -687,12 +688,21 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
{
const struct ni_board_struct *board = comedi_board(dev);
struct ni_private *devpriv = dev->private;
+ unsigned long timeout;
if (board->reg_type == ni_reg_6143) {
/* Flush the 6143 data FIFO */
ni_writel(0x10, AIFIFO_Control_6143); /* Flush fifo */
ni_writel(0x00, AIFIFO_Control_6143); /* Flush fifo */
- while (ni_readl(AIFIFO_Status_6143) & 0x10) ; /* Wait
for complete */
+ /* Wait for complete */
+ timeout = jiffies + msecs_to_jiffies(500);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ cpu_relax();
+ }
} else {
devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
if (board->reg_type == ni_reg_625x) {
Sorry this is your worst nightmare, Chase. Your patch is great and has
been modified in all the ways various people have suggested, but I don't
think it is suitable.
After examining the code, it turns out ni_clear_ai_fifo() is sometimes
called from an interrupt service routine and I don't think you can wait
for jiffies to change in that case. I think we need to go back to your
PATCH v2 and fix up the checkpatch.pl warning that Greg mentioned.
Sorry about that.
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
On Tue, Jan 14, K. Y. Srinivasan wrote:
This function should return valid numbers:
> +static ssize_t fcopy_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + int error = 0;
> +
> + if (count != sizeof(int))
> + return 0;
Should be an error.
> +
> + if (copy_from_user(&error, buf, sizeof(int)))
> + return -EFAULT;
> +
> + if (in_hand_shake) {
> + fcopy_handle_handshake();
> + return 0;
Should be sizeof(int).
> + }
> +
> + /*
> + * Complete the transaction by forwarding the result
> + * to the host. But first, cancel the timeout.
> + */
> + if (cancel_delayed_work_sync(&fcopy_work))
> + fcopy_respond_to_host(error);
> +
> + return sizeof(int);
> +}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On 2014-01-16 02:30, Greg KH wrote: On Wed, Jan 15, 2014 at 06:29:21PM +, Hartley Sweeten wrote: On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote: Sleep for at least 10, as I think that's the smallest time delay you can sleep for anyway (meaning it will be that long no matter what number you put there less than 10, depending on the hardware used of course.) A bit off topic here but I have a somewhat related question about timeouts. There are a number of comedi drivers that do a "wait for end-of-conversion" as part of the (*insn_read) for an analog input subdevice or (*insn_write) for an analog output subdevice. These functions return an errno if a timeout occurs. Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting returned to the user as the result of the unlocked_ioctl file operation. What is the more appropriate errno? Or is there is better one that should be used? I think they should all be -ETIMEDOUT, -ETIME is used for something else, and shouldn't be sent to userspace as I don't think it knows what to do with it. Linux libc ought to deal with both. glibc 2.17 has the following error strings in the "C" locale: ETIMEDOUT --> "Connection timed out" ETIME --> "Timer expired" I think BSD has: ETIMEDOUT --> "Operation timed out" ETIME doesn't exist so -ETIMEDOUT is better, even though the error string is a bit dodgy in some cases under Linux. This betrays the original purpose of the ETIMEDOUT error code, in the same way that the identifier and original purpose of the ENOTTY error code is betrayed by the original error string "Not a typewriter" (which is now "Inappropriate ioctl for device" in current glibc). -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
On Tue, Jan 14, K. Y. Srinivasan wrote:
> Implement the file copy service for Linux guests on Hyper-V. This permits the
> host to copy a file (over VMBUS) into the guest. This facility is part of
> "guest integration services" supported on the Windows platform.
> Here is a link that provides additional details on this functionality:
The change below fixes some warnings in the daemon code.
Compile tested only.
I also think the newlines in some of the syslog calls should be removed.
Olaf
hv_fcopy_daemon.c: In function 'hv_start_fcopy':
hv_fcopy_daemon.c:44:3: warning: format '%s' expects argument of type 'char *',
but argument 3 has type '__u16 *' [-Wformat=]
smsg->file_name);
^
hv_fcopy_daemon.c:44:3: warning: format '%s' expects argument of type 'char *',
but argument 5 has type '__u16 *' [-Wformat=]
hv_fcopy_daemon.c:57:6: warning: format '%s' expects argument of type 'char *',
but argument 3 has type '__u16 *' [-Wformat=]
errno, strerror(errno));
^
hv_fcopy_daemon.c:61:4: warning: format '%s' expects argument of type 'char *',
but argument 3 has type '__u16 *' [-Wformat=]
syslog(LOG_ERR, "Invalid path: %s\n", smsg->path_name);
^
hv_fcopy_daemon.c: In function 'main':
hv_fcopy_daemon.c:117:8: warning: ignoring return value of 'daemon', declared
with attribute warn_unused_result [-Wunused-result]
daemon(1, 0);
^
hv_fcopy_daemon.c:132:7: warning: ignoring return value of 'write', declared
with attribute warn_unused_result [-Wunused-result]
write(fcopy_fd, &version, sizeof(int));
^
hv_fcopy_daemon.c:171:9: warning: ignoring return value of 'pwrite', declared
with attribute warn_unused_result [-Wunused-result]
pwrite(fcopy_fd, &error, sizeof(int), 0);
^
Signed-off-by: Olaf Hering
diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
index c0e5c90..d1fadb7 100644
--- a/tools/hv/hv_fcopy_daemon.c
+++ b/tools/hv/hv_fcopy_daemon.c
@@ -35,14 +35,14 @@
#include
static int target_fd;
-char target_fname[W_MAX_PATH];
+static char target_fname[W_MAX_PATH];
static int hv_start_fcopy(struct hv_start_fcopy *smsg)
{
int error = HV_E_FAIL;
- sprintf(target_fname, "%s%s%s", smsg->path_name, "/",
- smsg->file_name);
+ snprintf(target_fname, sizeof(target_fname), "%s/%s",
+ (char *)smsg->path_name, (char*)smsg->file_name);
syslog(LOG_INFO, "Target file name: %s\n", target_fname);
/*
@@ -54,12 +54,12 @@ static int hv_start_fcopy(struct hv_start_fcopy *smsg)
if (mkdir((char *)smsg->path_name, 0755)) {
syslog(LOG_ERR,
"Failed to create '%s'; error: %d %s\n",
- smsg->path_name,
+ (char *)smsg->path_name,
errno, strerror(errno));
goto done;
}
} else {
- syslog(LOG_ERR, "Invalid path: %s\n", smsg->path_name);
+ syslog(LOG_ERR, "Invalid path: %s", (char
*)smsg->path_name);
goto done;
}
}
@@ -115,7 +115,8 @@ int main(void)
char *buffer[4096 * 2];
struct hv_fcopy_hdr *in_msg;
- daemon(1, 0);
+ if (daemon(1, 0))
+ return 1;
openlog("HV_FCOPY", 0, LOG_USER);
syslog(LOG_INFO, "HV_FCOPY starting; pid is:%d", getpid());
@@ -130,7 +131,10 @@ int main(void)
/*
* Register with the kernel.
*/
- write(fcopy_fd, &version, sizeof(int));
+ if (write(fcopy_fd, &version, sizeof(int)) != sizeof(int)) {
+ syslog(LOG_ERR, "write failed: %s",strerror(errno));
+ exit(EXIT_FAILURE);
+ }
while (1) {
/*
@@ -169,6 +173,9 @@ int main(void)
}
- pwrite(fcopy_fd, &error, sizeof(int), 0);
+ if (pwrite(fcopy_fd, &error, sizeof(int), 0) != sizeof(int)) {
+ syslog(LOG_ERR, "pwrite failed: %s",strerror(errno));
+ exit(EXIT_FAILURE);
+ }
}
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
On Tue, Jan 14, K. Y. Srinivasan wrote: > +++ b/include/linux/hyperv.h > @@ -26,6 +26,8 @@ > #define _HYPERV_H > > #include > +#include > +#include limits.h is not required. Olaf ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/1] Drivers: hv: Implement the file copy service
On Tue, Jan 14, K. Y. Srinivasan wrote:
> +enum hv_fcopy_op {
> + START_FILE_COPY = 0,
> + WRITE_TO_FILE,
> + COMPLETE_FCOPY,
> + CANCEL_FCOPY,
> +};
> +
> +struct hv_fcopy_hdr {
> + enum hv_fcopy_op operation;
> + uuid_le service_id0; /* currently unused */
> + uuid_le service_id1; /* currently unused */
> +} __attribute__((packed));
Is enum a fixed size? This struct is used in other structs, so I wonder
what will happen to the kernel/user protocol if any of that changes. Or
with a 64bit kernel and 32bit daemon. Maybe operation should be __u32?
Olaf
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] mfd: Add realtek USB card reader driver
> >>+static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr,
> >>+ unsigned int pipe, struct scatterlist *sg, int num_sg,
> >>+ unsigned int length, unsigned int *act_len, int timeout)
> >>+{
> >>+ int ret;
> >>+
> >>+ dev_dbg(&ucr->pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
> >>+ __func__, length, num_sg);
> >>+ ret = usb_sg_init(&ucr->current_sg, ucr->pusb_dev, pipe, 0,
> >>+ sg, num_sg, length, GFP_NOIO);
> >>+ if (ret)
> >>+ return ret;
> >>+
> >>+ ucr->sg_timer.expires = jiffies + msecs_to_jiffies(timeout);
> >>+ add_timer(&ucr->sg_timer);
> >>+ usb_sg_wait(&ucr->current_sg);
> >>+ del_timer(&ucr->sg_timer);
> >>+
> >>+ if (act_len)
> >>+ *act_len = ucr->current_sg.bytes;
> >>+
> >>+ return ucr->current_sg.status;
> >>+}
> >
> >Code looks fine, but shouldn't this live in a USB driver?
> >
> We have studied drivers in usb/storage, the place that most likely
> to put the driver. All of them are for STANDARD usb mass storage
> class(something like USB_CLASS_MASS_STORAGE). But this is not the
> same case. This driver is for our vendor-class device with
> completely different protocol. It is really an USB interfaced flash
> card command converter(or channel) device rather than a real
> storage. It also has two derived modules(rtsx_usb_sdmmc,
> rtsx_usb_memstick) for other two subsystems.
>
> We also have another driver: rtsx_pcr.c resident in mfd/ for similar
> devices but of PCIe interface. It is nature for us to choose the
> same place for this variant.
Very well, as long as it truly does not fit in drivers/usb. It would
be good to have one of the USB folk give the nod though.
> >>+ }
> >>+
> >>+ /* set USB interface data */
> >>+ usb_set_intfdata(intf, ucr);
> >>+
> >>+ ucr->vendor_id = id->idVendor;
> >>+ ucr->product_id = id->idProduct;
> >>+ ucr->cmd_buf = ucr->rsp_buf = ucr->iobuf;
> >>+
> >>+ mutex_init(&ucr->dev_mutex);
> >>+
> >>+ ucr->pusb_intf = intf;
> >>+
> >>+ /* initialize */
> >>+ ret = rtsx_usb_init_chip(ucr);
> >>+ if (ret)
> >>+ goto out_init_fail;
> >>+
> >>+ for (i = 0; i < ARRAY_SIZE(rtsx_usb_cells); i++) {
> >>+ rtsx_usb_cells[i].platform_data = &ucr;
> >
> >You've already put this in your drvdata (or ntfdata, as it's called
> >here). Why do you also need it in platform_data?
>
> Derived modules rtsx_usb_sdmmc and rtsx_usb_memstick will retrieve
> this from platform_data. They will not be aware of usb interface
> struct.
They don't need to. If the MMC or Memstick subsystems do not have
their own helpers you can just use something like:
struct rtsx_ucr *ucr = dev_get_drvdata(pdev->dev.parent);
Naturally this is untested code and might not work off the bat, but it
does give you some idea of what you can do without iterating through
and populating each sub-device's platform data.
> >>+#define DRV_NAME_RTSX_USB"rtsx_usb"
> >>+#define DRV_NAME_RTSX_USB_SDMMC "rtsx_usb_sdmmc"
> >>+#define DRV_NAME_RTSX_USB_MS "rtsx_usb_ms"
> >
> >Can you just put the names in the correct places please?
> >
> Do you mean just remove these definitions and fill the string
> directly at the using place?
I do.
I find these types of defines unhelpful and somewhat obfuscating.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 1/1] staging: fpgaboot: Xilinx FPGA firmware download driver
On Tue, Jan 14, 2014 at 10:00 AM, Greg KH wrote:
> On Tue, Jan 14, 2014 at 01:37:50AM -0800, Insop Song wrote:
>> On Tue, Jan 14, 2014 at 1:18 AM, Insop Song wrote:
>> > On Mon, Jan 13, 2014 at 10:44 AM, Greg KH
>> > wrote:
>> >> On Sat, Jan 11, 2014 at 04:37:32PM -0800, Insop Song wrote:
>> >>> On Sat, Jan 11, 2014 at 4:10 PM, Greg KH
>> >>> wrote:
>> >>> > On Sat, Jan 11, 2014 at 03:56:48PM -0800, Insop Song wrote:
>> >>> >> On Sat, Jan 11, 2014 at 1:05 PM, Greg KH
>> >>> >> wrote:
>> >>> >> > On Sat, Jan 11, 2014 at 12:51:28PM -0800, Greg KH wrote:
>> >>> >> >> On Thu, Jan 09, 2014 at 11:48:04AM -0800, Insop Song wrote:
>> >>> >> >> > This driver downloads Xilinx FPGA firmware using gpio pins.
>> >>> >> >> > It loads Xilinx FPGA bitstream format firmware image and
>> >>> >> >> > program the Xilinx FPGA using SelectMAP (parallel) mode.
>> >>> >> >> >
>> >>> >> >> > Signed-off-by: Insop Song
>> >>> >> >>
>> >>> >> >> This patch breaks the build on my machine, please be more careful:
>> >>> >> >>
>> >>> >> >> drivers/staging/gs_fpgaboot/gs_fpgaboot.c:30:28: fatal error:
>> >>> >> >> include/asm/io.h: No such file or directory
>> >>> >> >> #include
>> >>> >> >>
>> >>> >> >> Please fix this up, test it, and resend.
>> >>> >> >
>> >>> >> > Also, while you are fixing things up, please just remove the
>> >>> >> > character
>> >>> >> > device node from the driver, you aren't doing anything with it, so
>> >>> >> > it's
>> >>> >> > not needed.
>> >>> >> >
>> >>> >>
>> >>> >> Thank you for the suggestion, makes a lot of sense and code cleaner.
>> >>> >>
>> >>> >> Could you take a quick look?
>> >>> >> If it is good, will send out a new patch, v5, shortly.
>> >>> >>
>> >>> >> Main change is
>> >>> >>
>> >>> >> +/* fake device for request_firmware */
>> >>> >> +static struct platform_device*firmware_pdev;
>> >>> >>
>> >>> >> + firmware_pdev = platform_device_register_simple("fpgaboot", -1,
>> >>> >> + NULL, 0);
>> >>> >
>> >>> > Why do you need a platform device? Don't you have something on the
>> >>> > hardware that describes the fact that this device is in the system, or
>> >>> > not? Like a PCI id? DMI id? Device Tree entry?
>> >>> >
>> >>> > You must have something that you can trigger off of.
>> >>> >
>> >>>
>> >>> This driver is only to download fpga firmware, not intended to keep
>> >>> managing the fpga device after the downloading. The device we use is
>> >>> not part of the device tree or PCI.
>> >>
>> >> But how does the system know to load it or not, automatically?
>> >>
>> >>> It downloads the image and after that no need to be existing.
>> >>
>> >> Then why not have the module remove itself?
>> >>
>> >
>> > The following (insmod to load firmware and rmmod remove the module)
>> > commands are triggered from user space application. They are wrapped
>> > as a script and triggered from an application.
>> > Once the firmware is loaded, then this driver's job is done.
>
> Yes, but that's not the "normal" way kernel drivers interact with
> userspace. Please don't try to create something that is different than
> the thousands of other drivers out there, and rely on custom userspace
> scripts. Let's use the infrastructure we already have in place for
> doing this exact thing.
>
>> >>> Here is how we use
>> >>>
>> >>> $ insmod gs_fpga.ko file="xlinx_fpga_top_bitstream.bit"
>> >>> $ rmmod gs_fpga
>> >>
>> >> Linux modules should be able to be autoloaded based on hardware present
>> >> in the system, it's been that way for over a decade now.
>> >>
>> >
>> >
>> Greg, I think it's better to add more explanation for FPGA.
>
> I know what a FPGA is, thanks :)
>
>> FPGA (Field-programmable gate array) is not functional until the
>> firmware is downloaded.
>> That means that it is not easy, if not possible, to auto detect the
>> device. This is why "insmod" is manually called.
>
> That's not true, there has to be some way to detect the device is in the
> system, otherwise what's to prevent you from loading the module on a
> system that doesn't have the hardware and having bad things happen?
>
> There has to be some way to detect this device is present, right? What
> is it? Where is it located in the device tree description?
>
There is no way to detect FPGA until it is programmed.
This is a reason and the only reason of this driver to download the
program to the FPGA so that it can function.
If you look at this not from xilinx, which I refer,
http://www.xilinx.com/support/documentation/application_notes/xapp583-fpga-configuration.pdf,
You cannot get any device id or data from the fpga until it is programmed.
>> FPGA firmware download happen ever time system boots, so this is
>> different from occasional "upgrade".
>
> Just like all other modules that use the firmware interface for their
> hardware :)
>
Greg, I am not arguing about the way it is now.
I just want to raise a point that FPGA is different until it is
programmed, my driver is e
Re: [PATCH 1/3] mfd: Add realtek USB card reader driver
On 01/14/2014 09:04 PM, Lee Jones wrote:
From: Roger Tseng
Realtek USB card reader provides a channel to transfer command or data to flash
memory cards. This driver exports host instances for mmc and memstick subsystems
and handles basic works.
Signed-off-by: Roger Tseng
[snip]
+static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr,
+ unsigned int pipe, struct scatterlist *sg, int num_sg,
+ unsigned int length, unsigned int *act_len, int timeout)
+{
+ int ret;
+
+ dev_dbg(&ucr->pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
+ __func__, length, num_sg);
+ ret = usb_sg_init(&ucr->current_sg, ucr->pusb_dev, pipe, 0,
+ sg, num_sg, length, GFP_NOIO);
+ if (ret)
+ return ret;
+
+ ucr->sg_timer.expires = jiffies + msecs_to_jiffies(timeout);
+ add_timer(&ucr->sg_timer);
+ usb_sg_wait(&ucr->current_sg);
+ del_timer(&ucr->sg_timer);
+
+ if (act_len)
+ *act_len = ucr->current_sg.bytes;
+
+ return ucr->current_sg.status;
+}
Code looks fine, but shouldn't this live an a USB driver?
We have studied drivers in usb/storage, the place that most likely to
put the driver. All of them are for STANDARD usb mass storage
class(something like USB_CLASS_MASS_STORAGE). But this is not the same
case. This driver is for our vendor-class device with completely
different protocol. It is really an USB interfaced flash card command
converter(or channel) device rather than a real storage. It also has two
derived modules(rtsx_usb_sdmmc, rtsx_usb_memstick) for other two subsystems.
We also have another driver: rtsx_pcr.c resident in mfd/ for similar
devices but of PCIe interface. It is nature for us to choose the same
place for this variant.
[snip]
+ }
+
+ /* set USB interface data */
+ usb_set_intfdata(intf, ucr);
+
+ ucr->vendor_id = id->idVendor;
+ ucr->product_id = id->idProduct;
+ ucr->cmd_buf = ucr->rsp_buf = ucr->iobuf;
+
+ mutex_init(&ucr->dev_mutex);
+
+ ucr->pusb_intf = intf;
+
+ /* initialize */
+ ret = rtsx_usb_init_chip(ucr);
+ if (ret)
+ goto out_init_fail;
+
+ for (i = 0; i < ARRAY_SIZE(rtsx_usb_cells); i++) {
+ rtsx_usb_cells[i].platform_data = &ucr;
You've already put this in your drvdata (or ntfdata, as it's called
here). Why do you also need it in platform_data?
Derived modules rtsx_usb_sdmmc and rtsx_usb_memstick will retrieve this
from platform_data. They will not be aware of usb interface struct.
[snip]
+#ifdef CONFIG_PM
+static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct rtsx_ucr *ucr =
+ (struct rtsx_ucr *)usb_get_intfdata(intf);
+
+ dev_dbg(&intf->dev, "%s called with pm message 0x%04u\n",
+ __func__, message.event);
+
+ mutex_lock(&ucr->dev_mutex);
+ rtsx_usb_turn_off_led(ucr);
That's it? That's all you do during suspend? :)
Yes. The rest of power things like turning-off card power or clock
should be taken care by cores of derived modules(mmc, memstick). We put
only one task here to make sure the LED is off, preventing any module
from turn it on but doesn't turn off before suspend.
+ mutex_unlock(&ucr->dev_mutex);
+ return 0;
+}
+
+static int rtsx_usb_resume(struct usb_interface *intf)
+{
Don't you want to turn the LED back on here?
Or will that happen automatically when you write/read to/from it again?
Yes, it would blink again during use. The turn-off before suspend is not
a permanent power off.
+ return 0;
+}
+
+static int rtsx_usb_reset_resume(struct usb_interface *intf)
+{
+ struct rtsx_ucr *ucr =
+ (struct rtsx_ucr *)usb_get_intfdata(intf);
+
+ rtsx_usb_reset_chip(ucr);
+ return 0;
+}
+
+#else /* CONFIG_PM */
+
+#define rtsx_usb_suspend NULL
+#define rtsx_usb_resume NULL
+#define rtsx_usb_reset_resume NULL
+
+#endif /* CONFIG_PM */
+
+
+static int rtsx_usb_pre_reset(struct usb_interface *intf)
+{
+ struct rtsx_ucr *ucr = (struct rtsx_ucr *)usb_get_intfdata(intf);
+
+ mutex_lock(&ucr->dev_mutex);
Is this normal?
Yes. It is used to prevent commands during port reset. Same to the one
in usb/storage/usb.c.
[snip]
+#include
+
+/* related module names */
+#define RTSX_USB_SD_CARD 0
+#define RTSX_USB_MS_CARD 1
+
+#define DRV_NAME_RTSX_USB"rtsx_usb"
+#define DRV_NAME_RTSX_USB_SDMMC "rtsx_usb_sdmmc"
+#define DRV_NAME_RTSX_USB_MS "rtsx_usb_ms"
Can you just put the names in the correct places please?
Do you mean just remove these definitions and fill the string directly
at the using place?
+/* endpoint numbers */
+#define EP_BULK_OUT 1
+#define EP_BULK_IN 2
+#define EP_INTR_IN 3
I assume these aren't defined anywhere else?
It should not be defined anywhere else since it is really depends on the
hardware design but not any general
