| Message ID | 1469692512-16863-5-git-send-email-wagi@monom.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote: > From: Daniel Wagner <daniel.wagner@bmw-carit.de> > > Loading firmware is an operation many drivers implement in various > ways > around the completion API. And most of them do it almost in the same > way. Let's reuse the firmware_stat API which is used also by the > firmware_class loader. Apart of streamlining the firmware loading > states > we also document it slightly better. > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de> Irina added and tested that feature, so best for her to comment on this, as I don't have any hardware that would use this feature. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/28/2016 01:22 PM, Bastien Nocera wrote: > On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote: >> From: Daniel Wagner <daniel.wagner@bmw-carit.de> >> >> Loading firmware is an operation many drivers implement in various >> ways >> around the completion API. And most of them do it almost in the same >> way. Let's reuse the firmware_stat API which is used also by the >> firmware_class loader. Apart of streamlining the firmware loading >> states >> we also document it slightly better. >> >> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de> > > Irina added and tested that feature, so best for her to comment on > this, as I don't have any hardware that would use this feature. In case you have any comments on the API, let me know. I'll add Irina to the Cc list in the next version. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-07-28 at 13:59 +0200, Daniel Wagner wrote: > On 07/28/2016 01:22 PM, Bastien Nocera wrote: > > On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote: > > > From: Daniel Wagner <daniel.wagner@bmw-carit.de> > > > > > > Loading firmware is an operation many drivers implement in > > > various > > > ways > > > around the completion API. And most of them do it almost in the > > > same > > > way. Let's reuse the firmware_stat API which is used also by the > > > firmware_class loader. Apart of streamlining the firmware loading > > > states > > > we also document it slightly better. > > > > > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de> > > > > Irina added and tested that feature, so best for her to comment on > > this, as I don't have any hardware that would use this feature. > > In case you have any comments on the API, let me know. I'll add Irina > to > the Cc list in the next version. Looking at the API, I really don't like the mixing of namespaces. Either it's fw_ or it's firmware_ but not a mix of both. Also looks like fw_loading_start() would do nothing as the struct is likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an UNSET enum member? FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes of the other members. Ditto fw_loading_abort() which doesn't abort firmware loading but sets the status. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Looking at the API, I really don't like the mixing of namespaces. > Either it's fw_ or it's firmware_ but not a mix of both. I agree, that was not really clever. struct fw_loading { ... } __fw_loading_*() fw_loading_*() Would that make more sense? firmware_loading_ as prefix is a bit long IMO. > Also looks like fw_loading_start() would do nothing as the struct is > likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an > UNSET enum member? Good point, I cut a corner here a bit :). In the spirit of making things more clear fw_loading_start() should be used. > FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes > of the other members. Ditto fw_loading_abort() which doesn't abort > firmware loading but sets the status. Okay. Thanks a lot for the feedback. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 240b16f..67158d3 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -47,7 +47,7 @@ struct goodix_ts_data { u16 id; u16 version; const char *cfg_name; - struct completion firmware_loading_complete; + struct firmware_stat fwst; unsigned long irq_flags; }; @@ -683,7 +683,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx) err_release_cfg: release_firmware(cfg); - complete_all(&ts->firmware_loading_complete); + fw_loading_done(ts->fwst); } static int goodix_ts_probe(struct i2c_client *client, @@ -705,7 +705,7 @@ static int goodix_ts_probe(struct i2c_client *client, ts->client = client; i2c_set_clientdata(client, ts); - init_completion(&ts->firmware_loading_complete); + firmware_stat_init(&ts->fwst); error = goodix_get_gpio_config(ts); if (error) @@ -766,7 +766,7 @@ static int goodix_ts_remove(struct i2c_client *client) struct goodix_ts_data *ts = i2c_get_clientdata(client); if (ts->gpiod_int && ts->gpiod_rst) - wait_for_completion(&ts->firmware_loading_complete); + fw_loading_wait(ts->fwst); return 0; } @@ -781,7 +781,7 @@ static int __maybe_unused goodix_suspend(struct device *dev) if (!ts->gpiod_int || !ts->gpiod_rst) return 0; - wait_for_completion(&ts->firmware_loading_complete); + fw_loading_wait(ts->fwst); /* Free IRQ as IRQ pin is used as output in the suspend sequence */ goodix_free_irq(ts);