Re: [PATCH v1 2/6] power: supply: Add battery gauge driver for Acer Iconia Tab A500

[Dmitry Osipenko <digetx@gmail.com>]

24.08.2020 17:07, Sebastian Reichel пишет:
> Hi,
...
>> +static int a500_battery_get_serial_number(struct a500_battery *bat,
>> +					  union power_supply_propval *val)
>> +{
>> +	unsigned int i;
>> +	s32 ret = 0;
>> +
>> +	if (bat->serial[0])
>> +		goto done;
>> +
>> +	a500_ec_lock(bat->ec_chip);
>> +	for (i = 0; i < BATTERY_SERIAL_LEN / 2; i++) {
>> +		ret = a500_ec_read_locked(bat->ec_chip,
>> +					  &ec_data[REG_SERIAL_NUMBER].cmd);
>> +		if (ret < 0) {
>> +			bat->serial[0] = '\0';
>> +			break;
>> +		}
>> +
>> +		bat->serial[i * 2 + 0] = (ret >> 0) & 0xff;
>> +		bat->serial[i * 2 + 1] = (ret >> 8) & 0xff;
>> +	}
>> +	a500_ec_unlock(bat->ec_chip);
>> +done:
>> +	val->strval = bat->serial;
>> +
>> +	return ret;
>> +}
> 
> If battery is swapped, this will keep the old serial.

Hello, Sebastian! The battery isn't hot-swappable on A500, but it also
should be okay to always re-read the serialno. I'll consider removing
the caching in the v2, thanks.

...
>> +	bat->psy = devm_power_supply_register_no_ws(&pdev->dev,
>> +						    &a500_battery_desc,
>> +						    &psy_cfg);
>> +	err = PTR_ERR_OR_ZERO(bat->psy);
>> +	if (err) {
>> +		if (err == -EPROBE_DEFER)
>> +			dev_dbg(&pdev->dev, "failed to register battery, deferring probe\n");
>> +		else
>> +			dev_err(&pdev->dev, "failed to register battery: %d\n",
>> +				err);
>> +		return err;
>> +	}
> 
> if (IS_ERR(bat->psy))
>     return dev_err_probe(&pdev->dev, PTR_ERR(err), "failed to register battery\n");

I didn't know that dev_err_probe() is available now, very nice! I'll use
it in the v2, thanks.

...
>> +MODULE_DESCRIPTION("Battery gauge driver for Acer Iconia Tab A500");
>> +MODULE_AUTHOR("Dmitry Osipenko <digetx@gmail.com>");
>> +MODULE_ALIAS("platform:acer-a500-iconia-battery");
>> +MODULE_LICENSE("GPL v2");
> 
> MODULE_LICENSE("GPL");
> 
> Otherwise looks good to me.

Okay, thank you!