On Tue, Jun 16, 2009 at 7:44 PM, Mauro Carvalho Chehab<mchehab@xxxxxxxxxxxxx> wrote: > Em Tue, 16 Jun 2009 15:39:01 +0300 > "Udi Atar" <udia@xxxxxxxxxxxx> escreveu: > >> The README.patches file in v4l-dvb clearly states that it is OK to use version checking to allow backporting. >> >> ######################################################################## >> k) Sometimes it is necessary to introduce some testing code inside a >> module or remove parts that are not yet finished. Also, compatibility >> tests may be required to provide backporting. >> >> To allow compatibility tests, linux/version.h is automatically >> included by the building system. This allows adding tests like: >> >> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 16) >> #include <linux/mutex.h> >> #else >> #include <asm/semaphore.h> >> #endif >> ######################################################################## >> >> The patch allows older version of the kernel, and embedded platforms, that choose not to include the "request firmware" mechanism to continue working with the Siano driver. > > I don't see a big issue with this, provided that this won't affect the upstream driver. > >> As for the SMS_HOSTLIB_SUBSYS, the Siano driver supports standards which are not currently implemented in V4L (i.e. CMMB). I see no reason why we should create a duplicate driver for DVB-T and CMMB, if the codebase is exactly the same. > > The proper way is to add support for those standards at DVB API, instead of > using a proprietary API. > > In order to keep the merging process of the pending patches, I suggest you to > remove the the SMS_HOSTLIB_SUBSYS part from this patch and re-submit the > pending ones up to the point where the only pending issue to sync your codebase > with kernel being the API for those non-supported standards. After that, we can > discuss the API improvement needs to support the missing standards. >> >> Best regards, >> Udi Atar >> >> >> -----Original Message----- >> From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media-owner@xxxxxxxxxxxxxxx] On Behalf Of Michael Krufky >> Sent: Tuesday, May 19, 2009 7:24 PM >> To: Uri Shkolnik >> Cc: LinuxML; Mauro Carvalho Chehab >> Subject: Re: [PATCH] [09051_49] Siano: smscore - upgrade firmware loading engine >> >> On Tue, May 19, 2009 at 11:43 AM, Uri Shkolnik <urishk@xxxxxxxxx> wrote: >> > >> > # HG changeset patch >> > # User Uri Shkolnik <uris@xxxxxxxxxxxx> >> > # Date 1242748115 -10800 >> > # Node ID 4d75f9d1c4f96d65a8ad312c21e488a212ee58a3 >> > # Parent cfb4106f3ceaee9fe8f7e3acc9d4adec1baffe5e >> > [09051_49] Siano: smscore - upgrade firmware loading engine >> > >> > From: Uri Shkolnik <uris@xxxxxxxxxxxx> >> > >> > Upgrade the firmware loading (download and switching) engine. >> > >> > Priority: normal >> > >> > Signed-off-by: Uri Shkolnik <uris@xxxxxxxxxxxx> >> > >> > diff -r cfb4106f3cea -r 4d75f9d1c4f9 linux/drivers/media/dvb/siano/smscoreapi.c >> > --- a/linux/drivers/media/dvb/siano/smscoreapi.c Tue May 19 18:38:07 2009 +0300 >> > +++ b/linux/drivers/media/dvb/siano/smscoreapi.c Tue May 19 18:48:35 2009 +0300 >> > @@ -28,7 +28,7 @@ >> > #include <linux/dma-mapping.h> >> > #include <linux/delay.h> >> > #include <linux/io.h> >> > - >> > +#include <linux/uaccess.h> >> > #include <linux/firmware.h> >> > #include <linux/wait.h> >> > #include <asm/byteorder.h> >> > @@ -36,7 +36,13 @@ >> > #include "smscoreapi.h" >> > #include "sms-cards.h" >> > #include "smsir.h" >> > -#include "smsendian.h" >> > +#define MAX_GPIO_PIN_NUMBER 31 >> > + >> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 10) >> > +#define REQUEST_FIRMWARE_SUPPORTED >> > +#else >> > +#define DEFAULT_FW_FILE_PATH "/lib/firmware" >> > +#endif >> > >> > static int sms_dbg; >> > module_param_named(debug, sms_dbg, int, 0644); >> > @@ -459,8 +465,6 @@ static int smscore_init_ir(struct smscor >> > msg->msgData[0] = coredev->ir.controller; >> > msg->msgData[1] = coredev->ir.timeout; >> > >> > - smsendian_handle_tx_message( >> > - (struct SmsMsgHdr_ST2 *)msg); >> > rc = smscore_sendrequest_and_wait(coredev, msg, >> > msg->xMsgHeader. msgLength, >> > &coredev->ir_init_done); >> > @@ -486,12 +490,16 @@ static int smscore_init_ir(struct smscor >> > */ >> > int smscore_start_device(struct smscore_device_t *coredev) >> > { >> > - int rc = smscore_set_device_mode( >> > - coredev, smscore_registry_getmode(coredev->devpath)); >> > + int rc; >> > + >> > +#ifdef REQUEST_FIRMWARE_SUPPORTED >> > + rc = smscore_set_device_mode(coredev, smscore_registry_getmode( >> > + coredev->devpath)); >> > if (rc < 0) { >> > - sms_info("set device mode faile , rc %d", rc); >> > + sms_info("set device mode failed , rc %d", rc); >> > return rc; >> > } >> > +#endif >> > >> > kmutex_lock(&g_smscore_deviceslock); >> > >> > @@ -632,11 +640,14 @@ static int smscore_load_firmware_from_fi >> > loadfirmware_t loadfirmware_handler) >> > { >> > int rc = -ENOENT; >> > + u8 *fw_buf; >> > + u32 fw_buf_size; >> > + >> > +#ifdef REQUEST_FIRMWARE_SUPPORTED >> > const struct firmware *fw; >> > - u8 *fw_buffer; >> > >> > - if (loadfirmware_handler == NULL && !(coredev->device_flags & >> > - SMS_DEVICE_FAMILY2)) >> > + if (loadfirmware_handler == NULL && !(coredev->device_flags >> > + & SMS_DEVICE_FAMILY2)) >> > return -EINVAL; >> > >> > rc = request_firmware(&fw, filename, coredev->device); >> > @@ -645,26 +656,36 @@ static int smscore_load_firmware_from_fi >> > return rc; >> > } >> > sms_info("read FW %s, size=%zd", filename, fw->size); >> > - fw_buffer = kmalloc(ALIGN(fw->size, SMS_ALLOC_ALIGNMENT), >> > - GFP_KERNEL | GFP_DMA); >> > - if (fw_buffer) { >> > - memcpy(fw_buffer, fw->data, fw->size); >> > + fw_buf = kmalloc(ALIGN(fw->size, SMS_ALLOC_ALIGNMENT), >> > + GFP_KERNEL | GFP_DMA); >> > + if (!fw_buf) { >> > + sms_info("failed to allocate firmware buffer"); >> > + return -ENOMEM; >> > + } >> > + memcpy(fw_buf, fw->data, fw->size); >> > + fw_buf_size = fw->size; >> > +#else >> > + if (!coredev->fw_buf) { >> > + sms_info("missing fw file buffer"); >> > + return -EINVAL; >> > + } >> > + fw_buf = coredev->fw_buf; >> > + fw_buf_size = coredev->fw_buf_size; >> > +#endif >> > >> > - rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? >> > - smscore_load_firmware_family2(coredev, >> > - fw_buffer, >> > - fw->size) : >> > - loadfirmware_handler(coredev->context, >> > - fw_buffer, fw->size); >> > + rc = (coredev->device_flags & SMS_DEVICE_FAMILY2) ? >> > + smscore_load_firmware_family2(coredev, fw_buf, fw_buf_size) >> > + : loadfirmware_handler(coredev->context, fw_buf, >> > + fw_buf_size); >> > >> > - kfree(fw_buffer); >> > - } else { >> > - sms_info("failed to allocate firmware buffer"); >> > - rc = -ENOMEM; >> > - } >> > + kfree(fw_buf); >> > >> > +#ifdef REQUEST_FIRMWARE_SUPPORTED >> > release_firmware(fw); >> > - >> > +#else >> > + coredev->fw_buf = NULL; >> > + coredev->fw_buf_size = 0; >> > +#endif >> > return rc; >> > } >> > >> > @@ -911,6 +932,74 @@ int smscore_set_device_mode(struct smsco >> > } >> > >> > /** >> > + * calls device handler to get fw file name >> > + * >> > + * @param coredev pointer to a coredev object returned by >> > + * smscore_register_device >> > + * @param filename pointer to user buffer to fill the file name >> > + * >> > + * @return 0 on success, <0 on error. >> > + */ >> > +int smscore_get_fw_filename(struct smscore_device_t *coredev, int mode, >> > + char *filename) { >> > + int rc = 0; >> > + enum sms_device_type_st type; >> > + char tmpname[200]; >> > + >> > + type = smscore_registry_gettype(coredev->devpath); >> > + >> > +#ifdef REQUEST_FIRMWARE_SUPPORTED >> > + /* driver not need file system services */ >> > + tmpname[0] = '\0'; >> > +#else >> > + sprintf(tmpname, "%s/%s", DEFAULT_FW_FILE_PATH, >> > + smscore_fw_lkup[mode][type]); >> > +#endif >> > + if (copy_to_user(filename, tmpname, strlen(tmpname) + 1)) { >> > + sms_err("Failed copy file path to user buffer\n"); >> > + return -EFAULT; >> > + } >> > + return rc; >> > +} >> > + >> > +/** >> > + * calls device handler to keep fw buff for later use >> > + * >> > + * @param coredev pointer to a coredev object returned by >> > + * smscore_register_device >> > + * @param ufwbuf pointer to user fw buffer >> > + * @param size size in bytes of buffer >> > + * >> > + * @return 0 on success, <0 on error. >> > + */ >> > +int smscore_send_fw_file(struct smscore_device_t *coredev, u8 *ufwbuf, >> > + int size) { >> > + int rc = 0; >> > + >> > + /* free old buffer */ >> > + if (coredev->fw_buf != NULL) { >> > + kfree(coredev->fw_buf); >> > + coredev->fw_buf = NULL; >> > + } >> > + >> > + coredev->fw_buf = kmalloc(ALIGN(size, SMS_ALLOC_ALIGNMENT), GFP_KERNEL >> > + | GFP_DMA); >> > + if (!coredev->fw_buf) { >> > + sms_err("Failed allocate FW buffer memory\n"); >> > + return -EFAULT; >> > + } >> > + >> > + if (copy_from_user(coredev->fw_buf, ufwbuf, size)) { >> > + sms_err("Failed copy FW from user buffer\n"); >> > + kfree(coredev->fw_buf); >> > + return -EFAULT; >> > + } >> > + coredev->fw_buf_size = size; >> > + >> > + return rc; >> > +} >> > + >> > +/** >> > * calls device handler to get current mode of operation >> > * >> > * @param coredev pointer to a coredev object returned by >> > @@ -1280,7 +1369,7 @@ int smsclient_sendrequest(struct smscore >> > } >> > EXPORT_SYMBOL_GPL(smsclient_sendrequest); >> > >> > -#if 0 >> > +#ifdef SMS_HOSTLIB_SUBSYS >> > /** >> > * return the size of large (common) buffer >> > * >> > @@ -1329,7 +1418,7 @@ static int smscore_map_common_buffer(str >> > >> > return 0; >> > } >> > -#endif >> > +#endif /* SMS_HOSTLIB_SUBSYS */ >> > >> > /* old GPIO managments implementation */ >> > int smscore_configure_gpio(struct smscore_device_t *coredev, u32 pin, >> > @@ -1515,7 +1604,6 @@ int smscore_gpio_configure(struct smscor >> > pMsg->msgData[5] = 0; >> > } >> > >> > - smsendian_handle_tx_message((struct SmsMsgHdr_ST *)pMsg); >> > rc = smscore_sendrequest_and_wait(coredev, pMsg, totalLen, >> > &coredev->gpio_configuration_done); >> > >> > @@ -1565,7 +1653,6 @@ int smscore_gpio_set_level(struct smscor >> > pMsg->msgData[1] = NewLevel; >> > >> > /* Send message to SMS */ >> > - smsendian_handle_tx_message((struct SmsMsgHdr_ST *)pMsg); >> > rc = smscore_sendrequest_and_wait(coredev, pMsg, totalLen, >> > &coredev->gpio_set_level_done); >> > >> > @@ -1614,7 +1701,6 @@ int smscore_gpio_get_level(struct smscor >> > pMsg->msgData[1] = 0; >> > >> > /* Send message to SMS */ >> > - smsendian_handle_tx_message((struct SmsMsgHdr_ST *)pMsg); >> > rc = smscore_sendrequest_and_wait(coredev, pMsg, totalLen, >> > &coredev->gpio_get_level_done); >> > >> > >> > >> > >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-media" in >> > the body of a message to majordomo@xxxxxxxxxxxxxxx >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >> >> >> >> This patch should not be merged in its current form. >> >> Linux kernel driver development shall be against the current -rc >> kernel, and there is no need to reinvent the "REQUEST_FIRMWARE" >> mechanism. >> >> Furthermore, the changeset introduces more bits of this >> "SMS_HOSTLIB_SUBSYS" -- this requires a binary library present on the >> host system. This completely violates the "no multiple APIs in >> kernel" and "no proprietary APIs in kernel" guidelines. >> >> Uri, what are your plans for this? >> >> Regards, >> >> Mike >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> 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 majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > Cheers, > Mauro > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > OK. Will resubmit the relevant patches. Best regards, Udi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
