Prev: macbook webcam no longer works on .35-rc
Next: e1000e: receives no packets after resume (2.6.35-rc3-00262-g984bc96)
From: Jean Delvare on 2 Jul 2010 06:00
Hi Guenter,

On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > > +
> > > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > > +
> > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > > + = { 0xff, 0x67, 0xff, 0x69 };
> > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > > + = { 0xff, 0x68, 0xff, 0x6a };
> > > +
> > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
> >
> > Is it just me or these arrays aren't used anywhere?
> >
> > I think I would just drop them. The "0xff" are suspicious in the
> > original arrays, and the size difference between the common and
> > W83667HG-B cases is tricky. Anyone willing to add support for this
> > feature will need to read the datasheets anyway, so you don't add any
> > value by including the register addresses here.
>
> After removing the defines and trying to compile I remembered.
> I _knew_ there was a reason for not removing them.
> Guess it's too late (or early) here to do serious work.
>
> The defines _are_ used, in:
>
> fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> fan_functions(fan_step_output, FAN_STEP_OUTPUT)
>
> which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
>
> Tricky ... and that was also the reason why I retained the original
> global variables.

Tricky indeed. We normally don't accept code like this in the kernel.

> I'll move the pointers into per-device code as you suggested, but I'll
> have to think about how to do that w/o having to change a lot of code.

If code changes are desirable, let's just do them. You can do that in a
preliminary patch, and then your patch adding support for the
W83667HG-B goes on top of it.

> As for the 0xff - that pretty much applies to all chips supported by this driver.
> I guess it is supposed to mean "not supported", and as a result the code will
> write to a non-existing register. I don't really want to touch that.

I want you to touch that. Writing to non-existing registers is a bad
idea. You never know what actually happens when you do that.

> The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> configured to have only three pwm fan controllers (even though the W83667HG
> is supposed to have four per its datasheet). So the 4th element of the arrays
> will not be accessed by the code if W83667HG(-B) is detected.

OK.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 2 Jul 2010 06:00
On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > I'll also add a check for the HG-I.
> >
> > Do you have a datasheet for that one too?
> >
> No. However, since we have a tester, we will have test coverage,
> so I figured it should be worth a try. If it doesn't work
> I can still drop it from the final patch.

FYI, I have a datasheet, which unfortunately I am not allowed to share.
Please let me know if you want me to look up something for you.

> I suspect it is the same as the HG with a different chip ID.

Beware, it's a different device number (677 vs. 667) so I would expect
more differences.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Guenter Roeck on 2 Jul 2010 10:20
On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > > I'll also add a check for the HG-I.
> > >
> > > Do you have a datasheet for that one too?
> > >
> > No. However, since we have a tester, we will have test coverage,
> > so I figured it should be worth a try. If it doesn't work
> > I can still drop it from the final patch.
>
> FYI, I have a datasheet, which unfortunately I am not allowed to share.
> Please let me know if you want me to look up something for you.
>
How about the changed registers ? Would give us an idea if the chip is closer to
667 or to 667-B.

> > I suspect it is the same as the HG with a different chip ID.
>
> Beware, it's a different device number (677 vs. 667) so I would expect
> more differences.
>
Ah, I missed that. I thought it was 667-I. Yes, you are right, that suggests
more differences.

Guenter

> --
> Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 2 Jul 2010 11:00
On Fri, 2 Jul 2010 07:09:44 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 05:51:23AM -0400, Jean Delvare wrote:
> > On Fri, 2 Jul 2010 01:31:44 -0700, Guenter Roeck wrote:
> > > On Fri, Jul 02, 2010 at 04:13:45AM -0400, Jean Delvare wrote:
> > > > On Fri, 2 Jul 2010 01:07:04 -0700, Guenter Roeck wrote:
> > > > > I'll also add a check for the HG-I.
> > > >
> > > > Do you have a datasheet for that one too?
> > > >
> > > No. However, since we have a tester, we will have test coverage,
> > > so I figured it should be worth a try. If it doesn't work
> > > I can still drop it from the final patch.
> >
> > FYI, I have a datasheet, which unfortunately I am not allowed to share.
> > Please let me know if you want me to look up something for you.
> >
> How about the changed registers ? Would give us an idea if the chip is closer to
> 667 or to 667-B.

There is no summary of this available. The only way is to go through
both datasheets and compare all registers in sequence. This takes time,
which is exactly why I couldn't find the time to do it :(

> > > I suspect it is the same as the HG with a different chip ID.
> >
> > Beware, it's a different device number (677 vs. 667) so I would expect
> > more differences.
>
> Ah, I missed that. I thought it was 667-I. Yes, you are right, that suggests
> more differences.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Guenter Roeck on 2 Jul 2010 11:00
On Fri, Jul 02, 2010 at 05:49:49AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > > > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > > > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > > > + = { 0xff, 0x67, 0xff, 0x69 };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > > > + = { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
> > >
> > > Is it just me or these arrays aren't used anywhere?
> > >
> > > I think I would just drop them. The "0xff" are suspicious in the
> > > original arrays, and the size difference between the common and
> > > W83667HG-B cases is tricky. Anyone willing to add support for this
> > > feature will need to read the datasheets anyway, so you don't add any
> > > value by including the register addresses here.
> >
> > After removing the defines and trying to compile I remembered.
> > I _knew_ there was a reason for not removing them.
> > Guess it's too late (or early) here to do serious work.
> >
> > The defines _are_ used, in:
> >
> > fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> > fan_functions(fan_step_output, FAN_STEP_OUTPUT)
> >
> > which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
> >
> > Tricky ... and that was also the reason why I retained the original
> > global variables.
>
> Tricky indeed. We normally don't accept code like this in the kernel.
>
> > I'll move the pointers into per-device code as you suggested, but I'll
> > have to think about how to do that w/o having to change a lot of code.
>
> If code changes are desirable, let's just do them. You can do that in a
> preliminary patch, and then your patch adding support for the
> W83667HG-B goes on top of it.
>
Without the support for -B the changes are not really needed, so that patch
would not make much sense without it. Have you looked at v2 of the patch ?

> > As for the 0xff - that pretty much applies to all chips supported by this driver.
> > I guess it is supposed to mean "not supported", and as a result the code will
> > write to a non-existing register. I don't really want to touch that.
>
> I want you to touch that. Writing to non-existing registers is a bad
> idea. You never know what actually happens when you do that.
>
Good point.

Clean fix would be not to provide the unsupported attributes. Simple workaround
would be to return an error if a write is attempted on a non-supported attribute.
I am sure it would be better to not provide the attribute, but would you accept
the workaround ?

> > The size difference (3 entries vs. 4) doesn't matter, since the chips are both
> > configured to have only three pwm fan controllers (even though the W83667HG
> > is supposed to have four per its datasheet). So the 4th element of the arrays
> > will not be accessed by the code if W83667HG(-B) is detected.
>
> OK.
>
On a side note, any idea why the 4th pwm is disabled for the W83667HG ?

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: macbook webcam no longer works on .35-rc
Next: e1000e: receives no packets after resume (2.6.35-rc3-00262-g984bc96)