dd regulator support for pmbus and ltc2978

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1752

Quoted 7 lines · On Fri, Aug 22, 2014 at 04:11:33PM -0500, [email protected] wrote:

drivers/hwmon/pmbus/pmbus_core.c +7 -0

+	if (pdata && pdata->reg_init_data) {
+		config.init_data = pdata->reg_init_data;
+	} else {
+		config.init_data = of_get_regulator_init_data(dev, np);
+		if (!config.init_data)
+			return -ENOMEM;
+	}

It should be fine to start with no init data - just let the regulator core worry about it. This will allow users to read back the state even if they can't change anything which is useful for system bringup or general debugging.

Quoted 15 lines · Mark Brown

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1752

Quoted 7 lines · On Fri, Aug 22, 2014 at 04:11:33PM -0500, [email protected] wrote:

drivers/hwmon/pmbus/pmbus_core.c +7 -0

+	if (pdata && pdata->reg_init_data) {
+		config.init_data = pdata->reg_init_data;
+	} else {
+		config.init_data = of_get_regulator_init_data(dev, np);
+		if (!config.init_data)
+			return -ENOMEM;
+	}

It should be fine to start with no init data - just let the regulator core worry about it. This will allow users to read back the state even if they can't change anything which is useful for system bringup or general debugging.

of_get_regulator_init_data() will only have an error if it cannot alloc the regulator_init_data struct. That's why I did -ENOMEM. If there is no platform data and nothing about the regulator in the device tree, we should end up with a zeroed out regulator_init_data.

Quoted 4 lines · atull

of_get_regulator_init_data() will only have an error if it cannot alloc the regulator_init_data struct. That's why I did -ENOMEM. If there is no platform data and nothing about the regulator in the device tree, we should end up with a zeroed out regulator_init_data.

Yes, but if OF is not defined it will return NULL as well. Unless I am missing something, that means that the code will now fail if there is no platform init data and OF is not configured.

Quoted 6 lines · Guenter Roeck

Quoted 4 lines · atull

of_get_regulator_init_data() will only have an error if it cannot alloc the regulator_init_data struct. That's why I did -ENOMEM. If there is no platform data and nothing about the regulator in the device tree, we should end up with a zeroed out regulator_init_data.

Quoted 3 lines

Yes, but if OF is not defined it will return NULL as well. Unless I am missing something, that means that the code will now fail if there is no platform init data and OF is not configured.

Indeed, and that's the more interesting case than running out of memory.

From: Alan Tull <[email protected]>

Add regulator with support for enabling or disabling all supplies.

Quoted 4 lines · On Fri, Aug 22, 2014 at 04:11:34PM -0500, [email protected] wrote:

From: Alan Tull <[email protected]>

Add regulator with support for enabling or disabling all supplies.

Reviwed-by Mark Brown <[email protected]>

though it still looks like you should be able to create generic functions for the operations.

Sorry I didn't have time to review the code myself. I'll have to check the datasheet about turning regulators on and off. Using page 0xff for the lm2978 looks wrong, as the chip supports up to 8 channels which should be controlled separately (I would assume) instead of turning them all on and off in one go. Maybe I am missing something, but my assumption would have been to have a separate regulator for each channel, and that each channel would have its own regulator which would be

Quoted 6 lines · Mark Brown

Quoted 3 lines · On Fri, Aug 22, 2014 at 04:11:34PM -0500, [email protected] wrote:

From: Alan Tull <[email protected]>

Quoted 9 lines

Quoted 2 lines

Add regulator with support for enabling or disabling all supplies.

Reviwed-by Mark Brown <[email protected]>

though it still looks like you should be able to create generic functions for the operations.

Sorry I didn't have time to review the code myself. I'll have to check the datasheet about turning regulators on and off. Using page 0xff for the lm2978 looks wrong, as the chip supports up to 8 channels which should be controlled separately (I would assume) instead of turning them all on and off in one go. Maybe I am missing something, but my assumption would have been to have a separate regulator for each channel, and that each channel would have its own regulator which would be turned on and off separately. So I don't really understand the change between v1 and v2 of the core patch, which dropped the per-channel regulators. Someone will have to explain to me why that makes sense, especially since it means that I won't be able to use the regulator expansion in my system (which would require per-channel regulators, and which does not always have all channels enabled on a given chip).

In respect to generic functions, that really depends on the scope of the regulators. As written, where all regulators are turned on in a single operation, per-chip functions are needed. I thought we would only need per-chip configuration values, but that only applies if regulators are turned on one by one, not all in one go.

Either case, a wrapper for ltc2978_write_pmbus_operation needs to be added to pmbus_core.c as pmbus_write_byte_data and exported (as a separate patch). For paged reads, the existing pmbus_read_byte_data should be used. In general, avoid direct accesses to paged registers and use API functions instead if possible.

Thanks,
Guenter

Quoted 16 lines · Guenter Roeck

Quoted 6 lines · Mark Brown

Quoted 3 lines · On Fri, Aug 22, 2014 at 04:11:34PM -0500, [email protected] wrote:

From: Alan Tull <[email protected]>

Quoted 9 lines

Quoted 2 lines

Add regulator with support for enabling or disabling all supplies.

Reviwed-by Mark Brown <[email protected]>

though it still looks like you should be able to create generic functions for the operations.

Quoted 15 lines

Sorry I didn't have time to review the code myself. I'll have to check the datasheet about turning regulators on and off. Using page 0xff for the lm2978 looks wrong, as the chip supports up to 8 channels which should be controlled separately (I would assume) instead of turning them all on and off in one go. Maybe I am missing something, but my assumption would have been to have a separate regulator for each channel, and that each channel would have its own regulator which would be turned on and off separately. So I don't really understand the change between v1 and v2 of the core patch, which dropped the per-channel regulators. Someone will have to explain to me why that makes sense, especially since it means that I won't be able to use the regulator expansion in my system (which would require per-channel regulators, and which does not always have all channels enabled on a given chip).

The LTC2978 spec says that the OPERATION command register "responds to the global page command (PAGE=0xFF)." I originally took it to mean that it *only* responds to 0xFF. Now I get that yes I could turn on/off each of 8 channels separately.

I will make the change have one regulator for each supply. It would be useful to still have a global regulator for turning them all on/off together if that is not completely odious.

Quoted 6 lines

In respect to generic functions, that really depends on the scope of the regulators. As written, where all regulators are turned on in a single operation, per-chip functions are needed. I thought we would only need per-chip configuration values, but that only applies if regulators are turned on one by one, not all in one go.

For all the parts supported by ltc2978.c, a banked write of 0x80 to the OPERATION register turns a regulator on, writing 0x00 turns it off. I'm not sure how universal that is for other PMBUS parts. I will look at the PMBUS specs when I get back to the office Tuesday.

I'll look into it and see if I can push almost all of this code into pmbus_core.c and just have per-chip config values in
pmbus_driver_info. If many PMBUS parts have the same scheme (0x80 or
0x00 written to OPERATION register as a banked write) then a flag bit that is turned on in pmbus_driver_info could enable this scheme for this chip. If it is a different register or different on/off values, then I'll need to add those to pmbus_driver_info.

Quoted 7 lines

Either case, a wrapper for ltc2978_write_pmbus_operation needs to be added to pmbus_core.c as pmbus_write_byte_data and exported (as a separate patch). For paged reads, the existing pmbus_read_byte_data should be used. In general, avoid direct accesses to paged registers and use API functions instead if possible.

OK. Will add pmbus_write_byte_data as a separate patch and use the existing pmbus_read_byte_data.