Closed Bug 793809 Opened 7 years ago Closed 7 years ago

Take FM radio initialization off main thread

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #664161 - Flags: review?(justin.lebar+bug)
Depends on: 749053
>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
> class RadioUpdate : public nsRunnable {
>   hal::FMRadioOperation mOp;
>+  hal::FMRadioOperationStatus mStatus;
> public:
>-  RadioUpdate(hal::FMRadioOperation op)
>+  RadioUpdate(hal::FMRadioOperation op,
>+              hal::FMRadioOperationStatus status = hal::FM_RADIO_OPERATION_STATUS_SUCCESS)

I almost think this would be cleaner if we didn't have the default param here,
but up to you.

>     : mOp(op)
>+    , mStatus(status)
>   {}
> 
>   NS_IMETHOD Run() {
>     hal::FMRadioOperationInformation info;
>     info.operation() = mOp;
>-    info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS;
>+    info.status() = mStatus;
>     info.frequency() = GetFMRadioFrequency();
>     hal::NotifyFMRadioStatus(info);
>     return NS_OK;
>   }
> };
> 

>+static void
>+initTavaruaRadio(hal::FMRadioSettings &aInfo)
> {

>+  mozilla::ScopedClose fd(sRadioFD);
>   char command[64];
>-  snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", cap.version);
>-  rc = system(command);
>+  snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion);
>+  int rc = system(command);

I'd feel a lot safer if this was 

    int rc = system(nsPrintfCString("/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion).get());

because we can't overflow that.  But this is safe as-is, so you don't need to
change it.

>@@ -201,39 +143,116 @@ EnableFMRadio(const hal::FMRadioSettings& aInfo)

>+static void *
>+runTavaruaRadio(void *)

Please indicate either with a comment or an assertion what thread this is
running on.  Same for initTavaruaRadio.

>+void
>+EnableFMRadio(const hal::FMRadioSettings& aInfo)
>+{

ibid.

>+  sRadioFD = fd;
>+  sRadioSettings = aInfo;
>+  fd.forget();

I think you can do |sRadioFD = fd.forget();|.

r=me with nits picked.
Attachment #664161 - Flags: review?(justin.lebar+bug) → review+
Nits addressed except for the nsPrintfCString part. That line just looks weird to me..
Attachment #664161 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a25086637d1f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.