[PATCH] smb347_charger: Cleanup power supply registration code in probe

[PATCH] smb347_charger: Cleanup power supply registration code in probe

[Ramakrishna Pallala]

This patch checks if the usb or mains charging is enabled by the
platform before registering with the power supply class.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/power/smb347-charger.c |   64 ++++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
index ce1694d..1b5d2e2 100644
--- a/drivers/power/smb347-charger.c
+++ b/drivers/power/smb347-charger.c
@@ -1185,21 +1185,34 @@ static int smb347_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	smb->mains.name = "smb347-mains";
-	smb->mains.type = POWER_SUPPLY_TYPE_MAINS;
-	smb->mains.get_property = smb347_mains_get_property;
-	smb->mains.properties = smb347_mains_properties;
-	smb->mains.num_properties = ARRAY_SIZE(smb347_mains_properties);
-	smb->mains.supplied_to = battery;
-	smb->mains.num_supplicants = ARRAY_SIZE(battery);
-
-	smb->usb.name = "smb347-usb";
-	smb->usb.type = POWER_SUPPLY_TYPE_USB;
-	smb->usb.get_property = smb347_usb_get_property;
-	smb->usb.properties = smb347_usb_properties;
-	smb->usb.num_properties = ARRAY_SIZE(smb347_usb_properties);
-	smb->usb.supplied_to = battery;
-	smb->usb.num_supplicants = ARRAY_SIZE(battery);
+	if (smb->pdata->use_mains) {
+		smb->mains.name = "smb347-mains";
+		smb->mains.type = POWER_SUPPLY_TYPE_MAINS;
+		smb->mains.get_property = smb347_mains_get_property;
+		smb->mains.properties = smb347_mains_properties;
+		smb->mains.num_properties = ARRAY_SIZE(smb347_mains_properties);
+		smb->mains.supplied_to = battery;
+		smb->mains.num_supplicants = ARRAY_SIZE(battery);
+		ret = power_supply_register(dev, &smb->mains);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (smb->pdata->use_usb) {
+		smb->usb.name = "smb347-usb";
+		smb->usb.type = POWER_SUPPLY_TYPE_USB;
+		smb->usb.get_property = smb347_usb_get_property;
+		smb->usb.properties = smb347_usb_properties;
+		smb->usb.num_properties = ARRAY_SIZE(smb347_usb_properties);
+		smb->usb.supplied_to = battery;
+		smb->usb.num_supplicants = ARRAY_SIZE(battery);
+		ret = power_supply_register(dev, &smb->usb);
+		if (ret < 0) {
+			if (smb->pdata->use_mains)
+				power_supply_unregister(&smb->mains);
+			return ret;
+		}
+	}
 
 	smb->battery.name = "smb347-battery";
 	smb->battery.type = POWER_SUPPLY_TYPE_BATTERY;
@@ -1207,20 +1220,13 @@ static int smb347_probe(struct i2c_client *client,
 	smb->battery.properties = smb347_battery_properties;
 	smb->battery.num_properties = ARRAY_SIZE(smb347_battery_properties);
 
-	ret = power_supply_register(dev, &smb->mains);
-	if (ret < 0)
-		return ret;
-
-	ret = power_supply_register(dev, &smb->usb);
-	if (ret < 0) {
-		power_supply_unregister(&smb->mains);
-		return ret;
-	}
 
 	ret = power_supply_register(dev, &smb->battery);
 	if (ret < 0) {
-		power_supply_unregister(&smb->usb);
-		power_supply_unregister(&smb->mains);
+		if (smb->pdata->use_usb)
+			power_supply_unregister(&smb->usb);
+		if (smb->pdata->use_mains)
+			power_supply_unregister(&smb->mains);
 		return ret;
 	}
 
@@ -1255,8 +1261,10 @@ static int smb347_remove(struct i2c_client *client)
 	}
 
 	power_supply_unregister(&smb->battery);
-	power_supply_unregister(&smb->usb);
-	power_supply_unregister(&smb->mains);
+	if (smb->pdata->use_usb)
+		power_supply_unregister(&smb->usb);
+	if (smb->pdata->use_mains)
+		power_supply_unregister(&smb->mains);
 	return 0;
 }
 
-- 
1.7.0.4

Re: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Mika Westerberg]

On Fri, Apr 13, 2012 at 03:14:52PM +0530, Ramakrishna Pallala wrote:
> This patch checks if the usb or mains charging is enabled by the
> platform before registering with the power supply class.

Can you describe why this change is needed? The current code always registers
all the power-supplies but if we haven't enabled for example USB charging,
then the online property for USB ps is set to 0.

