Re: [PATCH v2] w1: Add Maxim/Dallas DS2780 Stand-Alone Fuel GaugeIC support.

From: Clifton Barnes
Date: Tue May 17 2011 - 11:34:30 EST


On Wednesday, May 11, 2011 Ryan Mallon wrote:

The encoding seems to have messed up on LKML so I'm resending in case
it is messed up for anyone else.

> > + if ((new_setting != 0) && (new_setting != 1)) {

> Don't need the inner parens.

Unless it's a common convention, I'd rather leave them because I think
it helps readability.

> > + ret = kstrtou8(buf, 10, &new_setting);

> Might be worth allowing people to write register values in hex also.

If I'm reading the code correctly, I change the base to 0 to achieve
this, correct?

> > +static ssize_t ds2780_get_user_eeprom(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + int ret;
> > + struct power_supply *psy = to_power_supply(dev);
> > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy);
> > +
> > + ret = w1_ds2780_read(dev_info->w1_dev, buf, DS2780_EEPROM_BLOCK0_START,
> > + DS2780_USER_EEPROM_SIZE);
> > + if (ret < 0)
> > + return ret;

> Not sure that this is really obeying the rules of sysfs which state that
> files should only contain a single value. There is the firmware
> subsystem, but I'm not sure that is really what you want either. Perhaps
> somebody else can suggest an alternative.

It looks like other EEPROMs use bin_attribute. I'll change it to use that
unless there's a better approach.

> > +config W1_SLAVE_DS2780
> > + tristate "Dallas 2780 battery monitor chip"
> > + depends on W1
> > + help
> > + If you enable this you will have the DS2780 battery monitor
> > + chip support.
> > +
> > + The battery monitor chip is used in many batteries/devices
> > + as the one who is responsible for charging/discharging/monitoring
> > + Li+ batteries.
> > +
> > + If you are unsure, say N.
> > +

> This should just be:
>
> config W1_SLAVE_DS2780:
> tristate
>
> since CONFIG_BATTERY_DS2780 selects this there is never any need for it
> to be a visible config option.

I did this the same way as the DS2760. Should this be different?

> > +static ssize_t w1_ds2780_read_bin(struct file *filp, struct kobject *kobj,
> > + struct bin_attribute *bin_attr,
> > + char *buf, loff_t off, size_t count)
> > +{
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + return w1_ds2780_read(dev, buf, off, count);
> > +}

> What is this for?

It reads raw registers from the device. It was implemented in the w1_ds2760.c
file so I kept it. I suppose you could use this driver without the battery
interface and read the registers that way.

> > +static int new_bat_id(void)
> > +{
> > + int ret;
> > +
> > + while (1) {
> > + int id;
> > +
> > + ret = idr_pre_get(&bat_idr, GFP_KERNEL);
> > + if (ret == 0)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&bat_idr_lock);
> > + ret = idr_get_new(&bat_idr, NULL, &id);
> > + mutex_unlock(&bat_idr_lock);
> > +
> > + if (ret == 0) {
> > + ret = id & MAX_ID_MASK;
> > + break;
> > + } else if (ret == -EAGAIN) {
> > + continue;
> > + } else {
> > + break;
> > + }
> > + }

> Is it common to do this in a while loop? In my experience if the
> idr_get_new fails an error should be returned.

Again, this came from the w1_ds2760.c driver. If it's more common to
error out, I can change it.

I'm in the process of making the other changes that were suggested.
How should I submit the changes? A new patch v3 or a patch to v2?
If a patch to v2, how should that be indicated?

-Clif


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/