search

Manouchehri / smi2021

smi2021 v4l2 kernel driver
25 stars 12 forks source link

Merging into the mainline kernel #29

Open Manouchehri opened 8 years ago

Manouchehri commented 8 years ago

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 commented 8 years ago

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

jonjonarnearne commented 8 years ago

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 https://github.com/Manouchehri/smi2021/issues/29#issuecomment-235500708, or mute the thread https://github.com/notifications/unsubscribe-auth/ABL2WIhOUKTBLHt1SNcqPDLkSiO5GM90ks5qZv5bgaJpZM4JVx-K .

jonjonarnearne commented 8 years ago

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 commented 6 years ago

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 commented 6 years ago

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

  1. 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.

  2. 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.

  3. 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.

  4. 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;
}
  5. 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.

  6.  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.

  7. 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.

  8. 
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.

9. 
```c
/************************************************************************
 *                                                                   *
 *          DEVICE  -  PROBE   &   DISCONNECT                                *
 *                                                                   *
 ***********************************************************************/

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

   /*
    * DEVICE  -  PROBE   &   DISCONNECT
    */
mastervolkov commented 6 years ago

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

  1.  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.

2.

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.
```c
     *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.
```c
     smi2021->cur_input = i;

     return 0; 
}
jonjonarnearne commented 6 years ago

I've fixed some (all?) of the issues mentioned here in the current git source, but it's been a couple of years so i don't remember exactly :)

On Feb 20, 2018 5:38 PM, "mastervolkov" notifications@github.com wrote:

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

1.

 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.

1.

smi2021_v4l2.c: 92static 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;

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Manouchehri/smi2021/issues/29#issuecomment-367037486, or mute the thread https://github.com/notifications/unsubscribe-auth/ABL2WDqxu3My0x--hbtE_XBLbQ9Cg3mQks5tWvTrgaJpZM4JVx-K .

AXDOOMER commented 6 years ago

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 commented 6 years ago

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?