lkml.org 
[lkml]   [2007]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/7] [RFC] ds2760 W1 slave
    On Thu, Apr 12, 2007 at 03:25:17AM +0400, Anton Vorontsov (cbou@mail.ru) wrote:
    > This is W1 slave for ds2760 chip, found inside almost every HP iPaq and
    > HTC PDAs/phones.

    Hi Anton, I have some cleanup-related comments, patch itself looks good.

    > diff --git a/drivers/w1/slaves/w1_ds2760.c b/drivers/w1/slaves/w1_ds2760.c
    > new file mode 100644
    > index 0000000..21b0ef6
    > --- /dev/null
    > +++ b/drivers/w1/slaves/w1_ds2760.c
    > @@ -0,0 +1,162 @@
    > +/*
    > + * 1-Wire implementation for the ds2760 chip
    > + *
    > + * Copyright (c) 2004-2005, Szabolcs Gyurko <szabolcs.gyurko@tlt.hu>
    > + *
    > + * Use consistent with the GNU GPL is permitted,
    > + * provided that this copyright notice is
    > + * preserved in its entirety in all copies and derived works.
    > + *
    > + */
    > +
    > +#include <asm/types.h>

    Is it really needed?

    > +
    > +#include <linux/kernel.h>
    > +#include <linux/module.h>
    > +#include <linux/moduleparam.h>
    > +#include <linux/device.h>
    > +#include <linux/types.h>
    > +#include <linux/jiffies.h>
    > +
    > +#include "../w1.h"
    > +#include "../w1_int.h"
    > +#include "../w1_family.h"
    > +#include "w1_ds2760.h"
    > +
    > +static int w1_ds2760_io(struct device *dev, char *buf, int addr, size_t count,
    > + int io)
    > +{
    > + struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
    > +
    > + if (!dev)
    > + return 0;
    > +
    > + mutex_lock(&sl->master->mutex);
    > +
    > + if (addr > DS2760_DATA_SIZE || addr < 0) {
    > + count = 0;
    > + goto out;
    > + }
    > + if (addr + count > DS2760_DATA_SIZE)
    > + count = DS2760_DATA_SIZE - addr;
    > +
    > + if (!w1_reset_select_slave(sl)) {
    > + if (!io) {
    > + w1_write_8(sl->master, W1_DS2760_READ_DATA);
    > + w1_write_8(sl->master, addr);
    > + count = w1_read_block(sl->master, buf, count);
    > + } else {
    > + w1_write_8(sl->master, W1_DS2760_WRITE_DATA);
    > + w1_write_8(sl->master, addr);
    > + w1_write_block(sl->master, buf, count);
    > + /* XXX w1_write_block returns void, not n_written */
    > + }
    > + }
    > +
    > +out:
    > + mutex_unlock(&sl->master->mutex);
    > +
    > + return count;
    > +}
    > +
    > +int w1_ds2760_read(struct device *dev, char *buf, int addr, size_t count)
    > +{
    > + return w1_ds2760_io(dev, buf, addr, count, 0);
    > +}
    > +
    > +int w1_ds2760_write(struct device *dev, char *buf, int addr, size_t count)
    > +{
    > + return w1_ds2760_io(dev, buf, addr, count, 1);
    > +}

    Both are exported, who use them?

    > +/* io = 0 means copy from EEPROM to SRAM, 1 means from SRAM to EEPROM */
    > +static int w1_ds2760_eeprom(struct device *dev, int addr, int io)
    > +{
    > + struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
    > + int ret = 0;
    > +
    > + mutex_lock(&sl->master->mutex);
    > +
    > + if (!w1_reset_select_slave(sl)) {
    > + if (!io)
    > + w1_write_8(sl->master, W1_DS2760_RECALL_DATA);
    > + else
    > + w1_write_8(sl->master, W1_DS2760_COPY_DATA);
    > + w1_write_8(sl->master, addr);
    > + }
    > +
    > + mutex_unlock(&sl->master->mutex);
    > +
    > + return ret;
    > +}
    > +
    > +int w1_ds2760_recall(struct device *dev, int addr)
    > +{
    > + return w1_ds2760_eeprom(dev, addr, 0);
    > +}
    > +
    > +int w1_ds2760_copy(struct device *dev, int addr)
    > +{
    > + return w1_ds2760_eeprom(dev, addr, 1);
    > +}

    The same.

    > +static ssize_t w1_ds2760_read_bin(struct kobject *kobj, char *buf, loff_t off,
    > + size_t count)
    > +{
    > + struct device *dev = container_of(kobj, struct device, kobj);
    > + return w1_ds2760_read(dev, buf, off, count);
    > +}
    > +
    > +static struct bin_attribute w1_ds2760_bin_attr = {
    > + .attr = {
    > + .name = "w1_slave",
    > + .mode = S_IRUGO,
    > + .owner = THIS_MODULE,
    > + },
    > + .size = DS2760_DATA_SIZE,
    > + .read = w1_ds2760_read_bin,
    > +};
    > +
    > +static int w1_ds2760_add_slave(struct w1_slave *sl)
    > +{
    > + return sysfs_create_bin_file(&sl->dev.kobj, &w1_ds2760_bin_attr);
    > +}
    > +
    > +static void w1_ds2760_remove_slave(struct w1_slave *sl)
    > +{
    > + sysfs_remove_bin_file(&sl->dev.kobj, &w1_ds2760_bin_attr);
    > +}
    > +
    > +static struct w1_family_ops w1_ds2760_fops = {
    > + .add_slave = w1_ds2760_add_slave,
    > + .remove_slave = w1_ds2760_remove_slave,
    > +};
    > +
    > +static struct w1_family w1_ds2760_family = {
    > + .fid = W1_FAMILY_DS2760,
    > + .fops = &w1_ds2760_fops,
    > +};
    > +
    > +static int __init w1_ds2760_init(void)
    > +{
    > + printk("1-Wire driver for the DS2760 battery monitor chip "
    > + " - (c) 2004-2005, Szabolcs Gyurko\n");
    > + return w1_register_family(&w1_ds2760_family);
    > +}

    Add something like KERN_INFO into printk.


    --
    Evgeniy Polyakov
    -
    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: 2007-04-12 09:11    [W:0.037 / U:0.304 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site