From: Jonathan Cameron <[email protected]>
Add support for changing the mode of the da9030 usb charge pump
Signed-off-by: Jonathan Cameron <[email protected]>
---
Minor changes from previous version to reflect changes in the
driver being patched. Addtional cc's as per Mark's suggestion.
This version simply adds the functionality to the da903x mfd driver
core. If anyone can suggest a way round simply maintaining a
global pointer to the device it would be good.
To be able to call this from a board config file the driver must be
built in rather than a module but enforcing that is down to the
board Kconfig entry.
diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
index 99f8dcf..70ec3b0 100644
--- a/drivers/mfd/da903x.c
+++ b/drivers/mfd/da903x.c
@@ -30,6 +30,7 @@
#define DA9030_SYS_CTRL_A 0x08
#define DA9030_SYS_CTRL_B 0x09
#define DA9030_FAULT_LOG 0x0a
+#define DA9030_USBPUMP_REG 0x19
#define DA9034_CHIP_ID 0x00
#define DA9034_EVENT_A 0x01
@@ -484,6 +485,32 @@ failed:
return ret;
}
+/* Single device assumption */
+struct device *da9030_mfd_dev;
+
+int da9030_set_usb_charge_pump_mode(int mode)
+{
+ uint8_t val = 0;
+
+ if (da9030_mfd_dev == NULL)
+ return -EINVAL;
+
+ switch (mode) {
+ case DA9030_USBPUMP_AUTO:
+ val = 0;
+ break;
+ case DA9030_USBPUMP_CHARGE_PUMP:
+ val = 0x40;
+ break;
+ case DA9030_USBPUMP_CURRENT_SOURCE:
+ val = 0x80;
+ break;
+ }
+
+ return da903x_write(da9030_mfd_dev, DA9030_USBPUMP_REG, val);
+}
+EXPORT_SYMBOL_GPL(da9030_set_usb_charge_pump_mode);
+
static int __devinit da903x_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -499,6 +526,7 @@ static int __devinit da903x_probe(struct i2c_client *client,
chip->client = client;
chip->dev = &client->dev;
chip->ops = &da903x_ops[id->driver_data];
+ da9030_mfd_dev = &client->dev;
mutex_init(&chip->lock);
INIT_WORK(&chip->irq_work, da903x_irq_work);
@@ -559,6 +587,7 @@ static struct i2c_driver da903x_driver = {
static int __init da903x_init(void)
{
+ da9030_mfd_dev = NULL;
return i2c_add_driver(&da903x_driver);
}
module_init(da903x_init);
diff --git a/include/linux/mfd/da903x.h b/include/linux/mfd/da903x.h
index 115dbe9..5e15305 100644
--- a/include/linux/mfd/da903x.h
+++ b/include/linux/mfd/da903x.h
@@ -227,6 +227,11 @@ extern int da903x_unregister_notifier(struct device *dev,
extern int da903x_query_status(struct device *dev, unsigned int status);
+#define DA9030_USBPUMP_AUTO 1
+#define DA9030_USBPUMP_CHARGE_PUMP 2
+#define DA9030_USBPUMP_CURRENT_SOURCE 3
+
+int da9030_set_usb_charge_pump_mode(int mode);
/* NOTE: the functions below are not intended for use outside
* of the DA903x sub-device drivers
On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>
> From: Jonathan Cameron <[email protected]>
>
> Add support for changing the mode of the da9030 usb charge pump
>
Well, if it is totally USB charger related, I'd suggest to move this into
the dedicated driver. This mfd/da903x.c serves as a common code
base for all sub-peripherals.
Eric Miao wrote:
> On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>> From: Jonathan Cameron <[email protected]>
>>
>> Add support for changing the mode of the da9030 usb charge pump
>>
>
> Well, if it is totally USB charger related, I'd suggest to move this into
> the dedicated driver. This mfd/da903x.c serves as a common code
> base for all sub-peripherals.
It's not exactly related to the charger, it's rather related to the USB voltage
supplied to USB devices attached to PXA OHCI.
Indeed the mfd/da903x.c serves as a common core for sub-peripherals, but IMHO
adding a subdevice driver because of single method doesn't worth the overhead.
I'm for the solution Jonathan proposes.
--
Sincerely yours,
Mike.
Jonathan,
Below are my two cents :)
Jonathan Cameron wrote:
> From: Jonathan Cameron <[email protected]>
>
> Add support for changing the mode of the da9030 usb charge pump
>
> Signed-off-by: Jonathan Cameron <[email protected]>
>
> ---
>
> Minor changes from previous version to reflect changes in the
> driver being patched. Addtional cc's as per Mark's suggestion.
>
> This version simply adds the functionality to the da903x mfd driver
> core. If anyone can suggest a way round simply maintaining a
> global pointer to the device it would be good.
I don't think the global pointer here is real problem. It should be *really*
weird design with two da903x.
> To be able to call this from a board config file the driver must be
> built in rather than a module but enforcing that is down to the
> board Kconfig entry.
Most probably the board with da9030 will anyway have it built-in...
> diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
> index 99f8dcf..70ec3b0 100644
> --- a/drivers/mfd/da903x.c
> +++ b/drivers/mfd/da903x.c
> @@ -30,6 +30,7 @@
> #define DA9030_SYS_CTRL_A 0x08
> #define DA9030_SYS_CTRL_B 0x09
> #define DA9030_FAULT_LOG 0x0a
> +#define DA9030_USBPUMP_REG 0x19
>
> #define DA9034_CHIP_ID 0x00
> #define DA9034_EVENT_A 0x01
> @@ -484,6 +485,32 @@ failed:
> return ret;
> }
>
> +/* Single device assumption */
> +struct device *da9030_mfd_dev;
> +
> +int da9030_set_usb_charge_pump_mode(int mode)
> +{
> + uint8_t val = 0;
> +
> + if (da9030_mfd_dev == NULL)
> + return -EINVAL;
> +
> + switch (mode) {
> + case DA9030_USBPUMP_AUTO:
> + val = 0;
> + break;
> + case DA9030_USBPUMP_CHARGE_PUMP:
> + val = 0x40;
> + break;
> + case DA9030_USBPUMP_CURRENT_SOURCE:
> + val = 0x80;
> + break;
> + }
> +
> + return da903x_write(da9030_mfd_dev, DA9030_USBPUMP_REG, val);
> +}
> +EXPORT_SYMBOL_GPL(da9030_set_usb_charge_pump_mode);
> +
Maybe we want to have 'da9030_set_usb_charge_pump_mode' as well?
> static int __devinit da903x_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -499,6 +526,7 @@ static int __devinit da903x_probe(struct i2c_client *client,
> chip->client = client;
> chip->dev = &client->dev;
> chip->ops = &da903x_ops[id->driver_data];
> + da9030_mfd_dev = &client->dev;
>
> mutex_init(&chip->lock);
> INIT_WORK(&chip->irq_work, da903x_irq_work);
> @@ -559,6 +587,7 @@ static struct i2c_driver da903x_driver = {
>
> static int __init da903x_init(void)
> {
> + da9030_mfd_dev = NULL;
> return i2c_add_driver(&da903x_driver);
> }
> module_init(da903x_init);
> diff --git a/include/linux/mfd/da903x.h b/include/linux/mfd/da903x.h
> index 115dbe9..5e15305 100644
> --- a/include/linux/mfd/da903x.h
> +++ b/include/linux/mfd/da903x.h
> @@ -227,6 +227,11 @@ extern int da903x_unregister_notifier(struct device *dev,
>
> extern int da903x_query_status(struct device *dev, unsigned int status);
>
> +#define DA9030_USBPUMP_AUTO 1
> +#define DA9030_USBPUMP_CHARGE_PUMP 2
> +#define DA9030_USBPUMP_CURRENT_SOURCE 3
> +
> +int da9030_set_usb_charge_pump_mode(int mode);
>
> /* NOTE: the functions below are not intended for use outside
> * of the DA903x sub-device drivers
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Sincerely yours,
Mike.
On Thu, Jan 15, 2009 at 2:34 PM, Mike Rapoport <[email protected]> wrote:
>
>
> Eric Miao wrote:
>> On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>>> From: Jonathan Cameron <[email protected]>
>>>
>>> Add support for changing the mode of the da9030 usb charge pump
>>>
>>
>> Well, if it is totally USB charger related, I'd suggest to move this into
>> the dedicated driver. This mfd/da903x.c serves as a common code
>> base for all sub-peripherals.
>
> It's not exactly related to the charger, it's rather related to the USB voltage
> supplied to USB devices attached to PXA OHCI.
> Indeed the mfd/da903x.c serves as a common core for sub-peripherals, but IMHO
> adding a subdevice driver because of single method doesn't worth the overhead.
> I'm for the solution Jonathan proposes.
>
Mmm... then who will be the invoker? I'm a bit upset about this
being exported while called out of control. If it works as the
name suggested, setting some mode, maybe we can have a
platform data field specifying this, and hide this totally within
the driver. I didn't look into this too much, just a concern here.
Eric Miao wrote:
> On Thu, Jan 15, 2009 at 2:34 PM, Mike Rapoport <[email protected]> wrote:
>>
>> Eric Miao wrote:
>>> On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>>>> From: Jonathan Cameron <[email protected]>
>>>>
>>>> Add support for changing the mode of the da9030 usb charge pump
>>>>
>>> Well, if it is totally USB charger related, I'd suggest to move this into
>>> the dedicated driver. This mfd/da903x.c serves as a common code
>>> base for all sub-peripherals.
>> It's not exactly related to the charger, it's rather related to the USB voltage
>> supplied to USB devices attached to PXA OHCI.
>> Indeed the mfd/da903x.c serves as a common core for sub-peripherals, but IMHO
>> adding a subdevice driver because of single method doesn't worth the overhead.
>> I'm for the solution Jonathan proposes.
>>
>
> Mmm... then who will be the invoker? I'm a bit upset about this
> being exported while called out of control. If it works as the
> name suggested, setting some mode, maybe we can have a
> platform data field specifying this, and hide this totally within
> the driver. I didn't look into this too much, just a concern here.
The USB charge pump should be enabled if DA9030 should supply USB VBUS for USB
devices connected to PXA OHCI port. In the most simple case, when the USB port
is configured to be host only the platform data can be used to setup the charge
pump once and forget about it. However, if the USB port is configured as OTG,
the charge pump should be toggled depending on OTG ID.
In EM-X270 I have a GPIO connected to OTG ID pin of the connector and according
to the GPIO state I toggle the da9030 USB charge pump.
--
Sincerely yours,
Mike.
On Thu, Jan 15, 2009 at 3:28 PM, Mike Rapoport <[email protected]> wrote:
>
>
> Eric Miao wrote:
>> On Thu, Jan 15, 2009 at 2:34 PM, Mike Rapoport <[email protected]> wrote:
>>>
>>> Eric Miao wrote:
>>>> On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>>>>> From: Jonathan Cameron <[email protected]>
>>>>>
>>>>> Add support for changing the mode of the da9030 usb charge pump
>>>>>
>>>> Well, if it is totally USB charger related, I'd suggest to move this into
>>>> the dedicated driver. This mfd/da903x.c serves as a common code
>>>> base for all sub-peripherals.
>>> It's not exactly related to the charger, it's rather related to the USB voltage
>>> supplied to USB devices attached to PXA OHCI.
>>> Indeed the mfd/da903x.c serves as a common core for sub-peripherals, but IMHO
>>> adding a subdevice driver because of single method doesn't worth the overhead.
>>> I'm for the solution Jonathan proposes.
>>>
>>
>> Mmm... then who will be the invoker? I'm a bit upset about this
>> being exported while called out of control. If it works as the
>> name suggested, setting some mode, maybe we can have a
>> platform data field specifying this, and hide this totally within
>> the driver. I didn't look into this too much, just a concern here.
>
> The USB charge pump should be enabled if DA9030 should supply USB VBUS for USB
> devices connected to PXA OHCI port. In the most simple case, when the USB port
> is configured to be host only the platform data can be used to setup the charge
> pump once and forget about it. However, if the USB port is configured as OTG,
> the charge pump should be toggled depending on OTG ID.
> In EM-X270 I have a GPIO connected to OTG ID pin of the connector and according
> to the GPIO state I toggle the da9030 USB charge pump.
>
OK, now I understand the problem. The optimal way would be to invent
an abstraction for this external charge pump (in terms of PXA27x OTG
controller) and make DA9030 a derived class of it. But the PXA27x OTG
solution is really a hybrid one, and the existing OTG code is in a mess
(without a clear abstraction), the sub-optimal way would let platform code
do this trick. Yet the platform code is currently suffering from a missed
API to retreive this dynamically scanned I2C device. Mmm...stumped.
I would expect the final solution to be clean enough, that requires some
dependent stuffs to be modified, and the final look of this patch may be a
bit different.
Mike Rapoport wrote:
> Jonathan,
> Below are my two cents :)
>
Thanks,
> Jonathan Cameron wrote:
>
>> From: Jonathan Cameron <[email protected]>
>>
>> Add support for changing the mode of the da9030 usb charge pump
>>
>> Signed-off-by: Jonathan Cameron <[email protected]>
>>
>> ---
>>
>> Minor changes from previous version to reflect changes in the
>> driver being patched. Addtional cc's as per Mark's suggestion.
>>
>> This version simply adds the functionality to the da903x mfd driver
>> core. If anyone can suggest a way round simply maintaining a
>> global pointer to the device it would be good.
>>
>
> I don't think the global pointer here is real problem. It should be *really*
> weird design with two da903x.
>
>
>> To be able to call this from a board config file the driver must be
>> built in rather than a module but enforcing that is down to the
>> board Kconfig entry.
>>
>
> Most probably the board with da9030 will anyway have it built-in...
>
>
>> diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
>> index 99f8dcf..70ec3b0 100644
>> --- a/drivers/mfd/da903x.c
>> +++ b/drivers/mfd/da903x.c
>> @@ -30,6 +30,7 @@
>> #define DA9030_SYS_CTRL_A 0x08
>> #define DA9030_SYS_CTRL_B 0x09
>> #define DA9030_FAULT_LOG 0x0a
>> +#define DA9030_USBPUMP_REG 0x19
>>
>> #define DA9034_CHIP_ID 0x00
>> #define DA9034_EVENT_A 0x01
>> @@ -484,6 +485,32 @@ failed:
>> return ret;
>> }
>>
>> +/* Single device assumption */
>> +struct device *da9030_mfd_dev;
>> +
>> +int da9030_set_usb_charge_pump_mode(int mode)
>> +{
>> + uint8_t val = 0;
>> +
>> + if (da9030_mfd_dev == NULL)
>> + return -EINVAL;
>> +
>> + switch (mode) {
>> + case DA9030_USBPUMP_AUTO:
>> + val = 0;
>> + break;
>> + case DA9030_USBPUMP_CHARGE_PUMP:
>> + val = 0x40;
>> + break;
>> + case DA9030_USBPUMP_CURRENT_SOURCE:
>> + val = 0x80;
>> + break;
>> + }
>> +
>> + return da903x_write(da9030_mfd_dev, DA9030_USBPUMP_REG, val);
>> +}
>> +EXPORT_SYMBOL_GPL(da9030_set_usb_charge_pump_mode);
>> +
>>
>
> Maybe we want to have 'da9030_set_usb_charge_pump_mode' as well?
>
(get?)
Possibly though not sure under what circumstances it would be needed
other than debugging?
Mike Rapoport wrote:
> Jonathan,
> Below are my two cents :)
Thanks.
> Jonathan Cameron wrote:
>> From: Jonathan Cameron <[email protected]>
>>
>> Add support for changing the mode of the da9030 usb charge pump
>>
>> Signed-off-by: Jonathan Cameron <[email protected]>
>>
>> ---
>>
>> Minor changes from previous version to reflect changes in the
>> driver being patched. Addtional cc's as per Mark's suggestion.
>>
>> This version simply adds the functionality to the da903x mfd driver
>> core. If anyone can suggest a way round simply maintaining a
>> global pointer to the device it would be good.
>
> I don't think the global pointer here is real problem. It should be *really*
> weird design with two da903x.
Would be 'unusual'!
>> To be able to call this from a board config file the driver must be
>> built in rather than a module but enforcing that is down to the
>> board Kconfig entry.
>
> Most probably the board with da9030 will anyway have it built-in...
Agreed.
>> +EXPORT_SYMBOL_GPL(da9030_set_usb_charge_pump_mode);
>> +
>
> Maybe we want to have 'da9030_set_usb_charge_pump_mode' as well?
(get?)
Possibly, though only purpose I can think of for this would be debugging
and from the datasheet I can't see any case under which the set can fail?
Eric Miao wrote:
> On Thu, Jan 15, 2009 at 3:28 PM, Mike Rapoport <[email protected]> wrote:
>
>> Eric Miao wrote:
>>
>>> On Thu, Jan 15, 2009 at 2:34 PM, Mike Rapoport <[email protected]> wrote:
>>>
>>>> Eric Miao wrote:
>>>>
>>>>> On Thu, Jan 15, 2009 at 2:16 AM, Jonathan Cameron <[email protected]> wrote:
>>>>>
>>>>>> From: Jonathan Cameron <[email protected]>
>>>>>>
>>>>>> Add support for changing the mode of the da9030 usb charge pump
>>>>>>
>>>>>>
>>>>> Well, if it is totally USB charger related, I'd suggest to move this into
>>>>> the dedicated driver. This mfd/da903x.c serves as a common code
>>>>> base for all sub-peripherals.
>>>>>
>>>> It's not exactly related to the charger, it's rather related to the USB voltage
>>>> supplied to USB devices attached to PXA OHCI.
>>>> Indeed the mfd/da903x.c serves as a common core for sub-peripherals, but IMHO
>>>> adding a subdevice driver because of single method doesn't worth the overhead.
>>>> I'm for the solution Jonathan proposes.
>>>>
>>>>
>>> Mmm... then who will be the invoker? I'm a bit upset about this
>>> being exported while called out of control. If it works as the
>>> name suggested, setting some mode, maybe we can have a
>>> platform data field specifying this, and hide this totally within
>>> the driver. I didn't look into this too much, just a concern here.
>>>
>> The USB charge pump should be enabled if DA9030 should supply USB VBUS for USB
>> devices connected to PXA OHCI port. In the most simple case, when the USB port
>> is configured to be host only the platform data can be used to setup the charge
>> pump once and forget about it. However, if the USB port is configured as OTG,
>> the charge pump should be toggled depending on OTG ID.
>> In EM-X270 I have a GPIO connected to OTG ID pin of the connector and according
>> to the GPIO state I toggle the da9030 USB charge pump.
>>
>>
>
> OK, now I understand the problem. The optimal way would be to invent
> an abstraction for this external charge pump (in terms of PXA27x OTG
> controller) and make DA9030 a derived class of it. But the PXA27x OTG
> solution is really a hybrid one, and the existing OTG code is in a mess
> (without a clear abstraction), the sub-optimal way would let platform code
> do this trick. Yet the platform code is currently suffering from a missed
> API to retreive this dynamically scanned I2C device. Mmm...stumped.
>
Agreed, that lack of a means to obtain i2c devices in board configs is
beginning to get in the way
of quite a few things. Perhaps we need to start a proper discussion on
how to sort that out.
(some sort of registered call backs perhaps to allow the right dev
pointers to be set on the i2c
device finally being allocated?) I'm guessing the i2c guys are going to
have strong views
on any changes to their allocation approach!
> I would expect the final solution to be clean enough, that requires some
> dependent stuffs to be modified, and the final look of this patch may be a
> bit different
>
Agreed. This definitely isn't the way to go in the long run (assuming
otg etc get cleaned up)
but would it be an acceptable stop gap? I'm just keen to get the
functionality available
so as to get a board config (intel stargate 2) sorted. (some of which
you were kind enough to
review a while back - thanks).
Jonathan
Jonathan Cameron wrote:
> Mike Rapoport wrote:
>> Jonathan,
>> Below are my two cents :)
>>
> Thanks,
>>>
>> Maybe we want to have 'da9030_set_usb_charge_pump_mode' as well?
>>
> (get?)
get of course :)
> Possibly though not sure under what circumstances it would be needed
> other than debugging?
>
Somehow I thought I used it in the battery charger, but it seems I was wrong.
--
Sincerely yours,
Mike.
")
Fcc: +sent-mail
On Thu, Jan 15, 2009 at 11:22:34AM +0000, Jonathan Cameron wrote:
> > I would expect the final solution to be clean enough, that requires some
> > dependent stuffs to be modified, and the final look of this patch may be a
> > bit different
> Agreed. This definitely isn't the way to go in the long run (assuming
> otg etc get cleaned up)
> but would it be an acceptable stop gap? I'm just keen to get the
> functionality available
> so as to get a board config (intel stargate 2) sorted. (some of which
> you were kind enough to
> review a while back - thanks).
Having looked at this problem just this week for some other designs I'm
thinking the regulator API might be a good fit for this. It is a supply
and the API provides a method to match up the supply with the USB
controller (some designs have multiple options there so that's useful).
I've not actually tried it yet to see what the pain is like, though.
Mark Brown wrote:
> ")
> Fcc: +sent-mail
>
> On Thu, Jan 15, 2009 at 11:22:34AM +0000, Jonathan Cameron wrote:
>
>>> I would expect the final solution to be clean enough, that requires some
>>> dependent stuffs to be modified, and the final look of this patch may be a
>>> bit different
>
>> Agreed. This definitely isn't the way to go in the long run (assuming
>> otg etc get cleaned up)
>> but would it be an acceptable stop gap? I'm just keen to get the
>> functionality available
>> so as to get a board config (intel stargate 2) sorted. (some of which
>> you were kind enough to
>> review a while back - thanks).
>
> Having looked at this problem just this week for some other designs I'm
> thinking the regulator API might be a good fit for this. It is a supply
> and the API provides a method to match up the supply with the USB
> controller (some designs have multiple options there so that's useful).
> I've not actually tried it yet to see what the pain is like, though.
I did at one point. The problem here is you aren't simply controlling the
current / voltage. You are switching it between various automatic modes
and a manual override. Only in the manually overridden states is it
much like a regulator and then it's one with very odd properties.
If these are consistent enough across different pmic's I guess it could
be blugeoned in, but I'm not convinced this is true.
Jonathan
On Thu, Jan 15, 2009 at 01:31:43PM +0000, Jonathan Cameron wrote:
> Mark Brown wrote:
> > Having looked at this problem just this week for some other designs I'm
> > thinking the regulator API might be a good fit for this. It is a supply
> > and the API provides a method to match up the supply with the USB
> > controller (some designs have multiple options there so that's useful).
> > I've not actually tried it yet to see what the pain is like, though.
> I did at one point. The problem here is you aren't simply controlling the
> current / voltage. You are switching it between various automatic modes
> and a manual override. Only in the manually overridden states is it
> much like a regulator and then it's one with very odd properties.
Yes, they're rather different and would require additional APIs for
controlling some of the features, though it may be that we can get away
with doing a lot of the configuration in platform data or board init
code with a more limited subset configured dynamically by the USB stack.
> If these are consistent enough across different pmic's I guess it could
> be blugeoned in, but I'm not convinced this is true.
Yes, that needs to be investigated. I'd hope there's *some* similarity
since at the end of the day it's all USB but I've not got to the point
of looking at that yet.