Hi Robert,
On Wed, Apr 01, 2015 at 06:35:31PM +0300, Robert Dolca wrote:
> On Thu, Mar 26, 2015 at 2:30 AM, Samuel Ortiz <sameo@xxxxxxxxxxxxxxx> wrote:
> >> + /* If a patch was applied the new version is checked */
> >> + if (patched) {
> >> + r = nci_init(ndev);
> >> + if (r)
> >> + goto error;
> >> +
> >> + r = fdp_nci_get_versions(ndev);
> >> + if (r)
> >> + goto error;
> >> +
> >> + if (info->otp_version != info->otp_patch_version ||
> >> + info->ram_version != info->ram_patch_version) {
> >> + pr_err("FRP firmware update failed");
> >> + r = -EINVAL;
> >> + }
> >> + }
> >> +
> >> + /* Check if the device has VSC */
> >> + if (fdp_post_fw_vsc_cfg[0]) {
> >> + /* Set the vendor specific configuration */
> >> + r = fdp_nci_set_production_data(ndev, fdp_post_fw_vsc_cfg[3],
> >> + &fdp_post_fw_vsc_cfg[4]);
> >> + if (r)
> >> + goto error;
> >> + }
> >> +
> >> + /* Set clock type and frequency */
> >> + r = fdp_nci_set_clock(ndev, 0, 26000);
> >> + if (r)
> >> + goto error;
> > The version checking, production data setting and clock setting should
> > be part of a post setup notification call. Please add an nci_dev
> > notify() ops that could get called on certain events, for example when
> > NCI is up. Bluetooth's HCI does something along those lines already.
> > From this notification hook you could implement this post setup stage.
> >
> > The idea is for your setup routine to only do firmware update and
> > nothing else. It will make it shorter, and thus easier to read as well.
> If the RAM patch wasn't applied successfully the device can't be used
> so the setup function should fail.
> If the production data (specifically the clock frequency) is not set
> the device can not be used. If the user space tries to start polling
> before the notification is sent the polling will fail. Having it
> called later would mean introducing a race condition.
Sure. Then I'd rather have an additional NCI hook (e.g.
ndev->ops->open()) called synchronously after the setup stage that
could fail and make open fail as well. The idea here is to separate the
2 parts of your logic and make the code more readable.
Cheers,
Samuel.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
