| Message ID | 20211030182813.116672-7-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | power-suppy/i2c/extcon: Add support for cht-wc PMIC without USB-PD support | expand |
On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On most X86/ACPI devices there is no devicetree to supply the necessary > init-data. Instead the firmware already fully initializes the bq25890 > charger at boot. > > At support for a new "ti,skip-init" boolean property to support this. > So far this new property is only used on X86/ACPI (non devicetree) devs, > IOW it is not used in actual devicetree files. The devicetree-bindings > maintainers have requested properties like these to not be added to the > devicetree-bindings, so the new property is deliberately not added > to the existing devicetree-bindings. With 'ti,' prefix it can be a potential collision in name space, for internal properties I would rather use 'linux,' one. ... > + init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init"); > + if (!init->write_cfg) > + return 0; Why to have double negation here? I would rather expect that you will have direct value in the structure and do a respective check in the functions.
Hi, On 10/31/21 00:07, Andy Shevchenko wrote: > On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> On most X86/ACPI devices there is no devicetree to supply the necessary >> init-data. Instead the firmware already fully initializes the bq25890 >> charger at boot. >> >> At support for a new "ti,skip-init" boolean property to support this. >> So far this new property is only used on X86/ACPI (non devicetree) devs, >> IOW it is not used in actual devicetree files. The devicetree-bindings >> maintainers have requested properties like these to not be added to the >> devicetree-bindings, so the new property is deliberately not added >> to the existing devicetree-bindings. > > With 'ti,' prefix it can be a potential collision in name space, for > internal properties I would rather use 'linux,' one. Good point, changed for v2. > ... > >> + init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init"); >> + if (!init->write_cfg) >> + return 0; > > Why to have double negation here? > I would rather expect that you will have direct value in the structure > and do a respective check in the functions. Because in all places except this one we want to know if we need to write the cfg to the device, removing the double negation here would mean adding negation to a init->skip_init check in many places, so this is cleaner. Regards, Hans
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 1ec93a565631..280821a6a6c6 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -73,6 +73,7 @@ enum bq25890_fields { /* initial field values, converted to register values */ struct bq25890_init_data { + u8 write_cfg; /* write firmware cfg to chip? */ u8 ichg; /* charge current */ u8 vreg; /* regulation voltage */ u8 iterm; /* termination current */ @@ -638,7 +639,7 @@ static int bq25890_chip_reset(struct bq25890_device *bq) static int bq25890_rw_init_data(struct bq25890_device *bq) { - bool write = true; + bool write = bq->init_data.write_cfg; int ret; int i; @@ -682,10 +683,12 @@ static int bq25890_hw_init(struct bq25890_device *bq) { int ret; - ret = bq25890_chip_reset(bq); - if (ret < 0) { - dev_dbg(bq->dev, "Reset failed %d\n", ret); - return ret; + if (bq->init_data.write_cfg) { + ret = bq25890_chip_reset(bq); + if (ret < 0) { + dev_dbg(bq->dev, "Reset failed %d\n", ret); + return ret; + } } /* disable watchdog */ @@ -920,6 +923,10 @@ static int bq25890_fw_probe(struct bq25890_device *bq) int ret; struct bq25890_init_data *init = &bq->init_data; + init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init"); + if (!init->write_cfg) + return 0; + ret = bq25890_fw_read_u32_props(bq); if (ret < 0) return ret; @@ -1033,8 +1040,10 @@ static int bq25890_remove(struct i2c_client *client) if (!IS_ERR_OR_NULL(bq->usb_phy)) usb_unregister_notifier(bq->usb_phy, &bq->usb_nb); - /* reset all registers to default values */ - bq25890_chip_reset(bq); + if (bq->init_data.write_cfg) { + /* reset all registers to default values */ + bq25890_chip_reset(bq); + } return 0; }
On most X86/ACPI devices there is no devicetree to supply the necessary init-data. Instead the firmware already fully initializes the bq25890 charger at boot. At support for a new "ti,skip-init" boolean property to support this. So far this new property is only used on X86/ACPI (non devicetree) devs, IOW it is not used in actual devicetree files. The devicetree-bindings maintainers have requested properties like these to not be added to the devicetree-bindings, so the new property is deliberately not added to the existing devicetree-bindings. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/bq25890_charger.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)