RE: [PATCH v2] w1: Add Maxim/Dallas DS2780 Stand-Alone Fuel GaugeIC support.
From: Barnes, Clifton A.
Date: Tue May 17 2011 - 11:22:01 EST
On Wednesday, May 11, 2011 Ryan Mallon wrote:
> > + 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
èº{.nÇ+·®+%Ëlzwm
ébëæìr¸zX§»®w¥{ayºÊÚë,j¢f£¢·hàz¹®w¥¢¸¢·¦j:+v¨wèjØm¶ÿ¾«êçzZ+ùÝj"ú!¶iOæ¬z·vØ^¶m§ÿðÃnÆàþY&