[PATCH 2/7] add-powersupply-ac-usb-automonitor.patch

[Paul Jimenez]

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.