| Message ID | 20200221164735.508324-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [resend,01/10] Input: goodix - Refactor IRQ pin GPIO accesses | expand |
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote: > At least on X86 ACPI platforms it is not necessary to load the > touchscreen > controller config from disk, if it needs to be loaded this has > already been > done by the BIOS / UEFI firmware. > > Even on other (e.g. devicetree) platforms the config-loading as > currently > done has the issue that the loaded cfg file is based on the > controller > model, but the actual cfg is device specific, so the cfg files are > not > part of linux-firmware and this can only work with a device specific > OS > image which includes the cfg file. > > And we do not need access to the GPIOs at all to load the config, if > we > do not have access we can still load the config. > > So all in all tying the decision to try to load the config from disk > to > being able to access the GPIOs is not desirable. This commit adds a > new > load_cfg_from_disk boolean to control the firmware loading instead. > > This commits sets the new bool to true when we set > irq_pin_access_method > to irq_pin_access_gpio, so there are no functional changes. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317 > BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10 > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207 > Cc: Dmitry Mastykin <mastichi@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Looks good. Reviewed-by: Bastien Nocera <hadess@hadess.net> > --- > drivers/input/touchscreen/goodix.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/touchscreen/goodix.c > b/drivers/input/touchscreen/goodix.c > index 08806a00a9b9..eccf07adfae1 100644 > --- a/drivers/input/touchscreen/goodix.c > +++ b/drivers/input/touchscreen/goodix.c > @@ -56,6 +56,7 @@ struct goodix_ts_data { > u16 id; > u16 version; > const char *cfg_name; > + bool load_cfg_from_disk; > struct completion firmware_loading_complete; > unsigned long irq_flags; > enum goodix_irq_pin_access_method irq_pin_access_method; > @@ -654,8 +655,10 @@ static int goodix_get_gpio_config(struct > goodix_ts_data *ts) > > ts->gpiod_rst = gpiod; > > - if (ts->gpiod_int && ts->gpiod_rst) > + if (ts->gpiod_int && ts->gpiod_rst) { > + ts->load_cfg_from_disk = true; > ts->irq_pin_access_method = irq_pin_access_gpio; > + } > > return 0; > } > @@ -952,7 +955,7 @@ static int goodix_ts_probe(struct i2c_client > *client, > > ts->chip = goodix_get_chip_data(ts->id); > > - if (ts->irq_pin_access_method == irq_pin_access_gpio) { > + if (ts->load_cfg_from_disk) { > /* update device config */ > ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL, > "goodix_%d_cfg.bin", ts- > >id); > @@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client > *client) > { > struct goodix_ts_data *ts = i2c_get_clientdata(client); > > - if (ts->irq_pin_access_method == irq_pin_access_gpio) > + if (ts->load_cfg_from_disk) > wait_for_completion(&ts->firmware_loading_complete); > > return 0; > @@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct > device *dev) > return 0; > } > > - wait_for_completion(&ts->firmware_loading_complete); > + if (ts->load_cfg_from_disk) > + wait_for_completion(&ts->firmware_loading_complete); > > /* Free IRQ as IRQ pin is used as output in the suspend > sequence */ > goodix_free_irq(ts);
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 08806a00a9b9..eccf07adfae1 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -56,6 +56,7 @@ struct goodix_ts_data { u16 id; u16 version; const char *cfg_name; + bool load_cfg_from_disk; struct completion firmware_loading_complete; unsigned long irq_flags; enum goodix_irq_pin_access_method irq_pin_access_method; @@ -654,8 +655,10 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts) ts->gpiod_rst = gpiod; - if (ts->gpiod_int && ts->gpiod_rst) + if (ts->gpiod_int && ts->gpiod_rst) { + ts->load_cfg_from_disk = true; ts->irq_pin_access_method = irq_pin_access_gpio; + } return 0; } @@ -952,7 +955,7 @@ static int goodix_ts_probe(struct i2c_client *client, ts->chip = goodix_get_chip_data(ts->id); - if (ts->irq_pin_access_method == irq_pin_access_gpio) { + if (ts->load_cfg_from_disk) { /* update device config */ ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL, "goodix_%d_cfg.bin", ts->id); @@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client *client) { struct goodix_ts_data *ts = i2c_get_clientdata(client); - if (ts->irq_pin_access_method == irq_pin_access_gpio) + if (ts->load_cfg_from_disk) wait_for_completion(&ts->firmware_loading_complete); return 0; @@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct device *dev) return 0; } - wait_for_completion(&ts->firmware_loading_complete); + if (ts->load_cfg_from_disk) + wait_for_completion(&ts->firmware_loading_complete); /* Free IRQ as IRQ pin is used as output in the suspend sequence */ goodix_free_irq(ts);
At least on X86 ACPI platforms it is not necessary to load the touchscreen controller config from disk, if it needs to be loaded this has already been done by the BIOS / UEFI firmware. Even on other (e.g. devicetree) platforms the config-loading as currently done has the issue that the loaded cfg file is based on the controller model, but the actual cfg is device specific, so the cfg files are not part of linux-firmware and this can only work with a device specific OS image which includes the cfg file. And we do not need access to the GPIOs at all to load the config, if we do not have access we can still load the config. So all in all tying the decision to try to load the config from disk to being able to access the GPIOs is not desirable. This commit adds a new load_cfg_from_disk boolean to control the firmware loading instead. This commits sets the new bool to true when we set irq_pin_access_method to irq_pin_access_gpio, so there are no functional changes. BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317 BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10 BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207 Cc: Dmitry Mastykin <mastichi@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/input/touchscreen/goodix.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)