| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Jan 2012 19:33:59 +0100
From: Pali Rohár <pali.rohar@...il.com>
To: Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc: linux-main <linux-kernel@...r.kernel.org>,
linux-omap <linux-omap@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Aliaksei Katovich <aliaksei.katovich@...ia.com>,
Vladimir Zapolskiy <vz@...ia.com>,
Felipe Contreras <felipe.contreras@...il.com>,
Anton Vorontsov <cbouatmailru@...il.com>,
Joerg Reisenweber <joerg@...nmoko.org>,
Sebastian Reichel <sre@...ian.org>,
???????????? ???????????????? <freemangordon@....bg>
Subject: Re: RFC 2: bq2415x_charger driver
On Friday 27 January 2012 16:24:55 Mark Brown wrote:
> On Fri, Jan 27, 2012 at 03:40:43AM +0100, Pali Roh?r wrote:
> > +static char *bq2415x_rev_name[] = {
> > + "1.0",
> > + "1.1",
> > + "1.2",
> > + "1.3",
> > + "1.4",
> > + "1.5",
> > + "1.6",
> > + "1.7",
>
> Looks like you can just store this as a number?
>
array removed, value stored as number
> > +struct bq2415x_device {
> > + struct device *dev;
> > + struct bq2415x_platform_data *platform_data;
>
> You should take a copy of the platform data so it can be marked
> initdata.
now driver copy platform_data and store it in bq2415x_device->init_data
>
> > +static DEFINE_MUTEX(bq2415x_id_mutex);
> > +static DEFINE_MUTEX(bq2415x_timer_mutex);
> > +static DEFINE_MUTEX(bq2415x_type_mutex);
> > +static DEFINE_MUTEX(bq2415x_i2c_mutex);
>
> Why are these global?
type_mutex was removed
id_mutex is needed for incrementing chip id (driver support more bq devices)
timer_mutex is used for periodicaly (every 10s) reseting chip timer
i2c_mutex is for locking i2c read/write functions in bq driver
>
> > +/* i2c read functions */
> > +
> > +static int bq2415x_i2c_read(struct bq2415x_device *bq, u8 reg)
> > +{
> > + struct i2c_client *client = to_i2c_client(bq->dev);
>
> You could save a bunch of code by moving all this I2C register I/O over
> to regmap.
regmap is not available in 2.6.28, so I cannot use it.
>
> > + case BQ2415X_BOOST_MODE_ENABLE:
> > + return bq2415x_i2c_write_bit(bq, BQ2415X_REG_CONTROL, 1,
> > BQ2415X_BIT_OPA_MODE);
> Keep things under 80 columns - see CodingStyle.
Fixed in whole file.
>
> > + if (client->addr == 0x6b) {
> >
> > + } else if (client->addr == 0x6a) {
>
> This looks like a switch statement.
Fixed.
>
> > + switch (chip) {
> > + case BQUNKNOWN:
> > + return bq2415x_rev_name[8];
>
> Magic numbers ahoy...
>
> > +static int bq2415x_vender_code(struct bq2415x_device *bq)
> > +{
> > + int ret = bq2415x_exec_command(bq, BQ2415X_VENDER_CODE);
> > + if (ret < 0)
> > + return 0;
> > + else /* convert to binary */
> > + return (ret & 0x1) + ((ret >> 1) & 0x1) * 10 + ((ret >> 2) & 0x1) *
> > 100;
>
> Just print it as hex?
In datasheet is in binary and number has only 3 bits.
>
> > +static int bq2415x_set_weak_battery_voltage(struct bq2415x_device *bq,
> > int mV) +{
> > + int val = mV/100 + (mV%100 > 0 ? 1 : 0) - 34;
>
> This could probably be written more clearly?
How?
>
> > +static int bq2415x_get_charge_current_sense_voltage(struct bq2415x_device
> > *bq) +{
> > + /* TODO */
> > + return -ENOSYS;
> > +}
>
> Just don't provide things that aren't there, the frameworks should do
> appropriate handling.
This is RFC driver and not all options was implemened yet. But new version has
this now implemented.
>
> > +static enum power_supply_property bq2415x_power_supply_props[] = {
> > + /* TODO */
> > + POWER_SUPPLY_PROP_STATUS,
> > + POWER_SUPPLY_PROP_MODEL_NAME,
>
> TODO?
No idea if power_supply needs more properties (I added info to TODO comment).
>
> > +static int bq2415x_power_supply_set_mode(struct bq2415x_device *bq, enum
> > bq2415x_mode mode) +{
> > + int ret = 0;
> > +
> > + switch (mode) {
> > +
> > + case BQ2415X_MODE_NONE: /* N/A */
>
> CodingStyle for the indentation.
Fixed.
>
> > + if (mode == BQ2415X_MODE_NONE) {
> > + dev_info(bq->dev, "mode: N/A\n");
> > + ret = bq2415x_set_current_limit(bq, 100);
> > + } else if (mode == BQ2415X_MODE_HOST_CHARGER) {
> > + dev_info(bq->dev, "mode: Host/HUB charger\n");
> > + ret = bq2415x_set_current_limit(bq, 500);
> > + } else {
> > + dev_info(bq->dev, "mode: Dedicated charger\n");
> > + ret = bq2415x_set_current_limit(bq, 1800);
> > + }
>
> This should be a switch statement.
Changed to switch.
>
> > + case BQ2415X_MODE_BOOST: /* Boost mode */
> > + dev_info(bq->dev, "mode: Boost\n");
>
> Is dev_info() really appropriate for this stuff?
>
How can driver write info that user (or other driver) changed chip mode?
This should be reported in dmesg!
> > + break;
> > +
> > + }
>
> No default case?
Added -EINVAL.
>
> > +static void bq2415x_power_supply_set_charger_type(int type, void *data)
> >
> > + if (type == 0)
> > + bq->charger_mode = BQ2415X_MODE_NONE;
> > + else if (type == 1)
> > + bq->charger_mode = BQ2415X_MODE_HOST_CHARGER;
> > + else if (type == 2)
> > + bq->charger_mode = BQ2415X_MODE_DEDICATED_CHARGER;
> > + else
> > + return;
>
> Switch statement. I never understood why this is such a common idiom...
>
> > + switch (error) {
> > + case 0: /* No error */
> > + break;
> > + case 6: /* Timer expired */
> > + dev_info(bq->dev, "Timer expired\n");
> > + break;
>
> Should we really be logging this at anything more than dev_dbg()?
Changed to dev_err(). This is error message and should be visible.
>
> > + case 7: /* N/A */
> > + bq2415x_power_supply_error(bq, "Unknow error");
> > + return;
>
> Typo: unknown.
Fixed
>
> > + case 5: /* Termal shutdown (too hot) */
> > + bq2415x_power_supply_error(bq, "Termal shutdown (too
hot)");
>
> Typo: thermal.
Fixed
>
> > +static int bq2415x_power_supply_get_property(struct power_supply *psy,
> > enum power_supply_property psp, union power_supply_propval *val)
> CodingStyle - wrapping (lots of this in the driver.
Wrapping and switches are fixed. Now I'm sending fixed version. This version
has now implemented configuration for charge_current and termination_current.
PS: Ignore code at end of file - it is platform code for nokia n900. It will
be moved to rx51 board section.
--
Pali Rohár
pali.rohar@...il.com
View attachment "bq2415x_charger.patch" of type "text/x-patch" (42519 bytes)
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)
Powered by blists - more mailing lists