> Hi Juerg,
Hi Jean,
Thanks for the review. It wasn't really ready for your consumption but
thanks anyways :-)
> On Fri, 10 Dec 2010 10:43:57 +0100, Juerg Haefliger wrote:
>> Attached is a modified driver with support for Vtrip monitoring (1.5V,
>> in7). Could you give it a try and let me know how it goes? The new
>> driver should expose the following new sysfs files: in7_input,
>> in7_min, in7_max, in7_alarm. No user-level scaling is required. The
>> nominal voltage is 1.5V.
>
> The changes look overall good to me, but I have a few comments if you
> are interested:
>
>> --- dme1737.c 2010-08-02 00:11:14.000000000 +0200
>> +++ /home/khali/dme1737.c   2010-12-10 14:39:04.000000000 +0100
>> @@ -75,16 +75,20 @@
>> Â * in4 Â +12V
>> Â * in5 Â VTR Â (+3.3V stby)
>> Â * in6 Â Vbat
>> + * in7 Â Vtrip (sch5127 only)
>> Â *
>> Â * --------------------------------------------------------------------- */
>>
>> -/* Voltages (in) numbered 0-6 (ix) */
>> -#define   ÂDME1737_REG_IN(ix)       Â((ix) < 5 ? 0x20 + (ix) \
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : 0x94 + (ix))
>> -#define   ÂDME1737_REG_IN_MIN(ix)     Â((ix) < 5 ? 0x44 + (ix) * 2 \
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : 0x91 + (ix) * 2)
>> -#define   ÂDME1737_REG_IN_MAX(ix)     Â((ix) < 5 ? 0x45 + (ix) * 2 \
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : 0x92 + (ix) * 2)
>> +/* Voltages (in) numbered 0-7 (ix) */
>> +#define   ÂDME1737_REG_IN(ix)       Â((ix) < 5 ? 0x20 + (ix) : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(ix) < 7 ? 0x94 + (ix) : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0x1f)
>> +#define   ÂDME1737_REG_IN_MIN(ix)     Â((ix) < 5 ? 0x44 + (ix) * 2 : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(ix) < 7 ? 0x91 + (ix) * 2 : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0x9f)
>> +#define   ÂDME1737_REG_IN_MAX(ix)     Â((ix) < 5 ? 0x45 + (ix) * 2 : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(ix) < 7 ? 0x92 + (ix) * 2 : \
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0xa0)
>
> Maybe it is time to give up macros and go with const arrays? The macros
> are never called with a constant parameters so the compiler can't
> optimize the calls.
I thought about that too. I keep it for now for the REG_IN for
consistency reasons (since MIN and MAX don't need any modification as
you pointed out in a subsequent email. duh!).
>>
>> Â/* Temperatures (temp) numbered 0-2 (ix) */
>> Â#define DME1737_REG_TEMP(ix) Â Â Â Â (0x25 + (ix))
>> @@ -99,10 +103,11 @@
>> Â * Â ÂIN_TEMP_LSB(1) = [temp3, temp1]
>> Â * Â ÂIN_TEMP_LSB(2) = [in4, temp2]
>> Â * Â ÂIN_TEMP_LSB(3) = [in3, in0]
>> - * Â ÂIN_TEMP_LSB(4) = [in2, in1] */
>> + * Â ÂIN_TEMP_LSB(4) = [in2, in1]
>> + * Â ÂIN_TEMP_LSB(5) = [res, in7] */
>> Â#define DME1737_REG_IN_TEMP_LSB(ix) Â(0x84 + (ix))
>> -static const u8 DME1737_REG_IN_LSB[] = {3, 4, 4, 3, 2, 0, 0};
>> -static const u8 DME1737_REG_IN_LSB_SHL[] = {4, 4, 0, 0, 0, 0, 4};
>> +static const u8 DME1737_REG_IN_LSB[] = {3, 4, 4, 3, 2, 0, 0, 5};
>> +static const u8 DME1737_REG_IN_LSB_SHL[] = {4, 4, 0, 0, 0, 0, 4, 4};
>> Âstatic const u8 DME1737_REG_TEMP_LSB[] = {1, 2, 1};
>> Âstatic const u8 DME1737_REG_TEMP_LSB_SHL[] = {4, 4, 0};
>>
>> @@ -143,7 +148,7 @@
>> Â#define DME1737_REG_ALARM1 Â Â Â Â Â 0x41
>> Â#define DME1737_REG_ALARM2 Â Â Â Â Â 0x42
>> Â#define DME1737_REG_ALARM3 Â Â Â Â Â 0x83
>> -static const u8 DME1737_BIT_ALARM_IN[] = {0, 1, 2, 3, 8, 16, 17};
>> +static const u8 DME1737_BIT_ALARM_IN[] = {0, 1, 2, 3, 8, 16, 17, 18};
>> Âstatic const u8 DME1737_BIT_ALARM_TEMP[] = {4, 5, 6};
>> Âstatic const u8 DME1737_BIT_ALARM_FAN[] = {10, 11, 12, 13, 22, 23};
>>
>> @@ -188,6 +193,7 @@
>> Â#define HAS_PWM_MIN Â Â Â Â Â(1 << 4) Â Â Â Â Â Â Â Â/* bit 4 */
>> Â#define HAS_FAN(ix) Â Â Â Â Â(1 << ((ix) + 5)) Â Â Â /* bits 5-10 */
>> Â#define HAS_PWM(ix) Â Â Â Â Â(1 << ((ix) + 11)) Â Â Â/* bits 11-16 */
>> +#define HAS_IN7 Â Â Â Â Â Â Â Â Â Â Â(1 << 17) Â Â Â Â Â Â Â /* bit 17 */
>>
>> Â/* ---------------------------------------------------------------------
>> Â * Data structures and manipulation thereof
>> @@ -245,7 +251,7 @@
>> Âstatic const int IN_NOMINAL_SCH5027[] = {5000, 2250, 3300, 1125, 1125, 3300,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â3300};
>> Âstatic const int IN_NOMINAL_SCH5127[] = {2500, 2250, 3300, 1125, 1125, 3300,
>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â3300};
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â3300, 1500};
>> Â#define IN_NOMINAL(type) Â Â ((type) == sch311x ? IN_NOMINAL_SCH311x : \
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(type) == sch5027 ? IN_NOMINAL_SCH5027 : \
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(type) == sch5127 ? IN_NOMINAL_SCH5127 : \
>> @@ -578,7 +584,7 @@
>> Â{
>> Â Â Â struct dme1737_data *data = dev_get_drvdata(dev);
>> Â Â Â int ix;
>> - Â Â u8 lsb[5];
>> + Â Â u8 lsb[6];
>>
>> Â Â Â mutex_lock(&data->update_lock);
>>
>> @@ -601,12 +607,14 @@
>> Â Â Â Â Â Â Â Â Â Â Â /* Voltage inputs are stored as 16 bit values even
>> Â Â Â Â Â Â Â Â Â Â Â Â* though they have only 12 bits resolution. This is
>> Â Â Â Â Â Â Â Â Â Â Â Â* to make it consistent with the temp inputs. */
>> - Â Â Â Â Â Â Â Â Â Â data->in[ix] = dme1737_read(data,
>> + Â Â Â Â Â Â Â Â Â Â if (ix != 7 || (data->has_features & HAS_IN7)) {
>
> It is less intrusive to do the opposite test:
>
> Â Â Â Â Â Â Â Â Â Â Â Âif (ix == 7 && !(data->has_features & HAS_IN7))
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcontinue;
Agreed.
> This avoids reindenting the whole block.
>
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â data->in[ix] = dme1737_read(data,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â DME1737_REG_IN(ix)) << 8;
>> - Â Â Â Â Â Â Â Â Â Â data->in_min[ix] = dme1737_read(data,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â data->in_min[ix] = dme1737_read(data,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â DME1737_REG_IN_MIN(ix));
>> - Â Â Â Â Â Â Â Â Â Â data->in_max[ix] = dme1737_read(data,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â data->in_max[ix] = dme1737_read(data,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â DME1737_REG_IN_MAX(ix));
>> + Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â }
>>
>> Â Â Â Â Â Â Â /* Temp registers */
>> @@ -633,12 +641,16 @@
>> Â Â Â Â Â Â Â Â* which the registers are read (MSB first, then LSB) is
>> Â Â Â Â Â Â Â Â* important! */
>> Â Â Â Â Â Â Â for (ix = 0; ix < ARRAY_SIZE(lsb); ix++) {
>> - Â Â Â Â Â Â Â Â Â Â lsb[ix] = dme1737_read(data,
>> + Â Â Â Â Â Â Â Â Â Â if (ix != 5 || (data->has_features & HAS_IN7)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â lsb[ix] = dme1737_read(data,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â DME1737_REG_IN_TEMP_LSB(ix));
>> + Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {
>> - Â Â Â Â Â Â Â Â Â Â data->in[ix] |= (lsb[DME1737_REG_IN_LSB[ix]] <<
>> + Â Â Â Â Â Â Â Â Â Â if (ix != 7 || (data->has_features & HAS_IN7)) {
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â data->in[ix] |= (lsb[DME1737_REG_IN_LSB[ix]] <<
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â DME1737_REG_IN_LSB_SHL[ix]) & 0xf0;
>> + Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â for (ix = 0; ix < ARRAY_SIZE(data->temp); ix++) {
>> Â Â Â Â Â Â Â Â Â Â Â data->temp[ix] |= (lsb[DME1737_REG_TEMP_LSB[ix]] <<
>> @@ -760,7 +772,7 @@
>>
>> Â/* ---------------------------------------------------------------------
>> Â * Voltage sysfs attributes
>> - * ix = [0-5]
>> + * ix = [0-6]
>
> Shouldn't this be [0-7]?
Yes. And increasing the actual in[] array in the 'data' structure
might be a good thing too to prevent array overruns :-) Duh! No wonder
Jeff's min/max values behave weird.
>> Â * --------------------------------------------------------------------- */
>>
>> Â#define SYS_IN_INPUT 0
>> @@ -1437,7 +1449,7 @@
>> Â * Sysfs device attribute defines and structs
>> Â * --------------------------------------------------------------------- */
>>
>> -/* Voltages 0-6 */
>> +/* Voltages 0-7 */
>>
>> Â#define SENSOR_DEVICE_ATTR_IN(ix) \
>> Âstatic SENSOR_DEVICE_ATTR_2(in##ix##_input, S_IRUGO, \
>> @@ -1456,6 +1468,7 @@
>> ÂSENSOR_DEVICE_ATTR_IN(4);
>> ÂSENSOR_DEVICE_ATTR_IN(5);
>> ÂSENSOR_DEVICE_ATTR_IN(6);
>> +SENSOR_DEVICE_ATTR_IN(7);
>>
>> Â/* Temperatures 1-3 */
>>
>> @@ -1678,8 +1691,7 @@
>> Â Â Â .attrs = dme1737_zone3_attr,
>> Â};
>>
>> -
>> -/* The following struct holds temp zone hysteresis Ârelated attributes, which
>> +/* The following struct holds temp zone hysteresis related attributes, which
>
> Cleanups are preferred as separate patches (makes backporting easier.)
OK. Will send a separate patch.
>> Â * are not available in all chips. The following chips support them:
>> Â * DME1737, SCH311x */
>> Âstatic struct attribute *dme1737_zone_hyst_attr[] = {
>> @@ -1693,6 +1705,21 @@
>> Â Â Â .attrs = dme1737_zone_hyst_attr,
>> Â};
>>
>> +/* The following struct holds voltage in7 related attributes, which
>> + * are not available in all chips. The following chips support them:
>> + * SCH5127 */
>> +static struct attribute *dme1737_in7_attr[] = {
>> + Â Â &sensor_dev_attr_in7_input.dev_attr.attr,
>> + Â Â &sensor_dev_attr_in7_min.dev_attr.attr,
>> + Â Â &sensor_dev_attr_in7_max.dev_attr.attr,
>> + Â Â &sensor_dev_attr_in7_alarm.dev_attr.attr,
>> + Â Â NULL
>> +};
>> +
>> +static const struct attribute_group dme1737_in7_group = {
>> + Â Â .attrs = dme1737_in7_attr,
>> +};
>> +
>> Â/* The following structs hold the PWM attributes, some of which are optional.
>> Â * Their creation depends on the chip configuration which is determined during
>> Â * module load. */
>> @@ -1984,6 +2011,9 @@
>> Â Â Â if (data->has_features & HAS_ZONE_HYST) {
>> Â Â Â Â Â Â Â sysfs_remove_group(&dev->kobj, &dme1737_zone_hyst_group);
>> Â Â Â }
>> + Â Â if (data->has_features & HAS_IN7) {
>> + Â Â Â Â Â Â sysfs_remove_group(&dev->kobj, &dme1737_in7_group);
>> + Â Â }
>> Â Â Â sysfs_remove_group(&dev->kobj, &dme1737_group);
>>
>> Â Â Â if (!data->client) {
>> @@ -2028,6 +2058,11 @@
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &dme1737_zone_hyst_group))) {
>> Â Â Â Â Â Â Â goto exit_remove;
>> Â Â Â }
>> + Â Â if ((data->has_features & HAS_IN7) &&
>> + Â Â Â Â (err = sysfs_create_group(&dev->kobj,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &dme1737_in7_group))) {
>> + Â Â Â Â Â Â goto exit_remove;
>> + Â Â }
>
> checkpatch.pl will complain. It wants:
>
> Â Â Â Âif (data->has_features & HAS_IN7) {
> Â Â Â Â Â Â Â Âerr = sysfs_create_group(&dev->kobj, &dme1737_in7_group);
> Â Â Â Â Â Â Â Âif (err)
> Â Â Â Â Â Â Â Â Â Â Â Âgoto exit_remove;
> Â Â Â Â}
OK. Assignments in if statements are being used all over the place in
this driver. I guess I should clean those up, shouldn't I?
>>
>> Â Â Â /* Create fan sysfs attributes */
>> Â Â Â for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) {
>> @@ -2186,7 +2221,7 @@
>> Â Â Â Â Â Â Â data->has_features |= HAS_ZONE3;
>> Â Â Â Â Â Â Â break;
>> Â Â Â case sch5127:
>> - Â Â Â Â Â Â data->has_features |= HAS_FAN(2) | HAS_PWM(2);
>> + Â Â Â Â Â Â data->has_features |= HAS_FAN(2) | HAS_PWM(2) | HAS_IN7;
>> Â Â Â Â Â Â Â break;
>> Â Â Â default:
>> Â Â Â Â Â Â Â break;
>>
>
> Thanks,
> --
> Jean Delvare
...Juerg
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
