Re: [RFC V1 7/8] smi2021: Add smi2021_bl.c

2013-03-20 Thread Jon Arne Jørgensen 
On Mon, Mar 18, 2013 at 10:31:14AM +0100, Bjørn Mork wrote:
> Jon Arne Jørgensen  writes:
> 
> > This is the smi2021-bootloader module.
> > This module will upload the firmware for the different somagic devices.
> 
> I really don't understand why you want to make that a separate module.
> Building both the bootlader driver and the real driver into the same
> module will make sure that either both or none are available, document
> the relationship to the end user, and allow you to tell the user that
> firmware files may be needed for the real driver by using the
> MODULE_FIRMWARE macro (which you should do, IMHO).
>

I made this a separate module because there is one version of this
device that doesn't need any firmware.
But I think it would be better to include the bootloader in the real driver
for simplicity.

> > +static unsigned int firmware_version;
> > +module_param(firmware_version, int, 0644);
> > +MODULE_PARM_DESC(firmware_version,
> > +   "Firmware version to be uploaded to device\n"
> > +   "if there are more than one firmware present");
> > +
> > +struct usb_device_id smi2021_bootloader_id_table[] = {
> > +   { USB_DEVICE(0x1c88, 0x0007) },
> > +   { }
> > +};
> > +
> > +struct smi2021_firmware {
> > +   int id;
> > +   const char  *name;
> > +   int found;
> > +};
> > +
> > +struct smi2021_firmware available_fw[] = {
> > +   {
> > +   .id = 0x3c,
> > +   .name = "smi2021_3c.bin",
> > +   },
> > +   {
> > +   .id = 0x3e,
> > +   .name = "smi2021_3e.bin",
> > +   },
> > +   {
> > +   .id = 0x3f,
> > +   .name = "smi2021_3f.bin",
> > +   }
> > +};
> 
> 
> How does the user know which firmware to select?  Will any of them work,
> or are they device specific? Either way I believe you should publish all
> three names using MODULE_FIRMWARE.
> 
>

The problem with this device is that there is no way for the user to
know what version of the device he's got before the firmware is uploaded
to the device.

Any device will accept any firmware, but it will not function without
the correct firmware.

The only way to see what version of the firmware you will need is to
test the driver on a Windows machine with the driver that came with the
device. I have no idea about what the windows driver will do if you have
more than one version of this device.

