On Thu, Sep 19, 2013 at 04:45:11PM -0400, Rhyland Klein wrote:
> On 9/19/2013 4:27 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Thu, Sep 19, 2013 at 12:18:33PM -0400, Rhyland Klein wrote:
> >> From: Darbha Sriharsha <dsriharsha@xxxxxxxxxx>
> >>
> >> Adding driver support for bq24735 charger chipset.
> ...snip
> >
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + charger_device->pdata = client->dev.platform_data;
> >> +
> >> + if (!charger_device->pdata && client->dev.of_node)
> >
> > If you use IS_ENABLED(CONFIG_OF) here, the compiler will see that it
> > evaluates to 0 if OF is not selected, in which case it will be clever
> > enough to see that bq24735_parse_dt_data() is not used and just discard
> > it (because it is static). Then the #ifdefery above is not needed and
> > you will get compile coverage whether or not OF has been selected. Which
> > is a good thing.
> >
> > That said, I've mentioned before that you may want to not support the
> > non-DT at all since there's no immediate need, so this may not even be
> > an issue.
>
> The main reason I don't want to break non-DT support (or just not
> implement it) is that this driver is going to be used in our downstream
> kernels, and I prefer to minimize the patches they will have on top of
> it so we don't diverge.
I was under the impression that our downstream kernels used DT for a lot
of devices already. This doesn't look like a very special binding, and I
don't see a reason why we'd have to use platform data in our downstream
kernels.
> >> + name = charger_device->pdata->name;
> >> + if (!name) {
> >> + name = kasprintf(GFP_KERNEL, "bq24735-%s",
> >> + dev_name(&client->dev));
> >
> > Won't the device name already include bq24735 because of the driver
> > name?
>
> In my experience this comes up with a name like "bq24735-5-0009". Thats
> why I added the bq24735 in the beginning, so the name is more descriptive.
Yes, you're right. Perhaps in that case it's even easier to just stick
with a static string such as "bq24735" or "bq24735-charger". It's likely
to be the only device of that type in a machine. If you want to include
the device name, perhaps something like "%s/bq24735" (5-0009/bq24735) is
clearer that 5-0009 is actually the bus-specific name.
> > Also I don't see where charging is disabled. Or enabled when AC power is
> > plugged after the device has been probed. How does that work?
>
> I believe charging is auto-disabled when the adapter is unplugged, but I
> will verify and if that doesn't seem to be the case. This is something
> that should likely be added to the ISR (enable/disable).
I can very well imagine that it's auto-disabled when the power supply is
unplugged, but probably more importantly charging should be reenabled
when the supply is plugged again.
> >> +#define BQ24735_CHG_OPT_REG 0x12
> >> +#define BQ24735_CHG_OPT_CHARGE_ENABLE (1 << 0)
> >> +#define BQ24735_ENABLE_CHARGING 0
> >> +#define BQ24735_DISABLE_CHARGING 1
> >
> > I don't think these are really useful. The field is already named
> > CHARGE_ENABLE, so it should be pretty clear what you're supposed to put
> > in here. For that matter, I'm not a huge fan of the whole "update bits"
> > API because it encourages these things and they are just confusing.
>
> The only thing about the enable bit is that isn't kind of inverted what
> what you might expect. 1 is disabling. Thats why I think the bit
> definitions for enable/disable make sense. What would you suggest to
> replace the "update bits" API?
Well, especially for single bits I find it much more intuitive to do
something like this:
value = read();
value |= ENABLE;
write(value);
or
value = read();
value &= ~ENABLE;
write(value);
And if the meaning of the bit is inverted, then you can just rename
ENABLE to DISABLE. "update bits" might be fine for fields wider than a
single bit, but even in those cases, I find something like the above
much easier to read. But perhaps that's just personal preference.
Thierry
Attachment:
pgpU9rAioH4Op.pgp
Description: PGP signature
