On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
> >
> >>>>
> >>>>>> + if (ret < 0)
> >>>>>> + return ret;
> >>>>>> +
> >>>>>> + priv->max_alert = priv->max_alert || !!(ret &
> >>>>>> STTS751_STATUS_TRIPH);
> >>>>>> + priv->min_alert = priv->min_alert || !!(ret &
> >>>>>> STTS751_STATUS_TRIPL);
> >>>>>> +
> >>>>>
> >>>>>
> >>>>> I am not really happy about the caching of alert values. This isn't
> >>>>> really
> >>>>> common. Usually we report current alert values when reading. I don't
> >>>>> really
> >>>>> see why this driver should be different to all the others.
> >>>>
> >>>>
> >>>>
> >>>> OK
> >>
> >>
> >> I've tried to avoid caching, but as soon as I read the status register
> >> in the alert handler, the device clears that reg, and it will remain
> >> cleared up to the next conversion.
> >>
> >> So for example if the reader is blocked on poll over the alert
> >> attribute, it gets unblocked on alert, and it will very likely read
> >> zero..
> >>
> >> The only way I can see for avoiding caching, and for changing the API
> >> about clearing events as you asked below, is performing a temperature
> >> read from the temperature registers and doing software compare with
> >> the limits in show_[max/min]_alert.
> >>
> >> Does this sounds OK for you ?
> >>
> >
> > Not really. That could be done from user space by just reading the
> > temperature
> > and comparing it against limits.
>
> Sorry, I don't see the point here.. Why the fact that something could
> be done in userspace should suggest we don't want to do it in the
> driver? We do, for example, a simple calculation to give an absolute
> value for the hysteresis; we could do this also in userspace, but we
> do this in the driver in order to support the proper API..
>
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
high limit). T HIGH is cleared when the status register is read, provided
the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
limit). T LOW is cleared when the status register is read, provided the
condition no longer exists.
This isn't exactly what you are claiming, since it says "cleared ... provided
that the condition no longer exists", which would be more in line with other
chips.
The THRM bit doesn't seem to self-clean either, at least not according to the
datasheet.
This means I'll need to get a sample and/or eval board to test this myself.
Guenter
> > If the chip clears the alert bit on read, why would you need a set attribute
> > to clear it ?
>
> That was used to clear the cached value, not the chip reg.
>
> > From what you are saying, the next alert should update all
> > alert bits, meaning they would be auto-updated at that time. User space
> > would then always read the current state.
> >
> > Sure, you would cache the value
> > in the alert function to be able to read it in the read function, but I
> > don't see why all the complexity around it in your code would be needed.
>
> The next alert is, by definition, an alert, so it will always update
> the cached value with a '1'. We don't have anything that tell us when
> the alert is gone. No one would clear the cached value with '0' when
> the alert is gone. (ok, we have two alert bits, so the _other_ one of
> them will be set to zero, but that's not the point)..
>
> So, in order to report the current alarm value, without relying to the
> user to clear it, as you asked for, caching seems not an option..
>
>
> > You might want to update the alert status in case there is no interrupt
> > support, though (maybe you do already; I didn't re-check the code).
>
> OK, good point, I'll make sure it works event in this case.
>
> > The 'sensors' code doesn't know that it is supposed to write into the
> > alert attribute to clear the status, thus unless you have a special
> > application, an alert would never be cleared. That doesn't sound very
> > appealing to me.
>
> So, OK to change the API as you said, however I don't see how this can
> be done except for doing the manual temperature/limit comparison in
> the driver:
>
> When you get into show_[max/min]_alert function you can't rely on
> reading the status register (because as I said in my prev mail it
> could have been just cleared by reading it in the event handler), but
> as I explained above it seems to me that you cannot also rely on a
> value cached in the event handler with this API, because no one will
> ever clear it..
>
> In other words you will see a status register that could be zero even
> when we have the alarm, or a cached value that is still 1 even if the
> alarm has gone time ago.. I could clear the cached value in the
> show_[max/min]_alert function itself, but this doesn't seems enough to
> me for reporting to the userspace the _current_ alarm value, as you
> asked for.
>
> Or am I missing something ?
>
> >
> >>>>
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void stts751_alert(struct i2c_client *client,
> >>>>>> + enum i2c_alert_protocol type, unsigned int data)
> >>>>>> +{
> >>>>>> + int ret;
> >>>>>> + struct stts751_priv *priv = i2c_get_clientdata(client);
> >>>>>> + bool prev_max = priv->max_alert;
> >>>>>> + bool prev_min = priv->min_alert;
> >>>>>> +
> >>>>>> + if (type != I2C_PROTOCOL_SMBUS_ALERT)
> >>>>>> + return;
> >>>>>> +
> >>>>>> + dev_dbg(&client->dev, "alert!");
> >>>>>> +
> >>>>>
> >>>>>
> >>>>>
> >>>>> Any chance to make this a bit more generic and also support
> >>>>> interrupts ?
> >>>>
> >>>>
> >>>>
> >>>> May you please elaborate?
> >>>>
> >>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
> >>>> HWmon driver is redundant, because it is already implemented in the
> >>>> SMBus layer, just right now you need patching SMBus layer for enabling
> >>>> it from the DT world (a couple of patches for this are floating
> >>>> around, but they seems not ready for merge yet)... Using this chip
> >>>> with a generic interrupt is indeed exactly what I'm doing..
> >>>>
> >>>> .. Or am I completely missing your point ?
> >>>>
> >>> "just right now you need patching SMBus layer for enabling it from the DT
> >>> world"
> >>>
> >>> So it _doesn't_ work, correct ?
> >>
> >>
> >> I mean, the generic smbus code works for stts751, and the chip seems
> >> happy. So, in this case, no need to reimplement it elsewhere. It would
> >> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
> >> some mechanism to hook smbus code from DT world..
> >>
> >
> > Ok, then let's just go with it. If it doesn't work in some application we'll
> > have to fix it up later.
> >
> >
> >>>>>> +
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
> >>>>>> + show_min, set_min, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
> >>>>>> + show_max, set_max, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
> >>>>>> + show_min_alert, set_min_alert, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
> >>>>>> + show_max_alert, set_max_alert, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
> >>>>>> + set_therm, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
> >>>>>> show_hyst,
> >>>>>> + set_hyst, 0);
> >>>>>
> >>>>>
> >>>>>
> >>>>> Please use standard attribute names.
> >>>>
> >>>>
> >>>>
> >>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
> >>>> temp1_alarm for the flag, but it's not obvious to be what should I use
> >>>> for the therm stuff.. May you please give me advices about this?
> >>>>
> >>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
> >>>> anyway...
> >>>>
> >>>
> >>> From the chip datasheet, this is supposed to reflect overtemperature
> >>> conditions,
> >>> stronger than high/low, thus 'crit' seems appropriate.
> >>
> >>
> >> I see.. I can go with 'crit' :)
> >>
> >> Just is there any place where I can document this? Despite the
> >> datasheet, I can easily imagine real-world usage scenarios, for
> >> example driving a fan from that signal, where 'crit' might be not so
> >> intuitive.. Indeed I would like to make it visible the 'crit'
> >> <->'therm' binding to users. Maybe an additional attribute
> >> 'temp1_crit_label' or something like that ?
> >>
> > You are supposed to provide Documentation/hwmon/stts751, which would
> > include such information.
> >
> > A label applies to a sensor, not to a sensor attribute, so that would
> > not be appropriate.
>
> OK to provide it, but it would be in kernel source tree. Any further
> hint that we could give to someone that looks at in /sys ?
>
> > Thanks,
> > Guenter
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