> > +static int smi2021_load_firmware(struct usb_device *udev,
> > +   const struct firmware *firmware)
> > +{
> > +   int i, size, rc = 0;
> > +   u8 *chunk;
> > +   u16 ack = 0x;
> > +
> > +   if (udev == NULL)
> > +   goto end_out;
> 
> Is this possible? 
> 
> 
> 

Probably not, I'll remove this check :)

> > +   size = FIRMWARE_CHUNK_SIZE + FIRMWARE_HEADER_SIZE;
> > +   chunk = kzalloc(size, GFP_KERNEL);
> > +   chunk[0] = 0x05;
> > +   chunk[1] = 0xff;
> > +
> > +   if (chunk == NULL) {
> 
> This on the other hand, will happen.  But you have already oopsed when
> you test it here...
> 
>

Yes, that is true.
I will fix,

thank you for your review.

> Bjørn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC V1 7/8] smi2021: Add smi2021_bl.c

2013-03-18 Thread Bjørn Mork 
Jon Arne Jørgensen  writes:

> This is the smi2021-bootloader module.
> This module will upload the firmware for the different somagic devices.

I really don't understand why you want to make that a separate module.
Building both the bootlader driver and the real driver into the same
module will make sure that either both or none are available, document
the relationship to the end user, and allow you to tell the user that
firmware files may be needed for the real driver by using the
MODULE_FIRMWARE macro (which you should do, IMHO).

> +static unsigned int firmware_version;
> +module_param(firmware_version, int, 0644);
> +MODULE_PARM_DESC(firmware_version,
> + "Firmware version to be uploaded to device\n"
> + "if there are more than one firmware present");
> +
> +struct usb_device_id smi2021_bootloader_id_table[] = {
> + { USB_DEVICE(0x1c88, 0x0007) },
> + { }
> +};
> +
> +struct smi2021_firmware {
> + int id;
> + const char  *name;
> + int found;
> +};
> +
> +struct smi2021_firmware available_fw[] = {
> + {
> + .id = 0x3c,
> + .name = "smi2021_3c.bin",
> + },
> + {
> + .id = 0x3e,
> + .name = "smi2021_3e.bin",
> + },
> + {
> + .id = 0x3f,
> + .name = "smi2021_3f.bin",
> + }
> +};


How does the user know which firmware to select?  Will any of them work,
or are they device specific? Either way I believe you should publish all
three names using MODULE_FIRMWARE.


> +static int smi2021_load_firmware(struct usb_device *udev,
> + const struct firmware *firmware)
> +{
> + int i, size, rc = 0;
> + u8 *chunk;
> + u16 ack = 0x;
> +
> + if (udev == NULL)
> + goto end_out;

Is this possible? 



> + size = FIRMWARE_CHUNK_SIZE + FIRMWARE_HEADER_SIZE;
> + chunk = kzalloc(size, GFP_KERNEL);
> + chunk[0] = 0x05;
> + chunk[1] = 0xff;
> +
> + if (chunk == NULL) {

This on the other hand, will happen.  But you have already oopsed when
you test it here...


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC V1 7/8] smi2021: Add smi2021_bl.c

2013-03-14 Thread Jon Arne Jørgensen 
This is the smi2021-bootloader module.
This module will upload the firmware for the different somagic devices.

Signed-off-by: Jon Arne Jørgensen 
---
 drivers/media/usb/smi2021/smi2021_bl.c | 254 +
 1 file changed, 254 insertions(+)
 create mode 100644 drivers/media/usb/smi2021/smi2021_bl.c

diff --git a/drivers/media/usb/smi2021/smi2021_bl.c 
b/drivers/media/usb/smi2021/smi2021_bl.c
new file mode 100644
index 000..025b06c
--- /dev/null
+++ b/drivers/media/usb/smi2021/smi2021_bl.c
@@ -0,0 +1,254 @@
+/***
+ * smi2021_bl.c
   *
+ **
+ * USB Driver for SMI2021 - EasyCAP
*
+ * USB ID 1c88:003c
*
+ * 
*
+ * 
*
+ *
+ * Copyright 2011-2013 Jon Arne Jørgensen
+ * 
+ *
+ * Copyright 2011, 2012 Tony Brown, Michal Demin, Jeffry Johnston
+ *
+ * This file is part of SMI2021
+ * http://code.google.com/p/easycap-somagic-linux/
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ *
+ * This driver is heavily influensed by the STK1160 driver.
+ * Copyright (C) 2012 Ezequiel Garcia
+ * 
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define FIRMWARE_CHUNK_SIZE62
+#define FIRMWARE_HEADER_SIZE   2
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jon Arne Jørgensen ");
+MODULE_DESCRIPTION("SMI2021 - Bootloader");
+MODULE_VERSION("0.1");
+
+static unsigned int firmware_version;
+module_param(firmware_version, int, 0644);
+MODULE_PARM_DESC(firmware_version,
+   "Firmware version to be uploaded to device\n"
+   "if there are more than one firmware present");
+
+struct usb_device_id smi2021_bootloader_id_table[] = {
+   { USB_DEVICE(0x1c88, 0x0007) },
+   { }
+};
+
+struct smi2021_firmware {
+   int id;
+   const char  *name;
+   int found;
+};
+
+struct smi2021_firmware available_fw[] = {
+   {
+   .id = 0x3c,
+   .name = "smi2021_3c.bin",
+   },
+   {
+   .id = 0x3e,
+   .name = "smi2021_3e.bin",
+   },
+   {
+   .id = 0x3f,
+   .name = "smi2021_3f.bin",
+   }
+};
+
+static const struct firmware *firmware[ARRAY_SIZE(available_fw)];
+static int firmwares = -1;
+
+static int smi2021_load_firmware(struct usb_device *udev,
+   const struct firmware *firmware)
+{
+   int i, size, rc = 0;
+   u8 *chunk;
+   u16 ack = 0x;
+
+   if (udev == NULL)
+   goto end_out;
+
+   size = FIRMWARE_CHUNK_SIZE + FIRMWARE_HEADER_SIZE;
+   chunk = kzalloc(size, GFP_KERNEL);
+   chunk[0] = 0x05;
+   chunk[1] = 0xff;
+
+   if (chunk == NULL) {
+   dev_err(&udev->dev,
+   "could not allocate space for firmware chunk\n");
+   goto end_out;
+   }
+
+   if (firmware == NULL) {
+   dev_err(&udev->dev, "firmware is NULL\n");
+   rc = -ENODEV;
+   goto free_out;
+   }
+
+   if (firmware->size % FIRMWARE_CHUNK_SIZE) {
+   dev_err(&udev->dev, "firmware has wrong size\n");
+   rc = -ENODEV;
+   goto free_out;
+   }
+
+   rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0x80), 0x01,
+   USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+   0x0001, 0x, &ack, sizeof(ack), 1000);
+
+   if (rc < 0) {
+   dev_err(&udev->dev, "could not prepare device for upload: %d\n",
+   rc);
+   goto free_out;
+   }
+   if (__cpu_to_be16(ack) != 0x0107) {
+   dev_err(&udev->dev, "could not prepare device for upload: %d\n",
+   rc);
+   goto free_out;
+   }
+
+   for (i = 0; i < firmware->size / FIRMWARE_CHUNK_SIZE; i++) {
+   memcpy(chunk + FIRMWARE_HEADER_SIZE,
+   firmware