Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merging into the mainline kernel #29

Open
Manouchehri opened this issue Jul 27, 2016 · 9 comments
Open

Merging into the mainline kernel #29

Manouchehri opened this issue Jul 27, 2016 · 9 comments
Labels
help wanted

Comments

@Manouchehri
Copy link
Owner

This module seems mature enough to be merged into the mainline kernel, we should probably send another email out.

Previous attempt: https://lkml.org/lkml/2013/9/1/148
@tosiara
Copy link
Contributor

tosiara commented Jul 27, 2016

@jonjonarnearne you are familiar with the process, can you please help?

@jonjonarnearne
Copy link
Collaborator

Hi, I would love to help, but I don't have time to handle the communication
with the kernel developers. I hope someone else can do that.

I've started to write down some notes about what have to be done, and I
hope to post that here later to day.

If I haven't posted anything here by tomorow, plase ping me again :)
On Jul 27, 2016 8:44 AM, "tosiara" notifications@github.com wrote:

@jonjonarnearne https://github.com/jonjonarnearne you are familiar with
the process, can you please help?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABL2WIhOUKTBLHt1SNcqPDLkSiO5GM90ks5qZv5bgaJpZM4JVx-K
.

@jonjonarnearne
Copy link
Collaborator

It's been some years since I last did this, but this is what needs to be done as I remember it.

  1. You have to clone the linux-media tree from here: https://git.linuxtv.org/media_tree.git
  2. Create a smi2021 directory under drivers/media/usb
  3. Update kconfig and Makefile in drivers/media/usb to add the smi2021 driver.
  4. Check that everything compiles.
  5. Test, and run v4l2-compliance. This program should be downloaded from linuxtv.org. We need to include the output from that program in the email with the patch.
  6. Make sure the coding style in the project is correct according to this: https://www.kernel.org/doc/Documentation/CodingStyle
  7. I think the next step is to create a new branch, and squash all commits into a single one, so we only get a single email in the next step.
  8. Now it's time to run format-patch to create patch files from the current branch. I think it's done like this: git format-patch HEAD~1 to create a single patch from the last commit.
  9. After you have created the patch with git format-patch, you should run the patch trough the checkpatch.pl script which can be found in the scrips directory of the kernel tree. This will do some automated checks to see that the patch follows the coding style of the kernel.
  10. Now you should run the get_maintainer.pl scrip from the same location on the patch. This should give you a list of who we have to send this patch to.
  11. Finish up the cover-letter you should have gotten out of the call to git format-patch in step 8, and add the output of the call to v4l2-compliance in step 5. Add any extra people to the cc list (please include me in the cc list).
  12. Run git send-email to have git send the patches to all selected recipient.

The reason we use the linux-media tree from linuxtv.org instead of the Linus' kernel tree, is that they have all the latest changes for the v4l2 subsystem. Also, if I'm not wrong or something have changed for the last two years, Mauro Carvalho Chehab and/or Hans Verkuil are maintaining that tree, and they are responsible for sending the media-drivers to the mainline kernel.

As this is written from memory, I've probably forgotten some steps, and some of this info might be totally wrong. I'll try to find my old hard-drive, and see if I have any notes there about what I did exactly.

The one thing I remember is that the process of sending these PATHCES/RFCs are pretty exhausting/time consuming, and one kernel-maintainer told me he could spend several days preparing patches for the kernel.

Also, please read:

https://www.kernel.org/doc/Documentation/SubmitChecklist

https://www.kernel.org/doc/Documentation/SubmittingPatches

Please don't hesitate to ask here if you need more help.

@AXDOOMER
Copy link

Previous attempt: https://lkml.org/lkml/2013/9/1/148

The linux-media maintainers have made comments about what needs to be fixed in the driver. The issues that they have found in the code must be fixed first before doing another attempt to get the driver accepted into the Linux kernel.

Mauro's review: https://lkml.org/lkml/2013/10/3/486
Hans' review: https://lkml.org/lkml/2013/10/4/77

@mastervolkov
Copy link
Collaborator

Short variant comments from Mauro Carvalho Chehab
https://lkml.org/lkml/2013/10/3/486

