Hi Hans,
On Thu, 21 Apr 2011 15:35:18 +0200, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
This patch adds the following warning:
CC [M] drivers/hwmon/sch5627.o
drivers/hwmon/sch5627.c:243:12: warning: âsch5627_write_virtual_regâ defined but not used
I get the point of splitting the changes into two separate patches, but
I'd rather introduce sch5627_write_virtual_reg() in the second patch,
where it is used, to avoid this warning.
Other than this, the code looks good, with just one suggestion below:
> ---
> drivers/hwmon/sch5627.c | 26 ++++++++++++++++++++++----
> 1 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> index 9a51dcc..09a47bf 100644
> --- a/drivers/hwmon/sch5627.c
> +++ b/drivers/hwmon/sch5627.c
> @@ -140,7 +140,7 @@ static inline void superio_exit(int base)
> release_region(base, 2);
> }
>
> -static int sch5627_read_virtual_reg(struct sch5627_data *data, u16 reg)
> +static int sch5627_send_cmd(struct sch5627_data *data, u8 cmd, u16 reg, u8 v)
> {
> u8 val;
> int i;
> @@ -163,10 +163,14 @@ static int sch5627_read_virtual_reg(struct sch5627_data *data, u16 reg)
> outb(0x80, data->addr + 3);
>
> /* Write Request Packet Header */
> - outb(0x02, data->addr + 4); /* Access Type: VREG read */
> + outb(cmd, data->addr + 4); /* VREG Access Type read:0x02 write:0x03 */
It might make sense to introduce defines for the read and write command
values? I think it would make the code more readable.
> outb(0x01, data->addr + 5); /* # of Entries: 1 Byte (8-bit) */
> outb(0x04, data->addr + 2); /* Mailbox AP to first data entry loc. */
>
> + /* Write Value field */
> + if (cmd == 0x03)
> + outb(v, data->addr + 4);
> +
> /* Write Address field */
> outb(reg & 0xff, data->addr + 6);
> outb(reg >> 8, data->addr + 7);
> @@ -224,8 +228,22 @@ static int sch5627_read_virtual_reg(struct sch5627_data *data, u16 reg)
> * But if we do that things don't work, so let's not.
> */
>
> - /* Read Data from Mailbox */
> - return inb(data->addr + 4);
> + /* Read Value field */
> + if (cmd == 0x02)
> + return inb(data->addr + 4);
> +
> + return 0;
> +}
> +
> +static int sch5627_read_virtual_reg(struct sch5627_data *data, u16 reg)
> +{
> + return sch5627_send_cmd(data, 0x02, reg, 0);
> +}
> +
> +static int sch5627_write_virtual_reg(struct sch5627_data *data,
> + u16 reg, u8 val)
> +{
> + return sch5627_send_cmd(data, 0x03, reg, val);
> }
>
> static int sch5627_read_virtual_reg16(struct sch5627_data *data, u16 reg)
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
