<html><body><p>Hi Joel,<br><br>Thank you for the detailed comments. I made changes according to your comments, with a few questions please see bellow.<br>I just sent out a pull request to openbmc/linux dev-4.3 branch:<br><br><a href="https://github.com/openbmc/linux/pull/22/files">https://github.com/openbmc/linux/pull/22/files</a><br><br>Please have a review.<br><br>Thanks,<br>-adam<br><br><tt>"openbmc" <openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org> wrote on 11/25/2015 07:34:35 AM:<br><br>> From: Joel Stanley <joel@jms.id.au></tt><br><tt>> To: OpenBMC Patches <openbmc-patches@stwcx.xyz></tt><br><tt>> Cc: adamliyi <adamliyi@msn.com>, OpenBMC Maillist <openbmc@lists.ozlabs.org></tt><br><tt>> Date: 11/25/2015 07:35 AM</tt><br><tt>> Subject: Re: [PATCH openbmc] add occ hwmon driver as kernel patch</tt><br><tt>> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org></tt><br><tt>> <br>> Hello Adam,<br>> <br>> Thanks for posting your code.<br>> <br>> I've had a quick look and pointed out some style related changes that<br>> you can make. This is important in getting upstream to accept your<br>> patch, but also to make the code clear.<br>> <br>> Once you've made the fixes I suggest, try running your patch through<br>> scripts/checkpatch.pl in the Linux source tree. This will point out<br>> lines that do not confirm to the coding standards the kernel uses. Try<br>> to fix all of these warnings, and send an updated version of the<br>> patch.<br>> <br>> Next time you send the patch, send it as a PR to the kernel tree at<br>> <a href="https://github.com/openbmc/linux">https://github.com/openbmc/linux</a>. This is easier than reading a<br>> patch-against-patch, and it's good practice for submitting the patch<br>> upstream.<br>> <br>> Please jump on the IRC channel at #openbmc on irc.freenode.net if you<br>> want do discuss any changes, or have any questions.<br>> <br>> Once you've fixed up the style issues, we will see what changes need<br>> to be made to the structure of the driver.<br>> <br>> Cheers,<br>> <br>> Joel<br>> <br>> <br>> > ++<br>> > ++typedef struct {<br>> > ++ char sensor_type[5];<br>> <br>> uint8_t instead of a char?</tt><br><br><tt>Kernel code prefers uint8_t than char? Any concerns with char?</tt><br><tt><br>> <br>> > ++<br>> > ++typedef struct {<br>> > ++ uint8_t status;<br>> > ++ uint8_t ext_status;<br>> > ++ uint8_t occs_present;<br>> > ++ uint8_t config;<br>> > ++ uint8_t occ_state;<br>> > ++ uint8_t reserved0;<br>> > ++ uint8_t reserved1;<br>> > ++ uint8_t error_log_id;<br>> > ++ uint32_t error_log_addr_start;<br>> > ++ uint16_t error_log_length;<br>> > ++ uint8_t reserved2;<br>> > ++ uint8_t reserved3;<br>> > ++ char occ_code_level[17];<br>> <br>> Code level is an IBM term. Perhaps call this version_string?<br>></tt><br><br><tt>I changed it to version_string. But I checked the OCC document, the document uses code_level.</tt><br><tt>I may need to change it back.</tt><br><tt> <br>> <br>> > ++ address = address << 1;<br>> > ++<br>> > ++ ret = occ_i2c_write(client, address_buf, sizeof(address));<br>> > ++ /* FIXME: ast i2c driver does not read corret value */<br>> > ++ //if (ret != sizeof(address))<br>> > ++ // return -I2C_WRITE_ERROR;<br>> > ++<br>> > ++ ret = occ_i2c_read(client, buf, sizeof(buf));<br>> > ++ //if (ret != sizeof(buf))<br>> > ++ // return -I2C_READ_ERROR;<br>> <br>> You've commented out the error handling here. Did you get to the<br>> bottom of why you're not getting the return value you're expecting<br>></tt><br><br><tt>The aspeed i2c driver does not return the correct bytes number sent/received when I was testing on Barreleye.</tt><br><tt>So I does not check the return value. I will fix it when the apseed-i2c driver works ok.</tt><br><tt> <br>> <br>> strncpy(o->data.occ_code_level, d[21], 16)<br>> <br>> > ++ strncpy(&o->data.sensor_eye_catcher[0], (const char*)&d[37], 6);<br>> > ++ o->data.sensor_eye_catcher[6]='\0';<br>> <br>> use strscpy which provdies the null termination for you.<br>> </tt><br><br><tt>strscpy is a new kernel API? I'd prefer strncpy just to be compatible with older kernel.</tt><br><tt><br>> > ++ o->data.num_of_sensor_blocks=d[43];<br>> > ++ o->data.sensor_data_version = d[44];<br>> > ++<br>> > ++ if (strcmp(o->data.sensor_eye_catcher, "SENSOR") != 0) {<br>> > ++ printk("ERROR: SENSOR not found at byte 37 (%s)<br>> \n",o->data.sensor_eye_catcher);<br>> > ++ return -1;<br>> > ++ }<br>> > ++<br>> > ++ if (o->data.num_of_sensor_blocks == 0) {<br>> > ++ printk("ERROR: SENSOR block num is 0\n");<br>> > ++ return -1;<br>> > ++ }<br>> <br>> Should these checks be before you parse the data?<br>></tt><br><br><tt>What do you mean here?</tt><br><tt> <br></tt><BR>
</body></html>