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

[Sean McNeil]

Paul Jimenez wrote:
> 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.

There is something to say about consistency with the other class
handlers, plus the easy addition of more property types. I doubt the
code is that much less efficient as a switch, so I'd prefer to leave
as-is for that reason. But of course I'd also prefer smaller and more
readable code. Please feel free to submit cleanup patches.

Cheers,
Sean