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