<div dir="ltr">Jagha is currently on leave.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <span dir="ltr"><<a href="mailto:patrick@stwcx.xyz" target="_blank">patrick@stwcx.xyz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Jaghu,<br>
<br>
Can you comment?  I'm looking at the version that was merged into Linus'<br>
master for 4.12:<br>
<br>
commit 2d7a548a3eff382da5cd743670693b<wbr>7657327714<br>
Author: Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>><br>
Date:   Tue Apr 4 17:52:41 2017 -0700<br>
<br>
    drivers: hwmon: Support for ASPEED PWM/Fan tach<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:<br>
> I can't comment on the driver implementation choices.  They have been<br>
> reviewed and accepted by upstream.  I'm not sure what version of the driver<br>
> you are looking at.<br>
><br>
> On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <<a href="mailto:patrick@stwcx.xyz">patrick@stwcx.xyz</a>> wrote:<br>
><br>
> > Rick,<br>
> ><br>
> > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:<br>
> > > I've chatted with Patrick V. separately about the driver.  AST2400/2500<br>
> > fan<br>
> > > tach hardware measures only one fan at a time.  I think we can adjust the<br>
> > > driver settings to reduce the measurement time but it will scale with #<br>
> > of<br>
> > > tachs being read.<br>
> ><br>
> > I never looked at this driver before but it looks like it is doing an<br>
> > 'msleep' in the hwmon read path after resetting a counter register and<br>
> > then counting rotations?  Two comments:<br>
> ><br>
> > 1. As it stands, it doesn't appear that this driver is actually<br>
> > multi-reader safe (either thread or process).  There is no locking or<br>
> > queueing to prevent one reader from resetting the result register while<br>
> > another is performing the msleep loop.  Multi-threading might "go<br>
> > faster" but it will give entirely wrong results by my reading.<br>
> ><br>
> > 2. It seems bad to do a long-running msleep in the hwmon read path to<br>
> > begin with.  Should this driver be restructured to have a kthread read<br>
> > the channels in the background on a polling interval instead of<br>
> > initiating by userspace action?<br>
> ><br>
> > --<br>
> > Patrick Williams<br>
> ><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Patrick Williams<br>
</font></span></blockquote></div><br></div>