On Sat, Aug 23, 2014 at 10:10 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On 08/22/2014 02:45 PM, Mark Brown wrote: >> >> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@xxxxxxxxxxxxxxxxxxxxx >> wrote: >>> >>> From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> >>> >>> Add regulator with support for enabling or disabling all >>> supplies. >> >> >> Reviwed-by: Mark Brown <broonie@xxxxxxxxxx> >> >> 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). 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. > > 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. > > 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. Thanks for the review! Alan > > Thanks, > Guenter > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
