Re: [linux-nfc] [PATCH 8/8] NFC: Add Intel FieldsPeak NFC solution driver

[Samuel Ortiz <sameo@linux.intel.com>]

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@linux.intel.com> 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.