From: Mauro Carvalho Chehab <mchehab@infradead.org>
To: "Udi Atar" <udia@siano-ms.com>
Cc: "Michael Krufky" <mkrufky@linuxtv.org>,
"Uri Shkolnik" <urishk@yahoo.com>,
"LinuxML" <linux-media@vger.kernel.org>
Subject: Re: [PATCH] [09051_49] Siano: smscore - upgrade firmware loading engine
Date: Tue, 16 Jun 2009 13:44:22 -0300 [thread overview]
Message-ID: <20090616134422.3006c142@pedra.chehab.org> (raw)
In-Reply-To: <3E442BA883529143B4AB72530285FC5D01E212C0@s-mail.siano-ms.ent>
Em Tue, 16 Jun 2009 15:39:01 +0300
"Udi Atar" <udia@siano-ms.com> 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@vger.kernel.org [mailto:linux-media-owner@vger.kernel.org] 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@yahoo.com> wrote:
> >
> > # HG changeset patch
> > # User Uri Shkolnik <uris@siano-ms.com>
> > # Date 1242748115 -10800
> > # Node ID 4d75f9d1c4f96d65a8ad312c21e488a212ee58a3
> > # Parent cfb4106f3ceaee9fe8f7e3acc9d4adec1baffe5e
> > [09051_49] Siano: smscore - upgrade firmware loading engine
> >
> > From: Uri Shkolnik <uris@siano-ms.com>
> >
> > Upgrade the firmware loading (download and switching) engine.
> >
> > Priority: normal
> >
> > Signed-off-by: Uri Shkolnik <uris@siano-ms.com>
> >
> > 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@vger.kernel.org
> > 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@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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Mauro
next prev parent reply other threads:[~2009-06-16 16:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-05-19 15:43 [PATCH] [09051_49] Siano: smscore - upgrade firmware loading engine Uri Shkolnik
2009-05-19 16:23 ` Michael Krufky
2009-06-16 12:39 ` Udi Atar
2009-06-16 16:44 ` Mauro Carvalho Chehab [this message]
2009-06-17 8:26 ` Udi Atar
2009-05-19 18:01 Uri Shkolnik
2009-05-20 2:41 ` Mauro Carvalho Chehab
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090616134422.3006c142@pedra.chehab.org \
--to=mchehab@infradead.org \
--cc=linux-media@vger.kernel.org \
--cc=mkrufky@linuxtv.org \
--cc=udia@siano-ms.com \
--cc=urishk@yahoo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.