Hi Rudolf, Jean
Thanks for your kind review. Will submit second round patch for review.
On Sat, 26 Apr 2008 09:50:11 +0200
Jean Delvare <[EMAIL PROTECTED]> wrote:
> Hi Rudolf,
>
> thanks for the review.
>
> One preliminary note about the driver name. I'm a bit worried by the
> name "i2c-sch" because there is another manufacturer (SMSC) making
> chips named SCH-something which could include an SMBus controller. This
> could be a bit confusing for the users. Maybe we could rename this
> driver to i2c-isch, to make it clearer that it's for Intel SCH chips?
>
> On Fri, 25 Apr 2008 23:00:35 +0200, Rudolf Marek wrote:
> > Hello Alek,
> >
> > Please check the review bellow. It took me hour and half which was quite
> > unexpected when I started. Jean would you please take a look too?
>
> I'll review the next iteration. Some comments on what you wrote:
>
> > > +/*
> > > + i2c-sch.c - Part of lm_sensors, Linux kernel modules for hardware
> > > + monitoring
>
> These drivers are no longer "part of lm_sensors".
>
> > > (...)
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/stddef.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/ioport.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/apm_bios.h>
>
> I'm very curious why you'd need this one? Presumably a leftover from
> the i2c-piix4 driver...
>
> > > +#include <linux/io.h>
>
> > (...)
> > Maybe your mailer did something to the tabs. You can run:
> >
> > indent -kr -i8 i2ci-sch.c
> >
> > It will fix all coding style issues. (I think except that it put labels
> > indented, you may revert that). We take here also text attachments, which
> > are
> > fine through thunderbird. You seem to use clawmail so I cant help much.
>
> There's scripts/Lindent which has all the options.
>
> > > +
> > > +/* insmod parameters */
> > > +
> > > +/* If force is set to anything different from 0, we forcibly enable the
> > > + SCH. DANGEROUS! */
> > > +static int force;
> > > +module_param(force, int, 0);
> > > +MODULE_PARM_DESC(force, "Forcibly enable the I2C. DANGEROUS!");
> >
> > Hmm not sure if it is good now. The newer BIOSes are much more saner than
> > the
> > old - but here it is some left_over - check bellow.
>
> I agree with Rudolf here, we don't want any "force" parameter that
> isn't strictly necessary. And even then we'd rather complain to the
> manufacturers quickly, so that the BIOS can be fixed and the driver can
> be kept clean.
>
> > > (...)
> > > + switch (size) {
> > > + case I2C_SMBUS_PROC_CALL:
> > > + dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not
> > > supported!\n");
> > > + return -EPERM;
> >
> > I think this should not happen.
>
> It could happen, but there's no need to handle unsupported cases
> explicitly, just have a default case at the end which catches all the
> unsupported sizes and returns -EINVAL.
>
> > (...)
> > MAybe you can test the driver with following commands. Assuming that you
> > have
> > SPD eeprom on 0x50.
> >
> > modprobe i2c-dev
> > modprobe i2c-sch
> >
> > i2cdump -l
>
> i2cdetect -l
>
> > will list the busses, assuming it is 0 please try following:
> >
> > i2cdetect 0
> >
> > It should detect all devices, maybe on 0x50 or 0x51 SPD eeprom. Use this
> > device
> > for other tests:
> >
> > i2cdump 0 0x50 b
> > i2cdump 0 0x50 c
> > i2cdump 0 0x50 W
> > i2cdump 0 0x50 s
> >
> > This commands should produce same results.
> >
> > i2cdump 0 0x50 w
> >
> > You should see above in daisy chained mode. If the byte mode was:
> > 00: 80 08 07 0d 0a 02 40 00 04 50 60 00 82 08 00 01 [EMAIL PROTECTED]
> >
> > Now you should see:
> >
> > 00: 0880 0708 0d07 0a0d 020a 4002 0040 0400
> > 08: 5004 6050 0060 8200 0882 0008 0100 0e01 (e1 is from offset 0x10)
> >
> > CHeck log aftewards if there is nothing unusual.
>
> This is very important to check that the contents of the EEPROMs have
> not been modified by the dumps. If they are, it means that there's a
> bug in the driver, and also your system probably won't boot next time
> because the memory module won't be recognized. Better do you tests with
> a cheap memory module ;)
>
> > > (...)
> > > + switch (size) {
> > > + case SCH_BYTE:
> > > + /* Where is the result put? I assume here it is in
> > > + SMBHSTDAT0 but it might just as well be in the
> > > + SMBHSTCMD. No clue in the docs */
> >
> > Well check with my test ;)
>
> The comment above is taken directly from the i2c-piix4 driver, does it
> apply to the SCH at all? We should probably remove it from the
> i2c-piix4 driver anyway, we know the answer by now.
>
> > > (...)
> > > +static struct pci_device_id sch_ids[] = {
> > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC),
> > > + .driver_data = 0xf8 },
> >
> > Please set it to 0x40 as suggested above.
>
> Or just don't use driver_data at all for now? With a single device
> supported at the moment, I don't see the idea.
>
> > > (...)
> > > +MODULE_AUTHOR("Frodo Looijaard <[EMAIL PROTECTED]> and "
> > > + "Philip Edelbrock <[EMAIL PROTECTED]> and "
> > > + "Jacob Pan <[EMAIL PROTECTED]> ");
>
> You can remove Frodo and Philip here, they aren't the authors of _this_
> driver.
>
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
