Re: [PATCH 1/2] Add support for OneWire (W1) devices family 0x26 (MAX17211/MAX17215)

Re: [PATCH 1/2] Add support for OneWire (W1) devices family 0x26 (MAX17211/MAX17215)

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi,

On Sat, Apr 29, 2017 at 05:34:28PM +0300, Alex A. Mihaylov wrote:
> Maxim Semiconductor MAX17211/MAX17215 single/multi-cell fuel gauge
> monitor with M5 Fuel Gauge algorithm
> 
> Slave device provide software layer for access to internal registers
> MAX17211/MAX17215 chip.

Please convert this to regmap. There should be lots of docs and
examples around, for example:

http://elinux.org/images/a/a3/Regmap-_The_Power_of_Subsystems_and_Abstractions.pdf

There is no generic w1 handler, but you can provide custom
read/write functions. Also it would be nice to have this
based on https://lkml.org/lkml/2017/3/16/604. Then everything
could go into the power-supply driver.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Add support for OneWire (W1) devices family 0x26 (MAX17211/MAX17215)

[Alex A. Mihaylov]

Hi,


>> Slave device provide software layer for access to internal registers
>> MAX17211/MAX17215 chip.
> Please convert this to regmap.There is no generic w1 handler, but you can provide custom
> read/write functions.
I think regmap be overkill for this driver. Here we need access to a 
small number of registers.  Registers are extremely simple on the 
internal device. As a result, the software layer of regmap will only 
complicate the readability and understanding of the code. And also 
increase the size and time of code execution, and even complicate the 
perception.

> Also it would be nice to have this
> based on https://lkml.org/lkml/2017/3/16/604. Then everything
> could go into the power-supply driver.
>
No problems. Once this set of patches will be accepted in the mainline.

Alex.

Re: [PATCH 1/2] Add support for OneWire (W1) devices family 0x26 (MAX17211/MAX17215)

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

Hi,

On Sun, Apr 30, 2017 at 08:21:51AM +0300, Alex A. Mihaylov wrote:
> > > Slave device provide software layer for access to internal registers
> > > MAX17211/MAX17215 chip.
> > Please convert this to regmap.There is no generic w1 handler, but you can provide custom
> > read/write functions.
> I think regmap be overkill for this driver. Here we need access to a small
> number of registers.  Registers are extremely simple on the internal device.
> As a result, the software layer of regmap will only complicate the
> readability and understanding of the code. And also increase the size and
> time of code execution, and even complicate the perception.

I did not ask for full usage of all regmap features. Just register
the read/write handler as regmap handler and use regmap_read/write
to get values.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] Add support for OneWire (W1) devices family 0x26 (MAX17211/MAX17215)

[Alex A. Mihaylov]

Hi!


30.04.17 23:53, Sebastian Reichel wrote::
>>>> Slave device provide software layer for access to internal registers
>>>> MAX17211/MAX17215 chip.
>>> Please convert this to regmap.There is no generic w1 handler, but you can provide custom
>>> read/write functions.
>> I think regmap be overkill for this driver. the perception.
> I did not ask for full usage of all regmap features. Just register
> the read/write handler as regmap handler and use regmap_read/write
> to get values.
>
OK, I think about this. But max17211-battery use ONLY 
w1_max12711x_reg_get(). This is very simple function.

Second function w1_max1721x_reg_set() writen for extend feature list 
(factory calibrating and init battery).  I think, this code must _NOT_ 
be writen, and must _NOT_ be accessible for end user. Change calibration 
values potentially can damage battery or device. Theory, I can unexport 
this function and remove them from drivers code.

So output registers of Maxim M5 Fuel Gauge algorithm very simple: one 
register - one value. In driver I have two points with bitfield. First - 
battery connected from power supply props. Second: very optional device 
type from probe. This value _NOT_ used from driver - just write device 
type, if manufacturer not fill this data in chip nvram.

This moment driver use 14 registers. Ok, let's some time above will be 
14*3=42 registers accessible with w1_max1721x_reg_get(). This value is 
greatly overestimated - so many properties are not in the class 
POWER_SUPPLY. But register range in MAX1721X is 0x1F0 (496 words or 992 
bytes) of RAM for cache registers. As embedded system developer I 
dislike so ram management.

As result I repeat: regmap will be overkill for this driver. But OK, I 
try to write this code. Just for fun.

