[PATCH linux] Add BMC HWMON driver to kernel. Format patch based on Joel's comments.
Yi TZ Li
shliyi at cn.ibm.com
Mon Dec 7 03:20:21 AEDT 2015
Hi Jeremy,
Thanks many for your review and comments.
I made changes to the driver according to your comments, with bellow
exceptions:
"openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org> wrote on
11/27/2015 03:20:31 PM:
> From: Jeremy Kerr <jk at ozlabs.org>
> To: openbmc at lists.ozlabs.org
> Cc: adamliyi <adamliyi at msn.com>
> Date: 11/27/2015 03:20 PM
> Subject: Re: [PATCH linux] Add BMC HWMON driver to kernel. Format
> patch based on Joel's comments.
> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org>
>
> Hi Adam,
>
> Thanks for v2. Some further comments:
>
>
> > + (const char *)&d[dnum], 4);
> > + o->data.blocks[b].reserved0 = d[dnum+4];
> > + o->data.blocks[b].sensor_format = d[dnum+5];
> > + o->data.blocks[b].sensor_length = d[dnum+6];
> > + o->data.blocks[b].num_of_sensors = d[dnum+7];
> > + dnum = dnum + 8;
>
> More magic numbers. Can we just use a struct assignment for things like
> this?
>
I agree these magic numbers are ugly. However, using struct assignemt would
need to take care of structure element alignment and endianess. In my new
code, I used
"
+ memcpy(&o->header, &d[RESP_HEADER_OFFSET], sizeof(o->
header));
+ o->
header.error_log_addr_start =
+ be32_to_cpu(o->
header.error_log_addr_start);
+ o->header.error_log_length = be16_to_cpu(o->
header.error_log_length);
"
to replace a 45-byte data block parsing.
For this 8-byte data parsing, I'd prefer to parse them one-by-one, since it
looks more easy to understand.
>
>
> > +static struct sensor_device_attribute temp_input[] = {
> > + SENSOR_ATTR(temp1_input, S_IRUGO, show_occ_temp_input, NULL, 1),
> > + SENSOR_ATTR(temp2_input, S_IRUGO, show_occ_temp_input, NULL, 2),
> > + SENSOR_ATTR(temp3_input, S_IRUGO, show_occ_temp_input, NULL, 3),
> > + SENSOR_ATTR(temp4_input, S_IRUGO, show_occ_temp_input, NULL, 4),
> > + SENSOR_ATTR(temp5_input, S_IRUGO, show_occ_temp_input, NULL, 5),
> > + SENSOR_ATTR(temp6_input, S_IRUGO, show_occ_temp_input, NULL, 6),
> > + SENSOR_ATTR(temp7_input, S_IRUGO, show_occ_temp_input, NULL, 7),
> > + SENSOR_ATTR(temp8_input, S_IRUGO, show_occ_temp_input, NULL, 8),
> > + SENSOR_ATTR(temp9_input, S_IRUGO, show_occ_temp_input, NULL, 9),
> > + SENSOR_ATTR(temp10_input, S_IRUGO, show_occ_temp_input, NULL, 10),
> > + SENSOR_ATTR(temp11_input, S_IRUGO, show_occ_temp_input, NULL, 11),
> > + SENSOR_ATTR(temp12_input, S_IRUGO, show_occ_temp_input, NULL, 12),
> > + SENSOR_ATTR(temp13_input, S_IRUGO, show_occ_temp_input, NULL, 13),
> > + SENSOR_ATTR(temp14_input, S_IRUGO, show_occ_temp_input, NULL, 14),
> > + SENSOR_ATTR(temp15_input, S_IRUGO, show_occ_temp_input, NULL, 15),
> > + SENSOR_ATTR(temp16_input, S_IRUGO, show_occ_temp_input, NULL, 16),
> > + SENSOR_ATTR(temp17_input, S_IRUGO, show_occ_temp_input, NULL, 17),
> > + SENSOR_ATTR(temp18_input, S_IRUGO, show_occ_temp_input, NULL, 18),
> > + SENSOR_ATTR(temp19_input, S_IRUGO, show_occ_temp_input, NULL, 19),
> > + SENSOR_ATTR(temp20_input, S_IRUGO, show_occ_temp_input, NULL, 20),
> > + SENSOR_ATTR(temp21_input, S_IRUGO, show_occ_temp_input, NULL, 21),
> > + SENSOR_ATTR(temp22_input, S_IRUGO, show_occ_temp_input, NULL, 22),
> > +};
> > +
> > +static struct sensor_device_attribute temp_label[] = {
> > + SENSOR_ATTR(temp1_label, S_IRUGO, show_occ_temp_label, NULL, 1),
> > + SENSOR_ATTR(temp2_label, S_IRUGO, show_occ_temp_label, NULL, 2),
> > + SENSOR_ATTR(temp3_label, S_IRUGO, show_occ_temp_label, NULL, 3),
> > + SENSOR_ATTR(temp4_label, S_IRUGO, show_occ_temp_label, NULL, 4),
> > + SENSOR_ATTR(temp5_label, S_IRUGO, show_occ_temp_label, NULL, 5),
> > + SENSOR_ATTR(temp6_label, S_IRUGO, show_occ_temp_label, NULL, 6),
> > + SENSOR_ATTR(temp7_label, S_IRUGO, show_occ_temp_label, NULL, 7),
> > + SENSOR_ATTR(temp8_label, S_IRUGO, show_occ_temp_label, NULL, 8),
> > + SENSOR_ATTR(temp9_label, S_IRUGO, show_occ_temp_label, NULL, 9),
> > + SENSOR_ATTR(temp10_label, S_IRUGO, show_occ_temp_label, NULL, 10),
> > + SENSOR_ATTR(temp11_label, S_IRUGO, show_occ_temp_label, NULL, 11),
> > + SENSOR_ATTR(temp12_label, S_IRUGO, show_occ_temp_label, NULL, 12),
> > + SENSOR_ATTR(temp13_label, S_IRUGO, show_occ_temp_label, NULL, 13),
> > + SENSOR_ATTR(temp14_label, S_IRUGO, show_occ_temp_label, NULL, 14),
> > + SENSOR_ATTR(temp15_label, S_IRUGO, show_occ_temp_label, NULL, 15),
> > + SENSOR_ATTR(temp16_label, S_IRUGO, show_occ_temp_label, NULL, 16),
> > + SENSOR_ATTR(temp17_label, S_IRUGO, show_occ_temp_label, NULL, 17),
> > + SENSOR_ATTR(temp18_label, S_IRUGO, show_occ_temp_label, NULL, 18),
> > + SENSOR_ATTR(temp19_label, S_IRUGO, show_occ_temp_label, NULL, 19),
> > + SENSOR_ATTR(temp20_label, S_IRUGO, show_occ_temp_label, NULL, 20),
> > + SENSOR_ATTR(temp21_label, S_IRUGO, show_occ_temp_label, NULL, 21),
> > + SENSOR_ATTR(temp22_label, S_IRUGO, show_occ_temp_label, NULL, 22),
> > +
> > +};
> > +
> > +#define TEMP_UNIT_ATTRS(X) \
> > +{ &temp_input[X].dev_attr.attr, \
> > + &temp_label[X].dev_attr.attr, \
> > + NULL \
> > +}
> > +
> > +/* 10-core CPU, occ has 22 temp sensors, more socket, more sensors */
> > +static struct attribute *occ_temp_attr[][3] = {
> > + TEMP_UNIT_ATTRS(0),
> > + TEMP_UNIT_ATTRS(1),
> > + TEMP_UNIT_ATTRS(2),
> > + TEMP_UNIT_ATTRS(3),
> > + TEMP_UNIT_ATTRS(4),
> > + TEMP_UNIT_ATTRS(5),
> > + TEMP_UNIT_ATTRS(6),
> > + TEMP_UNIT_ATTRS(7),
> > + TEMP_UNIT_ATTRS(8),
> > + TEMP_UNIT_ATTRS(9),
> > + TEMP_UNIT_ATTRS(10),
> > + TEMP_UNIT_ATTRS(11),
> > + TEMP_UNIT_ATTRS(12),
> > + TEMP_UNIT_ATTRS(13),
> > + TEMP_UNIT_ATTRS(14),
> > + TEMP_UNIT_ATTRS(15),
> > + TEMP_UNIT_ATTRS(16),
> > + TEMP_UNIT_ATTRS(17),
> > + TEMP_UNIT_ATTRS(18),
> > + TEMP_UNIT_ATTRS(19),
> > + TEMP_UNIT_ATTRS(20),
> > + TEMP_UNIT_ATTRS(21),
> > +};
> > +
> > +static const struct attribute_group occ_temp_attr_group[] = {
> > + { .attrs = occ_temp_attr[0] },
> > + { .attrs = occ_temp_attr[1] },
> > + { .attrs = occ_temp_attr[2] },
> > + { .attrs = occ_temp_attr[3] },
> > + { .attrs = occ_temp_attr[4] },
> > + { .attrs = occ_temp_attr[5] },
> > + { .attrs = occ_temp_attr[6] },
> > + { .attrs = occ_temp_attr[7] },
> > + { .attrs = occ_temp_attr[8] },
> > + { .attrs = occ_temp_attr[9] },
> > + { .attrs = occ_temp_attr[10] },
> > + { .attrs = occ_temp_attr[11] },
> > + { .attrs = occ_temp_attr[12] },
> > + { .attrs = occ_temp_attr[13] },
> > + { .attrs = occ_temp_attr[14] },
> > + { .attrs = occ_temp_attr[15] },
> > + { .attrs = occ_temp_attr[16] },
> > + { .attrs = occ_temp_attr[17] },
> > + { .attrs = occ_temp_attr[18] },
> > + { .attrs = occ_temp_attr[19] },
> > + { .attrs = occ_temp_attr[20] },
> > + { .attrs = occ_temp_attr[21] },
> > +};
> > +
> > +
> > +static struct sensor_device_attribute freq_input[] = {
> > + SENSOR_ATTR(freq1_input, S_IRUGO, show_occ_freq_input, NULL, 1),
> > + SENSOR_ATTR(freq2_input, S_IRUGO, show_occ_freq_input, NULL, 2),
> > + SENSOR_ATTR(freq3_input, S_IRUGO, show_occ_freq_input, NULL, 3),
> > + SENSOR_ATTR(freq4_input, S_IRUGO, show_occ_freq_input, NULL, 4),
> > + SENSOR_ATTR(freq5_input, S_IRUGO, show_occ_freq_input, NULL, 5),
> > + SENSOR_ATTR(freq6_input, S_IRUGO, show_occ_freq_input, NULL, 6),
> > + SENSOR_ATTR(freq7_input, S_IRUGO, show_occ_freq_input, NULL, 7),
> > + SENSOR_ATTR(freq8_input, S_IRUGO, show_occ_freq_input, NULL, 8),
> > + SENSOR_ATTR(freq9_input, S_IRUGO, show_occ_freq_input, NULL, 9),
> > + SENSOR_ATTR(freq10_input, S_IRUGO, show_occ_freq_input, NULL, 10),
> > +};
> > +
> > +static struct sensor_device_attribute freq_label[] = {
> > + SENSOR_ATTR(freq1_label, S_IRUGO, show_occ_freq_label, NULL, 1),
> > + SENSOR_ATTR(freq2_label, S_IRUGO, show_occ_freq_label, NULL, 2),
> > + SENSOR_ATTR(freq3_label, S_IRUGO, show_occ_freq_label, NULL, 3),
> > + SENSOR_ATTR(freq4_label, S_IRUGO, show_occ_freq_label, NULL, 4),
> > + SENSOR_ATTR(freq5_label, S_IRUGO, show_occ_freq_label, NULL, 5),
> > + SENSOR_ATTR(freq6_label, S_IRUGO, show_occ_freq_label, NULL, 6),
> > + SENSOR_ATTR(freq7_label, S_IRUGO, show_occ_freq_label, NULL, 7),
> > + SENSOR_ATTR(freq8_label, S_IRUGO, show_occ_freq_label, NULL, 8),
> > + SENSOR_ATTR(freq9_label, S_IRUGO, show_occ_freq_label, NULL, 9),
> > + SENSOR_ATTR(freq10_label, S_IRUGO, show_occ_freq_label, NULL, 10),
> > +
> > +};
> > +
> > +#define FREQ_UNIT_ATTRS(X) \
> > +{ &freq_input[X].dev_attr.attr, \
> > + &freq_label[X].dev_attr.attr, \
> > + NULL \
> > +}
> > +
> > +/* 10-core CPU, occ has 22 freq sensors, more socket, more sensors */
> > +static struct attribute *occ_freq_attr[][3] = {
> > + FREQ_UNIT_ATTRS(0),
> > + FREQ_UNIT_ATTRS(1),
> > + FREQ_UNIT_ATTRS(2),
> > + FREQ_UNIT_ATTRS(3),
> > + FREQ_UNIT_ATTRS(4),
> > + FREQ_UNIT_ATTRS(5),
> > + FREQ_UNIT_ATTRS(6),
> > + FREQ_UNIT_ATTRS(7),
> > + FREQ_UNIT_ATTRS(8),
> > + FREQ_UNIT_ATTRS(9),
> > +};
> > +
> > +static const struct attribute_group occ_freq_attr_group[] = {
> > + { .attrs = occ_freq_attr[0] },
> > + { .attrs = occ_freq_attr[1] },
> > + { .attrs = occ_freq_attr[2] },
> > + { .attrs = occ_freq_attr[3] },
> > + { .attrs = occ_freq_attr[4] },
> > + { .attrs = occ_freq_attr[5] },
> > + { .attrs = occ_freq_attr[6] },
> > + { .attrs = occ_freq_attr[7] },
> > + { .attrs = occ_freq_attr[8] },
> > + { .attrs = occ_freq_attr[9] },
> > +};
> > +
> > +static struct sensor_device_attribute_2 caps_curr_powercap[] = {
> > + SENSOR_ATTR_2(caps_curr_powercap, S_IRUGO, show_occ_caps, NULL, 0,
1),
> > +};
> > +static struct sensor_device_attribute_2 caps_curr_powerreading[] = {
> > + SENSOR_ATTR_2(caps_curr_powerreading, S_IRUGO,
> > + show_occ_caps, NULL, 1, 1),
> > +};
> > +static struct sensor_device_attribute_2 caps_norm_powercap[] = {
> > + SENSOR_ATTR_2(caps_norm_powercap, S_IRUGO, show_occ_caps,
> > + NULL, 2, 1),
> > +};
> > +static struct sensor_device_attribute_2 caps_max_powercap[] = {
> > + SENSOR_ATTR_2(caps_max_powercap, S_IRUGO, show_occ_caps, NULL, 3,
1),
> > +};
> > +static struct sensor_device_attribute_2 caps_min_powercap[] = {
> > + SENSOR_ATTR_2(caps_min_powercap, S_IRUGO, show_occ_caps, NULL, 4,
1),
> > +};
> > +static struct sensor_device_attribute_2 caps_user_powerlimit[] = {
> > + SENSOR_ATTR_2(caps_user_powerlimit, S_IRUGO, show_occ_caps, NULL,
5, 1),
> > +};
> > +#define CAPS_UNIT_ATTRS(X) \
> > +{ &caps_curr_powercap[X].dev_attr.attr, \
> > + &caps_curr_powerreading[X].dev_attr.attr, \
> > + &caps_norm_powercap[X].dev_attr.attr, \
> > + &caps_max_powercap[X].dev_attr.attr, \
> > + &caps_min_powercap[X].dev_attr.attr, \
> > + &caps_user_powerlimit[X].dev_attr.attr, \
> > + NULL \
> > +}
> > +
> > +/* 10-core CPU, occ has 1 caps sensors */
> > +static struct attribute *occ_caps_attr[][7] = {
> > + CAPS_UNIT_ATTRS(0),
> > +};
> > +static const struct attribute_group occ_caps_attr_group[] = {
> > + { .attrs = occ_caps_attr[0] },
> > +};
> > +
> > +static struct sensor_device_attribute power_input[] = {
> > + SENSOR_ATTR(power1_input, S_IRUGO, show_occ_power_input, NULL, 1),
> > + SENSOR_ATTR(power2_input, S_IRUGO, show_occ_power_input, NULL, 2),
> > + SENSOR_ATTR(power3_input, S_IRUGO, show_occ_power_input, NULL, 3),
> > + SENSOR_ATTR(power4_input, S_IRUGO, show_occ_power_input, NULL, 4),
> > + SENSOR_ATTR(power5_input, S_IRUGO, show_occ_power_input, NULL, 5),
> > + SENSOR_ATTR(power6_input, S_IRUGO, show_occ_power_input, NULL, 6),
> > + SENSOR_ATTR(power7_input, S_IRUGO, show_occ_power_input, NULL, 7),
> > + SENSOR_ATTR(power8_input, S_IRUGO, show_occ_power_input, NULL, 8),
> > + SENSOR_ATTR(power9_input, S_IRUGO, show_occ_power_input, NULL, 9),
> > + SENSOR_ATTR(power10_input, S_IRUGO, show_occ_power_input, NULL,
10),
> > + SENSOR_ATTR(power11_input, S_IRUGO, show_occ_power_input, NULL,
11),
> > +};
> > +
> > +static struct sensor_device_attribute power_label[] = {
> > + SENSOR_ATTR(power1_label, S_IRUGO, show_occ_power_label, NULL, 1),
> > + SENSOR_ATTR(power2_label, S_IRUGO, show_occ_power_label, NULL, 2),
> > + SENSOR_ATTR(power3_label, S_IRUGO, show_occ_power_label, NULL, 3),
> > + SENSOR_ATTR(power4_label, S_IRUGO, show_occ_power_label, NULL, 4),
> > + SENSOR_ATTR(power5_label, S_IRUGO, show_occ_power_label, NULL, 5),
> > + SENSOR_ATTR(power6_label, S_IRUGO, show_occ_power_label, NULL, 6),
> > + SENSOR_ATTR(power7_label, S_IRUGO, show_occ_power_label, NULL, 7),
> > + SENSOR_ATTR(power8_label, S_IRUGO, show_occ_power_label, NULL, 8),
> > + SENSOR_ATTR(power9_label, S_IRUGO, show_occ_power_label, NULL, 9),
> > + SENSOR_ATTR(power10_label, S_IRUGO, show_occ_power_label, NULL,
10),
> > + SENSOR_ATTR(power11_label, S_IRUGO, show_occ_power_label, NULL,
11),
> > +};
> > +
> > +#define POWER_UNIT_ATTRS(X) \
> > +{ &power_input[X].dev_attr.attr, \
> > + &power_label[X].dev_attr.attr, \
> > + NULL \
> > +}
> > +
> > +/* 10-core CPU, occ has 11 power sensors, more socket, more sensors */
> > +static struct attribute *occ_power_attr[][3] = {
> > + POWER_UNIT_ATTRS(0),
> > + POWER_UNIT_ATTRS(1),
> > + POWER_UNIT_ATTRS(2),
> > + POWER_UNIT_ATTRS(3),
> > + POWER_UNIT_ATTRS(4),
> > + POWER_UNIT_ATTRS(5),
> > + POWER_UNIT_ATTRS(6),
> > + POWER_UNIT_ATTRS(7),
> > + POWER_UNIT_ATTRS(8),
> > + POWER_UNIT_ATTRS(9),
> > + POWER_UNIT_ATTRS(10),
> > +};
> > +
> > +static const struct attribute_group occ_power_attr_group[] = {
> > + { .attrs = occ_power_attr[0] },
> > + { .attrs = occ_power_attr[1] },
> > + { .attrs = occ_power_attr[2] },
> > + { .attrs = occ_power_attr[3] },
> > + { .attrs = occ_power_attr[4] },
> > + { .attrs = occ_power_attr[5] },
> > + { .attrs = occ_power_attr[6] },
> > + { .attrs = occ_power_attr[7] },
> > + { .attrs = occ_power_attr[8] },
> > + { .attrs = occ_power_attr[9] },
> > + { .attrs = occ_power_attr[10] },
> > +};
>
> There's a lot of stuff repeated 10 times here. Can we not generate this
> device info at runtime instead? Do we need flexibility for different
> numbers of sensors? Should we be encoding this in the device tree node?
I talked with Norm and Patrick, the driver need to take care of the case
when some cores
and some sensors are broken. So the hwmon attributes number is not fixed.
And the hwmon drivers in drivers/hwmon/ are full of similar sensor
attribute definition.
I cannot think of a easier way to reduce these.
>
> > +
> > +static void occ_remove_sysfs_files(struct device *dev)
> > +{
> > + int i = 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(occ_temp_attr_group); i++)
> > + sysfs_remove_group(&dev->kobj, &occ_temp_attr_group[i]);
> > +
> > + for (i = 0; i < ARRAY_SIZE(occ_freq_attr_group); i++)
> > + sysfs_remove_group(&dev->kobj, &occ_freq_attr_group[i]);
> > +
> > + for (i = 0; i < ARRAY_SIZE(occ_power_attr_group); i++)
> > + sysfs_remove_group(&dev->kobj, &occ_power_attr_group[i]);
> > +
> > + for (i = 0; i < ARRAY_SIZE(occ_caps_attr_group); i++)
> > + sysfs_remove_group(&dev->kobj, &occ_caps_attr_group[i]);
> > +}
> > +
> > +
> > +static int occ_create_sysfs_attribute(struct device *dev)
> > +{
> > + /* The sensor number varies for different
> > + * platform depending on core number. We'd better
> > + * create them dynamically
> > + */
> > + struct occ_drv_data *drv_data = dev_get_drvdata(dev);
>
> You seem to be passing the struct device around a lot, and then doing
> dev_get_drvdata. Just pass your own struct instead.
>
Most of the "struct device" are used for dev_dbg(dev,..).. Maybe I should
remove those debug messages?
>
> > +/* used by old-style board info. */
> > +static const struct i2c_device_id occ_ids[] = {
> > + { OCC_I2C_NAME, occ_id, },
> > + { /* LIST END */ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, occ_ids);
>
> We should just rely on the DT instead of this.
>
I did some test, when removing this "i2c_device_id" definition,
"echo occ-i2c 0x50 > /sys/class/i2c-adaptor/i2c-3/new_device" would not
work.
We need this user space interface to instantiate i2c devices.
> Jeremy
> _______________________________________________
> openbmc mailing list
> openbmc at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151207/ce24de28/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ecblank.gif
Type: image/gif
Size: 45 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151207/ce24de28/attachment-0001.gif>
More information about the openbmc
mailing list