Werner Almesberger wrote:
> Andy Green wrote:
>
>> +static int usb_get_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + union power_supply_propval *val)
>> +{
>> + int ret = 0;
>> + struct bq27000_device_info *di = container_of(psy, struct
>> bq27000_device_info, usb);
>> +
>> + if (!(di->pdata->hdq_initialized)())
>> + return -EINVAL;
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + if (di->pdata->get_charger_online_status)
>> + val->intval = (di->pdata->get_charger_online_status)();
>> + else
>> + return -EINVAL;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> + return ret;
>> +}
>>
>
> This looks a bit chatty. Maybe a future style improvement could look like
> this ?
>
> {
> struct bq27000_device_info *di = container_of(psy, struct
> bq27000_device_info, usb);
>
> if (!di->pdata->hdq_initialized())
> return -EINVAL;
>
> switch (psp) {
> case POWER_SUPPLY_PROP_ONLINE:
> if (!di->pdata->get_charger_online_status)
> return -EINVAL;
> val->intval = di->pdata->get_charger_online_status();
> break;
> default:
> return -EINVAL;
> }
> return 0;
> }
>
> - Werner
>
>
I don't grok the use of switch() for a two case... case. Try:
{
struct bq27000_device_info *di = container_of(psy, struct
bq27000_device_info, usb);
if (!di->pdata->hdq_initialized())
return -EINVAL;
if (psp != POWER_SUPPLY_PROP_ONLINE ||
!di->pdata->get_charger_online_status)
return -EINVAL;
val->intval = di->pdata->get_charger_online_status();
return 0;
}
I don't know whether current style says to split up the || or join it with the
test above, but those are other possibilities.
