[PATCH openbmc] add occ hwmon driver as kernel patch

[Yi TZ Li]

Hi Joel,

Thank you for the detailed comments. I made changes according to your
comments, with a few questions please see bellow.
I just sent out a pull request to openbmc/linux dev-4.3 branch:

https://github.com/openbmc/linux/pull/22/files

Please have a review.

Thanks,
-adam

"openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org> wrote on
11/25/2015 07:34:35 AM:

> From: Joel Stanley <joel at jms.id.au>
> To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
> Cc: adamliyi <adamliyi at msn.com>, OpenBMC Maillist
<openbmc at lists.ozlabs.org>
> Date: 11/25/2015 07:35 AM
> Subject: Re: [PATCH openbmc] add occ hwmon driver as kernel patch
> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org>
>
> Hello Adam,
>
> Thanks for posting your code.
>
> I've had a quick look and pointed out some style related changes that
> you can make. This is important in getting upstream to accept your
> patch, but also to make the code clear.
>
> Once you've made the fixes I suggest, try running your patch through
> scripts/checkpatch.pl in the Linux source tree. This will point out
> lines that do not confirm to the coding standards the kernel uses. Try
> to fix all of these warnings, and send an updated version of the
> patch.
>
> Next time you send the patch, send it as a PR to the kernel tree at
> https://github.com/openbmc/linux. This is easier than reading a
> patch-against-patch, and it's good practice for submitting the patch
> upstream.
>
> Please jump on the IRC channel at #openbmc on irc.freenode.net if you
> want do discuss any changes, or have any questions.
>
> Once you've fixed up the style issues, we will see what changes need
> to be made to the structure of the driver.
>
> Cheers,
>
> Joel
>
>
> > ++
> > ++typedef struct {
> > ++      char sensor_type[5];
>
> uint8_t instead of a char?

Kernel code prefers uint8_t than char? Any concerns with char?

>
> > ++
> > ++typedef struct {
> > ++      uint8_t status;
> > ++      uint8_t ext_status;
> > ++      uint8_t occs_present;
> > ++      uint8_t config;
> > ++      uint8_t occ_state;
> > ++      uint8_t reserved0;
> > ++      uint8_t reserved1;
> > ++      uint8_t error_log_id;
> > ++      uint32_t error_log_addr_start;
> > ++      uint16_t error_log_length;
> > ++      uint8_t reserved2;
> > ++      uint8_t reserved3;
> > ++      char occ_code_level[17];
>
> Code level is an IBM term. Perhaps call this version_string?
>

I changed it to version_string. But I checked the OCC document, the
document uses code_level.
I may need to change it back.

>
> > ++      address = address << 1;
> > ++
> > ++      ret = occ_i2c_write(client, address_buf, sizeof(address));
> > ++      /* FIXME: ast i2c driver does not read corret value */
> > ++      //if (ret != sizeof(address))
> > ++      //      return -I2C_WRITE_ERROR;
> > ++
> > ++      ret = occ_i2c_read(client, buf, sizeof(buf));
> > ++      //if (ret != sizeof(buf))
> > ++      //      return -I2C_READ_ERROR;
>
> You've commented out the error handling here. Did you get to the
> bottom of why you're not getting the return value you're expecting
>

The aspeed i2c driver does not return the correct bytes number
sent/received when I was testing on Barreleye.
So I does not check the return value. I will fix it when the apseed-i2c
driver works ok.

>
>  strncpy(o->data.occ_code_level, d[21], 16)
>
> > ++      strncpy(&o->data.sensor_eye_catcher[0], (const char*)&d[37],
6);
> > ++      o->data.sensor_eye_catcher[6]='\0';
>
> use strscpy which provdies the null termination for you.
>

strscpy is a new kernel API? I'd prefer strncpy just to be compatible with
older kernel.

> > ++      o->data.num_of_sensor_blocks=d[43];
> > ++      o->data.sensor_data_version = d[44];
> > ++
> > ++      if (strcmp(o->data.sensor_eye_catcher, "SENSOR") != 0) {
> > ++              printk("ERROR: SENSOR not found at byte 37 (%s)
> \n",o->data.sensor_eye_catcher);
> > ++              return -1;
> > ++      }
> > ++
> > ++      if (o->data.num_of_sensor_blocks == 0) {
> > ++              printk("ERROR: SENSOR block num is 0\n");
> > ++              return -1;
> > ++      }
>
> Should these checks be before you parse the data?
>

What do you mean here?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151127/3c688fcb/attachment-0001.html>