From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1754871AbcK1Tkx (ORCPT <rfc822;w@1wt.eu>);
        Mon, 28 Nov 2016 14:40:53 -0500
Received: from mail-pf0-f193.google.com ([209.85.192.193]:34757 "EHLO
        mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1754811AbcK1Tkp (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 28 Nov 2016 14:40:45 -0500
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 10.1 \(3251\))
Subject: Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature
 sensor driver.
From: John Muir <john@jmuir.com>
In-Reply-To: <c0f07695-e823-5974-829c-6fd64bd97a5b@roeck-us.net>
Date: Mon, 28 Nov 2016 11:40:42 -0800
Cc: Jean Delvare <jdelvare@suse.com>, Jonathan Corbet <corbet@lwn.net>,
        Mark Rutland <mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>,
        Linux List <linux-kernel@vger.kernel.org>, linux-hwmon@vger.kernel.org,
        linux-doc@vger.kernel.org
Message-Id: <EDC5A938-C5F5-45A4-85BD-C19996BA6460@jmuir.com>
References: <1480223737-82080-2-git-send-email-john@jmuir.com>
 <c0f07695-e823-5974-829c-6fd64bd97a5b@roeck-us.net>
To: Guenter Roeck <linux@roeck-us.net>
X-Mailer: Apple Mail (2.3251)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uASJevxk018834


> On Nov 27, 2016, at 4:19 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On 11/26/2016 09:15 PM, John Muir wrote:
>> Add support for the TI TMP108 temperature sensor with some device
>> configuration parameters.
>> +- ti,alert-active-high : (boolean) make the alert pin active-high instead of
>> +      the default active-low.
> 
> The driver doesn't support interrupts/alerts. Do those properties really add value ?
Getting ahead of myself. I will create a patch for this in the future.

>> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
>> +{
>> +	tmp108->ready_time = jiffies;
>> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
>> +	    == TMP108_MODE_CONTINUOUS) {
> 
> Don't you want a "!" here ? Presumably the delay is only needed
> if the original configuration was not for continuous mode.
> 
>> +		tmp108->ready_time +=
>> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
>> +	}
>> +}
The delay is required for both really. When the device is set into continuous mode it starts converting and the first temperature is ready (just under) 30ms later.

For the (unsupported at this time) one-shot mode, there would likewise be a delay, but I was envisioning having the requesting task sleep while waiting for the data to be ready, rather than get an -EAGAIN until the data is ready. 

>> +	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
>> +						      tmp108, tmp108_groups);
> 
> Please consider using the devm_ function here.
> 
>> +	if (IS_ERR(hwmon_dev)) {
>> +		dev_dbg(dev, "unable to register hwmon device\n");
>> +		return PTR_ERR(hwmon_dev);
>> +	}
>> +
>> +	tmp108->hwmon_dev = hwmon_dev;
>> +	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
>> +						     &tmp108_of_thermal_ops);
>> +	if (IS_ERR(tmp108->tz))
>> +		return PTR_ERR(tmp108->tz);
> 
> hwmon device not unregistered here. That would be fixed by using the devm
> function above.
> 
For the above two registrations and .remove function, I was worried that there would be an order problem between the i2c->remove and the device-managed cleanup. I’ll get deeper into that code to determine if that is a problem.

For your other comments, I will make the necessary changes.

Thanks!

John.