From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752417AbcKQUVp convert rfc822-to-8bit (ORCPT ); Thu, 17 Nov 2016 15:21:45 -0500 Received: from mx6-09.smtp.antispamcloud.com ([95.211.2.200]:35252 "EHLO mx6-09.smtp.antispamcloud.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751788AbcKQUVl (ORCPT ); Thu, 17 Nov 2016 15:21:41 -0500 X-Greylist: delayed 1704 seconds by postgrey-1.27 at vger.kernel.org; Thu, 17 Nov 2016 15:21:40 EST Subject: Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes To: Guenter Roeck References: <1479384616-12479-1-git-send-email-tom.levens@cern.ch> <1479384616-12479-3-git-send-email-tom.levens@cern.ch> <410de6c9-a13e-51f7-4d66-6f4e2537c574@roeck-us.net> <582DEB81.6050806@topic.nl> <20161117185654.GA19338@roeck-us.net> CC: Tom Levens , , , , , , From: Mike Looijmans Organization: Topic Message-ID: <582E0A6C.5010307@topic.nl> Date: Thu, 17 Nov 2016 20:52:12 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20161117185654.GA19338@roeck-us.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8BIT X-Originating-IP: [85.150.144.104] X-EXCLAIMER-MD-CONFIG: 9833cda7-5b21-4d34-9a38-8d025ddc3664 X-EXCLAIMER-MD-BIFURCATION-INSTANCE: 0 X-Filter-ID: s0sct1PQhAABKnZB5plbIbbvfIHzQjPVmPLZeVYSu3xU9luQrU+8/8qthi+0Jd/W6KAUC/fjyuDn NXFr4uarw8mLG3++kr/h6adaE/w//0SME1GdOhKIDgMZPfHO4A3RBP2TI/0jFdLrsRIqaSrhuPkV 7R8q4kWSMV9KEDrT/1w4pg9rQ/oj+q3Qn1rX669NYy3IXdu/Iw4JYSSaW1v2nnUARsVG/S8N/3Jg CYcGCGg7mMYE/7AQ6szBIB8chic0cRAOQGaD///TBcVXso5v8lNiLZt/QXQnOBRD+jq1HsKMGzOU Rge7rkMhdD+Dfu87gVczbIZlP0chivHUjkME6Uuf+eM3l5KPnbp/eQthjVOxCQKB10W5fmzGGkBZ 4XgbXbfRhESVZwjumpphWdNAH4/u4dSqz8ECiP0sQixiMu6B/Zd98IjqQoXtn2BEf0y1fIhjbr/b dBv/zwq1RCWLGBvHcw+YMiJeyLa9LK8ld63aHUUQo0CcN8WKghDzBRjSn71UyEAmSYRWDakYlAJq ACK79xdvLH6I7mSN0vgXXN56UYABPEJmokgMViEN6xeV647lNwN4qOsSZg+fYhVZG9/v2nwMuQmV hCgbpWFZGIhkgoF5A4SlBpH7krY7cfkeowbovTu16yD6VpKabSrkCEPglvlGA31jXJulvcg9llMQ 7AtWD1sF4MyUTM9upFN/mzOOCyxd/ZxFeCr0KR7NnTgUoTeP+OuF9YFDyP0ha/LMHy4w5U3wUX9o k8B42p3lqQ7IcDrA4HEm4JTphG5KNyERbInMiTBIUBbQ/Dy6Ip7rSLKCozxuZDMpdF6sCRdOUbdU CZZ3YwgeRtnHK0EioxnfARpqwX25ys55X3DMzanGpKrx5va0CSpbOsnqijV7/f1FKqiVbbeu7dZA uWIwaPY4ocfmWv3Fe9Iziczdq+A= X-Report-Abuse-To: spam@quarantine2.antispamcloud.com X-Originating-IP: 37.74.225.130 X-SpamExperts-Domain: topic.nl X-SpamExperts-Username: 37.74.225.128/28 Authentication-Results: antispamcloud.com; auth=pass smtp.auth=37.74.225.128/28@topic.nl X-SpamExperts-Outgoing-Class: ham X-SpamExperts-Outgoing-Evidence: Combined (0.05) X-Classification: not-spam/combined X-Recommended-Action: accept Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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). The reason I wanted the property to be mandatory is to trigger users like me (probably I'm the only other user so far) to update their devicetree. But I'd notice quickly enough if it defaults to something else. So that's not very compelling. >>> Mike, would that break your application, or can you specify the mode in >>> devicetree ? >> >> I'm fine with specifying this in the devicetree. It will break things for >> me, but I've been warned and willing to bow for the greater good :) >> > I should have asked if your system uses devicetree. If it does, the problem > should be easy to fix for you. If not, we'll need to find a solution > for your use case. I'm using devicetree. I planned to 'mainline' the boards some time this year... > > Thanks, > Guenter > -- Mike Looijmans Kind regards, Mike Looijmans System Expert TOPIC Products Materiaalweg 4, NL-5681 RJ Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail