* [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a
@ 2011-04-21 13:35 Hans de Goede
2011-05-04 15:31 ` Jean Delvare
2011-05-04 17:53 ` Hans de Goede
0 siblings, 2 replies; 3+ messages in thread
From: Hans de Goede @ 2011-04-21 13:35 UTC (permalink / raw)
To: lm-sensors
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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 */
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)
--
1.7.4.4
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a
2011-04-21 13:35 [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a Hans de Goede
@ 2011-05-04 15:31 ` Jean Delvare
2011-05-04 17:53 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Jean Delvare @ 2011-05-04 15:31 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
On Thu, 21 Apr 2011 15:35:18 +0200, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a
2011-04-21 13:35 [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a Hans de Goede
2011-05-04 15:31 ` Jean Delvare
@ 2011-05-04 17:53 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2011-05-04 17:53 UTC (permalink / raw)
To: lm-sensors
Hi,
On 05/04/2011 05:31 PM, Jean Delvare wrote:
> Hi Hans,
>
> On Thu, 21 Apr 2011 15:35:18 +0200, Hans de Goede wrote:
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>
> 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.
>
Ok, I'll move the introduction of the sch5627_write_virtual_reg function to the
next patch, and adjust the commit msg.
> Other than this, the code looks good, with just one suggestion below:
Will take over your suggestion as well. I'll send a new version when you
answer my question to your comments on the next patch.
Regards,
Hans
>
>> ---
>> 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)
>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-04 17:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-21 13:35 [lm-sensors] [PATCH 1/2] hwmon/sch5627: Add a Hans de Goede
2011-05-04 15:31 ` Jean Delvare
2011-05-04 17:53 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.