void smi2021_stop_audio(struct smi2021 *smi2021)
{
     /*
      * HACK: Stop the audio subsystem,
      * without this, the pcm middle-layer will hang waiting for more data.
      *
      * Is there a better way to do this?
      */

Better to copy ALSA ML on this. They can give a better review about that,
and help to remove this hack.

smi2021_bootloader.c: 78:79
static const struct firmware *firmware[ARRAY_SIZE(available_fw)];
static int firmwares = -1;

That static "firmwares" var seem ugly. What happens if more than one
device gets connected?

Also, I don't see any need for it to be global. Just move it to the
probe routine as a non-static var, and everything will likely be ok.

static int smi2021_choose_firmware(struct usb_device *udev)
{
     int i, found, id;
     for (i = 0; i < ARRAY_SIZE(available_fw); i++) {
             found = available_fw[i].found;
             id = available_fw[i].id;
             if (firmware_version == id && found >= 0) {
                     dev_info(&udev->dev, "uploading firmware for 0x%x\n",
                                     id);
                     return smi2021_load_firmware(udev, firmware[found]);

Hmm... if all three versions are available, this logic will choose the lowest
found one (0x3c), as it is the first version listed at available_fw array.

It seems that the opposite logic would be better, e. g. to try 0x3f firmware
first.

int smi2021_bootloader_probe(struct usb_interface *intf,
                                     const struct usb_device_id *devid)
{
     struct usb_device *udev = interface_to_usbdev(intf);
     int rc, i;

     /* Check what firmwares are available in the system */
     for (i = 0; i < ARRAY_SIZE(available_fw); i++) {
             dev_info(&udev->dev, "Looking for: %s\n",
                      available_fw[i].name);
             rc = request_firmware(&firmware[firmwares + 1],
                     available_fw[i].name, &udev->dev);

             if (rc == 0) {
                     firmwares++;
                     available_fw[i].found = firmwares;
                     dev_info(&udev->dev, "Found firmware for 0x00%x\n",
                             available_fw[i].id);
             } else if (rc == -ENOENT) {
                     available_fw[i].found = -1;
             } else {
                     dev_err(&udev->dev,
                             "request_firmware failed with: %d\n", rc);
                     goto err_out;
             }

Why to waste memory loading all possible firmwares? Also, what happens if
someone copy or delete a new firmware file while the driver is running, and
hot-plug another device?

Also, we have the issue with the "firmwares" var there, that are not reset
to zero at the beginning of this function.

IMHO, there are lots of troubles there.

I would simply replace this complex logic for a simpler one like:

static int smi2021_choose_firmware(struct usb_device *udev)
{
       int i;
       for (i = 0; i < ARRAY_SIZE(available_fw); i++)
               if (firmware_version == available_fw[i].id)
                       return i;
       return -1;
}

int smi2021_bootloader_probe(struct usb_interface *intf,
                            const struct usb_device_id *devid)
{
       int i, rc;

       /*
        * If user manually request a firmware version, try it first. If it
        * is invalid, fall back to the seek logic.
        */
       if (firmware_version > 0) {
               i = smi2021_choose_firmware(udev, firmware_version);

               if (i >= 0) {
                       rc = request_firmware(firmware, available_fw[i].name, &udev->dev);
                       goto load;
               }
       }

       /* Start seek from the highest firmware version */
       for (i = ARRAY_SIZE(available_fw) - 1; i >= 0; i--) {
               rc = request_firmware(firmware, available_fw[i].name, &udev->dev);>
               if (!rc || rc != -ENOENT)
                       break;
       }

load:
       if (rc == 0) {
               rc = smi2021_load_firmware(udev, firmware);

               release_firmware(firmware);
       }

       return rc;
}

static int smi2021_set_mode(struct smi2021 *smi2021, u8 mode)
{
     int pipe, rc;
     struct mode_ctrl_transfer {
             u8 head;
             u8 mode;
     } *transfer_buf = kzalloc(sizeof(*transfer_buf), GFP_KERNEL);

Please break it into two statements. Saving one line here doesn't help and
make it harder to be read.

     if (transfer_buf == NULL)
             return -ENOMEM;

I would prefer to use here (and on similar occurrences):
if (!transfer_buf)
although Linux Coding Style accept both ways.

static int smi2021_set_reg(struct smi2021 *smi2021, u8 i2c_addr,
                        u16 reg, u8 val)
{
...
...
             transfer_buf->data.smi_data.reg_lo = __cpu_to_le16(reg) & 0xff;
             transfer_buf->data.smi_data.reg_hi = __cpu_to_le16(reg) >> 8;

Better to declare reg_lo/reg_hi as __le16, as this helps to rise a red warn
flag if the conversion is forgotten somewhere. Same on similar functions.

static u32 smi2021_i2c_functionality(struct i2c_adapter *adap)
{
     return I2C_FUNC_SMBUS_EMUL;
}

static struct i2c_algorithm smi2021_algo = {
     .master_xfer = smi2021_i2c_xfer,
     .functionality = smi2021_i2c_functionality,
};

/* gm7113c_init table overrides */
static enum saa7113_r10_ofts r10_ofts = SAA7113_OFTS_VFLAG_BY_VREF;
static bool r10_vrln = true;
static bool r13_adlsb = true;

Why to declare the above as static? That seems to be too risky, if there are
two devices plugged at the same time, and each use a different configuration.

/************************************************************************
 *                                                                   *
 *          DEVICE  -  PROBE   &   DISCONNECT                                *
 *                                                                   *
 ***********************************************************************/

Please use the standard comments multiline comments style at the Kernel:

   /*
    * DEVICE  -  PROBE   &   DISCONNECT
    */

@mastervolkov
Copy link
Collaborator

Short variant comments from Hans Verkuil
https://lkml.org/lkml/2013/10/4/77

     v4l2_device_call_all(&smi2021->v4l2_dev, 0, video, s_routing,
                     smi2021->vid_inputs[smi2021->cur_input].type, 0, 0);

Don't use v4l2_device_call_all for the s_routing op. The meaning of the s_routing
arguments is subdev specific, so this is one of the very few ops that you can't
'broadcast' using v4l2_device_call_all. Since you have only one subdev anyway, I
would suggest replacing v4l2_device_call_all by v4l2_subdev_call.

smi2021_v4l2.c: 92
static int vidioc_g_std(struct file *file, void *priv, v4l2_std_id *norm) 
{ 
     struct smi2021 *smi2021 = video_drvdata(file);

Please add an empty line here.

     *norm = smi2021->cur_norm;
     return 0;
}

static int vidioc_g_input(struct file *file, void *priv, unsigned int *i)
{
     struct smi2021 *smi2021 = video_drvdata(file);

Ditto.

     *i = smi2021->cur_input;
     return 0;
}

static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
{
     struct smi2021 *smi2021 = video_drvdata(file);

Check if the new norm == cur_norm and return 0 if that's the case. Some apps are known
to set the std again after REQBUFS.

     if (vb2_is_busy(&smi2021->vb2q))
             return -EBUSY;

     smi2021->cur_norm = norm;
     if (norm & V4L2_STD_525_60)
             smi2021->cur_height = SMI2021_NTSC_LINES;
     else if (norm & V4L2_STD_625_50)
             smi2021->cur_height = SMI2021_PAL_LINES;
     else
             return -EINVAL;

     v4l2_device_call_all(&smi2021->v4l2_dev, 0, core, s_std,
                          smi2021->cur_norm);
     return 0;
}

static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
{
     struct smi2021 *smi2021 = video_drvdata(file);
     if (i >= smi2021->vid_input_count)
             return -EINVAL;

     v4l2_device_call_all(&smi2021->v4l2_dev, 0, video, s_routing,
             smi2021->vid_inputs[i].type, 0, 0);

As mentioned before, you can't use v4l2_device_call_all for s_routing. This must
be handled per sub-device.

     smi2021->cur_input = i;

     return 0; 
}

@jonjonarnearne
Copy link
Collaborator

jonjonarnearne commented Feb 20, 2018 via email

@AXDOOMER
Copy link

Thanks @mastervolkov for copying the messages into this issue. We can check the source code and confirm if there aren't any changes left to do.

@AXDOOMER
Copy link

I bought two SMI devices from China and they both don't work. I can't test anything. That sucks. Do capture cards from China ever work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jonjonarnearne @mastervolkov @tosiara @AXDOOMER @Manouchehri