Re: [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck <linux@roeck-us.net>]

On 01/31/2017 11:31 PM, Andrea Merello wrote:
> On Tue, Jan 31, 2017 at 12:47 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/30/2017 11:11 PM, Andrea Merello wrote:
>>>
>>> On Mon, Jan 30, 2017 at 3:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 01/30/2017 01:40 AM, Andrea Merello wrote:
>>>>>
>>>>>
>>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>>> temperature sensors.
>>>>>
>>>>
>>>> Overall looks good. One thing I noticed, though: In the log function
>>>> calls,
>>>> you use a mix of priv->dev and &client->dev as log device. Also, you
>>>> sometimes
>>>> use &priv->client->dev even when a local client variable is available.
>>>> I can fix up the latter (no need to resend for that), but please have a
>>>> look at the overall use and either send a fixed up version or let me know
>>>> if the log devices are all intentional as-is.
>>>
>>>
>>> Hmm, you are right: there is a bit of mess here. It's not intentional
>>> indeed..
>>>
>>> I gave a look to lm90.c and lm63.c; it seems no one uses the hwmon
>>> device for this purpose. We can probably stick to &client->dev or
>>> &priv->client->dev when a local client variable is not already
>>> available.
>>
>>
>> It is really your call as the driver's author to make if you want to use
>> the hwmon device or &client->dev.
>
> I've decided to stick with &client->dev for all dev_dbg(), while I
> would use the hwmon device for dev_notice() and dev_warn(), that are
> supposed to be eventually read by the user.
>
> However, while keeping an eye on the log messages I've realized that
> if the temperature remains out-of-limit for a lot of time, especially
> if the conversion rate is set fast, we tend to spam the log with alert
> messages.. I learned that you are usually concerned by noisy drivers,
> so I guess you might want to mitigate this..
>
> I could use dev_notice_once(), but it seems inappropriate here, or I
> can silence the message at least until one reads the chip. Lm90 does
> even disable the alarm until that happen, but it has only one alarm;
> here we have two, so it seems not correct here.
>
> Any suggestion?
>

Use a flag that shows if the attribute in question was read ?

Guenter

>
>>>
>>>
>>> Also, it looks like we can change some &priv->client->dev in
>>> &client->dev in probe function when calling i2c read/write functions.
>>>
>>> If you ACK, I'll fix and resend.
>>>
>> SGTM.
>>
>> Thanks,
>> Guenter
>>
>