linux-kernel - Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 14 Apr 2010 15:51:45 +0100
From:	Jonathan Cameron <kernel@...23.retrosnub.co.uk>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
CC:	Jonathan Cameron <jic23@....ac.uk>,
	Alan Cox <alan@...ux.intel.com>, linux-i2c@...r.kernel.org,
	khali@...ux-fr.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

On 04/14/10 15:32, Alan Cox wrote:
>> This is in no way a hwmon chip.  Surely misc is a better location for now
>> (pending the usual discussion about all singing all dancing sensors frameworks).
> 
> Yep it's moving at the moment
> 
> 
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/hwmon.h>
>>
>> Why these extra hwmon includes?  At least at first glance I can't see
>> any uses of them.  The only call to hwmon is to stick this in the
>> hwmon class.
> 
> I inherited it for cleanup so these are good points (I've been staring at
> piles of these for so long extra input is very useful - this is the tip
> of the iceberg !)
Cool, post away.  Feel free to cc me in on anything in the category of general
sensors (accelerometers, magnetometers etc). If nothing else, I'm interested
to see them to get ideas for drivers in IIO etc.
> 
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/hwmon-vid.h>
>>> +#include <linux/err.h>
>>> +#include <linux/delay.h>
>> or any mutex usage?
>>> +#include <linux/mutex.h>
>>> +#include <linux/sysfs.h>
>>> +
>>
>> I guess this makes the driver look like many others, but why bother with
>> the wrapping structure?  This is only used to keep track of the hwmon
>> device to be able to remove it later.
> 
> Should go - agreed will remove
> 
>> These address changes looking a little unusual to me.  They may well be needed, but
>> if so can we have an explanatory comment?
> 
> Will investigate.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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