On 11/18/2016 04:23 AM, Tom Levens wrote:
On Fri, 18 Nov 2016, Guenter Roeck wrote:
On Thu, Nov 17, 2016 at 11:25:30PM +0000, Tom Levens wrote:
On 17 Nov 2016, at 22:54, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On Thu, Nov 17, 2016 at 08:52:12PM +0100, Mike Looijmans wrote:
On 17-11-2016 19:56, Guenter Roeck wrote:
On Thu, Nov 17, 2016 at 06:40:17PM +0100, Mike Looijmans wrote:
On 17-11-16 17:56, Guenter Roeck wrote:
On 11/17/2016 04:10 AM, Tom Levens wrote:
Updated version of the ltc2990 driver which supports all measurement
modes available in the chip. The mode can be set through a devicetree
attribute.
[ ... ]
static int ltc2990_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
int ret;
struct device *hwmon_dev;
+ struct ltc2990_data *data;
+ struct device_node *of_node = i2c->dev.of_node;
if (!i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_BYTE_DATA |
I2C_FUNC_SMBUS_WORD_DATA))
return -ENODEV;
- /* Setup continuous mode, current monitor */
+ data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2990_data),
GFP_KERNEL);
+ if (unlikely(!data))
+ return -ENOMEM;
+ data->i2c = i2c;
+
+ if (!of_node || of_property_read_u32(of_node, "lltc,mode",
&data->mode))
+ data->mode = LTC2990_CONTROL_MODE_DEFAULT;
Iam arguing with myself if we should still do this or if we should read
the mode
from the chip instead if it isn't provided (after all, it may have been
initialized
by the BIOS/ROMMON).
I think the mode should be explicitly set, without default. There's no way
to tell whether the BIOS or bootloader has really set it up or whether the
chip is just reporting whatever it happened to default to. And given the
chip's function, it's unlikely a bootloader would want to initialize it.
Unlikely but possible. Even if we all agree that the chip should be configured
by the driver, I don't like imposing that view on everyone else.
My advice would be to make it a required property. If not set, display an
error and bail out.
It is not that easy, unfortunately. It also has to work on a non-devicetree
system. I would not object to making the property mandatory, but we would
still need to provide non-DT support.
My "use case" for taking the current mode from the chip if not specified
is that it would enable me to run a module test with all modes. I consider
this extremely valuable.
Good point.
The chip defaults to measuring internal temperature only, and the mode
defaults to "0".
Choosing a mode that doesn't match the actual circuitry could be bad for the
chip or the board (though unlikely, it'll probably just be useless) since it
will actively drive some of the inputs in the temperature modes (which is
default for V3/V4 pins).
Instead of failing, one could choose to set the default mode to "7", which
just measures the 4 voltages, which would be a harmless mode in all cases.
As a way to let a bootloader set things up, I think it would be a good check
to see if CONTROL register bits 4:3 are set. If "00", the chip is not
acquiring data at all, and probably needs configuration still. In that case,
the mode must be provided by the devicetree (or the default "7").
If bits 4:3 are "11", it has already been set up to measure its inputs, and
it's okay to continue doing just that and use the current value of 2:0
register as default mode (if the devicetree didn't specify any mode at all).
At first glance, agreed, though by default b[3:4] are 00, and only the
internal temperature is measured. Actually, the 5 mode bits are all
relevant to determine what is measured. Maybe it would be better to take
all 5 bits into account instead of blindly setting b[34]:=11 and a specific
setting of b[0:2]. Sure, that would make the mode table a bit larger,
but then ltc2990_attrs_ena[] could be made an u16 array, and a table size
of 64 bytes would not be that bad.
I would tend to agree that it should be possible to configure all 5 mode
bits through the devicetree. What I would propose is as follows.
If a devicetree node exists, the mode parameter(s?) are required and the
chip is initialised.
If a devicetree node doesn't exist, it is assumed that the chip has
already been configured (by the BIOS, etc). The mode is read from the
chip to set the visibility of the sysfs attributes. In the worst case, where the
chip has not been configured by another source, it would only be possible
to measure the internal temperature -- but I think this is an acceptable
limitation.
SGTM.
The only case that this does not cover is if the device tree node
exists but the chip is expected to be configured by some other source.
Maybe I am wrong, but I would not expect this to be a terribly common
situation.
What do you think?
I would not bother about this case. Just make the mode property mandatory.
What do you think about making the devicetree property a list of two integers? Something like
lltc,mode = <7 3>;
which would set mode[2..0]=7 and mode[4..3]=3.
I would personally just use a single value for b[4..0]. But that is really up for bikeshedding
(eg should it be <7 3> or <3 7>. I'll leave that up to Rob to decide - he knows better than me
of what makes more sense from a DT perspective.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
