On 02/19/2013 02:30 AM, Guenter Roeck wrote:
> On Mon, Feb 18, 2013 at 02:38:57PM +0100, Lars-Peter Clausen wrote:
[...]
>> diff --git a/drivers/hwmon/adt7310.c b/drivers/hwmon/adt7310.c
>> new file mode 100644
>> index 0000000..2196ac3
>> --- /dev/null
>> +++ b/drivers/hwmon/adt7310.c
>> @@ -0,0 +1,115 @@
>> +/*
>> + * ADT7310/ADT7310 digital temperature sensor driver
>> + *
>> + * Copyright 2010-2013 Analog Devices Inc.
>
> Not really; copyright is yours, unless you are signing it off to analog or if
> you are working for them and have permission (or the duty) to sign off the
> copyright. Even then it should be 2013 only for this file.
I work for them. The copyright notice comes from the IIO adt7410/adt7310
driver, where also this code comes from. Although this portion of the code
was written in 2012, so maybe change it to 2012-2013.
[...]
>> diff --git a/drivers/hwmon/adt7x10.h b/drivers/hwmon/adt7x10.h
>> new file mode 100644
>> index 0000000..a7165e6
>> --- /dev/null
>> +++ b/drivers/hwmon/adt7x10.h
>> @@ -0,0 +1,48 @@
>> +#ifndef __HWMON_ADT7X10_H__
>> +#define __HWMON_ADT7X10_H__
>> +
>> +#include <linux/types.h>
>> +#include <linux/pm.h>
>> +
>> +/* ADT7410 registers definition */
>> +#define ADT7410_TEMPERATURE 0
>> +#define ADT7410_STATUS 2
>> +#define ADT7410_CONFIG 3
>> +#define ADT7410_T_ALARM_HIGH 4
>> +#define ADT7410_T_ALARM_LOW 6
>> +#define ADT7410_T_CRIT 8
>> +#define ADT7410_T_HYST 0xA
>> +#define ADT7410_ID 0xB
>> +
>> +/* ADT7310 registers definition */
>> +#define ADT7310_STATUS 0
>> +#define ADT7310_CONFIG 1
>> +#define ADT7310_TEMPERATURE 2
>> +#define ADT7310_ID 3
>> +#define ADT7310_T_CRIT 4
>> +#define ADT7310_T_HYST 5
>> +#define ADT7310_T_ALARM_HIGH 6
>> +#define ADT7310_T_ALARM_LOW 7
>> +
>> +struct device;
>> +
>> +struct adt7410_ops {
>> + int (*read_byte)(struct device *, u8 reg);
>> + int (*write_byte)(struct device *, u8 reg, u8 data);
>> + int (*read_word)(struct device *, u8 reg);
>> + int (*write_word)(struct device *, u8 reg, u16 data);
>> +};
>> +
>> +int adt7410_probe(struct device *dev, const char *name,
>> + const struct adt7410_ops *ops);
>> +int adt7410_remove(struct device *dev);
>> +
>
> I think the above should be adt7x10_ops, adt7x10_probe and adt7x10_remove, to
> indicate that those are common functions.
>
> I would actually suggest to rename all functions in adt7x10.c (not just the
> exported ones).
I normally don't like to rename all the symbols in one file just because
support for another part is added, since it just cause lots of noise in the
diff. My initial plan was to call the two new modules adt7410-i2c and
adt7410-spi, but then though it is probably going to be confusing to have
the driver for the adt7310 called adt7410-spi. In this case the renaming
probably doesn't hurt since we already have lots of diff noise anyway. It
probably would have made sense to make the review easier to split this up in
two patches, one which renames adt7410 to adt7x10 and a second one which
adds the new functionality.
Thanks,
- Lars
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
