On Tue, Jan 31, 2017 at 12:47 PM, Guenter Roeck <li...@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 <li...@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? >> >> >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
