lkml.org 
[lkml]   [2002]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: i2c-amd766 driver for 2.5.50
    Hi!

    > > +MOD_LIST_NAME := SENSORS_BUS_MODULES
    > > +
    > > +obj-$(CONFIG_I2C_MAINBOARD) += i2c-mainboard.o
    > > +obj-$(CONFIG_I2C_AMD756) += i2c-amd756.o
    > > +
    > > +include $(TOPDIR)/Rules.make
    >
    > MOD_LIST_NAME was deprecated in 2.3. 'include Rules.make' was
    > deprecated in 2.5. Also appears in drivers/i2c/chips/Makefile.

    I agree with MOD_LIST_NAME, but I still see include
    $(TOPDIR)/Rules.make used all over the kernel, so I kept it.

    > > +#ifndef PCI_DEVICE_ID_AMD_756
    > > +#define PCI_DEVICE_ID_AMD_756 0x740B
    > > +#endif
    > > +#ifndef PCI_DEVICE_ID_AMD_766
    > > +#define PCI_DEVICE_ID_AMD_766 0x7413
    > > +#endif
    > > +#ifndef PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS
    > > +#define PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS 0x01B4
    > > +#endif
    >
    > These are all in pci_ids.h already, under other names. If these names
    > are better, they should replace the others.

    Fixed.

    > > +struct sd {
    > > + const unsigned short vendor;
    > > + const unsigned short device;
    > > + const unsigned short function;
    > > + const char* name;
    > > + int amdsetup:1;
    > > +};
    > > +
    > > +static struct sd supported[] = {
    > > + {PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_756, 3, "AMD756", 1},
    > > + {PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_766, 3, "AMD766", 1},
    > > + {PCI_VENDOR_ID_AMD, 0x7443, 3, "AMD768", 1},
    > > + {PCI_VENDOR_ID_NVIDIA, 0x01B4, 1, "nVidia nForce", 0},
    > > + {0, 0, 0}
    > > +};
    >
    > You should also have a struct pci_device_id[] here, so you can have a
    > MODULE_DEVICE_TABLE().

    This one will have to wait...

    > > +/* OK, this is not exactly good programming practice, usually. But it is
    > > + very code-efficient in this case. */
    >
    > No need to apologise for goto error unwinding - it's all over the
    > > kernel.

    Yep, killed comment.

    > > +void adm1021_dec_use(struct i2c_client *client)
    > > +{
    > > +#ifdef MODULE
    > > + MOD_DEC_USE_COUNT;
    > > +#endif
    > > +}
    >
    > No need for #ifdef. Also found in lm75_inc_use() and elsewhere.

    Agreed, killed those I could find quickly.

    > > +void adm1021_update_client(struct i2c_client *client)
    > > +{
    > > + struct adm1021_data *data = client->data;
    > > +
    > > + down(&data->update_lock);
    > > +
    > > + if ((jiffies - data->last_updated > HZ + HZ / 2) ||
    > > + (jiffies < data->last_updated) || !data->valid) {
    >
    > if (time_after(jiffies, data->last_updated + HZ+HZ/2) || !data->valid) {
    >
    > It *appears* the (jiffies < data->last_updated) test is unnecessary.

    I guess I'd better leave it alone :-).

    > > +EXPORT_NO_SYMBOLS;
    >
    > Deprecated (from lm75.c).

    Killed.

    > General comment: what's up with /proc/sys/dev/ versus /proc/driver/
    > versus sysfs? Do we really need all three?

    sensors are sysctl-capable, /proc/sys/dev/ is just side effect, IIRC.

    Pavel

    --
    Casualities in World Trade Center: ~3k dead inside the building,
    cryptography in U.S.A. and free speech in Czech Republic.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 13:31    [W:0.024 / U:30.028 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site