Re: [PATCH] hwmon: add caseopen detection to w83627ehf driver

[Dmitry Artamonow]

Hi, Guenter!

On 07:34 Fri 05 Aug     , Guenter Roeck wrote:
> On Fri, Aug 05, 2011 at 09:00:43AM -0400, Dmitry Artamonow wrote:
> > Export caseopen alarm status into userspace for
> > Winbond w83627* chips and also implement alarm clear knob.
> > 
> > Signed-off-by: Dmitry Artamonow <mad_soft@xxxxxxxx>
> > ---
> >  drivers/hwmon/w83627ehf.c |   66 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 66 insertions(+), 0 deletions(-)
> > 
> > Tested on board with W83627DHG-P chip and also checked against datasheets
> > for other W83627 chips. NCT6775F / NCT6776F are currently excluded as I
> > haven't found datasheets for them online and can't check if they have the
> > same registers for caseopen detection. If anyone having access to these
> > datasheets can check that caseopen detection is read from 4th bit of
> > register 42h (Bank 0), and cleared by writing 1 and then 0 into bit 7 of
> > register 46h (Bank 0), then I'll throw away those 'if'-s and resend updated
> > patch.
> > 
> Hi Dmitry,
> 
> NCT6775F / NCT6776F support the same bits. NCT6776F has a second set of bits.
> 42/4 and 46/7 is CASEOPEN0, 42/6 and 46/6 is CASEOPEN1. Register 46/7 (and 46/6
> for NCT6776F) is self-clearing, meaning it is not necessary to write 0,
> but that should not matter.

Oh, that's nice to hear that NCTs are compatible! I'll see that can I do
about additional caseopen on NCT6776F.

[...]

> >  
> > +		data->caseopen = !!(w83627ehf_read_value(data,
> > +					W83627EHF_REG_CASEOPEN_DET) & 0x10);
> > +
> If you use !!, you should define caseopen as bool for consistency.
> And then you don't need !!  since the conversion to 0/1 will be done automatically.
> 
> Even better would be to just save the register value and enable support 
> for the second set of caseopen bits for NCT6776F (ie do the !!() in
> the show function).

Considering second caseopen on NCT6776F that sounds sensible.

> > +static ssize_t
> > +store_caseopen_clear(struct device *dev, struct device_attribute *attr,
> > +			const char *buf, size_t count)
> > +{
> > +	struct w83627ehf_data *data = dev_get_drvdata(dev);
> > +	unsigned long val;
> > +	unsigned int reg;
> > +
> > +	if (strict_strtoul(buf, 10, &val) || val != 0)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&data->update_lock);
> > +	reg = w83627ehf_read_value(data, W83627EHF_REG_CASEOPEN_CLR);
> > +	w83627ehf_write_value(data, W83627EHF_REG_CASEOPEN_CLR, reg | 0x80);
> > +	reg = w83627ehf_read_value(data, W83627EHF_REG_CASEOPEN_CLR);
> > +	w83627ehf_write_value(data, W83627EHF_REG_CASEOPEN_CLR, reg & ~0x80);
> > +	reg = w83627ehf_read_value(data, W83627EHF_REG_CASEOPEN_CLR);
> 
> I am a bit lost here. What are those additional two reads for ?
> Seems to be unnecessary.
> 
Ugh. These are remnants from my debugging printouts - I removed
printouts, but forgot to drop register readings as well. They are surely
not needed.

> > +static struct sensor_device_attribute sda_caseopen =
> > +	SENSOR_ATTR(intrusion0_alarm, S_IWUSR | S_IRUGO, show_caseopen,
> > +			store_caseopen_clear , 12);
> > +
> Formatting: s/ , /, /
> 
Yes. And this "12" shouldn't be here as well (though it doesn't really
matter, as it's not used by show_caseopen/store_caseopen_clear) - it was
the number of caseopen bit in alarm registers, when I was trying to use
show_alarm() for reading caseopen status. Sadly that never worked for
me, as in spite that datasheets told, this bit contained realtime state
of caseopen pin without any latch! - i.e. it was 0 when caseopen was
triggered and 1, when it wasn't. And once "case was closed" it again
became 1.

Thanks for review! I'll try to send updated patch shortly.

-- 
Best regards,
Dmitry "MAD" Artamonow

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/