mbox series

[v9,0/2] Introduce the BQ256XX family of chargers

Message ID 20210105202949.14677-1-r-rivera-matos@ti.com
Headers show
Series Introduce the BQ256XX family of chargers | expand

Message

Ricardo Rivera-Matos Jan. 5, 2021, 8:29 p.m. UTC 
Hello,

This patchset introduces the bq256xx family of charging ICs. The bq256xx
ICs are highly integrated, buck, switching chargers intended for use in 
smartphones, tablets, and portable electronics.

Ricardo Rivera-Matos (2):
  dt-bindings: power: Add the bq256xx dt bindings
  power: supply: bq256xx: Introduce the BQ256XX charger driver

 .../bindings/power/supply/bq256xx.yaml        |  110 ++
 drivers/power/supply/Kconfig                  |   11 +
 drivers/power/supply/Makefile                 |    1 +
 drivers/power/supply/bq256xx_charger.c        | 1745 +++++++++++++++++
 4 files changed, 1867 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq256xx.yaml
 create mode 100644 drivers/power/supply/bq256xx_charger.c

Comments

Sebastian Reichel Jan. 5, 2021, 9:26 p.m. UTC | #1 
Hi,

On Tue, Jan 05, 2021 at 02:29:49PM -0600, Ricardo Rivera-Matos wrote:
> The BQ256XX family of devices are highly integrated buck chargers
> for single cell batteries.
> 
> Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@ti.com>
> 
> v9 - resolves two warnings issued by kernel test robot

changelog needs to be below --- btw.
(so that git am does not pick it up :))

> ---
> [...]
> +	ret = bq256xx_parse_dt(bq, psy_cfg, dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to read device tree properties%d\n", ret);
> +		return ret;
> +	}
> [...]

If you want to change psy_cfg, you need to pass it by reference
and not by value (i.e. use &psy_cfg here and a pointer as argument
of bq256xx_parse_dt). Providing psy_cfg like this creates a copy
of the struct.

Did you runtime test this version? It should crash when accessing
the properties because of psy_cfg.drv_data being NULL.

> [...]
> +	ret = bq256xx_power_supply_init(bq, psy_cfg, dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register power supply\n");
> +		return ret;
> +	}

Here it's also better to just provide the address of psy_cfg
(but not strictly necessary).

-- Sebastian
Ricardo Rivera-Matos Jan. 5, 2021, 11:27 p.m. UTC | #2 
Sebastian,

On 1/5/21 3:26 PM, Sebastian Reichel wrote:
> Hi,
>
> On Tue, Jan 05, 2021 at 02:29:49PM -0600, Ricardo Rivera-Matos wrote:
>> The BQ256XX family of devices are highly integrated buck chargers
>> for single cell batteries.
>>
>> Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@ti.com>
>>
>> v9 - resolves two warnings issued by kernel test robot
> changelog needs to be below --- btw.
> (so that git am does not pick it up :))
ACK
>
>> ---
>> [...]
>> +	ret = bq256xx_parse_dt(bq, psy_cfg, dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to read device tree properties%d\n", ret);
>> +		return ret;
>> +	}
>> [...]
> If you want to change psy_cfg, you need to pass it by reference
> and not by value (i.e. use &psy_cfg here and a pointer as argument
> of bq256xx_parse_dt). Providing psy_cfg like this creates a copy
> of the struct.
ACK, understood.
>
> Did you runtime test this version? It should crash when accessing
> the properties because of psy_cfg.drv_data being NULL.
ACK, I did not, my mistake. v10 will get tested on the actual hardware.
>
>> [...]
>> +	ret = bq256xx_power_supply_init(bq, psy_cfg, dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register power supply\n");
>> +		return ret;
>> +	}
> Here it's also better to just provide the address of psy_cfg
> (but not strictly necessary).
>
> -- Sebastian
Thanks,
Ricardo