Re: [PATCH 2/2] Add driver for MAX17211/MAX17215 fuel gauge

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

Hi,

Please make sure you Cc the relevant people / mailing lists. You
can use ./scripts/get_maintainer.pl to find out who should receive
the patches.

On Sat, Apr 29, 2017 at 05:34:29PM +0300, Alex A. Mihaylov wrote:
> Maxim Semiconductor MAX17211/MAX17215 single/multi-cell fuel gauge
> monitor with M5 Fuel Gauge algorithm
> 
> This driver provide userspace access to MAX17211/MAX17215 data with
> power_supply class drivers.

> ---
>  drivers/power/supply/Kconfig            |   8 +
>  drivers/power/supply/Makefile           |   1 +

Please move the entries in both files before CONFIG_BATTERY_MAX17040.

> [...]
>
> +	 * FixMe:
> +	 * Device without no_thermal = true not register (err -22)
> +	 * Len of platform device name "max17211-battery.X.auto"
> +	 * more than 20 chars limit in THERMAL_NAME_LENGTH from
> +	 * include/uapi/linux/thermal.h
> +	 */

IIRC we already had problems with small THERMAL_NAME_LENGTH before.
I suggest to add another patch, that increases THERMAL_NAME_LENGTH
(don't forget to Cc/To the thermal subsystem people).

> +	info->bat_desc.no_thermal = true;
> +	psy_cfg.drv_data = info;
> +
> +	if (w1_max1721x_reg_get(info->w1_dev,
> +			MAX1721X_REG_NRSENSE, &info->rsense))
> +		return -ENODEV;
> +
> +	if (!info->rsense) {
> +		dev_warn(info->dev, "RSenese not calibrated, set 10 mOhms!\n");
> +		info->rsense = 1000; /* in regs in 10^-5 */
> +	}
> +	dev_dbg(info->dev, "RSense: %d mOhms.\n", info->rsense / 100);
> +
> +	if (get_string(info->w1_dev, MAX1721X_REG_MFG_STR,
> +			MAX1721X_REG_MFG_NUMB, info->ManufacturerName)) {
> +		dev_err(info->dev, "Can't read manufacturer. Hardware error.\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!info->ManufacturerName[0])
> +		strncpy(info->ManufacturerName, DEF_MFG_NAME,
> +			2 * MAX1721X_REG_MFG_NUMB);
> +
> +	if (get_string(info->w1_dev, MAX1721X_REG_DEV_STR,
> +			MAX1721X_REG_DEV_NUMB, info->DeviceName)) {
> +		dev_err(info->dev, "Can't read device. Hardware error.\n");
> +		return -ENODEV;
> +	}
> +	if (!info->DeviceName[0]) {
> +		uint16_t dev_name;
> +
> +		if (w1_max1721x_reg_get(info->w1_dev,
> +				MAX172XX_REG_DEVNAME, &dev_name)) {
> +			dev_err(info->w1_dev, "Can't read device name reg.\n");
> +			return -ENODEV;
> +		}
> +
> +		switch (dev_name & MAX172XX_DEV_MASK) {
> +		case MAX172X1_DEV:
> +			strncpy(info->DeviceName, DEF_DEV_NAME_MAX17211,
> +				2 * MAX1721X_REG_DEV_NUMB);
> +			break;
> +		case MAX172X5_DEV:
> +			strncpy(info->DeviceName, DEF_DEV_NAME_MAX17215,
> +				2 * MAX1721X_REG_DEV_NUMB);
> +			break;
> +		default:
> +			strncpy(info->DeviceName, DEF_DEV_NAME_UNKNOWN,
> +				2 * MAX1721X_REG_DEV_NUMB);
> +		}
> +	}
> +
> +	if (get_sn_string(info->w1_dev, info->SerialNumber)) {
> +		dev_err(info->dev, "Can't read serial. Hardware error.\n");
> +		return -ENODEV;
> +	}
> +
> +	info->bat = power_supply_register(&pdev->dev, &info->bat_desc,
> +						&psy_cfg);
> +	if (IS_ERR(info->bat)) {
> +		dev_err(info->dev, "failed to register battery\n");
> +		return PTR_ERR(info->bat);
> +	}

Please use devm_power_supply_register() and drop the remove
function.

> +	return 0;
> +}
> +
> +static int max1721x_battery_remove(struct platform_device *pdev)
> +{
> +	struct max17211_device_info *info = platform_get_drvdata(pdev);
> +
> +	power_supply_unregister(info->bat);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver max1721x_battery_driver = {
> +	.driver = {
> +		.name = "max1721x-battery",
> +	},
> +	.probe	  = max1721x_battery_probe,
> +	.remove   = max1721x_battery_remove,
> +};
> +module_platform_driver(max1721x_battery_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alex A. Mihaylov <minimumlaw@rambler.ru>");
> +MODULE_DESCRIPTION("Maxim MAX17211/MAX17215 Fuel Gauage IC driver");
> +MODULE_ALIAS("platform:max1721x-battery");

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] Add driver for MAX17211/MAX17215 fuel gauge

[Михайлов Алексей Анатольевич]

Hi!


>> [...]
>>
>> +	 * FixMe:
>> +	 * Device without no_thermal = true not register (err -22)
>> +	 * Len of platform device name "max17211-battery.X.auto"
>> +	 * more than 20 chars limit in THERMAL_NAME_LENGTH from
>> +	 * include/uapi/linux/thermal.h
>> +	 */
> IIRC we already had problems with small THERMAL_NAME_LENGTH before.
> I suggest to add another patch, that increases THERMAL_NAME_LENGTH
> (don't forget to Cc/To the thermal subsystem people).
May be rename "max17211-battery" to "max17211" and remove no_thermal = 
true? This case thermal zone will work again. In current (mainline) 
kernel all drivers "xxxxxx-battery" (like ds2780-battery or 
ds2760-battery) compiled, but not working (not registered). They can 
start working again without thermal zone by adding no_thermal = true or 
by remove "-battery" from platform device name. Alternative limit on 
THERMAL_NAME_LENGTH may be extended.
>> +	info->bat = power_supply_register(&pdev->dev, &info->bat_desc,
>> +						&psy_cfg);
>> +	if (IS_ERR(info->bat)) {
>> +		dev_err(info->dev, "failed to register battery\n");
>> +		return PTR_ERR(info->bat);
>> +	}
> Please use devm_power_supply_register() and drop the remove
> function.
Khm... probe/remove paired functions. I can use 
devm_power_supply_register in my code. But all other fuel gauge drivers 
use classic probe/remove pair. Which decision will be more correct?

Re: [PATCH 2/2] Add driver for MAX17211/MAX17215 fuel gauge

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

Hi,

On Sun, Apr 30, 2017 at 08:32:10PM +0300, Михайлов Алексей Анатольевич wrote:
> > IIRC we already had problems with small THERMAL_NAME_LENGTH before.
> > I suggest to add another patch, that increases THERMAL_NAME_LENGTH
> > (don't forget to Cc/To the thermal subsystem people).
> May be rename "max17211-battery" to "max17211" and remove no_thermal = true?
> This case thermal zone will work again. In current (mainline) kernel all
> drivers "xxxxxx-battery" (like ds2780-battery or ds2760-battery) compiled,
> but not working (not registered). They can start working again without
> thermal zone by adding no_thermal = true or by remove "-battery" from
> platform device name. Alternative limit on THERMAL_NAME_LENGTH may be
> extended.

I think increasing THERMAL_NAME_LENGTH is the right way.

> > > +	info->bat = power_supply_register(&pdev->dev, &info->bat_desc,
> > > +						&psy_cfg);
> > > +	if (IS_ERR(info->bat)) {
> > > +		dev_err(info->dev, "failed to register battery\n");
> > > +		return PTR_ERR(info->bat);
> > > +	}
> > Please use devm_power_supply_register() and drop the remove
> > function.
> Khm... probe/remove paired functions. I can use devm_power_supply_register
> in my code. But all other fuel gauge drivers use classic probe/remove pair.
> Which decision will be more correct?

Most drivers are older than devm_power_supply_register. We already
have a few fuel gauge drivers, which use it, though:

$ git grep devm_power_supply_register | grep "battery" 
88pm860x_battery.c:	info->battery = devm_power_supply_register(&pdev->dev,
da9150-fg.c:	fg->battery = devm_power_supply_register(dev, &fg_desc, NULL);
max17042_battery.c:	chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
max8997_charger.c:	charger->battery = devm_power_supply_register(&pdev->dev,
max8998_charger.c:	max8998->battery = devm_power_supply_register(max8998->dev,
sbs-battery.c:	chip->power_supply = devm_power_supply_register(&client->dev, sbs_desc,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]