RE: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Pallala, Ramakrishna]

> Sent: Thursday, April 19, 2012 12:41 PM
> To: Pallala, Ramakrishna
> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov; Anton Vorontsov
> Subject: Re: [PATCH] smb347_charger: Cleanup power supply registration code in probe
> 
> On Fri, Apr 13, 2012 at 03:14:52PM +0530, Ramakrishna Pallala wrote:
> > This patch checks if the usb or mains charging is enabled by the
> > platform before registering with the power supply class.
> 
> Can you describe why this change is needed? The current code always registers all the
> power-supplies but if we haven't enabled for example USB charging, then the online
> property for USB ps is set to 0.

If we haven't enabled USB charging then why to waste kernel resources?
What we will gain by registering with power supply class if USB charging is not enabled?

Thanks,
Ram

Re: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Mika Westerberg]

On Thu, Apr 19, 2012 at 2:10 PM, Pallala, Ramakrishna
<ramakrishna.pallala@intel.com> wrote:
>> Sent: Thursday, April 19, 2012 12:41 PM
>> To: Pallala, Ramakrishna
>> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov; Anton Vorontsov
>> Subject: Re: [PATCH] smb347_charger: Cleanup power supply registration code in probe
>>
>> On Fri, Apr 13, 2012 at 03:14:52PM +0530, Ramakrishna Pallala wrote:
>> > This patch checks if the usb or mains charging is enabled by the
>> > platform before registering with the power supply class.
>>
>> Can you describe why this change is needed? The current code always registers all the
>> power-supplies but if we haven't enabled for example USB charging, then the online
>> property for USB ps is set to 0.
>
> If we haven't enabled USB charging then why to waste kernel resources?
> What we will gain by registering with power supply class if USB charging is not enabled?

For one we have a consistent interface to userspace regardless of how
the platform has configured the driver. For example if USB charging is
not enabled userspace can still see the 'smb347-usb' directory but
'online' attribute is always set to zero.

RE: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Pallala, Ramakrishna]

> >>
> >> On Fri, Apr 13, 2012 at 03:14:52PM +0530, Ramakrishna Pallala wrote:
> >> > This patch checks if the usb or mains charging is enabled by the
> >> > platform before registering with the power supply class.
> >>
> >> Can you describe why this change is needed? The current code always
> >> registers all the power-supplies but if we haven't enabled for
> >> example USB charging, then the online property for USB ps is set to 0.
> >
> > If we haven't enabled USB charging then why to waste kernel resources?
> > What we will gain by registering with power supply class if USB charging is not
> enabled?
> 
> For one we have a consistent interface to userspace regardless of how the platform has
> configured the driver. For example if USB charging is not enabled userspace can still
> see the 'smb347-usb' directory but 'online' attribute is always set to zero.

what is gain and why user space need to see for usb online property and get disappointed when
platform has disabled USB charging( where user/user space has no idea about this).

if usb charging is  not enabled just don't register. I think it's clean and make more sense to me.

Thanks,
Ram

Re: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Mika Westerberg]

On Thu, Apr 19, 2012 at 2:30 PM, Pallala, Ramakrishna
<ramakrishna.pallala@intel.com> wrote:
>
> what is gain and why user space need to see for usb online property and get disappointed when
> platform has disabled USB charging( where user/user space has no idea about this).

Userspace doesn't have to know anything about how the platform is
configured. It just reads standard and documented attributes and
reacts on those.

> if usb charging is  not enabled just don't register. I think it's clean and make more sense to me.

I disagree but I'll let Anton to decide which is better and how it is
supposed to work.

Btw, I think you should also check those places where
power_supply_changed() is called for the supplies and make those
conditional to what is registered.

RE: [PATCH] smb347_charger: Cleanup power supply registration code in probe

[Pallala, Ramakrishna]

> On Thu, Apr 19, 2012 at 2:30 PM, Pallala, Ramakrishna <ramakrishna.pallala@intel.com>
> wrote:
> >
> > what is gain and why user space need to see for usb online property
> > and get disappointed when platform has disabled USB charging( where user/user space
> has no idea about this).
> 
> Userspace doesn't have to know anything about how the platform is configured. It just
> reads standard and documented attributes and reacts on those.
> 
> > if usb charging is  not enabled just don't register. I think it's clean and make more
> sense to me.
> 
> I disagree but I'll let Anton to decide which is better and how it is supposed to work.
> 
> Btw, I think you should also check those places where
> power_supply_changed() is called for the supplies and make those conditional to what is
> registered.

I will resubmit the patch.

Thanks,
Ram