RE: [PATCH 1/2] dt: power: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Krzysztof,

> 2015-10-30 1:37 GMT+09:00 Ramakrishna Pallala
> :
> > This patch adds the device tree documentation for TI BQ24261 charger.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Signed-off-by: Jenny TC 
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   34
> 
> >  1 file changed, 34 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..e18f6dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,34 @@
> > +Binding for TI bq24261 Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> 
> Just: 'should be "ti,bq24261"'.
Ok.

> > +- reg: integer, i2c address of the device.
> > +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> > +- ti,charge-current: integer, maximum charging current in uA.
> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in uA).
> 
> Same as proposed extension of bq24257.txt. I like it.
> 
> > +
> > +Optional properties:
> > +- ti,max-charge-current: integer, maximum charging current (in uA);
> 
> I did not look at the code yet but description of property is exactly the 
> same as
> "ti,charge-current"... Two different properties with same description?
> 
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in uV);
> 
> Ditto.
I will fix it.

> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> > +- ti,thermal-sensing: integer, enable(1) or disable(0) JEITA thermal 
> > control.
> 
> You made it integer instead of boolean, so just to be sure - you expect the
> possibility of overriding this property or extending it in further devices?
Yes. Thanks.

Thanks,
Ram

RE: [PATCH 1/2] dt: power: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Krzysztof,

> 2015-10-30 1:37 GMT+09:00 Ramakrishna Pallala
> :
> > This patch adds the device tree documentation for TI BQ24261 charger.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Signed-off-by: Jenny TC 
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   34
> 
> >  1 file changed, 34 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..e18f6dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,34 @@
> > +Binding for TI bq24261 Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> 
> Just: 'should be "ti,bq24261"'.
Ok.

> > +- reg: integer, i2c address of the device.
> > +- ti,battery-regulation-voltage: integer, maximum charging voltage in uV.
> > +- ti,charge-current: integer, maximum charging current in uA.
> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in uA).
> 
> Same as proposed extension of bq24257.txt. I like it.
> 
> > +
> > +Optional properties:
> > +- ti,max-charge-current: integer, maximum charging current (in uA);
> 
> I did not look at the code yet but description of property is exactly the 
> same as
> "ti,charge-current"... Two different properties with same description?
> 
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in uV);
> 
> Ditto.
I will fix it.

> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> > +- ti,thermal-sensing: integer, enable(1) or disable(0) JEITA thermal 
> > control.
> 
> You made it integer instead of boolean, so just to be sure - you expect the
> possibility of overriding this property or extending it in further devices?
Yes. Thanks.

Thanks,
Ram

RE: [PATCH 0/2] power: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Sebastian,

> On Thu, Oct 29, 2015 at 10:07:03PM +0530, Ramakrishna Pallala wrote:
> > This patch series adds the support for TI BQ24261 charger driver.
> >
> > TI BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> 
> I wonder if there is any new version of this patchset, that I missed with the
> comments from Krzysztof and Andreas being taken care of?

I was bit busy lately. I will address both Krzysztof and Andreas comments and 
submit the new patchset in the coming few days.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 0/2] power: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Sebastian,

> On Thu, Oct 29, 2015 at 10:07:03PM +0530, Ramakrishna Pallala wrote:
> > This patch series adds the support for TI BQ24261 charger driver.
> >
> > TI BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> 
> I wonder if there is any new version of this patchset, that I missed with the
> comments from Krzysztof and Andreas being taken care of?

I was bit busy lately. I will address both Krzysztof and Andreas comments and 
submit the new patchset in the coming few days.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas, 

> Hi Ram, thanks for submitting this, please see some feedback inlined...
> 
> On Sun, Sep 06, 2015 at 10:53:07PM +0530, Ramakrishna Pallala wrote:
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Signed-off-by: Jennt TC 
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   37 +
> >  drivers/power/Kconfig  |6 +
> >  drivers/power/Makefile |1 +
> >  drivers/power/bq24261_charger.c| 1208 
> > 
> >  include/linux/power/bq24261_charger.h  |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> 
> If you look at other chargers (bq2415x, bq24257, ...) this property should
> probably be called ti,battery-regulation-voltage.
Ok.


> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in mA);
> 
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> 
> What's the background of having these two properties? They don't impact any
> device HW settings, but rather look like some safeguard if somebody
> manipulates the sysfs to not exceed certain boundaries.
As we have mechanism to control the charge current and voltage from user space 
we need safety limits.


> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> All this does is passing these values down to the sysfs and exposing them
> through
> POWER_SUPPLY_PROP_TEMP_MAX/POWER_SUPPLY_PROP_TEMP_MIN. What
> value does this provide?
Same as above reason if we disable thermal sensing.


> > +
> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> I received feedback for one of my recent patches that it's better to use an
> integer property as it can be overridden and with that is more flexible.
> 
> > +- ti,enable-user-write: boolean, if present driver will allow the user 
> > space
> > +to control the charging current and voltage through sysfs;
> 
> This idea came from me :) but we should probably get rid of this capability. 
> And
> the writability of charge current/voltage properties through sysfs 
> altogether. My
> use case for this was to enable better testing but there is other ways this 
> can be
> accomplished. However since some other drivers expose such capability I
> wonder what other reasons/use cases there might be (besides helping
> development and testing)?

I have added module parameter to disable the sysfs write.

> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DEV_NAME "bq24261_charger"
> 
> Can we use a dash/hyphen rather than an underscore? This would align better
Ok.

> with bq2415x and bq24257 (=bq2425x).
> 
> > +
> > +#define EXCEPTION_MONITOR_DELAY(60 * HZ)
> > +#define WDT_RESET_DELAY(15 * HZ)
> > +
> > +/* BQ24261 registers */
> > +#define BQ24261_STAT_CTRL0_ADDR0x00
> > +#define BQ24261_CTRL_ADDR  0x01
> > +#define BQ24261_BATT_VOL_CTRL_ADDR 0x02
> > +#define BQ24261_VENDOR_REV_ADDR0x03
> > +#define BQ24261_TERM_FCC_ADDR  0x04
> > +#define BQ24261_VINDPM_STAT_ADDR   0x05
> > +#define BQ24261_ST_NTC_MON_ADDR0x06
> > +
> > +#define BQ24261_RESET_MASK (0x01 << 7)
> 
> What about using the BIT() macro for all of these?
Ok.

> 
> > +#define BQ24261_RESET_ENABLE   (0x01 << 7)
> > +
> > +#define BQ24261_FAULT_MASK 0x07
> > +#define BQ24261_STAT_MASK  (0x03 << 4)
> 
> What about the 

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas, 

> Hi Ram, thanks for submitting this, please see some feedback inlined...
> 
> On Sun, Sep 06, 2015 at 10:53:07PM +0530, Ramakrishna Pallala wrote:
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Signed-off-by: Jennt TC 
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   37 +
> >  drivers/power/Kconfig  |6 +
> >  drivers/power/Makefile |1 +
> >  drivers/power/bq24261_charger.c| 1208 
> > 
> >  include/linux/power/bq24261_charger.h  |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> 
> If you look at other chargers (bq2415x, bq24257, ...) this property should
> probably be called ti,battery-regulation-voltage.
Ok.


> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in mA);
> 
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> 
> What's the background of having these two properties? They don't impact any
> device HW settings, but rather look like some safeguard if somebody
> manipulates the sysfs to not exceed certain boundaries.
As we have mechanism to control the charge current and voltage from user space 
we need safety limits.


> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> All this does is passing these values down to the sysfs and exposing them
> through
> POWER_SUPPLY_PROP_TEMP_MAX/POWER_SUPPLY_PROP_TEMP_MIN. What
> value does this provide?
Same as above reason if we disable thermal sensing.


> > +
> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> I received feedback for one of my recent patches that it's better to use an
> integer property as it can be overridden and with that is more flexible.
> 
> > +- ti,enable-user-write: boolean, if present driver will allow the user 
> > space
> > +to control the charging current and voltage through sysfs;
> 
> This idea came from me :) but we should probably get rid of this capability. 
> And
> the writability of charge current/voltage properties through sysfs 
> altogether. My
> use case for this was to enable better testing but there is other ways this 
> can be
> accomplished. However since some other drivers expose such capability I
> wonder what other reasons/use cases there might be (besides helping
> development and testing)?

I have added module parameter to disable the sysfs write.

> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DEV_NAME "bq24261_charger"
> 
> Can we use a dash/hyphen rather than an underscore? This would align better
Ok.

> with bq2415x and bq24257 (=bq2425x).
> 
> > +
> > +#define EXCEPTION_MONITOR_DELAY(60 * HZ)
> > +#define WDT_RESET_DELAY(15 * HZ)
> > +
> > +/* BQ24261 registers */
> > +#define BQ24261_STAT_CTRL0_ADDR0x00
> > +#define BQ24261_CTRL_ADDR  0x01
> > +#define BQ24261_BATT_VOL_CTRL_ADDR 0x02
> > +#define BQ24261_VENDOR_REV_ADDR0x03
> > +#define BQ24261_TERM_FCC_ADDR  0x04
> > +#define BQ24261_VINDPM_STAT_ADDR   0x05
> > +#define BQ24261_ST_NTC_MON_ADDR0x06
> > +
> > +#define BQ24261_RESET_MASK (0x01 << 7)
> 
> What about using the BIT() macro for all of these?
Ok.

> 
> > +#define BQ24261_RESET_ENABLE   (0x01 << 7)
> > +
> > +#define BQ24261_FAULT_MASK 0x07
> > +#define 

RE: [PATCH 1/2] extcon: Modify the id and name of external connector

[Pallala, Ramakrishna]

> >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h index
> >> c0f8c4fc5d45..c8dd881e2b8d 100644
> >> --- a/include/linux/extcon.h
> >> +++ b/include/linux/extcon.h
> >> @@ -31,32 +31,42 @@
> >>  /*
> >>   * Define the unique id of supported external connectors
> >>   */
> >> -#define EXTCON_NONE   0
> >> -
> >> -#define EXTCON_USB1   /* USB connector */
> >> -#define EXTCON_USB_HOST   2
> >> -
> >> -#define EXTCON_TA 3   /* Charger connector */
> >> -#define EXTCON_FAST_CHARGER   4
> >> -#define EXTCON_SLOW_CHARGER   5
> >> -#define EXTCON_CHARGE_DOWNSTREAM  6
> >> -
> >> -#define EXTCON_LINE_IN7   /* Audio/Video
> >> connector */
> >> -#define EXTCON_LINE_OUT   8
> >> -#define EXTCON_MICROPHONE 9
> >> -#define EXTCON_HEADPHONE  10
> >> -#define EXTCON_HDMI   11
> >> -#define EXTCON_MHL12
> >> -#define EXTCON_DVI13
> >> -#define EXTCON_VGA14
> >> -#define EXTCON_SPDIF_IN   15
> >> -#define EXTCON_SPDIF_OUT  16
> >> -#define EXTCON_VIDEO_IN   17
> >> -#define EXTCON_VIDEO_OUT  18
> >> -
> >> -#define EXTCON_DOCK   19  /* Misc connector */
> >> -#define EXTCON_JIG20
> >> -#define EXTCON_MECHANICAL 21
> >> +#define EXTCON_NONE   0
> >> +
> >> +/* USB external connector */
> >> +#define EXTCON_USB1   /* Universal Serial Bus */
> >> +#define EXTCON_USB_HOST   2
> >> +
> >> +/* Charging external connector */
> >> +#define EXTCON_CHG_USB5   /* Standard Downstream Port
> >> */
> >> +#define EXTCON_CHG_USB_CDP6   /* Charging Downstream Port
> >> */
> >> +#define EXTCON_CHG_USB_DCP7   /* Dedicated Charging Port */
> >> +#define EXTCON_CHG_USB_DCP_FAST   8
> >> +#define EXTCON_CHG_USB_DCP_SLOW   9
> >> +#define EXTCON_CHG_USB_ACA10  /* Accessory Charger Adapter
> >> */
> >> +
> >> +/* Jack external connector */
> >> +#define EXTCON_JACK_MICROPHONE20
> >> +#define EXTCON_JACK_HEADPHONE 21
> >> +#define EXTCON_JACK_LINE_IN   22
> >> +#define EXTCON_JACK_LINE_OUT  23
> >> +#define EXTCON_JACK_VIDEO_IN  24
> >> +#define EXTCON_JACK_VIDEO_OUT 25
> >> +#define EXTCON_JACK_SPDIF_IN  26  /* Sony Philips Digital
> >> InterFace */
> >> +#define EXTCON_JACK_SPDIF_OUT 27
> >> +
> >> +/* Display external connector */
> >> +#define EXTCON_DISP_HDMI  40  /* High-Definition Multimedia
> >> Interface */
> >> +#define EXTCON_DISP_MHL   41  /* Mobile High-Definition Link
> >> */
> >> +#define EXTCON_DISP_DVI   42  /* Digital Visual Inteface */
> >> +#define EXTCON_DISP_VGA   43  /* Video Graphics Array */
> >> +
> >> +/* Miscellaneous external connector */
> >> +#define EXTCON_DOCK   60
> >> +#define EXTCON_JIG61
> >> +#define EXTCON_MECHANICAL 62
> >> +
> >> +#define EXTCON_NUM63
> >
> > Can we change the #define macro's to enum's? is there problem with that?
> 
> It is possible. But, the unique id of external connector will be used on 
> device tree
> file as following patch[1]. Following patch[1] defines the include/dt-
> bindings/extcon/extcon.h
> which is include in *.dts file. In *.dts file, we can not use the 'enum'.
> [1] https://lkml.org/lkml/2015/10/5/38
> So, I use the "#define" keyword instead of enum.
> 
> I used the 'enum' on first patch to define the unique id but I altered it by 
> using
> '#define' instead of 'enum' on following patch[2].
> [2] commit 73b6ecdb93e8e ("extcon: Redefine the unique id of supported
> external connectors without 'enum extcon' type")

Ok. Thanks.

-Ram

RE: [PATCH 1/2] extcon: Modify the id and name of external connector

[Pallala, Ramakrishna]

Hi Choi,


> Subject: [PATCH 1/2] extcon: Modify the id and name of external connector
> 
> This patch modifies the id and name of external connector with the additional
> prefix to clarify both attribute and meaning of external connector as 
> following:
> - EXTCON_CHG_* mean the charger connector.
> - EXTCON_JACK_* mean the jack connector.
> - EXTCON_DISP_* mean the display port connector.
> 
> Following table show the new name of external connector with old name:
> --
> Old extcon name | New extcon name|
> --
> EXTCON_TA   | EXTCON_CHG_USB_DCP |
> EXTCON_FAST_CHARGER | EXTCON_CHG_USB_DCP_FAST|
> EXTCON_SLOW_CHARGER | EXTCON_CHG_USB_DCP_SLOW|
> EXTCON_CHARGE_DOWNSTREAM| EXTCON_CHG_USB_CDP |
> --
> EXTCON_MICROPHONE   | EXTCON_JACK_MICROPHONE |
> EXTCON_HEADPHONE| EXTCON_JACK_HEADPHONE  |
> EXTCON_LINE_IN  | EXTCON_JACK_LINE_IN|
> EXTCON_LINE_OUT | EXTCON_JACK_LINE_OUT   |
> EXTCON_VIDEO_IN | EXTCON_JACK_VIDEO_IN   |
> EXTCON_VIDEO_OUT| EXTCON_JACK_VIDEO_OUT  |
> EXTCON_SPDIF_IN | EXTCON_JACK_SPDIF_IN   |
> EXTCON_SPDIF_OUT| EXTCON_JACK_SPDIF_OUT  |
> --
> EXTCON_HMDI | EXTCON_DISP_HDMI   |
> EXTCON_MHL  | EXTCON_DISP_MHL|
> EXTCON_DVI  | EXTCON_DISP_DVI|
> EXTCON_VGA  | EXTCON_DISP_VGA|
> --
> 
> And, when altering the name of USB charger connector, EXTCON refers to the
> "Battery Charging v1.2 Spec and Adopters Agreement"[1] to use the standard
> name of USB charging port as following. Following name of USB charging port
> are already used in power_supply subsystem. We chan check it on patch[2].
> - EXTCON_CHG_USB  /* Standard Downstream Port */
> - EXTCON_CHG_USB_DCP  /* Dedicated Charging Port */
> - EXTCON_CHG_USB_CDP  /* Charging Downstream Port */
> - EXTCON_CHG_USB_ACA  /* Accessory Charger Adapter */
> 
> [1] www.usb.org/developers/docs/devclass_docs/BCv1.2_070312.zip
> [2] commit 85efc8a18ce ("[PATCH] power_supply: Add types for USB chargers")
> 
> Signed-off-by: Chanwoo Choi 
> [ckeepax: For the Arizona changes]
> Acked-by: Charles Keepax 
> ---
>  drivers/extcon/extcon-arizona.c  | 18 ++--
>  drivers/extcon/extcon-axp288.c   | 12 
>  drivers/extcon/extcon-max14577.c | 17 +--  drivers/extcon/extcon-
> max77693.c | 32 +++--  drivers/extcon/extcon-max77843.c | 27
> +  drivers/extcon/extcon-max8997.c  | 21 +++---
> drivers/extcon/extcon-rt8973a.c  |  4 +--
>  drivers/extcon/extcon-sm5502.c   |  4 +--
>  drivers/extcon/extcon.c  | 61 ---
>  include/linux/extcon.h   | 62 
> +++-
>  10 files changed, 139 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index a1ab0a56b798..e4890dd4fefd 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -137,9 +137,9 @@ static const int arizona_micd_levels[] = {
> 
>  static const unsigned int arizona_cable[] = {
>   EXTCON_MECHANICAL,
> - EXTCON_MICROPHONE,
> - EXTCON_HEADPHONE,
> - EXTCON_LINE_OUT,
> + EXTCON_JACK_MICROPHONE,
> + EXTCON_JACK_HEADPHONE,
> + EXTCON_JACK_LINE_OUT,
>   EXTCON_NONE,
>  };
> 
> @@ -600,7 +600,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>   struct arizona_extcon_info *info = data;
>   struct arizona *arizona = info->arizona;
>   int id_gpio = arizona->pdata.hpdet_id_gpio;
> - unsigned int report = EXTCON_HEADPHONE;
> + unsigned int report = EXTCON_JACK_HEADPHONE;
>   int ret, reading;
>   bool mic = false;
> 
> @@ -645,9 +645,9 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
> 
>   /* Report high impedence cables as line outputs */
>   if (reading >= 5000)
> - report = EXTCON_LINE_OUT;
> + report = EXTCON_JACK_LINE_OUT;
>   else
> - report = EXTCON_HEADPHONE;
> + report = EXTCON_JACK_HEADPHONE;
> 
>   ret = extcon_set_cable_state_(info->edev, report, true);
>   if (ret != 0)
> @@ -732,7 +732,7 @@ err:
>  ARIZONA_ACCDET_MODE_MASK,
> ARIZONA_ACCDET_MODE_MIC);
> 
>   /* Just report headphone */
> - ret = extcon_set_cable_state_(info->edev, EXTCON_HEADPHONE, true);
> + ret = extcon_set_cable_state_(info->edev, EXTCON_JACK_HEADPHONE,
> +true);
>   if (ret != 0)
>   dev_err(arizona->dev, "Failed to report headphone: %d\n", ret);
> 
> @@ -789,7 +789,7 @@ err:
>  ARIZONA_ACCDET_MODE_MASK,
> ARIZONA_ACCDET_MODE_MIC);
> 
>   /* Just 

RE: [PATCH 1/2] extcon: Modify the id and name of external connector

[Pallala, Ramakrishna]

> >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h index
> >> c0f8c4fc5d45..c8dd881e2b8d 100644
> >> --- a/include/linux/extcon.h
> >> +++ b/include/linux/extcon.h
> >> @@ -31,32 +31,42 @@
> >>  /*
> >>   * Define the unique id of supported external connectors
> >>   */
> >> -#define EXTCON_NONE   0
> >> -
> >> -#define EXTCON_USB1   /* USB connector */
> >> -#define EXTCON_USB_HOST   2
> >> -
> >> -#define EXTCON_TA 3   /* Charger connector */
> >> -#define EXTCON_FAST_CHARGER   4
> >> -#define EXTCON_SLOW_CHARGER   5
> >> -#define EXTCON_CHARGE_DOWNSTREAM  6
> >> -
> >> -#define EXTCON_LINE_IN7   /* Audio/Video
> >> connector */
> >> -#define EXTCON_LINE_OUT   8
> >> -#define EXTCON_MICROPHONE 9
> >> -#define EXTCON_HEADPHONE  10
> >> -#define EXTCON_HDMI   11
> >> -#define EXTCON_MHL12
> >> -#define EXTCON_DVI13
> >> -#define EXTCON_VGA14
> >> -#define EXTCON_SPDIF_IN   15
> >> -#define EXTCON_SPDIF_OUT  16
> >> -#define EXTCON_VIDEO_IN   17
> >> -#define EXTCON_VIDEO_OUT  18
> >> -
> >> -#define EXTCON_DOCK   19  /* Misc connector */
> >> -#define EXTCON_JIG20
> >> -#define EXTCON_MECHANICAL 21
> >> +#define EXTCON_NONE   0
> >> +
> >> +/* USB external connector */
> >> +#define EXTCON_USB1   /* Universal Serial Bus */
> >> +#define EXTCON_USB_HOST   2
> >> +
> >> +/* Charging external connector */
> >> +#define EXTCON_CHG_USB5   /* Standard Downstream Port
> >> */
> >> +#define EXTCON_CHG_USB_CDP6   /* Charging Downstream Port
> >> */
> >> +#define EXTCON_CHG_USB_DCP7   /* Dedicated Charging Port */
> >> +#define EXTCON_CHG_USB_DCP_FAST   8
> >> +#define EXTCON_CHG_USB_DCP_SLOW   9
> >> +#define EXTCON_CHG_USB_ACA10  /* Accessory Charger Adapter
> >> */
> >> +
> >> +/* Jack external connector */
> >> +#define EXTCON_JACK_MICROPHONE20
> >> +#define EXTCON_JACK_HEADPHONE 21
> >> +#define EXTCON_JACK_LINE_IN   22
> >> +#define EXTCON_JACK_LINE_OUT  23
> >> +#define EXTCON_JACK_VIDEO_IN  24
> >> +#define EXTCON_JACK_VIDEO_OUT 25
> >> +#define EXTCON_JACK_SPDIF_IN  26  /* Sony Philips Digital
> >> InterFace */
> >> +#define EXTCON_JACK_SPDIF_OUT 27
> >> +
> >> +/* Display external connector */
> >> +#define EXTCON_DISP_HDMI  40  /* High-Definition Multimedia
> >> Interface */
> >> +#define EXTCON_DISP_MHL   41  /* Mobile High-Definition Link
> >> */
> >> +#define EXTCON_DISP_DVI   42  /* Digital Visual Inteface */
> >> +#define EXTCON_DISP_VGA   43  /* Video Graphics Array */
> >> +
> >> +/* Miscellaneous external connector */
> >> +#define EXTCON_DOCK   60
> >> +#define EXTCON_JIG61
> >> +#define EXTCON_MECHANICAL 62
> >> +
> >> +#define EXTCON_NUM63
> >
> > Can we change the #define macro's to enum's? is there problem with that?
> 
> It is possible. But, the unique id of external connector will be used on 
> device tree
> file as following patch[1]. Following patch[1] defines the include/dt-
> bindings/extcon/extcon.h
> which is include in *.dts file. In *.dts file, we can not use the 'enum'.
> [1] https://lkml.org/lkml/2015/10/5/38
> So, I use the "#define" keyword instead of enum.
> 
> I used the 'enum' on first patch to define the unique id but I altered it by 
> using
> '#define' instead of 'enum' on following patch[2].
> [2] commit 73b6ecdb93e8e ("extcon: Redefine the unique id of supported
> external connectors without 'enum extcon' type")

Ok. Thanks.

-Ram

RE: [PATCH 1/2] extcon: Modify the id and name of external connector

[Pallala, Ramakrishna]

Hi Choi,


> Subject: [PATCH 1/2] extcon: Modify the id and name of external connector
> 
> This patch modifies the id and name of external connector with the additional
> prefix to clarify both attribute and meaning of external connector as 
> following:
> - EXTCON_CHG_* mean the charger connector.
> - EXTCON_JACK_* mean the jack connector.
> - EXTCON_DISP_* mean the display port connector.
> 
> Following table show the new name of external connector with old name:
> --
> Old extcon name | New extcon name|
> --
> EXTCON_TA   | EXTCON_CHG_USB_DCP |
> EXTCON_FAST_CHARGER | EXTCON_CHG_USB_DCP_FAST|
> EXTCON_SLOW_CHARGER | EXTCON_CHG_USB_DCP_SLOW|
> EXTCON_CHARGE_DOWNSTREAM| EXTCON_CHG_USB_CDP |
> --
> EXTCON_MICROPHONE   | EXTCON_JACK_MICROPHONE |
> EXTCON_HEADPHONE| EXTCON_JACK_HEADPHONE  |
> EXTCON_LINE_IN  | EXTCON_JACK_LINE_IN|
> EXTCON_LINE_OUT | EXTCON_JACK_LINE_OUT   |
> EXTCON_VIDEO_IN | EXTCON_JACK_VIDEO_IN   |
> EXTCON_VIDEO_OUT| EXTCON_JACK_VIDEO_OUT  |
> EXTCON_SPDIF_IN | EXTCON_JACK_SPDIF_IN   |
> EXTCON_SPDIF_OUT| EXTCON_JACK_SPDIF_OUT  |
> --
> EXTCON_HMDI | EXTCON_DISP_HDMI   |
> EXTCON_MHL  | EXTCON_DISP_MHL|
> EXTCON_DVI  | EXTCON_DISP_DVI|
> EXTCON_VGA  | EXTCON_DISP_VGA|
> --
> 
> And, when altering the name of USB charger connector, EXTCON refers to the
> "Battery Charging v1.2 Spec and Adopters Agreement"[1] to use the standard
> name of USB charging port as following. Following name of USB charging port
> are already used in power_supply subsystem. We chan check it on patch[2].
> - EXTCON_CHG_USB  /* Standard Downstream Port */
> - EXTCON_CHG_USB_DCP  /* Dedicated Charging Port */
> - EXTCON_CHG_USB_CDP  /* Charging Downstream Port */
> - EXTCON_CHG_USB_ACA  /* Accessory Charger Adapter */
> 
> [1] www.usb.org/developers/docs/devclass_docs/BCv1.2_070312.zip
> [2] commit 85efc8a18ce ("[PATCH] power_supply: Add types for USB chargers")
> 
> Signed-off-by: Chanwoo Choi 
> [ckeepax: For the Arizona changes]
> Acked-by: Charles Keepax 
> ---
>  drivers/extcon/extcon-arizona.c  | 18 ++--
>  drivers/extcon/extcon-axp288.c   | 12 
>  drivers/extcon/extcon-max14577.c | 17 +--  drivers/extcon/extcon-
> max77693.c | 32 +++--  drivers/extcon/extcon-max77843.c | 27
> +  drivers/extcon/extcon-max8997.c  | 21 +++---
> drivers/extcon/extcon-rt8973a.c  |  4 +--
>  drivers/extcon/extcon-sm5502.c   |  4 +--
>  drivers/extcon/extcon.c  | 61 ---
>  include/linux/extcon.h   | 62 
> +++-
>  10 files changed, 139 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index a1ab0a56b798..e4890dd4fefd 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -137,9 +137,9 @@ static const int arizona_micd_levels[] = {
> 
>  static const unsigned int arizona_cable[] = {
>   EXTCON_MECHANICAL,
> - EXTCON_MICROPHONE,
> - EXTCON_HEADPHONE,
> - EXTCON_LINE_OUT,
> + EXTCON_JACK_MICROPHONE,
> + EXTCON_JACK_HEADPHONE,
> + EXTCON_JACK_LINE_OUT,
>   EXTCON_NONE,
>  };
> 
> @@ -600,7 +600,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>   struct arizona_extcon_info *info = data;
>   struct arizona *arizona = info->arizona;
>   int id_gpio = arizona->pdata.hpdet_id_gpio;
> - unsigned int report = EXTCON_HEADPHONE;
> + unsigned int report = EXTCON_JACK_HEADPHONE;
>   int ret, reading;
>   bool mic = false;
> 
> @@ -645,9 +645,9 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
> 
>   /* Report high impedence cables as line outputs */
>   if (reading >= 5000)
> - report = EXTCON_LINE_OUT;
> + report = EXTCON_JACK_LINE_OUT;
>   else
> - report = EXTCON_HEADPHONE;
> + report = EXTCON_JACK_HEADPHONE;
> 
>   ret = extcon_set_cable_state_(info->edev, report, true);
>   if (ret != 0)
> @@ -732,7 +732,7 @@ err:
>  ARIZONA_ACCDET_MODE_MASK,
> ARIZONA_ACCDET_MODE_MIC);
> 
>   /* Just report headphone */
> - ret = extcon_set_cable_state_(info->edev, EXTCON_HEADPHONE, true);
> + ret = extcon_set_cable_state_(info->edev, EXTCON_JACK_HEADPHONE,
> +true);
>   if (ret != 0)
>   dev_err(arizona->dev, "Failed to report headphone: %d\n", ret);
> 
> @@ -789,7 +789,7 @@ err:
>  

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

> Hi,
> 
> On Fri, Sep 11, 2015 at 09:58:40AM +0900, Krzysztof Kozlowski wrote:
> > On 11.09.2015 01:42, Andrew F. Davis wrote:
> > > On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
> > > +- ti,enable-user-write: boolean, if present driver will allow
> > > +the
> > > user space
> > > +to control the charging current and voltage through sysfs;
> > 
> >  This is not DT property. It does not describe hardware.
> > >>> We needed a mechanism to enable the sysfs writes on certain properties.
> > >>> If DT is not the place where should it go?
> > >>
> > >> DT is not the place. As I discussed later with Andreas, if you
> > >> really need this and if mainline is a place for that then probably
> > >> this should be compile option (a Kconfig symbol).
> > >>
> > >
> > > I think this would actually be a good use for module parameters,
> > > this way it could still be set at boot without re-compiling.
> > >
> > > I think compile-time disabling sysfs properties because they are
> > > "dangerous" is a little bit too artificially restricting and
> > > controlling, you can set permissions so only root can change them
> > > already. The kernel should not be restricting root, I understand the
> > > fear of someone rooting a machine and remotely over charging a
> > > LiPo[1], but these physical limits are hardware descriptions and can
> > > and should be set by DT, beyond this root should have full control
> > > over their machine.
> >
> >
> > Indeed module parameters could be used for enabling/disabling debug
> > options... but as fair as I understand these are for purely
> > development purposes. That is why they got into DT initially, right?
> > To allow the developer to play with it on the development board?
> >
> > This is why I am really not convinced that this should go to mainline.
> >
> > Anyway if it goes, then maybe compiling it out is the safest choice?
> > What's the purpose of having it in kernel all the time? If this was a
> > debug option, than some experienced user could turn it on and report
> > to LKML with extended debug data. But it's not a debug but development
> option?
> 
> Changing the current limit is useful for "expert" users with custom usb power
> supplies, that are not correctly detected by extcon. I also think a module
> parameter would be the best option here.

Ok. I will resubmit the patches along with fixing the other comments.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

> Hi,
> 
> On Fri, Sep 11, 2015 at 09:58:40AM +0900, Krzysztof Kozlowski wrote:
> > On 11.09.2015 01:42, Andrew F. Davis wrote:
> > > On 09/09/2015 06:47 PM, Krzysztof Kozlowski wrote:
> > > +- ti,enable-user-write: boolean, if present driver will allow
> > > +the
> > > user space
> > > +to control the charging current and voltage through sysfs;
> > 
> >  This is not DT property. It does not describe hardware.
> > >>> We needed a mechanism to enable the sysfs writes on certain properties.
> > >>> If DT is not the place where should it go?
> > >>
> > >> DT is not the place. As I discussed later with Andreas, if you
> > >> really need this and if mainline is a place for that then probably
> > >> this should be compile option (a Kconfig symbol).
> > >>
> > >
> > > I think this would actually be a good use for module parameters,
> > > this way it could still be set at boot without re-compiling.
> > >
> > > I think compile-time disabling sysfs properties because they are
> > > "dangerous" is a little bit too artificially restricting and
> > > controlling, you can set permissions so only root can change them
> > > already. The kernel should not be restricting root, I understand the
> > > fear of someone rooting a machine and remotely over charging a
> > > LiPo[1], but these physical limits are hardware descriptions and can
> > > and should be set by DT, beyond this root should have full control
> > > over their machine.
> >
> >
> > Indeed module parameters could be used for enabling/disabling debug
> > options... but as fair as I understand these are for purely
> > development purposes. That is why they got into DT initially, right?
> > To allow the developer to play with it on the development board?
> >
> > This is why I am really not convinced that this should go to mainline.
> >
> > Anyway if it goes, then maybe compiling it out is the safest choice?
> > What's the purpose of having it in kernel all the time? If this was a
> > debug option, than some experienced user could turn it on and report
> > to LKML with extended debug data. But it's not a debug but development
> option?
> 
> Changing the current limit is useful for "expert" users with custom usb power
> supplies, that are not correctly detected by extcon. I also think a module
> parameter would be the best option here.

Ok. I will resubmit the patches along with fixing the other comments.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi, 

> From: k.kozlowsk...@gmail.com [mailto:k.kozlowsk...@gmail.com] On Behalf
> Of Krzysztof Kozlowski
> Sent: Monday, September 7, 2015 9:28 AM
> To: Pallala, Ramakrishna
> Cc: linux-kernel@vger.kernel.org; linux...@vger.kernel.org;
> devicet...@vger.kernel.org; Sebastian Reichel; Tc, Jenny; Andreas Dannenberg
> Subject: Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261
> charger
> 
> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala
> :
> >
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Signed-off-by: Jennt TC 
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   37 +
> >  drivers/power/Kconfig  |6 +
> >  drivers/power/Makefile |1 +
> >  drivers/power/bq24261_charger.c| 1208 
> > 
> >  include/linux/power/bq24261_charger.h  |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> 
> Please split the bindings into separate patch (the first patch in patchset).
Ok.


> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in mA);
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover additional
> devices"
> http://www.spinics.net/lists/devicetree/msg92134.html
> 
> could you and Andreas figure out common bindings? Look at this:
> 
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,charge-current: integer, default charging current (in mA);
> 
> Different meaning and different units. This is madness! :)
This is being closed by Andreas and we would probably go with mA/mV

> In the same time you are adding TI-common bindings (not device specific, there
> is no prefix) so I would expect exactly the same bindings if it is possible.
Ok...i will check with other charger driver DT settings and fix it.

> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> What is the requirement for thermal-sensing? Can it be enabled always?
> If yes, then this is not really a hardware property.
TI BQ24261 has provision to add Battery Pack thermistor but it has no ADC read 
it.
So a HW designer would or may not add the thermistor to charger and instead he 
can
connect to the Fuel Gauge.

> > +- ti,enable-user-write: boolean, if present driver will allow the user 
> > space
> > +to control the charging current and voltage through sysfs;
> 
> This is not DT property. It does not describe hardware.
We needed a mechanism to enable the sysfs writes on certain properties.
If DT is not the place where should it go?

Thanks,
Ram

N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi, 

> From: k.kozlowsk...@gmail.com [mailto:k.kozlowsk...@gmail.com] On Behalf
> Of Krzysztof Kozlowski
> Sent: Monday, September 7, 2015 9:28 AM
> To: Pallala, Ramakrishna
> Cc: linux-kernel@vger.kernel.org; linux...@vger.kernel.org;
> devicet...@vger.kernel.org; Sebastian Reichel; Tc, Jenny; Andreas Dannenberg
> Subject: Re: [PATCH] power: bq24261_charger: Add support for TI BQ24261
> charger
> 
> 2015-09-07 2:23 GMT+09:00 Ramakrishna Pallala
> <ramakrishna.pall...@intel.com>:
> >
> > Add new charger driver support for BQ24261 charger IC.
> >
> > BQ24261 charger driver relies on extcon notifications to get the
> > charger cable type and based on that it will set the charging parameters.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pall...@intel.com>
> > Signed-off-by: Jennt TC <jenny...@intel.com>
> > ---
> >  .../devicetree/bindings/power/bq24261.txt  |   37 +
> >  drivers/power/Kconfig  |6 +
> >  drivers/power/Makefile |1 +
> >  drivers/power/bq24261_charger.c| 1208 
> > 
> >  include/linux/power/bq24261_charger.h  |   27 +
> >  5 files changed, 1279 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/bq24261.txt
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
> >
> > diff --git a/Documentation/devicetree/bindings/power/bq24261.txt
> > b/Documentation/devicetree/bindings/power/bq24261.txt
> > new file mode 100644
> > index 000..25fc5c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/bq24261.txt
> > @@ -0,0 +1,37 @@
> > +Binding for TI bq24261 Li-Ion Charger
> 
> Please split the bindings into separate patch (the first patch in patchset).
Ok.


> > +
> > +Required properties:
> > +- compatible: Should contain one of the following:
> > +* "ti,bq24261"
> > +- reg: integer, i2c address of the device.
> > +- ti,charge-current: integer, default charging current (in mA);
> > +- ti,charge-voltage: integer, default charging voltage (in mV);
> > +- ti,termination-current: integer, charge will be terminated when current 
> > in
> > +constant-voltage phase drops below this value (in mA);
> > +- ti,max-charge-current: integer, maximum charging current (in mA);
> > +- ti,max-charge-voltage: integer, maximum charging voltage (in mV);
> > +- ti,min-charge-temperature: integer, minimum charging temperature
> > +(in DegC);
> > +- ti,max-charge-temperature: integer, maximum charging temperature (in
> DegC).
> 
> Before accepting "[PATCH 13/13] dt: power: bq24257-charger: Cover additional
> devices"
> http://www.spinics.net/lists/devicetree/msg92134.html
> 
> could you and Andreas figure out common bindings? Look at this:
> 
> +- ti,charge-current: integer, maximum charging current in uA.
> +- ti,charge-current: integer, default charging current (in mA);
> 
> Different meaning and different units. This is madness! :)
This is being closed by Andreas and we would probably go with mA/mV

> In the same time you are adding TI-common bindings (not device specific, there
> is no prefix) so I would expect exactly the same bindings if it is possible.
Ok...i will check with other charger driver DT settings and fix it.

> > +Optional properties:
> > +- ti,thermal-sensing: boolean, if present thermal regulation will be
> > +enabled;
> 
> What is the requirement for thermal-sensing? Can it be enabled always?
> If yes, then this is not really a hardware property.
TI BQ24261 has provision to add Battery Pack thermistor but it has no ADC read 
it.
So a HW designer would or may not add the thermistor to charger and instead he 
can
connect to the Fuel Gauge.

> > +- ti,enable-user-write: boolean, if present driver will allow the user 
> > space
> > +to control the charging current and voltage through sysfs;
> 
> This is not DT property. It does not describe hardware.
We needed a mechanism to enable the sysfs writes on certain properties.
If DT is not the place where should it go?

Thanks,
Ram

N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas,

> On Wed, Aug 26, 2015 at 11:03:00AM +0000, Pallala, Ramakrishna wrote:
> > Hi Andreas,
> >
> > I went on a unplanned leave and I came back to office recently. I will go
> through your comments and get back to you.
> 
> Hi Ram,
> hope all is well. Please let me know your plans/timeline for completing this
> because I'm on the hook for delivering support for bq24261M and
> bq24262 and I was planning on adding it to your driver.
> 
> Also should bandwidth be an issue I will be happy to help out in any way I can
> even taking over the completion of this driver if needed - just let me know.

I will resubmit the patch by this weekend. I Hope this works for you.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas,

> On Wed, Aug 26, 2015 at 11:03:00AM +0000, Pallala, Ramakrishna wrote:
> > Hi Andreas,
> >
> > I went on a unplanned leave and I came back to office recently. I will go
> through your comments and get back to you.
> 
> Hi Ram,
> hope all is well. Please let me know your plans/timeline for completing this
> because I'm on the hook for delivering support for bq24261M and
> bq24262 and I was planning on adding it to your driver.
> 
> Also should bandwidth be an issue I will be happy to help out in any way I can
> even taking over the completion of this driver if needed - just let me know.

I will resubmit the patch by this weekend. I Hope this works for you.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas,

I went on a unplanned leave and I came back to office recently. I will go 
through your comments and get back to you.


> Subject: Re: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261
> charger
> 
> Hi,
> 
> On Tue, Aug 18, 2015 at 11:19:35PM +0530, Ramakrishna Pallala wrote:
> > Add new charger driver support for BQ24261 charger IC.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/power/Kconfig |6 +
> >  drivers/power/Makefile|1 +
> >  drivers/power/bq24261_charger.c   | 1127
> +
> >  include/linux/power/bq24261_charger.h |   26 +
> >  4 files changed, 1160 insertions(+)
> >  create mode 100644 drivers/power/bq24261_charger.c  create mode
> > 100644 include/linux/power/bq24261_charger.h
.
.
.
 
> Thanks Ram for submitting your driver. I think it's a good base to merge my
> (unplished) work supporting the bq24261M and bq24262.
> 
> Laurentiu and I were having an offline discussion whether to make the above
> constant charge current / voltage writable through sysfs on the
> bq24257_charger.c driver as I'm merging my bq24250/bq24251 work there.
> While helpful for debugging and to empower certain user-space applications
> those properties also are somewhat dangerous to expose at the same time if an
> unskilled user gets hold of them...
> 
> (Thanks Laurentiu for digging up below thread)
> http://marc.info/?l=linux-pm=143080413218161=2
> 
> In this context I was thinking, what about introducing a DT property for this 
> and
> other charger drivers called "batt_param_write_enable" that by default is OFF
> and controls the writability of above two parameters? I think this would add 
> one
> layer of safety while at the same time allowing more flexibility for where 
> it's
> needed.
> 
> (more comments below)
> 
> > +
> > +static enum power_supply_property bq24261_usb_props[] = {
> > +   POWER_SUPPLY_PROP_PRESENT,
> > +   POWER_SUPPLY_PROP_ONLINE,
> > +   POWER_SUPPLY_PROP_TYPE,
> > +   POWER_SUPPLY_PROP_HEALTH,
> > +   POWER_SUPPLY_PROP_STATUS,
> > +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> > +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> > +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> > +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> > +   POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > +   POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> > +   POWER_SUPPLY_PROP_TEMP_MAX,
> > +   POWER_SUPPLY_PROP_TEMP_MIN,
> > +};
> > +
> > +static char *bq24261_charger_supplied_to[] = {
> > +   "main-battery",
> > +};
> > +
> > +static struct power_supply_desc bq24261_charger_desc = {
> > +   .name   = DEV_NAME,
> > +   .type   = POWER_SUPPLY_TYPE_USB,
> > +   .properties = bq24261_usb_props,
> > +   .num_properties = ARRAY_SIZE(bq24261_usb_props),
> > +   .get_property   = bq24261_usb_get_property,
> > +   .set_property   = bq24261_usb_set_property,
> > +   .property_is_writeable  = bq24261_property_is_writeable,
> > +};
> > +
> > +static void bq24261_wdt_reset_worker(struct work_struct *work) {
> > +
> > +   struct bq24261_charger *chip = container_of(work,
> > +   struct bq24261_charger, wdt_work.work);
> > +   int ret;
> > +
> > +   ret = bq24261_reset_wdt_timer(chip);
> > +   if (ret)
> > +   dev_err(>client->dev, "WDT timer reset error(%d)\n",
> ret);
> > +
> > +   schedule_delayed_work(>wdt_work, WDT_RESET_DELAY); }
> > +
> > +static void bq24261_irq_worker(struct work_struct *work) {
> > +   struct bq24261_charger *chip =
> > +   container_of(work, struct bq24261_charger, irq_work);
> > +   int ret;
> > +
> > +   /*
> > +* Lock to ensure that interrupt register readings are done
> > +* and processed sequentially. The interrupt Fault registers
> > +* are read on clear and without sequential processing double
> > +* fault interrupts or fault recovery cannot be handlled propely
> > +*/
> > +
> > +   mutex_lock(>lock);
> > +
> > +   ret = bq24261_read_reg(chip->client, BQ24261_STAT_CTRL0_ADDR);
> > +   if (ret < 0) {
> > +   dev_err(>client->dev,
> > +   "Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n",
> ret);
> > +   goto irq_out;
> > +   }
> > +
> > +   if (!chip->cable.boost) {
> > +   bq24261_handle_status(chip, ret);
> > +   bq24261_handle_health(chip, ret);
> > +   power_supply_changed(chip->psy_usb);
> > +   }
> > +
> > +irq_out:
> > +   mutex_unlock(>lock);
> > +}
> > +
> > +static irqreturn_t bq24261_thread_handler(int id, void *data) {
> > +   struct bq24261_charger *chip = (struct bq24261_charger *)data;
> > +
> > +   queue_work(system_highpri_wq, >irq_work);
> > +   return IRQ_HANDLED;
> > +}
> > +
> > +static void bq24261_fault_mon_work(struct work_struct *work) {
> > +   struct bq24261_charger *chip = container_of(work,
> > +   struct bq24261_charger, fault_mon_work.work);
> > +   int ret;
> 

RE: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261 charger

[Pallala, Ramakrishna]

Hi Andreas,

I went on a unplanned leave and I came back to office recently. I will go 
through your comments and get back to you.


 Subject: Re: [RFC PATCH] power: bq24261_charger: Add support for TI BQ24261
 charger
 
 Hi,
 
 On Tue, Aug 18, 2015 at 11:19:35PM +0530, Ramakrishna Pallala wrote:
  Add new charger driver support for BQ24261 charger IC.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/power/Kconfig |6 +
   drivers/power/Makefile|1 +
   drivers/power/bq24261_charger.c   | 1127
 +
   include/linux/power/bq24261_charger.h |   26 +
   4 files changed, 1160 insertions(+)
   create mode 100644 drivers/power/bq24261_charger.c  create mode
  100644 include/linux/power/bq24261_charger.h
.
.
.
 
 Thanks Ram for submitting your driver. I think it's a good base to merge my
 (unplished) work supporting the bq24261M and bq24262.
 
 Laurentiu and I were having an offline discussion whether to make the above
 constant charge current / voltage writable through sysfs on the
 bq24257_charger.c driver as I'm merging my bq24250/bq24251 work there.
 While helpful for debugging and to empower certain user-space applications
 those properties also are somewhat dangerous to expose at the same time if an
 unskilled user gets hold of them...
 
 (Thanks Laurentiu for digging up below thread)
 http://marc.info/?l=linux-pmm=143080413218161w=2
 
 In this context I was thinking, what about introducing a DT property for this 
 and
 other charger drivers called batt_param_write_enable that by default is OFF
 and controls the writability of above two parameters? I think this would add 
 one
 layer of safety while at the same time allowing more flexibility for where 
 it's
 needed.
 
 (more comments below)
 
  +
  +static enum power_supply_property bq24261_usb_props[] = {
  +   POWER_SUPPLY_PROP_PRESENT,
  +   POWER_SUPPLY_PROP_ONLINE,
  +   POWER_SUPPLY_PROP_TYPE,
  +   POWER_SUPPLY_PROP_HEALTH,
  +   POWER_SUPPLY_PROP_STATUS,
  +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
  +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
  +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
  +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
  +   POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
  +   POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
  +   POWER_SUPPLY_PROP_TEMP_MAX,
  +   POWER_SUPPLY_PROP_TEMP_MIN,
  +};
  +
  +static char *bq24261_charger_supplied_to[] = {
  +   main-battery,
  +};
  +
  +static struct power_supply_desc bq24261_charger_desc = {
  +   .name   = DEV_NAME,
  +   .type   = POWER_SUPPLY_TYPE_USB,
  +   .properties = bq24261_usb_props,
  +   .num_properties = ARRAY_SIZE(bq24261_usb_props),
  +   .get_property   = bq24261_usb_get_property,
  +   .set_property   = bq24261_usb_set_property,
  +   .property_is_writeable  = bq24261_property_is_writeable,
  +};
  +
  +static void bq24261_wdt_reset_worker(struct work_struct *work) {
  +
  +   struct bq24261_charger *chip = container_of(work,
  +   struct bq24261_charger, wdt_work.work);
  +   int ret;
  +
  +   ret = bq24261_reset_wdt_timer(chip);
  +   if (ret)
  +   dev_err(chip-client-dev, WDT timer reset error(%d)\n,
 ret);
  +
  +   schedule_delayed_work(chip-wdt_work, WDT_RESET_DELAY); }
  +
  +static void bq24261_irq_worker(struct work_struct *work) {
  +   struct bq24261_charger *chip =
  +   container_of(work, struct bq24261_charger, irq_work);
  +   int ret;
  +
  +   /*
  +* Lock to ensure that interrupt register readings are done
  +* and processed sequentially. The interrupt Fault registers
  +* are read on clear and without sequential processing double
  +* fault interrupts or fault recovery cannot be handlled propely
  +*/
  +
  +   mutex_lock(chip-lock);
  +
  +   ret = bq24261_read_reg(chip-client, BQ24261_STAT_CTRL0_ADDR);
  +   if (ret  0) {
  +   dev_err(chip-client-dev,
  +   Error (%d) in reading BQ24261_STAT_CTRL0_ADDR\n,
 ret);
  +   goto irq_out;
  +   }
  +
  +   if (!chip-cable.boost) {
  +   bq24261_handle_status(chip, ret);
  +   bq24261_handle_health(chip, ret);
  +   power_supply_changed(chip-psy_usb);
  +   }
  +
  +irq_out:
  +   mutex_unlock(chip-lock);
  +}
  +
  +static irqreturn_t bq24261_thread_handler(int id, void *data) {
  +   struct bq24261_charger *chip = (struct bq24261_charger *)data;
  +
  +   queue_work(system_highpri_wq, chip-irq_work);
  +   return IRQ_HANDLED;
  +}
  +
  +static void bq24261_fault_mon_work(struct work_struct *work) {
  +   struct bq24261_charger *chip = container_of(work,
  +   struct bq24261_charger, fault_mon_work.work);
  +   int ret;
  +
  +   if ((chip-chg_health == POWER_SUPPLY_HEALTH_OVERVOLTAGE) ||
  +   (chip-chg_health == POWER_SUPPLY_HEALTH_DEAD)) {
  +
  +   mutex_lock(chip-lock);
  +  

RE: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi Krzysztof Kozlowski,

>On Tue, Jun 23, 2015 at 09:58:41AM +0900, Krzysztof Kozlowski wrote:
>> 2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski :
>> > 2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala 
>> > :
>> >> This patch adds the support for following battery properties to 
> >> max17042 fuel gauge driver.
>> >>
>> >> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> >> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
>> >> POWER_SUPPLY_PROP_TEMP_MIN
>> >> POWER_SUPPLY_PROP_TEMP_MAX
>> >> POWER_SUPPLY_PROP_HEALTH
>> >
>> > I wonder, have you tested the patch? After booting on Trats2 device
>> > (max77693 which identifies itself as 17047-like) the values are:
>> > POWER_SUPPLY_TEMP_ALERT_MIN=1280
>> > POWER_SUPPLY_TEMP_ALERT_MAX=1270
>> > POWER_SUPPLY_TEMP=257
>> > This is okay, datasheet says that register after booting will have 
>> > value of 0x7f80.
>> >
>> > However setting them to some value which should trigger interrupts 
>> > (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I 
>> > added a printk in max17042_thread_handler().
>> >
>> > Is the temperature alert feature working?
>> 
>> Can you reply to my question above?
>> If this feature is not working, then it should be removed.

>What's the status of this? I cannot test the feature, since I don't have the 
>hardware. I agree, that this should be removed, if it's not working.

I missed this email (may be overlooked it). To have the interrupts enabled we 
need the config registers(0x1Dh) bit's BIT(9), BIT(4) and BIT92) should be 1 
and  BIT(8) should be 0.

Can you dump the status(00h), Talrt(02H) Temp(08h) and config(1Dh) registers 
values and share?

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi Krzysztof Kozlowski,

On Tue, Jun 23, 2015 at 09:58:41AM +0900, Krzysztof Kozlowski wrote:
 2015-06-08 10:22 GMT+09:00 Krzysztof Kozlowski k.kozlow...@samsung.com:
  2015-05-24 12:41 GMT+09:00 Ramakrishna Pallala 
  ramakrishna.pall...@intel.com:
  This patch adds the support for following battery properties to 
  max17042 fuel gauge driver.
 
  POWER_SUPPLY_PROP_TEMP_ALERT_MIN
  POWER_SUPPLY_PROP_TEMP_ALERT_MAX
  POWER_SUPPLY_PROP_TEMP_MIN
  POWER_SUPPLY_PROP_TEMP_MAX
  POWER_SUPPLY_PROP_HEALTH
 
  I wonder, have you tested the patch? After booting on Trats2 device
  (max77693 which identifies itself as 17047-like) the values are:
  POWER_SUPPLY_TEMP_ALERT_MIN=1280
  POWER_SUPPLY_TEMP_ALERT_MAX=1270
  POWER_SUPPLY_TEMP=257
  This is okay, datasheet says that register after booting will have 
  value of 0x7f80.
 
  However setting them to some value which should trigger interrupts 
  (like 300 for MIN or 200 for MAX) does not trigger the interrupt. I 
  added a printk in max17042_thread_handler().
 
  Is the temperature alert feature working?
 
 Can you reply to my question above?
 If this feature is not working, then it should be removed.

What's the status of this? I cannot test the feature, since I don't have the 
hardware. I agree, that this should be removed, if it's not working.

I missed this email (may be overlooked it). To have the interrupts enabled we 
need the config registers(0x1Dh) bit's BIT(9), BIT(4) and BIT92) should be 1 
and  BIT(8) should be 0.

Can you dump the status(00h), Talrt(02H) Temp(08h) and config(1Dh) registers 
values and share?

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v1] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

> Hi,
> 
> On Tue, May 05, 2015 at 05:10:32AM +0530, Ramakrishna Pallala wrote:
> > This patch adds the support for following battery properties to
> > max17042 fuel gauge driver.
> >
> > POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> > POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> > POWER_SUPPLY_PROP_TEMP_MIN
> > POWER_SUPPLY_PROP_TEMP_MAX
> > POWER_SUPPLY_PROP_HEALTH
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Reviewed-by: Krzysztof Kozlowski 
> 
> The patch does not work for me:
> 
> drivers/power/max17042_battery.c: In function
> ‘max17042_get_battery_health’:
> drivers/power/max17042_battery.c:150:34: error:
> ‘MAX17042_VMAX_TOLERENCE’ undeclared (first use in this function)
>   if (vbatt > chip->pdata->vmax + MAX17042_VMAX_TOLERENCE) {

My bad :( :(

I will fix it submit the patch v2

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

 Hi,
 
 On Tue, May 05, 2015 at 05:10:32AM +0530, Ramakrishna Pallala wrote:
  This patch adds the support for following battery properties to
  max17042 fuel gauge driver.
 
  POWER_SUPPLY_PROP_TEMP_ALERT_MIN
  POWER_SUPPLY_PROP_TEMP_ALERT_MAX
  POWER_SUPPLY_PROP_TEMP_MIN
  POWER_SUPPLY_PROP_TEMP_MAX
  POWER_SUPPLY_PROP_HEALTH
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  Reviewed-by: Krzysztof Kozlowski k.kozlowsk...@gmail.com
 
 The patch does not work for me:
 
 drivers/power/max17042_battery.c: In function
 ‘max17042_get_battery_health’:
 drivers/power/max17042_battery.c:150:34: error:
 ‘MAX17042_VMAX_TOLERENCE’ undeclared (first use in this function)
   if (vbatt  chip-pdata-vmax + MAX17042_VMAX_TOLERENCE) {

My bad :( :(

I will fix it submit the patch v2

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi Sebastian,

Any feedback on this patch?

> This patch adds the support for following battery properties to max17042 fuel
> gauge driver.
> 
> POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> POWER_SUPPLY_PROP_TEMP_MIN
> POWER_SUPPLY_PROP_TEMP_MAX
> POWER_SUPPLY_PROP_HEALTH
> 
> Signed-off-by: Ramakrishna Pallala 
> Reviewed-by: Krzysztof Kozlowski 
> ---
>  drivers/power/max17042_battery.c   |  190
> ++--
>  include/linux/power/max17042_battery.h |4 +
>  2 files changed, 183 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/max17042_battery.c
> b/drivers/power/max17042_battery.c
> index 6cc5e87..9af610c 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -63,6 +63,8 @@
>  #define dP_ACC_100   0x1900
>  #define dP_ACC_200   0x3200
> 
> +#define MAX17042_VMAX_TOLERANCE  50 /* 50 mV */
> +
>  struct max17042_chip {
>   struct i2c_client *client;
>   struct regmap *regmap;
> @@ -85,10 +87,94 @@ static enum power_supply_property
> max17042_battery_props[] = {
>   POWER_SUPPLY_PROP_CHARGE_FULL,
>   POWER_SUPPLY_PROP_CHARGE_COUNTER,
>   POWER_SUPPLY_PROP_TEMP,
> + POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> + POWER_SUPPLY_PROP_TEMP_MIN,
> + POWER_SUPPLY_PROP_TEMP_MAX,
> + POWER_SUPPLY_PROP_HEALTH,
>   POWER_SUPPLY_PROP_CURRENT_NOW,
>   POWER_SUPPLY_PROP_CURRENT_AVG,
>  };
> 
> +static int max17042_get_temperature(struct max17042_chip *chip, int
> +*temp) {
> + int ret;
> + u32 data;
> + struct regmap *map = chip->regmap;
> +
> + ret = regmap_read(map, MAX17042_TEMP, );
> + if (ret < 0)
> + return ret;
> +
> + *temp = data;
> + /* The value is signed. */
> + if (*temp & 0x8000) {
> + *temp = (0x7fff & ~*temp) + 1;
> + *temp *= -1;
> + }
> +
> + /* The value is converted into deci-centigrade scale */
> + /* Units of LSB = 1 / 256 degree Celsius */
> + *temp = *temp * 10 / 256;
> + return 0;
> +}
> +
> +static int max17042_get_battery_health(struct max17042_chip *chip, int
> +*health) {
> + int temp, vavg, vbatt, ret;
> + u32 val;
> +
> + ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, );
> + if (ret < 0)
> + goto health_error;
> +
> + /* bits [0-3] unused */
> + vavg = val * 625 / 8;
> + /* Convert to millivolts */
> + vavg /= 1000;
> +
> + ret = regmap_read(chip->regmap, MAX17042_VCELL, );
> + if (ret < 0)
> + goto health_error;
> +
> + /* bits [0-3] unused */
> + vbatt = val * 625 / 8;
> + /* Convert to millivolts */
> + vbatt /= 1000;
> +
> + if (vavg < chip->pdata->vmin) {
> + *health = POWER_SUPPLY_HEALTH_DEAD;
> + goto out;
> + }
> +
> + if (vbatt > chip->pdata->vmax + MAX17042_VMAX_TOLERENCE) {
> + *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> + goto out;
> + }
> +
> + ret = max17042_get_temperature(chip, );
> + if (ret < 0)
> + goto health_error;
> +
> + if (temp <= chip->pdata->temp_min) {
> + *health = POWER_SUPPLY_HEALTH_COLD;
> + goto out;
> + }
> +
> + if (temp >= chip->pdata->temp_max) {
> + *health = POWER_SUPPLY_HEALTH_OVERHEAT;
> + goto out;
> + }
> +
> + *health = POWER_SUPPLY_HEALTH_GOOD;
> +
> +out:
> + return 0;
> +
> +health_error:
> + return ret;
> +}
> +
>  static int max17042_get_property(struct power_supply *psy,
>   enum power_supply_property psp,
>   union power_supply_propval *val) @@ -181,19
> +267,34 @@ static int max17042_get_property(struct power_supply *psy,
>   val->intval = data * 1000 / 2;
>   break;
>   case POWER_SUPPLY_PROP_TEMP:
> - ret = regmap_read(map, MAX17042_TEMP, );
> + ret = max17042_get_temperature(chip, >intval);
> + if (ret < 0)
> + return ret;
> + break;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> + ret = regmap_read(map, MAX17042_TALRT_Th, );
> + if (ret < 0)
> + return ret;
> + /* LSB is Alert Minimum. In deci-centigrade */
> + val->intval = (data & 0xff) * 10;
> + break;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> + ret = regmap_read(map, MAX17042_TALRT_Th, );
> + if (ret < 0)
> + return ret;
> + /* MSB is Alert Maximum. In deci-centigrade */
> + val->intval = (data >> 8) * 10;
> + break;
> + case POWER_SUPPLY_PROP_TEMP_MIN:
> + val->intval = chip->pdata->temp_min;
> + break;
> + case POWER_SUPPLY_PROP_TEMP_MAX:
> + val->intval = chip->pdata->temp_max;
> + break;
> + 

RE: [PATCH v1] power: axp288_charger: axp288 charger driver

[Pallala, Ramakrishna]

Hi Sebastian,

Any feedback on this patch?

> This patch adds new power supply charger driver support for X-Power AXP288
> PMIC integrated charger.
> 
> This driver interfaces with the axp20x mfd driver as a cell and listens to 
> extcon
> cable events for setting up charging.
> 
> Signed-off-by: Ramakrishna Pallala 
> Acked-by: Lee Jones 
> ---
>  drivers/power/Kconfig  |7 +
>  drivers/power/Makefile |1 +
>  drivers/power/axp288_charger.c |  941
> 
>  include/linux/mfd/axp20x.h |7 +
>  4 files changed, 956 insertions(+)
>  create mode 100644 drivers/power/axp288_charger.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index
> 4091fb0..c7d7e8b 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -204,6 +204,13 @@ config CHARGER_DA9150
> This driver can also be built as a module. If so, the module will be
> called da9150-charger.
> 
> +config AXP288_CHARGER
> + tristate "X-Powers AXP288 Charger"
> + depends on MFD_AXP20X && EXTCON_AXP288
> + help
> +   Say yes here to have support X-Power AXP288 power management IC
> (PMIC)
> +   integrated charger.
> +
>  config AXP288_FUEL_GAUGE
>   tristate "X-Powers AXP288 Fuel Gauge"
>   depends on MFD_AXP20X && IIO
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile index
> b7b0181..5acae5f 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -64,3 +64,4 @@ obj-$(CONFIG_CHARGER_SMB347)+= smb347-charger.o
>  obj-$(CONFIG_CHARGER_TPS65090)   += tps65090-charger.o
>  obj-$(CONFIG_POWER_RESET)+= reset/
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
> +obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
> diff --git a/drivers/power/axp288_charger.c b/drivers/power/axp288_charger.c
> new file mode 100644 index 000..5680317
> --- /dev/null
> +++ b/drivers/power/axp288_charger.c
> @@ -0,0 +1,941 @@
> +/*
> + * axp288_charger.c - X-power AXP288 PMIC Charger driver
> + *
> + * Copyright (C) 2014 Intel Corporation
> + * Author: Ramakrishna Pallala 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PS_STAT_VBUS_TRIGGER (1 << 0)
> +#define PS_STAT_BAT_CHRG_DIR (1 << 2)
> +#define PS_STAT_VBAT_ABOVE_VHOLD (1 << 3)
> +#define PS_STAT_VBUS_VALID   (1 << 4)
> +#define PS_STAT_VBUS_PRESENT (1 << 5)
> +
> +#define CHRG_STAT_BAT_SAFE_MODE  (1 << 3)
> +#define CHRG_STAT_BAT_VALID  (1 << 4)
> +#define CHRG_STAT_BAT_PRESENT(1 << 5)
> +#define CHRG_STAT_CHARGING   (1 << 6)
> +#define CHRG_STAT_PMIC_OTP   (1 << 7)
> +
> +#define VBUS_ISPOUT_CUR_LIM_MASK 0x03
> +#define VBUS_ISPOUT_CUR_LIM_BIT_POS  0
> +#define VBUS_ISPOUT_CUR_LIM_900MA0x0 /* 900mA */
> +#define VBUS_ISPOUT_CUR_LIM_1500MA   0x1 /* 1500mA */
> +#define VBUS_ISPOUT_CUR_LIM_2000MA   0x2 /* 2000mA */
> +#define VBUS_ISPOUT_CUR_NO_LIM   0x3 /* 2500mA */
> +#define VBUS_ISPOUT_VHOLD_SET_MASK   0x31
> +#define VBUS_ISPOUT_VHOLD_SET_BIT_POS0x3
> +#define VBUS_ISPOUT_VHOLD_SET_OFFSET 4000/* 4000mV */
> +#define VBUS_ISPOUT_VHOLD_SET_LSB_RES100 /* 100mV */
> +#define VBUS_ISPOUT_VHOLD_SET_4300MV 0x3 /* 4300mV */
> +#define VBUS_ISPOUT_VBUS_PATH_DIS(1 << 7)
> +
> +#define CHRG_CCCV_CC_MASK0xf /* 4 bits */
> +#define CHRG_CCCV_CC_BIT_POS 0
> +#define CHRG_CCCV_CC_OFFSET  200 /* 200mA */
> +#define CHRG_CCCV_CC_LSB_RES 200 /* 200mA */
> +#define CHRG_CCCV_ITERM_20P  (1 << 4)/* 20% of CC */
> +#define CHRG_CCCV_CV_MASK0x60/* 2 bits */
> +#define CHRG_CCCV_CV_BIT_POS 5
> +#define CHRG_CCCV_CV_4100MV  0x0 /* 4.10V */
> +#define CHRG_CCCV_CV_4150MV  0x1 /* 4.15V */
> +#define CHRG_CCCV_CV_4200MV  0x2 /* 4.20V */
> +#define CHRG_CCCV_CV_4350MV  0x3 /* 4.35V */
> +#define CHRG_CCCV_CHG_EN (1 << 7)
> +
> +#define CNTL2_CC_TIMEOUT_MASK0x3 /* 2 bits */
> +#define CNTL2_CC_TIMEOUT_OFFSET  6   /* 6 Hrs */
> +#define CNTL2_CC_TIMEOUT_LSB_RES 2   /* 2 Hrs */
> +#define CNTL2_CC_TIMEOUT_12HRS   0x3 /* 12 Hrs */
> +#define 

RE: [PATCH v1] power: axp288_charger: axp288 charger driver

[Pallala, Ramakrishna]

Hi Sebastian,

Any feedback on this patch?

 This patch adds new power supply charger driver support for X-Power AXP288
 PMIC integrated charger.
 
 This driver interfaces with the axp20x mfd driver as a cell and listens to 
 extcon
 cable events for setting up charging.
 
 Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
 Acked-by: Lee Jones lee.jo...@linaro.org
 ---
  drivers/power/Kconfig  |7 +
  drivers/power/Makefile |1 +
  drivers/power/axp288_charger.c |  941
 
  include/linux/mfd/axp20x.h |7 +
  4 files changed, 956 insertions(+)
  create mode 100644 drivers/power/axp288_charger.c
 
 diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index
 4091fb0..c7d7e8b 100644
 --- a/drivers/power/Kconfig
 +++ b/drivers/power/Kconfig
 @@ -204,6 +204,13 @@ config CHARGER_DA9150
 This driver can also be built as a module. If so, the module will be
 called da9150-charger.
 
 +config AXP288_CHARGER
 + tristate X-Powers AXP288 Charger
 + depends on MFD_AXP20X  EXTCON_AXP288
 + help
 +   Say yes here to have support X-Power AXP288 power management IC
 (PMIC)
 +   integrated charger.
 +
  config AXP288_FUEL_GAUGE
   tristate X-Powers AXP288 Fuel Gauge
   depends on MFD_AXP20X  IIO
 diff --git a/drivers/power/Makefile b/drivers/power/Makefile index
 b7b0181..5acae5f 100644
 --- a/drivers/power/Makefile
 +++ b/drivers/power/Makefile
 @@ -64,3 +64,4 @@ obj-$(CONFIG_CHARGER_SMB347)+= smb347-charger.o
  obj-$(CONFIG_CHARGER_TPS65090)   += tps65090-charger.o
  obj-$(CONFIG_POWER_RESET)+= reset/
  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 +obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
 diff --git a/drivers/power/axp288_charger.c b/drivers/power/axp288_charger.c
 new file mode 100644 index 000..5680317
 --- /dev/null
 +++ b/drivers/power/axp288_charger.c
 @@ -0,0 +1,941 @@
 +/*
 + * axp288_charger.c - X-power AXP288 PMIC Charger driver
 + *
 + * Copyright (C) 2014 Intel Corporation
 + * Author: Ramakrishna Pallala ramakrishna.pall...@intel.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include linux/module.h
 +#include linux/device.h
 +#include linux/regmap.h
 +#include linux/workqueue.h
 +#include linux/delay.h
 +#include linux/platform_device.h
 +#include linux/usb/otg.h
 +#include linux/notifier.h
 +#include linux/power_supply.h
 +#include linux/notifier.h
 +#include linux/property.h
 +#include linux/mfd/axp20x.h
 +#include linux/extcon.h
 +
 +#define PS_STAT_VBUS_TRIGGER (1  0)
 +#define PS_STAT_BAT_CHRG_DIR (1  2)
 +#define PS_STAT_VBAT_ABOVE_VHOLD (1  3)
 +#define PS_STAT_VBUS_VALID   (1  4)
 +#define PS_STAT_VBUS_PRESENT (1  5)
 +
 +#define CHRG_STAT_BAT_SAFE_MODE  (1  3)
 +#define CHRG_STAT_BAT_VALID  (1  4)
 +#define CHRG_STAT_BAT_PRESENT(1  5)
 +#define CHRG_STAT_CHARGING   (1  6)
 +#define CHRG_STAT_PMIC_OTP   (1  7)
 +
 +#define VBUS_ISPOUT_CUR_LIM_MASK 0x03
 +#define VBUS_ISPOUT_CUR_LIM_BIT_POS  0
 +#define VBUS_ISPOUT_CUR_LIM_900MA0x0 /* 900mA */
 +#define VBUS_ISPOUT_CUR_LIM_1500MA   0x1 /* 1500mA */
 +#define VBUS_ISPOUT_CUR_LIM_2000MA   0x2 /* 2000mA */
 +#define VBUS_ISPOUT_CUR_NO_LIM   0x3 /* 2500mA */
 +#define VBUS_ISPOUT_VHOLD_SET_MASK   0x31
 +#define VBUS_ISPOUT_VHOLD_SET_BIT_POS0x3
 +#define VBUS_ISPOUT_VHOLD_SET_OFFSET 4000/* 4000mV */
 +#define VBUS_ISPOUT_VHOLD_SET_LSB_RES100 /* 100mV */
 +#define VBUS_ISPOUT_VHOLD_SET_4300MV 0x3 /* 4300mV */
 +#define VBUS_ISPOUT_VBUS_PATH_DIS(1  7)
 +
 +#define CHRG_CCCV_CC_MASK0xf /* 4 bits */
 +#define CHRG_CCCV_CC_BIT_POS 0
 +#define CHRG_CCCV_CC_OFFSET  200 /* 200mA */
 +#define CHRG_CCCV_CC_LSB_RES 200 /* 200mA */
 +#define CHRG_CCCV_ITERM_20P  (1  4)/* 20% of CC */
 +#define CHRG_CCCV_CV_MASK0x60/* 2 bits */
 +#define CHRG_CCCV_CV_BIT_POS 5
 +#define CHRG_CCCV_CV_4100MV  0x0 /* 4.10V */
 +#define CHRG_CCCV_CV_4150MV  0x1 /* 4.15V */
 +#define CHRG_CCCV_CV_4200MV  0x2 /* 4.20V */
 +#define CHRG_CCCV_CV_4350MV  0x3 /* 4.35V */
 +#define CHRG_CCCV_CHG_EN (1  7)
 +
 +#define CNTL2_CC_TIMEOUT_MASK0x3 /* 2 bits */
 +#define CNTL2_CC_TIMEOUT_OFFSET  6   /* 6 Hrs */
 +#define 

RE: [PATCH v1] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi Sebastian,

Any feedback on this patch?

 This patch adds the support for following battery properties to max17042 fuel
 gauge driver.
 
 POWER_SUPPLY_PROP_TEMP_ALERT_MIN
 POWER_SUPPLY_PROP_TEMP_ALERT_MAX
 POWER_SUPPLY_PROP_TEMP_MIN
 POWER_SUPPLY_PROP_TEMP_MAX
 POWER_SUPPLY_PROP_HEALTH
 
 Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
 Reviewed-by: Krzysztof Kozlowski k.kozlowsk...@gmail.com
 ---
  drivers/power/max17042_battery.c   |  190
 ++--
  include/linux/power/max17042_battery.h |4 +
  2 files changed, 183 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/power/max17042_battery.c
 b/drivers/power/max17042_battery.c
 index 6cc5e87..9af610c 100644
 --- a/drivers/power/max17042_battery.c
 +++ b/drivers/power/max17042_battery.c
 @@ -63,6 +63,8 @@
  #define dP_ACC_100   0x1900
  #define dP_ACC_200   0x3200
 
 +#define MAX17042_VMAX_TOLERANCE  50 /* 50 mV */
 +
  struct max17042_chip {
   struct i2c_client *client;
   struct regmap *regmap;
 @@ -85,10 +87,94 @@ static enum power_supply_property
 max17042_battery_props[] = {
   POWER_SUPPLY_PROP_CHARGE_FULL,
   POWER_SUPPLY_PROP_CHARGE_COUNTER,
   POWER_SUPPLY_PROP_TEMP,
 + POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
 + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
 + POWER_SUPPLY_PROP_TEMP_MIN,
 + POWER_SUPPLY_PROP_TEMP_MAX,
 + POWER_SUPPLY_PROP_HEALTH,
   POWER_SUPPLY_PROP_CURRENT_NOW,
   POWER_SUPPLY_PROP_CURRENT_AVG,
  };
 
 +static int max17042_get_temperature(struct max17042_chip *chip, int
 +*temp) {
 + int ret;
 + u32 data;
 + struct regmap *map = chip-regmap;
 +
 + ret = regmap_read(map, MAX17042_TEMP, data);
 + if (ret  0)
 + return ret;
 +
 + *temp = data;
 + /* The value is signed. */
 + if (*temp  0x8000) {
 + *temp = (0x7fff  ~*temp) + 1;
 + *temp *= -1;
 + }
 +
 + /* The value is converted into deci-centigrade scale */
 + /* Units of LSB = 1 / 256 degree Celsius */
 + *temp = *temp * 10 / 256;
 + return 0;
 +}
 +
 +static int max17042_get_battery_health(struct max17042_chip *chip, int
 +*health) {
 + int temp, vavg, vbatt, ret;
 + u32 val;
 +
 + ret = regmap_read(chip-regmap, MAX17042_AvgVCELL, val);
 + if (ret  0)
 + goto health_error;
 +
 + /* bits [0-3] unused */
 + vavg = val * 625 / 8;
 + /* Convert to millivolts */
 + vavg /= 1000;
 +
 + ret = regmap_read(chip-regmap, MAX17042_VCELL, val);
 + if (ret  0)
 + goto health_error;
 +
 + /* bits [0-3] unused */
 + vbatt = val * 625 / 8;
 + /* Convert to millivolts */
 + vbatt /= 1000;
 +
 + if (vavg  chip-pdata-vmin) {
 + *health = POWER_SUPPLY_HEALTH_DEAD;
 + goto out;
 + }
 +
 + if (vbatt  chip-pdata-vmax + MAX17042_VMAX_TOLERENCE) {
 + *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
 + goto out;
 + }
 +
 + ret = max17042_get_temperature(chip, temp);
 + if (ret  0)
 + goto health_error;
 +
 + if (temp = chip-pdata-temp_min) {
 + *health = POWER_SUPPLY_HEALTH_COLD;
 + goto out;
 + }
 +
 + if (temp = chip-pdata-temp_max) {
 + *health = POWER_SUPPLY_HEALTH_OVERHEAT;
 + goto out;
 + }
 +
 + *health = POWER_SUPPLY_HEALTH_GOOD;
 +
 +out:
 + return 0;
 +
 +health_error:
 + return ret;
 +}
 +
  static int max17042_get_property(struct power_supply *psy,
   enum power_supply_property psp,
   union power_supply_propval *val) @@ -181,19
 +267,34 @@ static int max17042_get_property(struct power_supply *psy,
   val-intval = data * 1000 / 2;
   break;
   case POWER_SUPPLY_PROP_TEMP:
 - ret = regmap_read(map, MAX17042_TEMP, data);
 + ret = max17042_get_temperature(chip, val-intval);
 + if (ret  0)
 + return ret;
 + break;
 + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
 + ret = regmap_read(map, MAX17042_TALRT_Th, data);
 + if (ret  0)
 + return ret;
 + /* LSB is Alert Minimum. In deci-centigrade */
 + val-intval = (data  0xff) * 10;
 + break;
 + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
 + ret = regmap_read(map, MAX17042_TALRT_Th, data);
 + if (ret  0)
 + return ret;
 + /* MSB is Alert Maximum. In deci-centigrade */
 + val-intval = (data  8) * 10;
 + break;
 + case POWER_SUPPLY_PROP_TEMP_MIN:
 + val-intval = chip-pdata-temp_min;
 + break;
 + case POWER_SUPPLY_PROP_TEMP_MAX:
 + val-intval = chip-pdata-temp_max;
 + break;
 + case POWER_SUPPLY_PROP_HEALTH:
 + ret = max17042_get_battery_health(chip, val-intval);
  

RE: [PATCH 1/2] extcon: Use the unique id for external connector instead of string

[Pallala, Ramakrishna]

> This patch uses the unique id to identify the type of external connector 
> instead
> of string name. The string name have the many potential issues. So, this patch
> defines the 'extcon' enumeration which includes all supported external
> connector
> on EXTCON subsystem. If new external connector is necessary, the unique id of
> new connector have to be added in 'extcon' enumeration. There are current
> supported external connector in 'enum extcon' as following:
> 
> enum extcon {
>   EXTCON_NONE = 0x0,  /* NONE */
> 
>   /* USB external connector */
>   EXTCON_USB  = 0x1,  /* USB */
>   EXTCON_USB_HOST = 0x2,  /* USB-HOST */
> 
>   /* Charger external connector */
>   EXTCON_TA   = 0x10, /* TA */
>   EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */
>   EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */
>   EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-
> DOWNSTREAM */
> 
>   /* Audio and video external connector */
>   EXTCON_LINE_IN  = 0x20, /* LINE-IN */
>   EXTCON_LINE_OUT = 0x21, /* LINE-OUT */
>   EXTCON_MICROPHONE   = 0x22, /* MICROPHONE */
>   EXTCON_HEADPHONE= 0x23, /* HEADPHONE */
> 
>   EXTCON_HDMI = 0x30, /* HDMI */
>   EXTCON_MHL  = 0x31, /* MHL */
>   EXTCON_DVI  = 0x32, /* DVI */
>   EXTCON_VGA  = 0x33, /* VGA */
>   EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */
>   EXTCON_SPDIF_OUT= 0x35, /* SPDIF-OUT */
>   EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */
>   EXTCON_VIDEO_OUT= 0x37, /* VIDEO-OUT */
> 
>   /* Miscellaneous external connector */
>   EXTCON_DOCK = 0x50, /* DOCK */
>   EXTCON_JIG  = 0x51, /* JIG */
>   EXTCON_MECHANICAL   = 0x52, /* MECHANICAL */
> 
>   __EXTCON_END,
> };
> 
> For exmaple in extcon-arizoan.c:
> To use unique id removes the potential issue about handling
> the inconsistent name of external connector with string.
> - Previously, use the string to register the type of arizona jack connector
> static const char *arizona_cable[] = {
>   "Mechanical",
>   "Microphone",
>   "Headphone",
>   "Line-out",
> };
> - Newly, use the unique id to register the type of arizona jack connector
> static const enum extcon arizona_cable[] = {
>   EXTCON_MECHANICAL,
>   EXTCON_MICROPHONE,
>   EXTCON_HEADPHONE,
>   EXTCON_LINE_OUT,
> 
>   EXTCON_NONE,
> };
> 
> And this patch modify the prototype of extcon_{get|set}_cable_state_() which
> uses the 'enum extcon id' instead of 'cable_index'. Because although one more
> extcon drivers support USB cable, each extcon driver might has the differnt
> 'cable_index' for USB cable. All extcon drivers can use the unique id number
> for same external connector with modified extcon_{get|set}_cable_state_().
> 
> - Previously, use 'cable_index' on these functions:
> extcon_get_cable_state_(struct extcon_dev*, int cable_index)
> extcon_set_cable_state_(struct extcon_dev*, int cable_index, bool state)
> 
> -Newly, use 'enum extcon id' on these functions:
> extcon_get_cable_state_(struct extcon_dev*, enum extcon id)
> extcon_set_cable_state_(struct extcon_dev*, enum extcon id, bool state)
> 
> Signed-off-by: Chanwoo Choi 
> Cc: MyungJoo Ham 
> Cc: Krzysztof Kozlowski 
> Cc: Charles Keepax 
> Cc: Graeme Gregory 
> Cc: Kishon Vijay Abraham I 
> Cc: Jaewon Kim 
> Cc: Roger Quadros 
> Cc: Ramakrishna Pallala 

For drivers/extcon/extcon-axp288.c
Acked-by: Ramakrishna Pallala 

Thanks,
Ram

> ---
>  drivers/extcon/extcon-arizona.c|  38 +++-
>  drivers/extcon/extcon-axp288.c |  24 ++---
>  drivers/extcon/extcon-max14577.c   |  45 -
>  drivers/extcon/extcon-max77693.c   |  95 +--
>  drivers/extcon/extcon-max77843.c   |  56 +--
>  drivers/extcon/extcon-max8997.c|  59 +---
>  drivers/extcon/extcon-palmas.c |  22 +++--
>  drivers/extcon/extcon-rt8973a.c|  40 +++-
>  drivers/extcon/extcon-sm5502.c |  32 ++-
>  drivers/extcon/extcon-usb-gpio.c   |  32 ++-
>  drivers/extcon/extcon.c| 166 
> -
>  include/linux/extcon.h |  92 +-
>  include/linux/extcon/extcon-adc-jack.h |   5 +-
>  13 files changed, 326 insertions(+), 380 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 1ec06b4..9262b45 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -118,17 +118,12 @@ static const int arizona_micd_levels[] = {
>   1257,
>  };
> 
> -#define ARIZONA_CABLE_MECHANICAL 0
> -#define ARIZONA_CABLE_MICROPHONE 1
> -#define ARIZONA_CABLE_HEADPHONE  2
> -#define ARIZONA_CABLE_LINEOUT3
> -
> -static const char *arizona_cable[] = {
> - "Mechanical",
> - "Microphone",
> - 

RE: [PATCH 1/2] extcon: Use the unique id for external connector instead of string

[Pallala, Ramakrishna]

 This patch uses the unique id to identify the type of external connector 
 instead
 of string name. The string name have the many potential issues. So, this patch
 defines the 'extcon' enumeration which includes all supported external
 connector
 on EXTCON subsystem. If new external connector is necessary, the unique id of
 new connector have to be added in 'extcon' enumeration. There are current
 supported external connector in 'enum extcon' as following:
 
 enum extcon {
   EXTCON_NONE = 0x0,  /* NONE */
 
   /* USB external connector */
   EXTCON_USB  = 0x1,  /* USB */
   EXTCON_USB_HOST = 0x2,  /* USB-HOST */
 
   /* Charger external connector */
   EXTCON_TA   = 0x10, /* TA */
   EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */
   EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */
   EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-
 DOWNSTREAM */
 
   /* Audio and video external connector */
   EXTCON_LINE_IN  = 0x20, /* LINE-IN */
   EXTCON_LINE_OUT = 0x21, /* LINE-OUT */
   EXTCON_MICROPHONE   = 0x22, /* MICROPHONE */
   EXTCON_HEADPHONE= 0x23, /* HEADPHONE */
 
   EXTCON_HDMI = 0x30, /* HDMI */
   EXTCON_MHL  = 0x31, /* MHL */
   EXTCON_DVI  = 0x32, /* DVI */
   EXTCON_VGA  = 0x33, /* VGA */
   EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */
   EXTCON_SPDIF_OUT= 0x35, /* SPDIF-OUT */
   EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */
   EXTCON_VIDEO_OUT= 0x37, /* VIDEO-OUT */
 
   /* Miscellaneous external connector */
   EXTCON_DOCK = 0x50, /* DOCK */
   EXTCON_JIG  = 0x51, /* JIG */
   EXTCON_MECHANICAL   = 0x52, /* MECHANICAL */
 
   __EXTCON_END,
 };
 
 For exmaple in extcon-arizoan.c:
 To use unique id removes the potential issue about handling
 the inconsistent name of external connector with string.
 - Previously, use the string to register the type of arizona jack connector
 static const char *arizona_cable[] = {
   Mechanical,
   Microphone,
   Headphone,
   Line-out,
 };
 - Newly, use the unique id to register the type of arizona jack connector
 static const enum extcon arizona_cable[] = {
   EXTCON_MECHANICAL,
   EXTCON_MICROPHONE,
   EXTCON_HEADPHONE,
   EXTCON_LINE_OUT,
 
   EXTCON_NONE,
 };
 
 And this patch modify the prototype of extcon_{get|set}_cable_state_() which
 uses the 'enum extcon id' instead of 'cable_index'. Because although one more
 extcon drivers support USB cable, each extcon driver might has the differnt
 'cable_index' for USB cable. All extcon drivers can use the unique id number
 for same external connector with modified extcon_{get|set}_cable_state_().
 
 - Previously, use 'cable_index' on these functions:
 extcon_get_cable_state_(struct extcon_dev*, int cable_index)
 extcon_set_cable_state_(struct extcon_dev*, int cable_index, bool state)
 
 -Newly, use 'enum extcon id' on these functions:
 extcon_get_cable_state_(struct extcon_dev*, enum extcon id)
 extcon_set_cable_state_(struct extcon_dev*, enum extcon id, bool state)
 
 Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
 Cc: MyungJoo Ham myungjoo@samsung.com
 Cc: Krzysztof Kozlowski k.kozlow...@samsung.com
 Cc: Charles Keepax ckee...@opensource.wolfsonmicro.com
 Cc: Graeme Gregory g...@slimlogic.co.uk
 Cc: Kishon Vijay Abraham I kis...@ti.com
 Cc: Jaewon Kim jaewon02@samsung.com
 Cc: Roger Quadros rog...@ti.com
 Cc: Ramakrishna Pallala ramakrishna.pall...@intel.com

For drivers/extcon/extcon-axp288.c
Acked-by: Ramakrishna Pallala ramakrishna.pall...@intel.com

Thanks,
Ram

 ---
  drivers/extcon/extcon-arizona.c|  38 +++-
  drivers/extcon/extcon-axp288.c |  24 ++---
  drivers/extcon/extcon-max14577.c   |  45 -
  drivers/extcon/extcon-max77693.c   |  95 +--
  drivers/extcon/extcon-max77843.c   |  56 +--
  drivers/extcon/extcon-max8997.c|  59 +---
  drivers/extcon/extcon-palmas.c |  22 +++--
  drivers/extcon/extcon-rt8973a.c|  40 +++-
  drivers/extcon/extcon-sm5502.c |  32 ++-
  drivers/extcon/extcon-usb-gpio.c   |  32 ++-
  drivers/extcon/extcon.c| 166 
 -
  include/linux/extcon.h |  92 +-
  include/linux/extcon/extcon-adc-jack.h |   5 +-
  13 files changed, 326 insertions(+), 380 deletions(-)
 
 diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
 index 1ec06b4..9262b45 100644
 --- a/drivers/extcon/extcon-arizona.c
 +++ b/drivers/extcon/extcon-arizona.c
 @@ -118,17 +118,12 @@ static const int arizona_micd_levels[] = {
   1257,
  };
 
 -#define ARIZONA_CABLE_MECHANICAL 0
 -#define ARIZONA_CABLE_MICROPHONE 1
 -#define ARIZONA_CABLE_HEADPHONE  2
 -#define ARIZONA_CABLE_LINEOUT3

RE: [PATCH] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi,

> W dniu 05.05.2015 o 07:18, Ramakrishna Pallala pisze:
> > This patch adds the support for following battery properties to
> > max17042 fuel gauge driver.
> 
> The patchset itself looks good. Only minor nits and a question at the end.
> 
> >
> > POWER_SUPPLY_PROP_TEMP_ALERT_MIN
> > POWER_SUPPLY_PROP_TEMP_ALERT_MAX
> > POWER_SUPPLY_PROP_TEMP_MIN
> > POWER_SUPPLY_PROP_TEMP_MAX
> > POWER_SUPPLY_PROP_HEALTH
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/power/max17042_battery.c   |  190
> ++--
> >  include/linux/power/max17042_battery.h |4 +
> >  2 files changed, 183 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/power/max17042_battery.c
> > b/drivers/power/max17042_battery.c
> > index 6cc5e87..d8f15ce 100644
> > --- a/drivers/power/max17042_battery.c
> > +++ b/drivers/power/max17042_battery.c
> > @@ -63,6 +63,8 @@
> >  #define dP_ACC_100 0x1900
> >  #define dP_ACC_200 0x3200
> >
> > +#define MAX17042_VMAX_TOLERENCE50 /* 50 mV */
> 
> s/TOLERENCE/TOLERANCE/
Ok..

[snip]

> > +
> > +static int max17042_get_battery_health(struct max17042_chip *chip,
> > +int *health) {
> > +   int temp, vavg, vbatt, ret;
> > +   u32 val;
> > +
> > +   ret = regmap_read(chip->regmap, MAX17042_AvgVCELL, );
> > +   if (ret < 0)
> > +   goto health_error;
> > +
> > +   /* bits [0-3] unused */
> > +   vavg = val * 625 / 8;
> > +   /* Convert to milli volts */
> 
> s/milli volts/millivolts/
Ok..

[snip]

> > @@ -665,6 +829,8 @@ static const struct power_supply_desc
> max17042_psy_desc = {
> > .name   = "max170xx_battery",
> > .type   = POWER_SUPPLY_TYPE_BATTERY,
> > .get_property   = max17042_get_property,
> > +   .set_property   = max17042_set_property,
> > +   .property_is_writeable  = max17042_property_is_writeable,
> > .properties = max17042_battery_props,
> > .num_properties = ARRAY_SIZE(max17042_battery_props),
> >  };
> > @@ -673,6 +839,8 @@ static const struct power_supply_desc
> max17042_no_current_sense_psy_desc = {
> > .name   = "max170xx_battery",
> > .type   = POWER_SUPPLY_TYPE_BATTERY,
> > .get_property   = max17042_get_property,
> > +   .set_property   = max17042_set_property,
> > +   .property_is_writeable  = max17042_property_is_writeable,
> > .properties = max17042_battery_props,
> > .num_properties = ARRAY_SIZE(max17042_battery_props) - 2,
> >  };
> > diff --git a/include/linux/power/max17042_battery.h
> > b/include/linux/power/max17042_battery.h
> > index cf112b4..89ca4a8 100644
> > --- a/include/linux/power/max17042_battery.h
> > +++ b/include/linux/power/max17042_battery.h
> > @@ -215,6 +215,10 @@ struct max17042_platform_data {
> >  * the datasheet although it can be changed by board designers.
> >  */
> > unsigned int r_sns;
> > +   int vmin;   /* in milli volts */
> > +   int vmax;   /* in milli volts */
> 
> s/milli volts/millivolts/
Ok..

> > +   int temp_min;   /* in tenths of degree Celsius */
> > +   int temp_max;   /* in tenths of degree Celsius */
> >  };
> >
> >  #endif /* __MAX17042_BATTERY_H_ */
> 
> The question is who will set these values in pdata? We do not have board files
> anymore so I think you should extend the DT bindings so this could be used.

In our platform we don't use device tree...the enumeration is based on SFI 
model and
there is board file in our platform.

To complete the platforms which use device tree, I can add 
of_property_read_u32() calls for
new pdata variables and submit the patch.

Thanks,
Ram




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] power: max17042_battery: add HEALTH and TEMP_* properties support

[Pallala, Ramakrishna]

Hi,

 W dniu 05.05.2015 o 07:18, Ramakrishna Pallala pisze:
  This patch adds the support for following battery properties to
  max17042 fuel gauge driver.
 
 The patchset itself looks good. Only minor nits and a question at the end.
 
 
  POWER_SUPPLY_PROP_TEMP_ALERT_MIN
  POWER_SUPPLY_PROP_TEMP_ALERT_MAX
  POWER_SUPPLY_PROP_TEMP_MIN
  POWER_SUPPLY_PROP_TEMP_MAX
  POWER_SUPPLY_PROP_HEALTH
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/power/max17042_battery.c   |  190
 ++--
   include/linux/power/max17042_battery.h |4 +
   2 files changed, 183 insertions(+), 11 deletions(-)
 
  diff --git a/drivers/power/max17042_battery.c
  b/drivers/power/max17042_battery.c
  index 6cc5e87..d8f15ce 100644
  --- a/drivers/power/max17042_battery.c
  +++ b/drivers/power/max17042_battery.c
  @@ -63,6 +63,8 @@
   #define dP_ACC_100 0x1900
   #define dP_ACC_200 0x3200
 
  +#define MAX17042_VMAX_TOLERENCE50 /* 50 mV */
 
 s/TOLERENCE/TOLERANCE/
Ok..

[snip]

  +
  +static int max17042_get_battery_health(struct max17042_chip *chip,
  +int *health) {
  +   int temp, vavg, vbatt, ret;
  +   u32 val;
  +
  +   ret = regmap_read(chip-regmap, MAX17042_AvgVCELL, val);
  +   if (ret  0)
  +   goto health_error;
  +
  +   /* bits [0-3] unused */
  +   vavg = val * 625 / 8;
  +   /* Convert to milli volts */
 
 s/milli volts/millivolts/
Ok..

[snip]

  @@ -665,6 +829,8 @@ static const struct power_supply_desc
 max17042_psy_desc = {
  .name   = max170xx_battery,
  .type   = POWER_SUPPLY_TYPE_BATTERY,
  .get_property   = max17042_get_property,
  +   .set_property   = max17042_set_property,
  +   .property_is_writeable  = max17042_property_is_writeable,
  .properties = max17042_battery_props,
  .num_properties = ARRAY_SIZE(max17042_battery_props),
   };
  @@ -673,6 +839,8 @@ static const struct power_supply_desc
 max17042_no_current_sense_psy_desc = {
  .name   = max170xx_battery,
  .type   = POWER_SUPPLY_TYPE_BATTERY,
  .get_property   = max17042_get_property,
  +   .set_property   = max17042_set_property,
  +   .property_is_writeable  = max17042_property_is_writeable,
  .properties = max17042_battery_props,
  .num_properties = ARRAY_SIZE(max17042_battery_props) - 2,
   };
  diff --git a/include/linux/power/max17042_battery.h
  b/include/linux/power/max17042_battery.h
  index cf112b4..89ca4a8 100644
  --- a/include/linux/power/max17042_battery.h
  +++ b/include/linux/power/max17042_battery.h
  @@ -215,6 +215,10 @@ struct max17042_platform_data {
   * the datasheet although it can be changed by board designers.
   */
  unsigned int r_sns;
  +   int vmin;   /* in milli volts */
  +   int vmax;   /* in milli volts */
 
 s/milli volts/millivolts/
Ok..

  +   int temp_min;   /* in tenths of degree Celsius */
  +   int temp_max;   /* in tenths of degree Celsius */
   };
 
   #endif /* __MAX17042_BATTERY_H_ */
 
 The question is who will set these values in pdata? We do not have board files
 anymore so I think you should extend the DT bindings so this could be used.

In our platform we don't use device tree...the enumeration is based on SFI 
model and
there is board file in our platform.

To complete the platforms which use device tree, I can add 
of_property_read_u32() calls for
new pdata variables and submit the patch.

Thanks,
Ram




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Thanks you. I will add linux-pm also in CC list going forward.

> On Wed, Apr 29, 2015 at 06:09:29AM +0000, Pallala, Ramakrishna wrote:
> > It's been almost 45 days since I submitted this patch and I did not
> > receive any feedback from you.
> > Can you take a look at this patch now?
> 
> Right, I forgot about this and it was sorted into the wrong mailing box, since
> linux-pm was not CC'd. I queued this into fixes for 4.1:
> 
> http://git.infradead.org/battery-
> 2.6.git/commit/409e718e09885de78cebcb57f0f908eaed5304df

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v9] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> Hi Ram,
> 
> This patch has still one minor issue on below comment.
> But I fix it and will apply it on extcon-next branch after discussing how to 
> apply
> this patch with Lee Jones (MFD maintainer).
Ok. Thanks.

> Dear Lee,
> Do you want to send pull request after I make the immutable branch for this
> patch? or apply this patch on extcon git without any pull request?
> 
> Thanks,
> Chanwoo Choi
> 
> On 05/01/2015 12:14 AM, Ramakrishna Pallala wrote:
> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > Acked-by: Lee Jones 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  385
> 
> >  include/linux/mfd/axp20x.h |5 +
> >  4 files changed, 398 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> 
> [snip]
> 
> > +   vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
> > +   :
> EXTCON_GPIO_MUX_SEL_PMIC);
> > +
> > +   atomic_notifier_call_chain(>otg->notifier,
> > +   vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE,
> NULL);
> > +   }
> > +
> > +   if (notify_charger)
> > +   extcon_set_cable_state(info->edev, cable, vbus_attach);
> > +
> > +   /* Clear the flags on disconnect event */
> > +   if (!vbus_attach)
> > +   notify_otg = notify_charger = false;
> > +
> > +   return 0;
> > +
> > +dev_det_ret:
> > +   if (ret < 0)
> > +   dev_err(info->dev, "BC Mod detection error\n");
> 
> You miss the fix about following comment.
> "BC Mod detection error\n" -> "failed to detect BC Mod\n"

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v8] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

I think I missed some. I will fix them and submit the v9 patch.

> Hi Ram,
> 
> I added some comment.
> If you fix minor issue according to comment, I'll apply it.
> 
> On 04/30/2015 10:43 AM, Ramakrishna Pallala wrote:
> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  386
> 
> >  include/linux/mfd/axp20x.h |5 +
> >  4 files changed, 399 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> 
> [snip]
> 
> > +static int axp288_handle_chrg_det_event(struct axp288_extcon_info
> > +*info) {
> > +   static bool notify_otg, notify_charger;
> > +   static char *cable;
> > +   int ret, stat, cfg, pwr_stat;
> > +   u8 chrg_type;
> > +   bool vbus_attach = false;
> > +
> > +   ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, _stat);
> > +   if (ret < 0) {
> > +   dev_err(info->dev, "failed to read vbus status\n");
> > +   return ret;
> > +   }
> > +
> > +   vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> > +   if (!vbus_attach)
> > +   goto notify_otg;
> > +
> > +   /* Check charger detection completion status */
> > +   ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, );
> > +   if (ret < 0)
> > +   goto dev_det_ret;
> > +   if (cfg & BC_GLOBAL_DET_STAT) {
> > +   dev_dbg(info->dev, "can't complete the charger detection\n");
> > +   goto dev_det_ret;
> > +   }
> > +
> > +   ret = regmap_read(info->regmap, AXP288_BC_DET_STAT_REG, );
> > +   if (ret < 0)
> > +   goto dev_det_ret;
> > +   dev_dbg(info->dev, "status:%x, config:%x\n", stat, cfg);
> 
> As I said on previous reply, it is not necessary about just register value 
> without
> appropriate log message.
Ok...

> 
> > +
> > +   chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
> > +
> > +   switch (chrg_type) {
> > +   case DET_STAT_SDP:
> > +   dev_dbg(info->dev, "sdp cable is connecetd\n");
> > +   notify_otg = true;
> > +   notify_charger = true;
> > +   cable = AXP288_EXTCON_SLOW_CHARGER;
> > +   break;
> > +   case DET_STAT_CDP:
> > +   dev_dbg(info->dev, "cdp cable is connecetd\n");
> > +   notify_otg = true;
> > +   notify_charger = true;
> > +   cable = AXP288_EXTCON_DOWNSTREAM_CHARGER;
> > +   break;
> > +   case DET_STAT_DCP:
> > +   dev_dbg(info->dev, "dcp cable is connecetd\n");
> > +   notify_charger = true;
> > +   cable = AXP288_EXTCON_FAST_CHARGER;
> > +   break;
> > +   default:
> > +   dev_warn(info->dev,
> > +   "disconnect or unknown or ID event\n");
> > +   }
> > +
> > +notify_otg:
> > +   if (notify_otg) {
> > +   /*
> > +* If VBUS is absent Connect D+/D- lines to PMIC for BC
> > +* detection. Else connect them to SOC for USB communication.
> > +*/
> > +   if (info->pdata->gpio_mux_cntl)
> > +   gpiod_set_value(info->pdata->gpio_mux_cntl,
> > +   vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
> > +   :
> EXTCON_GPIO_MUX_SEL_PMIC);
> > +
> > +   atomic_notifier_call_chain(>otg->notifier,
> > +   vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE,
> NULL);
> > +   }
> > +
> > +   if (notify_charger)
> > +   extcon_set_cable_state(info->edev, cable, vbus_attach);
> > +
> > +   /* Clear the flags on disconnect event */
> > +   if (!vbus_attach)
> > +   notify_otg = notify_charger = false;
> > +
> > +   return 0;
> > +
> > +dev_det_ret:
> > +   if (ret < 0)
> > +   dev_err(info->dev, "BC Mod detection error\n");
> 
> "BC Mod detection error\n" -> "failed to detect BC Mod\n"
> 
> > +
> > +   return ret;
> > +}
> > +
> > +static irqreturn_t axp288_extcon_isr(int irq, void *data) {
> > +   struct axp288_extcon_info *info = data;
> > +   int ret;
> > +
> > +   ret = axp288_handle_chrg_det_event(info);
> > +   if (ret < 0)
> > +   dev_err(info->dev, "error in PWRSRC INT handling\n");
> 
> As I saied on previous reply, you better to modify log message as following:
>   "failed to handle the interrupt\n"
Ok.

> 
> > +
> > +   return IRQ_HANDLED;
> > +}
> > +
> > +static void axp288_extcon_enable_irq(struct axp288_extcon_info *info)
> > +{
> > +   /* Unmask VBUS interrupt */
> > +   regmap_write(info->regmap, AXP288_PWRSRC_IRQ_CFG_REG,
> > +   PWRSRC_IRQ_CFG_MASK);
> > +   regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
> > +   BC_GLOBAL_RUN, 0);
> > +   /* Unmask the BC1.2 complete 

RE: [PATCH v9] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

 Hi Ram,
 
 This patch has still one minor issue on below comment.
 But I fix it and will apply it on extcon-next branch after discussing how to 
 apply
 this patch with Lee Jones (MFD maintainer).
Ok. Thanks.

 Dear Lee,
 Do you want to send pull request after I make the immutable branch for this
 patch? or apply this patch on extcon git without any pull request?
 
 Thanks,
 Chanwoo Choi
 
 On 05/01/2015 12:14 AM, Ramakrishna Pallala wrote:
  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  Acked-by: Lee Jones lee.jo...@linaro.org
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  385
 
   include/linux/mfd/axp20x.h |5 +
   4 files changed, 398 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
 
 [snip]
 
  +   vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
  +   :
 EXTCON_GPIO_MUX_SEL_PMIC);
  +
  +   atomic_notifier_call_chain(info-otg-notifier,
  +   vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE,
 NULL);
  +   }
  +
  +   if (notify_charger)
  +   extcon_set_cable_state(info-edev, cable, vbus_attach);
  +
  +   /* Clear the flags on disconnect event */
  +   if (!vbus_attach)
  +   notify_otg = notify_charger = false;
  +
  +   return 0;
  +
  +dev_det_ret:
  +   if (ret  0)
  +   dev_err(info-dev, BC Mod detection error\n);
 
 You miss the fix about following comment.
 BC Mod detection error\n - failed to detect BC Mod\n

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v8] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

I think I missed some. I will fix them and submit the v9 patch.

 Hi Ram,
 
 I added some comment.
 If you fix minor issue according to comment, I'll apply it.
 
 On 04/30/2015 10:43 AM, Ramakrishna Pallala wrote:
  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  386
 
   include/linux/mfd/axp20x.h |5 +
   4 files changed, 399 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
 [snip]
 
  +static int axp288_handle_chrg_det_event(struct axp288_extcon_info
  +*info) {
  +   static bool notify_otg, notify_charger;
  +   static char *cable;
  +   int ret, stat, cfg, pwr_stat;
  +   u8 chrg_type;
  +   bool vbus_attach = false;
  +
  +   ret = regmap_read(info-regmap, AXP288_PS_STAT_REG, pwr_stat);
  +   if (ret  0) {
  +   dev_err(info-dev, failed to read vbus status\n);
  +   return ret;
  +   }
  +
  +   vbus_attach = (pwr_stat  PS_STAT_VBUS_PRESENT);
  +   if (!vbus_attach)
  +   goto notify_otg;
  +
  +   /* Check charger detection completion status */
  +   ret = regmap_read(info-regmap, AXP288_BC_GLOBAL_REG, cfg);
  +   if (ret  0)
  +   goto dev_det_ret;
  +   if (cfg  BC_GLOBAL_DET_STAT) {
  +   dev_dbg(info-dev, can't complete the charger detection\n);
  +   goto dev_det_ret;
  +   }
  +
  +   ret = regmap_read(info-regmap, AXP288_BC_DET_STAT_REG, stat);
  +   if (ret  0)
  +   goto dev_det_ret;
  +   dev_dbg(info-dev, status:%x, config:%x\n, stat, cfg);
 
 As I said on previous reply, it is not necessary about just register value 
 without
 appropriate log message.
Ok...

 
  +
  +   chrg_type = (stat  DET_STAT_MASK)  DET_STAT_SHIFT;
  +
  +   switch (chrg_type) {
  +   case DET_STAT_SDP:
  +   dev_dbg(info-dev, sdp cable is connecetd\n);
  +   notify_otg = true;
  +   notify_charger = true;
  +   cable = AXP288_EXTCON_SLOW_CHARGER;
  +   break;
  +   case DET_STAT_CDP:
  +   dev_dbg(info-dev, cdp cable is connecetd\n);
  +   notify_otg = true;
  +   notify_charger = true;
  +   cable = AXP288_EXTCON_DOWNSTREAM_CHARGER;
  +   break;
  +   case DET_STAT_DCP:
  +   dev_dbg(info-dev, dcp cable is connecetd\n);
  +   notify_charger = true;
  +   cable = AXP288_EXTCON_FAST_CHARGER;
  +   break;
  +   default:
  +   dev_warn(info-dev,
  +   disconnect or unknown or ID event\n);
  +   }
  +
  +notify_otg:
  +   if (notify_otg) {
  +   /*
  +* If VBUS is absent Connect D+/D- lines to PMIC for BC
  +* detection. Else connect them to SOC for USB communication.
  +*/
  +   if (info-pdata-gpio_mux_cntl)
  +   gpiod_set_value(info-pdata-gpio_mux_cntl,
  +   vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
  +   :
 EXTCON_GPIO_MUX_SEL_PMIC);
  +
  +   atomic_notifier_call_chain(info-otg-notifier,
  +   vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE,
 NULL);
  +   }
  +
  +   if (notify_charger)
  +   extcon_set_cable_state(info-edev, cable, vbus_attach);
  +
  +   /* Clear the flags on disconnect event */
  +   if (!vbus_attach)
  +   notify_otg = notify_charger = false;
  +
  +   return 0;
  +
  +dev_det_ret:
  +   if (ret  0)
  +   dev_err(info-dev, BC Mod detection error\n);
 
 BC Mod detection error\n - failed to detect BC Mod\n
 
  +
  +   return ret;
  +}
  +
  +static irqreturn_t axp288_extcon_isr(int irq, void *data) {
  +   struct axp288_extcon_info *info = data;
  +   int ret;
  +
  +   ret = axp288_handle_chrg_det_event(info);
  +   if (ret  0)
  +   dev_err(info-dev, error in PWRSRC INT handling\n);
 
 As I saied on previous reply, you better to modify log message as following:
   failed to handle the interrupt\n
Ok.

 
  +
  +   return IRQ_HANDLED;
  +}
  +
  +static void axp288_extcon_enable_irq(struct axp288_extcon_info *info)
  +{
  +   /* Unmask VBUS interrupt */
  +   regmap_write(info-regmap, AXP288_PWRSRC_IRQ_CFG_REG,
  +   PWRSRC_IRQ_CFG_MASK);
  +   regmap_update_bits(info-regmap, AXP288_BC_GLOBAL_REG,
  +   BC_GLOBAL_RUN, 0);
  +   /* Unmask the BC1.2 complete interrupts */
  +   regmap_write(info-regmap, AXP288_BC12_IRQ_CFG_REG,
 BC12_IRQ_CFG_MASK);
  +   /* Enable the charger detection logic */
  +   regmap_update_bits(info-regmap, AXP288_BC_GLOBAL_REG,
  +   BC_GLOBAL_RUN, BC_GLOBAL_RUN);
  +}
 

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Thanks you. I will add linux-pm also in CC list going forward.

 On Wed, Apr 29, 2015 at 06:09:29AM +, Pallala, Ramakrishna wrote:
  It's been almost 45 days since I submitted this patch and I did not
  receive any feedback from you.
  Can you take a look at this patch now?
 
 Right, I forgot about this and it was sorted into the wrong mailing box, since
 linux-pm was not CC'd. I queued this into fixes for 4.1:
 
 http://git.infradead.org/battery-
 2.6.git/commit/409e718e09885de78cebcb57f0f908eaed5304df

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v8] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> 
> When you send next versions, the convention is for you to apply Acks that
> you've collected.

Ok..
Do you want me to resubmit the patch?


> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  386
> 
> >  include/linux/mfd/axp20x.h |5 +
> >  4 files changed, 399 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > fdc0bf0..2305edc 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> >   with Wolfson Arizona devices. These are audio CODECs with
> >   advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > +   tristate "X-Power AXP288 EXTCON support"
> > +   depends on MFD_AXP20X && USB_PHY
> > +   help
> > + Say Y here to enable support for USB peripheral detection
> > + and USB MUX switching by X-Power AXP288 PMIC.
> > +
> >  config EXTCON_GPIO
> > tristate "GPIO extcon support"
> > depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 9204114..ba787d0 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_EXTCON)   += extcon.o
> >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> >  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
> >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
> >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 000..ea61bf2
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,386 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + * Author: Ramakrishna Pallala 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Power source status register */
> > +#define PS_STAT_VBUS_TRIGGER   BIT(0)
> > +#define PS_STAT_BAT_CHRG_DIR   BIT(2)
> > +#define PS_STAT_VBUS_ABOVE_VHOLD   BIT(3)
> > +#define PS_STAT_VBUS_VALID BIT(4)
> > +#define PS_STAT_VBUS_PRESENT   BIT(5)
> > +
> > +/* BC module global register */
> > +#define BC_GLOBAL_RUN  BIT(0)
> > +#define BC_GLOBAL_DET_STAT BIT(2)
> > +#define BC_GLOBAL_DBP_TOUT BIT(3)
> > +#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
> > +#define BC_GLOBAL_DCD_TOUT_MASK(BIT(6)|BIT(5))
> > +#define BC_GLOBAL_DCD_TOUT_300MS   0
> > +#define BC_GLOBAL_DCD_TOUT_100MS   1
> > +#define BC_GLOBAL_DCD_TOUT_500MS   2
> > +#define BC_GLOBAL_DCD_TOUT_900MS   3
> > +#define BC_GLOBAL_DCD_DET_SEL  BIT(7)
> > +
> > +/* BC module vbus control and status register */
> > +#define VBUS_CNTL_DPDM_PD_EN   BIT(4)
> > +#define VBUS_CNTL_DPDM_FD_EN   BIT(5)
> > +#define VBUS_CNTL_FIRST_PO_STATBIT(6)
> > +
> > +/* BC USB status register */
> > +#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
> > +#define USB_STAT_BUS_STAT_SHIFT0
> > +#define USB_STAT_BUS_STAT_ATHD 0
> > +#define USB_STAT_BUS_STAT_CONN 1
> > +#define USB_STAT_BUS_STAT_SUSP 2
> > +#define USB_STAT_BUS_STAT_CONF 3
> > +#define USB_STAT_USB_SS_MODE   BIT(4)
> > +#define USB_STAT_DEAD_BAT_DET  BIT(6)
> > +#define USB_STAT_DBP_UNCFG BIT(7)
> > +
> > +/* BC detect status register */
> > +#define DET_STAT_MASK  (BIT(7)|BIT(6)|BIT(5))
> > +#define DET_STAT_SHIFT 5
> > +#define DET_STAT_SDP   1
> > +#define DET_STAT_CDP   2
> > +#define DET_STAT_DCP   

RE: [PATCH v7] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Thank you for the comments. I will be fixing them and submit the v8 patch.

> On Wed, Apr 29, 2015 at 11:22 AM, Ramakrishna Pallala
>  wrote:
> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  403
> 
> >  include/linux/mfd/axp20x.h |5 +
> >  4 files changed, 416 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > fdc0bf0..2305edc 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> >   with Wolfson Arizona devices. These are audio CODECs with
> >   advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > +   tristate "X-Power AXP288 EXTCON support"
> > +   depends on MFD_AXP20X && USB_PHY
> > +   help
> > + Say Y here to enable support for USB peripheral detection
> > + and USB MUX switching by X-Power AXP288 PMIC.
> > +
> >  config EXTCON_GPIO
> > tristate "GPIO extcon support"
> > depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 9204114..ba787d0 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_EXTCON)   += extcon.o
> >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> >  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
> >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
> >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o diff --git
> > a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c new
> > file mode 100644 index 000..7d72d44
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,403 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> 
> 2014 -> 2015
Ok

> > + * Decode and log the given "reset source indicator" (rsi)
> > + * register and then clear it.
> > + */
> > +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) {
> > +   char **rsi;
> > +   unsigned int val, i, clear_mask = 0;
> > +   int ret;
> > +
> > +   ret = regmap_read(info->regmap, AXP288_PS_BOOT_REASON_REG,
> );
> > +   for (i = 0, rsi = axp288_pwr_up_down_info; *rsi; rsi++, i++) {
> > +   if (val & BIT(i)) {
> > +   dev_dbg(info->dev, "%s\n", *rsi);
> > +   clear_mask |= BIT(i);
> > +   }
> > +   }
> > +
> > +   /* Clear the register value for next reboot (write 1 to clear bit) 
> > */
> > +   regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG,
> > +clear_mask); }
> > +
> > +static int handle_chrg_det_event(struct axp288_extcon_info *info)
> 
> Add 'axp288' prefix to function name as following:
> - handle_chrg_det_event -> axp288_handle_chrg_det_event
Ok

> 
> > +{
> > +   static bool notify_otg, notify_charger;
> > +   static char *cable;
> > +   int ret, stat, cfg, pwr_stat;
> > +   u8 chrg_type;
> > +   bool vbus_attach = false;
> > +
> > +   ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, _stat);
> > +   if (ret < 0) {
> > +   dev_err(info->dev, "vbus status read error\n");
> 
> I recommend that you use the consistency log pattern as following:
> "failed to read vbus status\n"
> 
Ok

> > +   return ret;
> > +   }
> > +
> > +   vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> > +   if (vbus_attach) {
> > +   dev_dbg(info->dev, "vbus present\n");
> > +   } else {
> > +   dev_dbg(info->dev, "vbus not present\n");
> > +   goto notify_otg;
> > +   }
> 
> I think that you can simplify upper if/else statement as following without any
> unneeded debug log.
>  if (!vbus_attach) {
>  goto notify_otg;
Ok.


> > +
> > +   /* Check charger detection completion status */
> > +   ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, );
> > +   if (ret < 0)
> > +   goto dev_det_ret;
> > +   if (cfg & BC_GLOBAL_DET_STAT) {
> > +   dev_dbg(info->dev, "charger detection not
> > + complete!!\n");
> 
> ditto. Don't need to add '!' at the end of log message.
> "charger detection not complete!!\n" -> "can't complete the charger
> detection\n"
Ok.

> 
> > +   goto dev_det_ret;
> > +   }
> > +
> > +   ret = regmap_read(info->regmap, 

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Hi Sebastian,

It's been almost 45 days since I submitted this patch and I did not receive any
feedback from you.

Can you take a look at this patch now?
 
> On Fri, Mar 13, 2015 at 09:49:09PM +0530, Ramakrishna Pallala wrote:
> > Add the original author details of the axp288_fuel_gauge driver.
> >
> Acked-by: Todd Brandt 
> 
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/power/axp288_fuel_gauge.c |1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/power/axp288_fuel_gauge.c
> > b/drivers/power/axp288_fuel_gauge.c
> > index c86e709..fc38979 100644
> > --- a/drivers/power/axp288_fuel_gauge.c
> > +++ b/drivers/power/axp288_fuel_gauge.c
> > @@ -1146,6 +1146,7 @@ static struct platform_driver
> > axp288_fuel_gauge_driver = {
> >
> >  module_platform_driver(axp288_fuel_gauge_driver);
> >
> > +MODULE_AUTHOR("Ramakrishna Pallala ");
> >  MODULE_AUTHOR("Todd Brandt ");
> >  MODULE_DESCRIPTION("Xpower AXP288 Fuel Gauge Driver");
> > MODULE_LICENSE("GPL");

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Hi Sebastian,

It's been almost 45 days since I submitted this patch and I did not receive any
feedback from you.

Can you take a look at this patch now?
 
 On Fri, Mar 13, 2015 at 09:49:09PM +0530, Ramakrishna Pallala wrote:
  Add the original author details of the axp288_fuel_gauge driver.
 
 Acked-by: Todd Brandt todd.e.bra...@linux.intel.com
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/power/axp288_fuel_gauge.c |1 +
   1 file changed, 1 insertion(+)
 
  diff --git a/drivers/power/axp288_fuel_gauge.c
  b/drivers/power/axp288_fuel_gauge.c
  index c86e709..fc38979 100644
  --- a/drivers/power/axp288_fuel_gauge.c
  +++ b/drivers/power/axp288_fuel_gauge.c
  @@ -1146,6 +1146,7 @@ static struct platform_driver
  axp288_fuel_gauge_driver = {
 
   module_platform_driver(axp288_fuel_gauge_driver);
 
  +MODULE_AUTHOR(Ramakrishna Pallala ramakrishna.pall...@intel.com);
   MODULE_AUTHOR(Todd Brandt todd.e.bra...@linux.intel.com);
   MODULE_DESCRIPTION(Xpower AXP288 Fuel Gauge Driver);
  MODULE_LICENSE(GPL);

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v7] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Thank you for the comments. I will be fixing them and submit the v8 patch.

 On Wed, Apr 29, 2015 at 11:22 AM, Ramakrishna Pallala
 ramakrishna.pall...@intel.com wrote:
  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  403
 
   include/linux/mfd/axp20x.h |5 +
   4 files changed, 416 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
  diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
  fdc0bf0..2305edc 100644
  --- a/drivers/extcon/Kconfig
  +++ b/drivers/extcon/Kconfig
  @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.
 
  +config EXTCON_AXP288
  +   tristate X-Power AXP288 EXTCON support
  +   depends on MFD_AXP20X  USB_PHY
  +   help
  + Say Y here to enable support for USB peripheral detection
  + and USB MUX switching by X-Power AXP288 PMIC.
  +
   config EXTCON_GPIO
  tristate GPIO extcon support
  depends on GPIOLIB
  diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
  9204114..ba787d0 100644
  --- a/drivers/extcon/Makefile
  +++ b/drivers/extcon/Makefile
  @@ -5,6 +5,7 @@
   obj-$(CONFIG_EXTCON)   += extcon.o
   obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
   obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
  +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
   obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
   obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
   obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o diff --git
  a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c new
  file mode 100644 index 000..7d72d44
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
  @@ -0,0 +1,403 @@
  +/*
  + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
  +driver
  + *
  + * Copyright (C) 2014 Intel Corporation
 
 2014 - 2015
Ok

  + * Decode and log the given reset source indicator (rsi)
  + * register and then clear it.
  + */
  +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info) {
  +   char **rsi;
  +   unsigned int val, i, clear_mask = 0;
  +   int ret;
  +
  +   ret = regmap_read(info-regmap, AXP288_PS_BOOT_REASON_REG,
 val);
  +   for (i = 0, rsi = axp288_pwr_up_down_info; *rsi; rsi++, i++) {
  +   if (val  BIT(i)) {
  +   dev_dbg(info-dev, %s\n, *rsi);
  +   clear_mask |= BIT(i);
  +   }
  +   }
  +
  +   /* Clear the register value for next reboot (write 1 to clear bit) 
  */
  +   regmap_write(info-regmap, AXP288_PS_BOOT_REASON_REG,
  +clear_mask); }
  +
  +static int handle_chrg_det_event(struct axp288_extcon_info *info)
 
 Add 'axp288' prefix to function name as following:
 - handle_chrg_det_event - axp288_handle_chrg_det_event
Ok

 
  +{
  +   static bool notify_otg, notify_charger;
  +   static char *cable;
  +   int ret, stat, cfg, pwr_stat;
  +   u8 chrg_type;
  +   bool vbus_attach = false;
  +
  +   ret = regmap_read(info-regmap, AXP288_PS_STAT_REG, pwr_stat);
  +   if (ret  0) {
  +   dev_err(info-dev, vbus status read error\n);
 
 I recommend that you use the consistency log pattern as following:
 failed to read vbus status\n
 
Ok

  +   return ret;
  +   }
  +
  +   vbus_attach = (pwr_stat  PS_STAT_VBUS_PRESENT);
  +   if (vbus_attach) {
  +   dev_dbg(info-dev, vbus present\n);
  +   } else {
  +   dev_dbg(info-dev, vbus not present\n);
  +   goto notify_otg;
  +   }
 
 I think that you can simplify upper if/else statement as following without any
 unneeded debug log.
  if (!vbus_attach) {
  goto notify_otg;
Ok.


  +
  +   /* Check charger detection completion status */
  +   ret = regmap_read(info-regmap, AXP288_BC_GLOBAL_REG, cfg);
  +   if (ret  0)
  +   goto dev_det_ret;
  +   if (cfg  BC_GLOBAL_DET_STAT) {
  +   dev_dbg(info-dev, charger detection not
  + complete!!\n);
 
 ditto. Don't need to add '!' at the end of log message.
 charger detection not complete!!\n - can't complete the charger
 detection\n
Ok.

 
  +   goto dev_det_ret;
  +   }
  +
  +   ret = regmap_read(info-regmap, AXP288_BC_DET_STAT_REG, stat);
  +   if (ret  0)
  +   goto dev_det_ret;
  +   dev_dbg(info-dev, stat:%x, cfg:%x\n, stat, cfg);
 
 I don't prefer to show the just register value without any log 

RE: [PATCH v8] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
 
 When you send next versions, the convention is for you to apply Acks that
 you've collected.

Ok..
Do you want me to resubmit the patch?


  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  386
 
   include/linux/mfd/axp20x.h |5 +
   4 files changed, 399 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
  diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
  fdc0bf0..2305edc 100644
  --- a/drivers/extcon/Kconfig
  +++ b/drivers/extcon/Kconfig
  @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.
 
  +config EXTCON_AXP288
  +   tristate X-Power AXP288 EXTCON support
  +   depends on MFD_AXP20X  USB_PHY
  +   help
  + Say Y here to enable support for USB peripheral detection
  + and USB MUX switching by X-Power AXP288 PMIC.
  +
   config EXTCON_GPIO
  tristate GPIO extcon support
  depends on GPIOLIB
  diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
  9204114..ba787d0 100644
  --- a/drivers/extcon/Makefile
  +++ b/drivers/extcon/Makefile
  @@ -5,6 +5,7 @@
   obj-$(CONFIG_EXTCON)   += extcon.o
   obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
   obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
  +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
   obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
   obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
   obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
  diff --git a/drivers/extcon/extcon-axp288.c
  b/drivers/extcon/extcon-axp288.c new file mode 100644 index
  000..ea61bf2
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
  @@ -0,0 +1,386 @@
  +/*
  + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
  +driver
  + *
  + * Copyright (C) 2015 Intel Corporation
  + * Author: Ramakrishna Pallala ramakrishna.pall...@intel.com
  + *
  + * This program is free software; you can redistribute it and/or
  +modify
  + * it under the terms of the GNU General Public License version 2 as
  + * published by the Free Software Foundation.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + */
  +
  +#include linux/module.h
  +#include linux/kernel.h
  +#include linux/io.h
  +#include linux/slab.h
  +#include linux/interrupt.h
  +#include linux/platform_device.h
  +#include linux/property.h
  +#include linux/usb/phy.h
  +#include linux/notifier.h
  +#include linux/extcon.h
  +#include linux/regmap.h
  +#include linux/gpio.h
  +#include linux/gpio/consumer.h
  +#include linux/mfd/axp20x.h
  +
  +/* Power source status register */
  +#define PS_STAT_VBUS_TRIGGER   BIT(0)
  +#define PS_STAT_BAT_CHRG_DIR   BIT(2)
  +#define PS_STAT_VBUS_ABOVE_VHOLD   BIT(3)
  +#define PS_STAT_VBUS_VALID BIT(4)
  +#define PS_STAT_VBUS_PRESENT   BIT(5)
  +
  +/* BC module global register */
  +#define BC_GLOBAL_RUN  BIT(0)
  +#define BC_GLOBAL_DET_STAT BIT(2)
  +#define BC_GLOBAL_DBP_TOUT BIT(3)
  +#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
  +#define BC_GLOBAL_DCD_TOUT_MASK(BIT(6)|BIT(5))
  +#define BC_GLOBAL_DCD_TOUT_300MS   0
  +#define BC_GLOBAL_DCD_TOUT_100MS   1
  +#define BC_GLOBAL_DCD_TOUT_500MS   2
  +#define BC_GLOBAL_DCD_TOUT_900MS   3
  +#define BC_GLOBAL_DCD_DET_SEL  BIT(7)
  +
  +/* BC module vbus control and status register */
  +#define VBUS_CNTL_DPDM_PD_EN   BIT(4)
  +#define VBUS_CNTL_DPDM_FD_EN   BIT(5)
  +#define VBUS_CNTL_FIRST_PO_STATBIT(6)
  +
  +/* BC USB status register */
  +#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
  +#define USB_STAT_BUS_STAT_SHIFT0
  +#define USB_STAT_BUS_STAT_ATHD 0
  +#define USB_STAT_BUS_STAT_CONN 1
  +#define USB_STAT_BUS_STAT_SUSP 2
  +#define USB_STAT_BUS_STAT_CONF 3
  +#define USB_STAT_USB_SS_MODE   BIT(4)
  +#define USB_STAT_DEAD_BAT_DET  BIT(6)
  +#define USB_STAT_DBP_UNCFG BIT(7)
  +
  +/* BC detect status register */
  +#define DET_STAT_MASK  (BIT(7)|BIT(6)|BIT(5))
  +#define DET_STAT_SHIFT 5
  +#define DET_STAT_SDP   1
  +#define DET_STAT_CDP   2
  +#define 

RE: [PATCH v6] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Lee Jones,

I have agreed to move the AXP288_EXTCON_* macros to driver source file as
Choi is going to submit another extcon class driver patch to fix the consumer 
driver
access issue.

I will submit the v7 patch now with this change. Thank you for your time.

> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  399
> 
> >  include/linux/mfd/axp20x.h |9 +
> >  4 files changed, 416 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> 
> [...]
> 
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index dfabd6d..38653e0 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >  };
> >
> > +#define AXP288_EXTCON_SLOW_CHARGER "SLOW-CHARGER"
> > +#define AXP288_EXTCON_DOWNSTREAM_CHARGER   "CHARGE-
> DOWNSTREAM"
> > +#define AXP288_EXTCON_FAST_CHARGER "FAST-CHARGER"
> > +
> > +struct axp288_extcon_pdata {
> > +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > +   struct gpio_desc *gpio_mux_cntl;
> > +};
> > +
> >  #endif /* __LINUX_MFD_AXP20X_H */
> 
> Not sure what you decided since sending this, but in order for me to avoid
> becoming the bottle-neck:
> 
> Acked-by: Lee Jones 

Thanks,
Ram

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> On Tue, Apr 28, 2015 at 9:17 PM, Pallala, Ramakrishna
>  wrote:
> > Hi Choi,
> >
> >> On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
> >> >> > > > diff --git a/include/linux/mfd/axp20x.h
> >> >> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> >> >> > > > --- a/include/linux/mfd/axp20x.h
> >> >> > > > +++ b/include/linux/mfd/axp20x.h
> >> >> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> >> >> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >> >> > > >  };
> >> >> > > >
> >> >> > > > +#define AXP288_EXTCON_CABLE_SDP"SLOW-CHARGER"
> >> >> > > > +#define AXP288_EXTCON_CABLE_CDP"CHARGE-
> >> >> > > DOWNSTREAM"
> >> >> > > > +#define AXP288_EXTCON_CABLE_DCP"FAST-CHARGER"
> >> >> > >
> >> >> > > #defines are meant to make things *easier* for humans to read.
> >> >> > > It is my opinion that these negatively impact the readability of the
> code.
> >> >> > These macros will be used in extcon and charger drivers. How can
> >> >> > I make it
> >> >> more readable?
> >> >>
> >> >> If the things you're defining are more readable than the defines
> >> >> themselves you're doing the opposite of what I'd expect.
> >> >>
> >> >> How about:
> >> >>
> >> >>   AXP288_EXTCON_SLOW_CHARGER_CABLE
> >> >>   AXP288_EXTCON_FAST_CHARGER_CABLE
> >> >>   AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
> >> >
> >> > Ok I will change the names similar to the above ones.
> >>
> >> This cable names are used in only extcon driver.
> >> I think that you don't need to define them on header file.
> >> You better to move this definition to extcon driver.
> >>
> >> Also, I'll review your patch on other mail.
> >
> > Based on the these cable types I will be setting the charging parameters.
> > How will I know which charger cable is connected in extcon consumer driver?
> 
> I'm working to implement the unique id for all external connectors instead of
> string name when registering the external connectors [1]. But It is not
> completed and ongoing.
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=v4.
> 2/extcon-next=52d1f16a650cc3b20ab5d5e281b874cfc5f659bc
> 
> Also, extcon consumer driver can use the unique id (enum extcon) when
> executing extcon_register_interest() function.
> After implementing patch[1], all extcon provider/consumer driver would not
> pass the string of external connector directly.

Sounds good. Can you review the patch v6 and let me know if you have
any other comments else I can submit the patch with these changes.

Thanks,
Ram

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

> On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
> >> > > > diff --git a/include/linux/mfd/axp20x.h
> >> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> >> > > > --- a/include/linux/mfd/axp20x.h
> >> > > > +++ b/include/linux/mfd/axp20x.h
> >> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> >> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >> > > >  };
> >> > > >
> >> > > > +#define AXP288_EXTCON_CABLE_SDP"SLOW-CHARGER"
> >> > > > +#define AXP288_EXTCON_CABLE_CDP"CHARGE-
> >> > > DOWNSTREAM"
> >> > > > +#define AXP288_EXTCON_CABLE_DCP"FAST-CHARGER"
> >> > >
> >> > > #defines are meant to make things *easier* for humans to read. It
> >> > > is my opinion that these negatively impact the readability of the code.
> >> > These macros will be used in extcon and charger drivers. How can I
> >> > make it
> >> more readable?
> >>
> >> If the things you're defining are more readable than the defines
> >> themselves you're doing the opposite of what I'd expect.
> >>
> >> How about:
> >>
> >>   AXP288_EXTCON_SLOW_CHARGER_CABLE
> >>   AXP288_EXTCON_FAST_CHARGER_CABLE
> >>   AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
> >
> > Ok I will change the names similar to the above ones.
> 
> This cable names are used in only extcon driver.
> I think that you don't need to define them on header file.
> You better to move this definition to extcon driver.
> 
> Also, I'll review your patch on other mail.

Based on the these cable types I will be setting the charging parameters.
How will I know which charger cable is connected in extcon consumer driver?

Thanks,
Ram

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> On Tue, 28 Apr 2015, Pallala, Ramakrishna wrote:
> 
> > > > This patch adds the extcon support for AXP288 PMIC which has the
> > > > BC1.2 charger detection capability. Additionally it also adds the
> > > > USB mux switching support b/w SOC and PMIC based on GPIO control.
> > > >
> > > > Signed-off-by: Ramakrishna Pallala 
> > > > ---
> > > >  drivers/extcon/Kconfig |7 +
> > > >  drivers/extcon/Makefile|1 +
> > > >  drivers/extcon/extcon-axp288.c |  399
> > > 
> > > >  include/linux/mfd/axp20x.h |9 +
> > > >  4 files changed, 416 insertions(+)  create mode 100644
> > > > drivers/extcon/extcon-axp288.c
> > > >
> > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > > > fdc0bf0..2305edc 100644
> > > > --- a/drivers/extcon/Kconfig
> > > > +++ b/drivers/extcon/Kconfig
> > > > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> > > >   with Wolfson Arizona devices. These are audio CODECs with
> > > >   advanced audio accessory detection support.
> > > >
> > > > +config EXTCON_AXP288
> > > > +   tristate "X-Power AXP288 EXTCON support"
> > > > +   depends on MFD_AXP20X && USB_PHY
> > > > +   help
> > > > + Say Y here to enable support for USB peripheral detection
> > > > + and USB MUX switching by X-Power AXP288 PMIC.
> > > > +
> > > >  config EXTCON_GPIO
> > > > tristate "GPIO extcon support"
> > > > depends on GPIOLIB
> > > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > > > index
> > > > 9204114..ba787d0 100644
> > > > --- a/drivers/extcon/Makefile
> > > > +++ b/drivers/extcon/Makefile
> > > > @@ -5,6 +5,7 @@
> > > >  obj-$(CONFIG_EXTCON)   += extcon.o
> > > >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> > > >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> > > > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> > > >  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
> > > >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
> > > >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
> > > > diff --git a/drivers/extcon/extcon-axp288.c
> > > > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > > > 000..91d764c
> > > > --- /dev/null
> > > > +++ b/drivers/extcon/extcon-axp288.c
> > > > @@ -0,0 +1,399 @@
> > > > +/*
> > > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > > > +driver
> > > > + *
> > > > + * Copyright (C) 2014 Intel Corporation
> > > > + * Author: Ramakrishna Pallala 
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or
> > > > +modify
> > > > + * it under the terms of the GNU General Public License version 2
> > > > +as
> > > > + * published by the Free Software Foundation.
> > > > + *
> > > > + * This program is distributed in the hope that it will be
> > > > +useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > + * GNU General Public License for more details.
> > > > + */
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include  #include 
> > > > +#include  #include  #include
> > > > + #include  #include
> > > > + #include  #include
> > > > +
> >
> > [snip]
> >
> > > > +module_platform_driver(axp288_extcon_driver);
> > > > +
> > > > +MODULE_AUTHOR("Ramakrishna Pallala
> > > > +");
> > > > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/include/linux/mfd/axp20x.h
> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
> > > > --- a/include/linux/mfd/axp20x.h
> > > > +++ b/include/linux/mfd/axp20x.h
> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> > > >  };
> > > >
> > > > +#define AXP288_EXTCON_CABLE_SDP"SLOW-CHARGER"
> > > > +#define AXP288_EXTCON_CABLE_CDP"CHARGE-
> > > DOWNSTREAM"
> > > > +#define AXP288_EXTCON_CABLE_DCP"FAST-CHARGER"
> > >
> > > #defines are meant to make things *easier* for humans to read. It is
> > > my opinion that these negatively impact the readability of the code.
> > These macros will be used in extcon and charger drivers. How can I make it
> more readable?
> 
> If the things you're defining are more readable than the defines themselves
> you're doing the opposite of what I'd expect.
> 
> How about:
> 
>   AXP288_EXTCON_SLOW_CHARGER_CABLE
>   AXP288_EXTCON_FAST_CHARGER_CABLE
>   AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE

Ok I will change the names similar to the above ones.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  399
> 
> >  include/linux/mfd/axp20x.h |9 +
> >  4 files changed, 416 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > fdc0bf0..2305edc 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> >   with Wolfson Arizona devices. These are audio CODECs with
> >   advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > +   tristate "X-Power AXP288 EXTCON support"
> > +   depends on MFD_AXP20X && USB_PHY
> > +   help
> > + Say Y here to enable support for USB peripheral detection
> > + and USB MUX switching by X-Power AXP288 PMIC.
> > +
> >  config EXTCON_GPIO
> > tristate "GPIO extcon support"
> > depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 9204114..ba787d0 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_EXTCON)   += extcon.o
> >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> >  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
> >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
> >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 000..91d764c
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,399 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + * Author: Ramakrishna Pallala 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 

[snip]

> > +module_platform_driver(axp288_extcon_driver);
> > +
> > +MODULE_AUTHOR("Ramakrishna Pallala ");
> > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index dfabd6d..81152e2 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
> > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >  };
> >
> > +#define AXP288_EXTCON_CABLE_SDP"SLOW-CHARGER"
> > +#define AXP288_EXTCON_CABLE_CDP"CHARGE-
> DOWNSTREAM"
> > +#define AXP288_EXTCON_CABLE_DCP"FAST-CHARGER"
> 
> #defines are meant to make things *easier* for humans to read. It is my 
> opinion
> that these negatively impact the readability of the code.
These macros will be used in extcon and charger drivers. How can I make it more 
readable?

> > +struct axp288_extcon_pdata {
> > +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > +   struct gpio_desc *gpio_mux_cntl;
> > +};
> 
> Are you going to add to this?
Yes, this is needed.

Thanks,
Ram

RE: [PATCH v4] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

> 
> Only a license nit.
> 
> On Tue, 2015-04-28 at 04:02 +0530, Ramakrishna Pallala wrote:
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> 
> > + * This program is free software; you can redistribute it and/or
> > + modify
> > + * it under the terms of the GNU General Public License as published
> > + by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> This states the license is GPL v2 or later.
> 
> > +MODULE_LICENSE("GPL v2");
> 
> And, according to include/linux/module.h, this states the license is
> (just) GPL v2. So I think that either the comment at the top of this file or 
> the
> ident used in the MODULE_LICENSE() macro should be changed.

Ok I will fix it.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

 On Tue, 28 Apr 2015, Pallala, Ramakrishna wrote:
 
This patch adds the extcon support for AXP288 PMIC which has the
BC1.2 charger detection capability. Additionally it also adds the
USB mux switching support b/w SOC and PMIC based on GPIO control.
   
Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
---
 drivers/extcon/Kconfig |7 +
 drivers/extcon/Makefile|1 +
 drivers/extcon/extcon-axp288.c |  399
   
 include/linux/mfd/axp20x.h |9 +
 4 files changed, 416 insertions(+)  create mode 100644
drivers/extcon/extcon-axp288.c
   
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
fdc0bf0..2305edc 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -28,6 +28,13 @@ config EXTCON_ARIZONA
  with Wolfson Arizona devices. These are audio CODECs with
  advanced audio accessory detection support.
   
+config EXTCON_AXP288
+   tristate X-Power AXP288 EXTCON support
+   depends on MFD_AXP20X  USB_PHY
+   help
+ Say Y here to enable support for USB peripheral detection
+ and USB MUX switching by X-Power AXP288 PMIC.
+
 config EXTCON_GPIO
tristate GPIO extcon support
depends on GPIOLIB
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index
9204114..ba787d0 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_EXTCON)   += extcon.o
 obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
 obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
+obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
 obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
 obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
diff --git a/drivers/extcon/extcon-axp288.c
b/drivers/extcon/extcon-axp288.c new file mode 100644 index
000..91d764c
--- /dev/null
+++ b/drivers/extcon/extcon-axp288.c
@@ -0,0 +1,399 @@
+/*
+ * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
+driver
+ *
+ * Copyright (C) 2014 Intel Corporation
+ * Author: Ramakrishna Pallala ramakrishna.pall...@intel.com
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2
+as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be
+useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/io.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/platform_device.h #include linux/property.h
+#include linux/usb/phy.h #include linux/notifier.h #include
+linux/extcon.h #include linux/regmap.h #include
+linux/gpio.h #include linux/gpio/consumer.h #include
+linux/mfd/axp20x.h
 
  [snip]
 
+module_platform_driver(axp288_extcon_driver);
+
+MODULE_AUTHOR(Ramakrishna Pallala
+ramakrishna.pall...@intel.com);
+MODULE_DESCRIPTION(X-Powers AXP288 extcon driver);
+MODULE_LICENSE(GPL v2);
diff --git a/include/linux/mfd/axp20x.h
b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
 };
   
+#define AXP288_EXTCON_CABLE_SDPSLOW-CHARGER
+#define AXP288_EXTCON_CABLE_CDPCHARGE-
   DOWNSTREAM
+#define AXP288_EXTCON_CABLE_DCPFAST-CHARGER
  
   #defines are meant to make things *easier* for humans to read. It is
   my opinion that these negatively impact the readability of the code.
  These macros will be used in extcon and charger drivers. How can I make it
 more readable?
 
 If the things you're defining are more readable than the defines themselves
 you're doing the opposite of what I'd expect.
 
 How about:
 
   AXP288_EXTCON_SLOW_CHARGER_CABLE
   AXP288_EXTCON_FAST_CHARGER_CABLE
   AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE

Ok I will change the names similar to the above ones.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

 On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
 diff --git a/include/linux/mfd/axp20x.h
 b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
 --- a/include/linux/mfd/axp20x.h
 +++ b/include/linux/mfd/axp20x.h
 @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
 int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
  };

 +#define AXP288_EXTCON_CABLE_SDPSLOW-CHARGER
 +#define AXP288_EXTCON_CABLE_CDPCHARGE-
DOWNSTREAM
 +#define AXP288_EXTCON_CABLE_DCPFAST-CHARGER
   
#defines are meant to make things *easier* for humans to read. It
is my opinion that these negatively impact the readability of the code.
   These macros will be used in extcon and charger drivers. How can I
   make it
  more readable?
 
  If the things you're defining are more readable than the defines
  themselves you're doing the opposite of what I'd expect.
 
  How about:
 
AXP288_EXTCON_SLOW_CHARGER_CABLE
AXP288_EXTCON_FAST_CHARGER_CABLE
AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
 
  Ok I will change the names similar to the above ones.
 
 This cable names are used in only extcon driver.
 I think that you don't need to define them on header file.
 You better to move this definition to extcon driver.
 
 Also, I'll review your patch on other mail.

Based on the these cable types I will be setting the charging parameters.
How will I know which charger cable is connected in extcon consumer driver?

Thanks,
Ram

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

 On Tue, Apr 28, 2015 at 9:17 PM, Pallala, Ramakrishna
 ramakrishna.pall...@intel.com wrote:
  Hi Choi,
 
  On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
  diff --git a/include/linux/mfd/axp20x.h
  b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
  --- a/include/linux/mfd/axp20x.h
  +++ b/include/linux/mfd/axp20x.h
  @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
  int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
   };
 
  +#define AXP288_EXTCON_CABLE_SDPSLOW-CHARGER
  +#define AXP288_EXTCON_CABLE_CDPCHARGE-
 DOWNSTREAM
  +#define AXP288_EXTCON_CABLE_DCPFAST-CHARGER

 #defines are meant to make things *easier* for humans to read.
 It is my opinion that these negatively impact the readability of the
 code.
These macros will be used in extcon and charger drivers. How can
I make it
   more readable?
  
   If the things you're defining are more readable than the defines
   themselves you're doing the opposite of what I'd expect.
  
   How about:
  
 AXP288_EXTCON_SLOW_CHARGER_CABLE
 AXP288_EXTCON_FAST_CHARGER_CABLE
 AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
  
   Ok I will change the names similar to the above ones.
 
  This cable names are used in only extcon driver.
  I think that you don't need to define them on header file.
  You better to move this definition to extcon driver.
 
  Also, I'll review your patch on other mail.
 
  Based on the these cable types I will be setting the charging parameters.
  How will I know which charger cable is connected in extcon consumer driver?
 
 I'm working to implement the unique id for all external connectors instead of
 string name when registering the external connectors [1]. But It is not
 completed and ongoing.
 [1]
 https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=v4.
 2/extcon-nextid=52d1f16a650cc3b20ab5d5e281b874cfc5f659bc
 
 Also, extcon consumer driver can use the unique id (enum extcon) when
 executing extcon_register_interest() function.
 After implementing patch[1], all extcon provider/consumer driver would not
 pass the string of external connector directly.

Sounds good. Can you review the patch v6 and let me know if you have
any other comments else I can submit the patch with these changes.

Thanks,
Ram

RE: [PATCH v4] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

 
 Only a license nit.
 
 On Tue, 2015-04-28 at 04:02 +0530, Ramakrishna Pallala wrote:
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
 
  + * This program is free software; you can redistribute it and/or
  + modify
  + * it under the terms of the GNU General Public License as published
  + by
  + * the Free Software Foundation; either version 2 of the License, or
  + * (at your option) any later version.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
 
 This states the license is GPL v2 or later.
 
  +MODULE_LICENSE(GPL v2);
 
 And, according to include/linux/module.h, this states the license is
 (just) GPL v2. So I think that either the comment at the top of this file or 
 the
 ident used in the MODULE_LICENSE() macro should be changed.

Ok I will fix it.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v6] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Lee Jones,

I have agreed to move the AXP288_EXTCON_* macros to driver source file as
Choi is going to submit another extcon class driver patch to fix the consumer 
driver
access issue.

I will submit the v7 patch now with this change. Thank you for your time.

  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  399
 
   include/linux/mfd/axp20x.h |9 +
   4 files changed, 416 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
 [...]
 
  diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
  index dfabd6d..38653e0 100644
  --- a/include/linux/mfd/axp20x.h
  +++ b/include/linux/mfd/axp20x.h
  @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
  int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
   };
 
  +#define AXP288_EXTCON_SLOW_CHARGER SLOW-CHARGER
  +#define AXP288_EXTCON_DOWNSTREAM_CHARGER   CHARGE-
 DOWNSTREAM
  +#define AXP288_EXTCON_FAST_CHARGER FAST-CHARGER
  +
  +struct axp288_extcon_pdata {
  +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
  +   struct gpio_desc *gpio_mux_cntl;
  +};
  +
   #endif /* __LINUX_MFD_AXP20X_H */
 
 Not sure what you decided since sending this, but in order for me to avoid
 becoming the bottle-neck:
 
 Acked-by: Lee Jones lee.jo...@linaro.org

Thanks,
Ram

RE: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  399
 
   include/linux/mfd/axp20x.h |9 +
   4 files changed, 416 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
  diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
  fdc0bf0..2305edc 100644
  --- a/drivers/extcon/Kconfig
  +++ b/drivers/extcon/Kconfig
  @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.
 
  +config EXTCON_AXP288
  +   tristate X-Power AXP288 EXTCON support
  +   depends on MFD_AXP20X  USB_PHY
  +   help
  + Say Y here to enable support for USB peripheral detection
  + and USB MUX switching by X-Power AXP288 PMIC.
  +
   config EXTCON_GPIO
  tristate GPIO extcon support
  depends on GPIOLIB
  diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
  9204114..ba787d0 100644
  --- a/drivers/extcon/Makefile
  +++ b/drivers/extcon/Makefile
  @@ -5,6 +5,7 @@
   obj-$(CONFIG_EXTCON)   += extcon.o
   obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
   obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
  +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
   obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
   obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
   obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
  diff --git a/drivers/extcon/extcon-axp288.c
  b/drivers/extcon/extcon-axp288.c new file mode 100644 index
  000..91d764c
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
  @@ -0,0 +1,399 @@
  +/*
  + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
  +driver
  + *
  + * Copyright (C) 2014 Intel Corporation
  + * Author: Ramakrishna Pallala ramakrishna.pall...@intel.com
  + *
  + * This program is free software; you can redistribute it and/or
  +modify
  + * it under the terms of the GNU General Public License version 2 as
  + * published by the Free Software Foundation.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + */
  +
  +#include linux/module.h
  +#include linux/kernel.h
  +#include linux/io.h
  +#include linux/slab.h
  +#include linux/interrupt.h
  +#include linux/platform_device.h
  +#include linux/property.h
  +#include linux/usb/phy.h
  +#include linux/notifier.h
  +#include linux/extcon.h
  +#include linux/regmap.h
  +#include linux/gpio.h
  +#include linux/gpio/consumer.h
  +#include linux/mfd/axp20x.h

[snip]

  +module_platform_driver(axp288_extcon_driver);
  +
  +MODULE_AUTHOR(Ramakrishna Pallala ramakrishna.pall...@intel.com);
  +MODULE_DESCRIPTION(X-Powers AXP288 extcon driver);
  +MODULE_LICENSE(GPL v2);
  diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
  index dfabd6d..81152e2 100644
  --- a/include/linux/mfd/axp20x.h
  +++ b/include/linux/mfd/axp20x.h
  @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
  int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
   };
 
  +#define AXP288_EXTCON_CABLE_SDPSLOW-CHARGER
  +#define AXP288_EXTCON_CABLE_CDPCHARGE-
 DOWNSTREAM
  +#define AXP288_EXTCON_CABLE_DCPFAST-CHARGER
 
 #defines are meant to make things *easier* for humans to read. It is my 
 opinion
 that these negatively impact the readability of the code.
These macros will be used in extcon and charger drivers. How can I make it more 
readable?

  +struct axp288_extcon_pdata {
  +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
  +   struct gpio_desc *gpio_mux_cntl;
  +};
 
 Are you going to add to this?
Yes, this is needed.

Thanks,
Ram

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please see my response below.

> >>
> >>> + ret = PTR_ERR(info->otg);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + for (i = 0; i < EXTCON_IRQ_END; i++) {
> >>> + pirq = platform_get_irq(pdev, i);
> >>> + info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
> >>> + if (info->irq[i] < 0) {
> >>> + dev_warn(>dev,
> >>> + "regmap_irq get virq failed for IRQ %d: %d\n",
> >>> + pirq, info->irq[i]);
> >>> + info->irq[i] = -1;
> >>> + goto intr_reg_failed;
> >>> + }
> >>
> >> Need to add blank line
> > Not sure how it came here...i don't see the line in vim
> 
> I think that need the blank line between '}' and devm_request_threaded_irq()
> sentence.
Ok.

> >>> +
> >>> + /* Unmask VBUS interrupt */
> >>> + regmap_write(info->regmap, AXP288_PWRSRC_IRQ_CFG_REG,
> >>> + PWRSRC_IRQ_CFG_MASK);
> >>> + regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
> >>> + BC_GLOBAL_RUN, 0);
> >>> + /* Unmask the BC1.2 complte interrupts */
> >>> + regmap_write(info->regmap, AXP288_BC12_IRQ_CFG_REG,
> >> BC12_IRQ_CFG_MASK);
> >>> + /* Enable the charger detection logic */
> >>> + regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
> >>> + BC_GLOBAL_RUN, BC_GLOBAL_RUN);
> >>
> >> I think that you better to move the initialization code before extcon
> registration.
> >> The initialization should be executed before extcon and interrupt 
> >> registration.
> > My intention is to enable the interrupts once after setting all the 
> > interrupt and
> extcon handlers.
> 
> OK.
> But, I'd like you to add following functions to include the init code for 
> enabling
> interrupt.
>   axp288_extcon_enable_irq() or axp288_extcon_init()
> 
Ok I will move the interrupt enabling settings to axp288_extcon_enable_irq()

One more thing, mfd cell changes are already merged by Lee Jones so I will only 
push the
extcon-axp288 driver patch under v4.  Hope this is ok...

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please find my response below. 

> On 04/09/2015 02:12 AM, Ramakrishna Pallala wrote:
> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  462
> 
> >  include/linux/mfd/axp20x.h |5 +
> >  4 files changed, 475 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > 6a1f7de..f6e8b2a 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> >   with Wolfson Arizona devices. These are audio CODECs with
> >   advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > +   tristate "X-Power AXP288 EXTCON support"
> > +   depends on MFD_AXP20X && USB_PHY
> > +   help
> > + Say Y here to enable support for USB peripheral detection
> > + and USB MUX switching by X-Power AXP288 PMIC.
> > +
> >  config EXTCON_GPIO
> > tristate "GPIO extcon support"
> > depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 0370b42..5429377 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_EXTCON)   += extcon-class.o
> >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
> >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> >  obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
> >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
> >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 000..363552c
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,462 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + * Author: Ramakrishna Pallala 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Power source status register */
> > +#define PS_STAT_VBUS_TRIGGER   BIT(0)
> > +#define PS_STAT_BAT_CHRG_DIR   BIT(2)
> > +#define PS_STAT_VBUS_ABOVE_VHOLD   BIT(3)
> > +#define PS_STAT_VBUS_VALID BIT(4)
> > +#define PS_STAT_VBUS_PRESENT   BIT(5)
> > +
> > +/* BC module global register */
> > +#define BC_GLOBAL_RUN  BIT(0)
> > +#define BC_GLOBAL_DET_STAT BIT(2)
> > +#define BC_GLOBAL_DBP_TOUT BIT(3)
> > +#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
> > +#define BC_GLOBAL_DCD_TOUT_MASK(BIT(6)|BIT(5))
> > +#define BC_GLOBAL_DCD_TOUT_300MS   0
> > +#define BC_GLOBAL_DCD_TOUT_100MS   1
> > +#define BC_GLOBAL_DCD_TOUT_500MS   2
> > +#define BC_GLOBAL_DCD_TOUT_900MS   3
> > +#define BC_GLOBAL_DCD_DET_SEL  BIT(7)
> > +
> > +/* BC module vbus control and status register */
> > +#define VBUS_CNTL_DPDM_PD_EN   BIT(4)
> > +#define VBUS_CNTL_DPDM_FD_EN   BIT(5)
> > +#define VBUS_CNTL_FIRST_PO_STATBIT(6)
> > +
> > +/* BC USB status register */
> > +#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
> > +#define USB_STAT_BUS_STAT_SHIFT0
> > +#define USB_STAT_BUS_STAT_ATHD 0
> > +#define USB_STAT_BUS_STAT_CONN 1
> > +#define USB_STAT_BUS_STAT_SUSP 2
> > +#define USB_STAT_BUS_STAT_CONF 3
> > +#define USB_STAT_USB_SS_MODE   BIT(4)
> > +#define USB_STAT_DEAD_BAT_DET  BIT(6)
> > +#define USB_STAT_DBP_UNCFG BIT(7)
> > +
> > +/* BC detect status register */
> > +#define DET_STAT_MASK  (BIT(7)|BIT(6)|BIT(5))
> > +#define DET_STAT_SHIFT 5
> > +#define DET_STAT_SDP   1
> > +#define DET_STAT_CDP   2
> > 

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please find my response below. 

 On 04/09/2015 02:12 AM, Ramakrishna Pallala wrote:
  This patch adds the extcon support for AXP288 PMIC which has the BC1.2
  charger detection capability. Additionally it also adds the USB mux
  switching support b/w SOC and PMIC based on GPIO control.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  462
 
   include/linux/mfd/axp20x.h |5 +
   4 files changed, 475 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
  diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
  6a1f7de..f6e8b2a 100644
  --- a/drivers/extcon/Kconfig
  +++ b/drivers/extcon/Kconfig
  @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
with Wolfson Arizona devices. These are audio CODECs with
advanced audio accessory detection support.
 
  +config EXTCON_AXP288
  +   tristate X-Power AXP288 EXTCON support
  +   depends on MFD_AXP20X  USB_PHY
  +   help
  + Say Y here to enable support for USB peripheral detection
  + and USB MUX switching by X-Power AXP288 PMIC.
  +
   config EXTCON_GPIO
  tristate GPIO extcon support
  depends on GPIOLIB
  diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
  0370b42..5429377 100644
  --- a/drivers/extcon/Makefile
  +++ b/drivers/extcon/Makefile
  @@ -5,6 +5,7 @@
   obj-$(CONFIG_EXTCON)   += extcon-class.o
   obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
   obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
  +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
   obj-$(CONFIG_EXTCON_GPIO)  += extcon-gpio.o
   obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
   obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
  diff --git a/drivers/extcon/extcon-axp288.c
  b/drivers/extcon/extcon-axp288.c new file mode 100644 index
  000..363552c
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
  @@ -0,0 +1,462 @@
  +/*
  + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
  +driver
  + *
  + * Copyright (C) 2015 Intel Corporation
  + * Author: Ramakrishna Pallala ramakrishna.pall...@intel.com
  + *
  + * This program is free software; you can redistribute it and/or
  +modify
  + * it under the terms of the GNU General Public License as published
  +by
  + * the Free Software Foundation; either version 2 of the License, or
  + * (at your option) any later version.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + */
  +
  +#include linux/module.h
  +#include linux/kernel.h
  +#include linux/io.h
  +#include linux/slab.h
  +#include linux/interrupt.h
  +#include linux/platform_device.h
  +#include linux/property.h
  +#include linux/usb/phy.h
  +#include linux/notifier.h
  +#include linux/extcon.h
  +#include linux/regmap.h
  +#include linux/gpio.h
  +#include linux/gpio/consumer.h
  +#include linux/mfd/axp20x.h
  +
  +/* Power source status register */
  +#define PS_STAT_VBUS_TRIGGER   BIT(0)
  +#define PS_STAT_BAT_CHRG_DIR   BIT(2)
  +#define PS_STAT_VBUS_ABOVE_VHOLD   BIT(3)
  +#define PS_STAT_VBUS_VALID BIT(4)
  +#define PS_STAT_VBUS_PRESENT   BIT(5)
  +
  +/* BC module global register */
  +#define BC_GLOBAL_RUN  BIT(0)
  +#define BC_GLOBAL_DET_STAT BIT(2)
  +#define BC_GLOBAL_DBP_TOUT BIT(3)
  +#define BC_GLOBAL_VLGC_COM_SEL BIT(4)
  +#define BC_GLOBAL_DCD_TOUT_MASK(BIT(6)|BIT(5))
  +#define BC_GLOBAL_DCD_TOUT_300MS   0
  +#define BC_GLOBAL_DCD_TOUT_100MS   1
  +#define BC_GLOBAL_DCD_TOUT_500MS   2
  +#define BC_GLOBAL_DCD_TOUT_900MS   3
  +#define BC_GLOBAL_DCD_DET_SEL  BIT(7)
  +
  +/* BC module vbus control and status register */
  +#define VBUS_CNTL_DPDM_PD_EN   BIT(4)
  +#define VBUS_CNTL_DPDM_FD_EN   BIT(5)
  +#define VBUS_CNTL_FIRST_PO_STATBIT(6)
  +
  +/* BC USB status register */
  +#define USB_STAT_BUS_STAT_MASK (BIT(3)|BIT(2)|BIT(1)|BIT(0))
  +#define USB_STAT_BUS_STAT_SHIFT0
  +#define USB_STAT_BUS_STAT_ATHD 0
  +#define USB_STAT_BUS_STAT_CONN 1
  +#define USB_STAT_BUS_STAT_SUSP 2
  +#define USB_STAT_BUS_STAT_CONF 3
  +#define USB_STAT_USB_SS_MODE   BIT(4)
  +#define USB_STAT_DEAD_BAT_DET  BIT(6)
  +#define USB_STAT_DBP_UNCFG BIT(7)
  +
  +/* BC detect status register */
  +#define DET_STAT_MASK  (BIT(7)|BIT(6)|BIT(5))
  +#define DET_STAT_SHIFT 5
  +#define DET_STAT_SDP   1
  +#define DET_STAT_CDP   

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please see my response below.

 
  + ret = PTR_ERR(info-otg);
  + return ret;
  + }
  +
  + for (i = 0; i  EXTCON_IRQ_END; i++) {
  + pirq = platform_get_irq(pdev, i);
  + info-irq[i] = regmap_irq_get_virq(info-regmap_irqc, pirq);
  + if (info-irq[i]  0) {
  + dev_warn(pdev-dev,
  + regmap_irq get virq failed for IRQ %d: %d\n,
  + pirq, info-irq[i]);
  + info-irq[i] = -1;
  + goto intr_reg_failed;
  + }
 
  Need to add blank line
  Not sure how it came here...i don't see the line in vim
 
 I think that need the blank line between '}' and devm_request_threaded_irq()
 sentence.
Ok.

  +
  + /* Unmask VBUS interrupt */
  + regmap_write(info-regmap, AXP288_PWRSRC_IRQ_CFG_REG,
  + PWRSRC_IRQ_CFG_MASK);
  + regmap_update_bits(info-regmap, AXP288_BC_GLOBAL_REG,
  + BC_GLOBAL_RUN, 0);
  + /* Unmask the BC1.2 complte interrupts */
  + regmap_write(info-regmap, AXP288_BC12_IRQ_CFG_REG,
  BC12_IRQ_CFG_MASK);
  + /* Enable the charger detection logic */
  + regmap_update_bits(info-regmap, AXP288_BC_GLOBAL_REG,
  + BC_GLOBAL_RUN, BC_GLOBAL_RUN);
 
  I think that you better to move the initialization code before extcon
 registration.
  The initialization should be executed before extcon and interrupt 
  registration.
  My intention is to enable the interrupts once after setting all the 
  interrupt and
 extcon handlers.
 
 OK.
 But, I'd like you to add following functions to include the init code for 
 enabling
 interrupt.
   axp288_extcon_enable_irq() or axp288_extcon_init()
 
Ok I will move the interrupt enabling settings to axp288_extcon_enable_irq()

One more thing, mfd cell changes are already merged by Lee Jones so I will only 
push the
extcon-axp288 driver patch under v4.  Hope this is ok...

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Hi Sebastian,

> -Original Message-
> From: Todd E Brandt [mailto:todd.e.bra...@linux.intel.com]
> Sent: Saturday, March 14, 2015 9:29 AM
> To: Pallala, Ramakrishna
> Cc: linux-kernel@vger.kernel.org; Sebastian Reichel; Brandt, Todd E;
> Woodhouse, David
> Subject: Re: [PATCH] axp288_fuel_gauge: Add original author details
> 
> On Fri, Mar 13, 2015 at 09:49:09PM +0530, Ramakrishna Pallala wrote:
> > Add the original author details of the axp288_fuel_gauge driver.
> >
> Acked-by: Todd Brandt 
> 
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/power/axp288_fuel_gauge.c |1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/power/axp288_fuel_gauge.c
> > b/drivers/power/axp288_fuel_gauge.c
> > index c86e709..fc38979 100644
> > --- a/drivers/power/axp288_fuel_gauge.c
> > +++ b/drivers/power/axp288_fuel_gauge.c
> > @@ -1146,6 +1146,7 @@ static struct platform_driver
> > axp288_fuel_gauge_driver = {
> >
> >  module_platform_driver(axp288_fuel_gauge_driver);
> >
> > +MODULE_AUTHOR("Ramakrishna Pallala ");
> >  MODULE_AUTHOR("Todd Brandt ");
> >  MODULE_DESCRIPTION("Xpower AXP288 Fuel Gauge Driver");
> > MODULE_LICENSE("GPL");

Todd has acked the patch, can you merge this patch?

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] axp288_fuel_gauge: Add original author details

[Pallala, Ramakrishna]

Hi Sebastian,

 -Original Message-
 From: Todd E Brandt [mailto:todd.e.bra...@linux.intel.com]
 Sent: Saturday, March 14, 2015 9:29 AM
 To: Pallala, Ramakrishna
 Cc: linux-kernel@vger.kernel.org; Sebastian Reichel; Brandt, Todd E;
 Woodhouse, David
 Subject: Re: [PATCH] axp288_fuel_gauge: Add original author details
 
 On Fri, Mar 13, 2015 at 09:49:09PM +0530, Ramakrishna Pallala wrote:
  Add the original author details of the axp288_fuel_gauge driver.
 
 Acked-by: Todd Brandt todd.e.bra...@linux.intel.com
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/power/axp288_fuel_gauge.c |1 +
   1 file changed, 1 insertion(+)
 
  diff --git a/drivers/power/axp288_fuel_gauge.c
  b/drivers/power/axp288_fuel_gauge.c
  index c86e709..fc38979 100644
  --- a/drivers/power/axp288_fuel_gauge.c
  +++ b/drivers/power/axp288_fuel_gauge.c
  @@ -1146,6 +1146,7 @@ static struct platform_driver
  axp288_fuel_gauge_driver = {
 
   module_platform_driver(axp288_fuel_gauge_driver);
 
  +MODULE_AUTHOR(Ramakrishna Pallala ramakrishna.pall...@intel.com);
   MODULE_AUTHOR(Todd Brandt todd.e.bra...@linux.intel.com);
   MODULE_DESCRIPTION(Xpower AXP288 Fuel Gauge Driver);
  MODULE_LICENSE(GPL);

Todd has acked the patch, can you merge this patch?

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

> When submitting patches, please observe the $SUBJECT line formatting of the
> subsystem you're sending to.
> 
> You can do so by: `git log --oneline -- drivers/`
> 
> In the case of MFD it's:
> 
> mfd: : 
> 
> Note the ':' separators and the capitalisation of the first letter of the 
> description
> string.
> 
> > This patch adds the mfd cell info for axp288 extcon device.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/mfd/axp20x.c |   28 
> >  1 file changed, 28 insertions(+)
> 
> Nevertheless, patch applied, thanks.

Thanks you. I will take care next time.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v3 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

 When submitting patches, please observe the $SUBJECT line formatting of the
 subsystem you're sending to.
 
 You can do so by: `git log --oneline -- drivers/subsystem`
 
 In the case of MFD it's:
 
 mfd: device: Description
 
 Note the ':' separators and the capitalisation of the first letter of the 
 description
 string.
 
  This patch adds the mfd cell info for axp288 extcon device.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/mfd/axp20x.c |   28 
   1 file changed, 28 insertions(+)
 
 Nevertheless, patch applied, thanks.

Thanks you. I will take care next time.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v2 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

> Hi Ramakrishna,
> 
> When I apply this patch for build test on extcon-next branch, conflict happen.
> you have to implement this patchset on latest extcon-next branch.
Ok I will create the patches on 
git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

> Also, This patch must need more clean-up and use latest extcon helper API.
> When you implement extcon-axp288.c, I recommend you refer to merged
> extcon drivers.
Ok.

> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -93,4 +93,11 @@ config EXTCON_SM5502
> >   Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >   detector and switch.
> >
> > +config EXTCON_AXP288
> > +   tristate "AXP288 EXTCON support"
> 
> I recommend to add manufactor name of AXP288 PMIC chip.
Ok.

> > +   depends on MFD_AXP20X && USB_PHY
> > +   help
> > + Say Y here to enable support for USB peripheral detection
> > + and USB MUX switching by AXP288 PMIC.
> > +
> 
> Need to relocate EXTCON_AXP288 configuration alpabetically.
Ok.

> >  endif # MULTISTATE_SWITCH
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 0370b42..832ad79 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-
> max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
> 
> ditto.
Ok.

> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 000..2e75d45
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,479 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + * Ramakrishna Pallala 
> 
> Need to add 'Author' in front of name.
Ok.

> > +#define AXP288_BC_DET_STAT_REG 0x2f
> > +#define DET_STAT_MASK  0xe0
> > +#define DET_STAT_OFFSET5
> > +#define DET_STAT_SDP   0x1
> > +#define DET_STAT_CDP   0x2
> > +#define DET_STAT_DCP   0x3
> > +
> > +#define AXP288_PS_BOOT_REASON_REG  0x2
> > +
> > +#define AXP288_PWRSRC_IRQ_CFG_REG  0x40
> > +#define PWRSRC_IRQ_CFG_MASK0x1c
> > +
> > +#define AXP288_BC12_IRQ_CFG_REG0x45
> > +#define BC12_IRQ_CFG_MASK  0x2
> 
> I recommend to gather axp288 registers as following by using 'enum' keyword
> and then add the mask definition using the BIT(n) macro.
> 
> enum axp288_reg {
>   AXP288_REG_PS_STAT_REG = 0x00,
>   AXP288_REG_BC_GLOBAL_REG = 0x2c,
>   AXP288_REG_BC_VBUS_CNTL_REG = 0x2d,
>   AXP288_REG_BC_USB_STAT_REG = 0x2e,
>   AXP288_REG_BC_DET_STAT_REG = 0x2f,
>   ...
> 
>   AXP288_REG_END
> };
> 
> Also, I think need more description about mask/offset definition.
> 
> > +
> > +#define AXP288_PWRSRC_INTR_NUM 4
> 
> Don't need this definition. You can add AXP288_
> 
> > +
> > +#define AXP288_DRV_NAME"axp288_extcon"
> 
> Don't need this definition. You use drive name in structure
> axp288_extcon_driver.
Ok.

> 
> > +
> > +#define AXP288_EXTCON_CABLE_SDP"Slow-charger"
> > +#define AXP288_EXTCON_CABLE_CDP"Charge-downstream"
> > +#define AXP288_EXTCON_CABLE_DCP"Fast-charger"
> 
> Use the capital letter for cable name instead of small letter.
> I'll change all cable names by using capital letter.
You mean like this... "Slow-charger" => "SLOW-CHARGER"?

> 
> > +
> > +#define EXTCON_GPIO_MUX_SEL_PMIC   0
> > +#define EXTCON_GPIO_MUX_SEL_SOC1
> > +
> > +enum {
> 
> Need to add enum name to improve readability and catch the correct meaning
> of this definitions.
Ok.

> > +   VBUS_FALLING_IRQ = 0,
> > +   VBUS_RISING_IRQ,
> > +   MV_CHNG_IRQ,
> > +   BC_USB_CHNG_IRQ,
> 
> If AXP288_PWRSRC_INTR_NUM means the number of this interrupt, you can
> add AXP288_PWRSRC_INTR_NUM value in this enum list without separate
> defintion (#define AXP288_PWRSRC_INTR_NUM).
Ok.

> > +static const char *axp288_extcon_cables[] = {
> > +   AXP288_EXTCON_CABLE_SDP,
> > +   AXP288_EXTCON_CABLE_CDP,
> > +   AXP288_EXTCON_CABLE_DCP,
> > +   NULL,
> > +};
> > +
> > +struct axp288_extcon_info {
> > +   struct platform_device *pdev;
> 
> Define 'struct device *dev' instead of 'struct platform_device *pdev'.
> I think you can support all case by using 'dev' instance in extcon driver.
Ok I will check.

> > +   struct regmap *regmap;
> > +   struct regmap_irq_chip_data *regmap_irqc;
> > +   struct axp288_extcon_pdata *pdata;
> 
> Are you sure that pdata is necessary? The axp288_extcon_pdata includes only
> gpio.
Yes, as I have plans to include more info to pdata struct.

> You have to get the gpio value by using gpiod 

RE: [PATCH v2 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

 Hi Ramakrishna,
 
 When I apply this patch for build test on extcon-next branch, conflict happen.
 you have to implement this patchset on latest extcon-next branch.
Ok I will create the patches on 
git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

 Also, This patch must need more clean-up and use latest extcon helper API.
 When you implement extcon-axp288.c, I recommend you refer to merged
 extcon drivers.
Ok.

  --- a/drivers/extcon/Kconfig
  +++ b/drivers/extcon/Kconfig
  @@ -93,4 +93,11 @@ config EXTCON_SM5502
Silicon Mitus SM5502. The SM5502 is a USB port accessory
detector and switch.
 
  +config EXTCON_AXP288
  +   tristate AXP288 EXTCON support
 
 I recommend to add manufactor name of AXP288 PMIC chip.
Ok.

  +   depends on MFD_AXP20X  USB_PHY
  +   help
  + Say Y here to enable support for USB peripheral detection
  + and USB MUX switching by AXP288 PMIC.
  +
 
 Need to relocate EXTCON_AXP288 configuration alpabetically.
Ok.

   endif # MULTISTATE_SWITCH
  diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
  0370b42..832ad79 100644
  --- a/drivers/extcon/Makefile
  +++ b/drivers/extcon/Makefile
  @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)  += extcon-
 max8997.o
   obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
   obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
   obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
  +obj-$(CONFIG_EXTCON_AXP288)+= extcon-axp288.o
 
 ditto.
Ok.

  diff --git a/drivers/extcon/extcon-axp288.c
  b/drivers/extcon/extcon-axp288.c new file mode 100644 index
  000..2e75d45
  --- /dev/null
  +++ b/drivers/extcon/extcon-axp288.c
  @@ -0,0 +1,479 @@
  +/*
  + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
  +driver
  + *
  + * Copyright (C) 2015 Intel Corporation
  + * Ramakrishna Pallala ramakrishna.pall...@intel.com
 
 Need to add 'Author' in front of name.
Ok.

  +#define AXP288_BC_DET_STAT_REG 0x2f
  +#define DET_STAT_MASK  0xe0
  +#define DET_STAT_OFFSET5
  +#define DET_STAT_SDP   0x1
  +#define DET_STAT_CDP   0x2
  +#define DET_STAT_DCP   0x3
  +
  +#define AXP288_PS_BOOT_REASON_REG  0x2
  +
  +#define AXP288_PWRSRC_IRQ_CFG_REG  0x40
  +#define PWRSRC_IRQ_CFG_MASK0x1c
  +
  +#define AXP288_BC12_IRQ_CFG_REG0x45
  +#define BC12_IRQ_CFG_MASK  0x2
 
 I recommend to gather axp288 registers as following by using 'enum' keyword
 and then add the mask definition using the BIT(n) macro.
 
 enum axp288_reg {
   AXP288_REG_PS_STAT_REG = 0x00,
   AXP288_REG_BC_GLOBAL_REG = 0x2c,
   AXP288_REG_BC_VBUS_CNTL_REG = 0x2d,
   AXP288_REG_BC_USB_STAT_REG = 0x2e,
   AXP288_REG_BC_DET_STAT_REG = 0x2f,
   ...
 
   AXP288_REG_END
 };
 
 Also, I think need more description about mask/offset definition.
 
  +
  +#define AXP288_PWRSRC_INTR_NUM 4
 
 Don't need this definition. You can add AXP288_
 
  +
  +#define AXP288_DRV_NAMEaxp288_extcon
 
 Don't need this definition. You use drive name in structure
 axp288_extcon_driver.
Ok.

 
  +
  +#define AXP288_EXTCON_CABLE_SDPSlow-charger
  +#define AXP288_EXTCON_CABLE_CDPCharge-downstream
  +#define AXP288_EXTCON_CABLE_DCPFast-charger
 
 Use the capital letter for cable name instead of small letter.
 I'll change all cable names by using capital letter.
You mean like this... Slow-charger = SLOW-CHARGER?

 
  +
  +#define EXTCON_GPIO_MUX_SEL_PMIC   0
  +#define EXTCON_GPIO_MUX_SEL_SOC1
  +
  +enum {
 
 Need to add enum name to improve readability and catch the correct meaning
 of this definitions.
Ok.

  +   VBUS_FALLING_IRQ = 0,
  +   VBUS_RISING_IRQ,
  +   MV_CHNG_IRQ,
  +   BC_USB_CHNG_IRQ,
 
 If AXP288_PWRSRC_INTR_NUM means the number of this interrupt, you can
 add AXP288_PWRSRC_INTR_NUM value in this enum list without separate
 defintion (#define AXP288_PWRSRC_INTR_NUM).
Ok.

  +static const char *axp288_extcon_cables[] = {
  +   AXP288_EXTCON_CABLE_SDP,
  +   AXP288_EXTCON_CABLE_CDP,
  +   AXP288_EXTCON_CABLE_DCP,
  +   NULL,
  +};
  +
  +struct axp288_extcon_info {
  +   struct platform_device *pdev;
 
 Define 'struct device *dev' instead of 'struct platform_device *pdev'.
 I think you can support all case by using 'dev' instance in extcon driver.
Ok I will check.

  +   struct regmap *regmap;
  +   struct regmap_irq_chip_data *regmap_irqc;
  +   struct axp288_extcon_pdata *pdata;
 
 Are you sure that pdata is necessary? The axp288_extcon_pdata includes only
 gpio.
Yes, as I have plans to include more info to pdata struct.

 You have to get the gpio value by using gpiod helper funciton as following:
 You can refer other extcon driver(extcon-usb-gpio.c)/
 
   devm_gpiod_get(dev, mux_cntl);
 
I will check.

  +   int irq[AXP288_PWRSRC_INTR_NUM];
  +   struct extcon_dev 

RE: [PATCH v1 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

> > > > diff --git a/include/linux/mfd/axp20x.h
> > > > b/include/linux/mfd/axp20x.h index dfabd6d..4ed8071 100644
> > > > --- a/include/linux/mfd/axp20x.h
> > > > +++ b/include/linux/mfd/axp20x.h
> > > > @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
> > > > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> > > >  };
> > > >
> > > > +struct axp288_extcon_pdata {
> > > > +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > > > +   struct gpio_desc *gpio_mux_cntl; };
> > >
> > > This chunk of code is unrelated to this patch.
> > As it's axp20x.h file change I added it here...if you don’t feel logical to 
> > have it
> in the patch I can move this to patch 2/2.
> 
> You should add it to the patch that first uses it.
Ok I will fix it.

Thanks,
Ram

RE: [PATCH v1 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

Hi,

> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index dfabd6d..4ed8071 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
> > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >  };
> >
> > +struct axp288_extcon_pdata {
> > +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > +   struct gpio_desc *gpio_mux_cntl;
> > +};
> 
> This chunk of code is unrelated to this patch.
As it's axp20x.h file change I added it here...if you don’t feel logical to 
have it in the patch I can move this to patch 2/2.

>
> ... and does it need to be in a struct of its own?
Yes, I am planning to add another gpio control for VBUS boost.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

Hi,

  diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
  index dfabd6d..4ed8071 100644
  --- a/include/linux/mfd/axp20x.h
  +++ b/include/linux/mfd/axp20x.h
  @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
  int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
   };
 
  +struct axp288_extcon_pdata {
  +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
  +   struct gpio_desc *gpio_mux_cntl;
  +};
 
 This chunk of code is unrelated to this patch.
As it's axp20x.h file change I added it here...if you don’t feel logical to 
have it in the patch I can move this to patch 2/2.


 ... and does it need to be in a struct of its own?
Yes, I am planning to add another gpio control for VBUS boost.

Thanks,
Ram
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

diff --git a/include/linux/mfd/axp20x.h
b/include/linux/mfd/axp20x.h index dfabd6d..4ed8071 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
 };
   
+struct axp288_extcon_pdata {
+   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
+   struct gpio_desc *gpio_mux_cntl; };
  
   This chunk of code is unrelated to this patch.
  As it's axp20x.h file change I added it here...if you don’t feel logical to 
  have it
 in the patch I can move this to patch 2/2.
 
 You should add it to the patch that first uses it.
Ok I will fix it.

Thanks,
Ram

RE: [PATCH 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

> On Thu, 02 Apr 2015, Ramakrishna Pallala wrote:
> 
> > This patch adds the mfd cell info for axp288 extcon device.
> >
> > Signed-off-by: Ramakrishna Pallala 
> > ---
> >  drivers/mfd/axp20x.c   |   28 
> >  include/linux/mfd/axp20x.h |5 +
> >  2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c index
> > 0acbe52..30af1a3 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
> > },
> >  };
> >
> > +static struct resource axp288_extcon_resources[] = {
> > +   {
> > +   .start = AXP288_IRQ_VBUS_FALL,
> > +   .end   = AXP288_IRQ_VBUS_FALL,
> > +   .flags = IORESOURCE_IRQ,
> > +   },
> > +   {
> > +   .start = AXP288_IRQ_VBUS_RISE,
> > +   .end   = AXP288_IRQ_VBUS_RISE,
> > +   .flags = IORESOURCE_IRQ,
> > +   },
> > +   {
> > +   .start = AXP288_IRQ_MV_CHNG,
> > +   .end   = AXP288_IRQ_MV_CHNG,
> > +   .flags = IORESOURCE_IRQ,
> > +   },
> > +   {
> > +   .start = AXP288_IRQ_BC_USB_CHNG,
> > +   .end   = AXP288_IRQ_BC_USB_CHNG,
> > +   .flags = IORESOURCE_IRQ,
> > +   },
> > +};
> > +
> >  static struct resource axp288_charger_resources[] = {
> > {
> > .start = AXP288_IRQ_OV,
> > @@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
> > .resources = axp288_adc_resources,
> > },
> > {
> > +   .name = "extcon-axp288",
> 
> This naming convention is not consistent with the others in the container.
Ok agreed. I will fix it.


> > +   .num_resources = ARRAY_SIZE(axp288_extcon_resources),
> > +   .resources = axp288_extcon_resources,
> > +   },
> > +   {
> > .name = "axp288_charger",
> > .num_resources = ARRAY_SIZE(axp288_charger_resources),
> > .resources = axp288_charger_resources, diff --git
> > a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h index
> > dfabd6d..4ed8071 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
> > int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
> >  };
> >
> > +struct axp288_extcon_pdata {
> > +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> > +   struct gpio_desc *gpio_mux_cntl;
> > +};
> 
> Is this going to be added to?  Seems like overkill to have a struct with only 
> one
> attribute.
Yes, I will be adding to this struct in future.

Thanks,
Ram

RE: [PATCH 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

 On Thu, 02 Apr 2015, Ramakrishna Pallala wrote:
 
  This patch adds the mfd cell info for axp288 extcon device.
 
  Signed-off-by: Ramakrishna Pallala ramakrishna.pall...@intel.com
  ---
   drivers/mfd/axp20x.c   |   28 
   include/linux/mfd/axp20x.h |5 +
   2 files changed, 33 insertions(+)
 
  diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c index
  0acbe52..30af1a3 100644
  --- a/drivers/mfd/axp20x.c
  +++ b/drivers/mfd/axp20x.c
  @@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
  },
   };
 
  +static struct resource axp288_extcon_resources[] = {
  +   {
  +   .start = AXP288_IRQ_VBUS_FALL,
  +   .end   = AXP288_IRQ_VBUS_FALL,
  +   .flags = IORESOURCE_IRQ,
  +   },
  +   {
  +   .start = AXP288_IRQ_VBUS_RISE,
  +   .end   = AXP288_IRQ_VBUS_RISE,
  +   .flags = IORESOURCE_IRQ,
  +   },
  +   {
  +   .start = AXP288_IRQ_MV_CHNG,
  +   .end   = AXP288_IRQ_MV_CHNG,
  +   .flags = IORESOURCE_IRQ,
  +   },
  +   {
  +   .start = AXP288_IRQ_BC_USB_CHNG,
  +   .end   = AXP288_IRQ_BC_USB_CHNG,
  +   .flags = IORESOURCE_IRQ,
  +   },
  +};
  +
   static struct resource axp288_charger_resources[] = {
  {
  .start = AXP288_IRQ_OV,
  @@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
  .resources = axp288_adc_resources,
  },
  {
  +   .name = extcon-axp288,
 
 This naming convention is not consistent with the others in the container.
Ok agreed. I will fix it.


  +   .num_resources = ARRAY_SIZE(axp288_extcon_resources),
  +   .resources = axp288_extcon_resources,
  +   },
  +   {
  .name = axp288_charger,
  .num_resources = ARRAY_SIZE(axp288_charger_resources),
  .resources = axp288_charger_resources, diff --git
  a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h index
  dfabd6d..4ed8071 100644
  --- a/include/linux/mfd/axp20x.h
  +++ b/include/linux/mfd/axp20x.h
  @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
  int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
   };
 
  +struct axp288_extcon_pdata {
  +   /* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
  +   struct gpio_desc *gpio_mux_cntl;
  +};
 
 Is this going to be added to?  Seems like overkill to have a struct with only 
 one
 attribute.
Yes, I will be adding to this struct in future.

Thanks,
Ram

RE: [PATCH v0 0/2] Add extcon support for AXP288 PMIC

[Pallala, Ramakrishna]

> > This patch series adds the support for axp288 extcon driver and also
> > adds the cell info for extcon device in axp20x mfd driver.
> >
> > Ramakrishna Pallala (2):
> >   mfd/axp20x: add support for extcon cell
> >   extcon-axp288: Add axp288 extcon driver support
> >
> >  drivers/extcon/Kconfig |7 +
> >  drivers/extcon/Makefile|1 +
> >  drivers/extcon/extcon-axp288.c |  479
> 
> >  drivers/mfd/axp20x.c   |   28 +++
> >  include/linux/mfd/axp20x.h |5 +
> >  5 files changed, 520 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> 
> Ensure that when you're sending patch-set, they are linked.
> 
> See --threaded in `git send-email --help`.

Ok. I will resubmit the patches.

Thanks,
Ram

RE: [PATCH v0 0/2] Add extcon support for AXP288 PMIC

[Pallala, Ramakrishna]

  This patch series adds the support for axp288 extcon driver and also
  adds the cell info for extcon device in axp20x mfd driver.
 
  Ramakrishna Pallala (2):
mfd/axp20x: add support for extcon cell
extcon-axp288: Add axp288 extcon driver support
 
   drivers/extcon/Kconfig |7 +
   drivers/extcon/Makefile|1 +
   drivers/extcon/extcon-axp288.c |  479
 
   drivers/mfd/axp20x.c   |   28 +++
   include/linux/mfd/axp20x.h |5 +
   5 files changed, 520 insertions(+)
   create mode 100644 drivers/extcon/extcon-axp288.c
 
 Ensure that when you're sending patch-set, they are linked.
 
 See --threaded in `git send-email --help`.

Ok. I will resubmit the patches.

Thanks,
Ram

RE: [PATCH 4/4] iio/adc/axp288: add support for axp288 gpadc

[Pallala, Ramakrishna]

Hi Jacob,

> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME) static int 
> +axp288_gpadc_suspend(struct device *dev) {
> + int ret;
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct gpadc_info *info = iio_priv(indio_dev);
> +
> + mutex_lock(_dev->mlock);
> + ret = axp288_gpadc_enable(info->regmap, false);
> + mutex_unlock(_dev->mlock);
> +
> + return ret;
> +}

AXP288 has integrated fuel gauge which relies on VBATT and IBATT ADC 
measurements. If we disable the ADC then Fuel Gauge will get stuck.
So for the proper functioning of this integrated fuel gauging ADC measurements 
should always be ON.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 4/4] iio/adc/axp288: add support for axp288 gpadc

[Pallala, Ramakrishna]

Hi Jacob,

 +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME) static int 
 +axp288_gpadc_suspend(struct device *dev) {
 + int ret;
 + struct iio_dev *indio_dev = dev_get_drvdata(dev);
 + struct gpadc_info *info = iio_priv(indio_dev);
 +
 + mutex_lock(indio_dev-mlock);
 + ret = axp288_gpadc_enable(info-regmap, false);
 + mutex_unlock(indio_dev-mlock);
 +
 + return ret;
 +}

AXP288 has integrated fuel gauge which relies on VBATT and IBATT ADC 
measurements. If we disable the ADC then Fuel Gauge will get stuck.
So for the proper functioning of this integrated fuel gauging ADC measurements 
should always be ON.

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: how to include already compiled object files to kernel

[Pallala, Ramakrishna]

Hi Dirk/Everyone,

First of all I would like to apologize for sending this e-mail out on LKML. It 
was not intended to come here but was for an internal group. My mail client 
seems to have auto predicted the wrong group and I was stupid enough not to 
verify this before hitting the send button (I guess the late Friday push was 
too hard). 

I promise to be more careful next time.

Thanks & Regards,
Ram

-Original Message-
From: Hohndel, Dirk 
Sent: Saturday, April 26, 2014 3:31 AM
To: linux-kernel@vger.kernel.org
Cc: Pallala, Ramakrishna
Subject: Re: how to include already compiled object files to kernel

On Fri, 2014-04-25 at 16:24 +0000, Pallala, Ramakrishna wrote:
> HI All,
> 
> I have received few kernel library .o (object files) from third party and I 
> am planning to include in our kernel tree. As the library contains some 
> proprietary algorithms  the sources are not shared.
> 
> How can I include this library .o files to my kernel and if this kernel is 
> release to customers will there be any license issues? Third party says the 
> sources are under GPL.
> 

Oh joy. Just what I wanted to read today...

Our apologies; this is not a question or issue that should have been posted to 
LKML.  Legal questions (such as what is permitted by any particular license) by 
Intel employees are supposed to be directed to Intel's legal department and not 
to public mailing lists.  Intel also has a technical and business review 
process for reviewing *all* Open Source software activities, including all 
Linux kernel contributions, and the very first response from that review 
process would have been to direct questions about license interpretation and 
permissible licensing models to Intel's Legal Department.
 
We'll be following up with the development of this not-yet-released product to 
ensure that it is fully compliant with the GPL and other open source licenses 
as well as community-accepted practices, that it follows Intel's normal review 
processes, and that all the developers involved are familiar with Intel's 
policies for working with open source.
 
For those who have responded to the original mail, both privately and
publicly: thank you for your diligence and suggestions; we have been working 
internally for the last few hours (literally since WE saw it on the mailing 
list). But it's good to see that the community is as responsive to these issues 
as we are.

And now back to our regularly scheduled emergencies.

/D


--
Dirk Hohndel
Chief Linux and Open Source Technologist Intel Open Source Technology Center
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: how to include already compiled object files to kernel

[Pallala, Ramakrishna]

Hi Dirk/Everyone,

First of all I would like to apologize for sending this e-mail out on LKML. It 
was not intended to come here but was for an internal group. My mail client 
seems to have auto predicted the wrong group and I was stupid enough not to 
verify this before hitting the send button (I guess the late Friday push was 
too hard). 

I promise to be more careful next time.

Thanks  Regards,
Ram

-Original Message-
From: Hohndel, Dirk 
Sent: Saturday, April 26, 2014 3:31 AM
To: linux-kernel@vger.kernel.org
Cc: Pallala, Ramakrishna
Subject: Re: how to include already compiled object files to kernel

On Fri, 2014-04-25 at 16:24 +, Pallala, Ramakrishna wrote:
 HI All,
 
 I have received few kernel library .o (object files) from third party and I 
 am planning to include in our kernel tree. As the library contains some 
 proprietary algorithms  the sources are not shared.
 
 How can I include this library .o files to my kernel and if this kernel is 
 release to customers will there be any license issues? Third party says the 
 sources are under GPL.
 

Oh joy. Just what I wanted to read today...

Our apologies; this is not a question or issue that should have been posted to 
LKML.  Legal questions (such as what is permitted by any particular license) by 
Intel employees are supposed to be directed to Intel's legal department and not 
to public mailing lists.  Intel also has a technical and business review 
process for reviewing *all* Open Source software activities, including all 
Linux kernel contributions, and the very first response from that review 
process would have been to direct questions about license interpretation and 
permissible licensing models to Intel's Legal Department.
 
We'll be following up with the development of this not-yet-released product to 
ensure that it is fully compliant with the GPL and other open source licenses 
as well as community-accepted practices, that it follows Intel's normal review 
processes, and that all the developers involved are familiar with Intel's 
policies for working with open source.
 
For those who have responded to the original mail, both privately and
publicly: thank you for your diligence and suggestions; we have been working 
internally for the last few hours (literally since WE saw it on the mailing 
list). But it's good to see that the community is as responsive to these issues 
as we are.

And now back to our regularly scheduled emergencies.

/D


--
Dirk Hohndel
Chief Linux and Open Source Technologist Intel Open Source Technology Center
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

how to include already compiled object files to kernel

[Pallala, Ramakrishna]

HI All,

I have received few kernel library .o (object files) from third party and I am 
planning to include in our kernel tree. As the library contains some 
proprietary algorithms  the sources are not shared.

How can I include this library .o files to my kernel and if this kernel is 
release to customers will there be any license issues? Third party says the 
sources are under GPL.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

how to include already compiled object files to kernel

[Pallala, Ramakrishna]

HI All,

I have received few kernel library .o (object files) from third party and I am 
planning to include in our kernel tree. As the library contains some 
proprietary algorithms  the sources are not shared.

How can I include this library .o files to my kernel and if this kernel is 
release to customers will there be any license issues? Third party says the 
sources are under GPL.

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [Linaro-acpi] How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

>> > > And initialize the platform data in either driver or in separate 
>> > > module which gets compiled along with driver?
>> > 
>> > Typically it has been done in the same driver but I don't see any 
>> > problems having a separate module as well.
>> > 
>> > > static const struct acpi_device_id my_acpi_match[] = {
>> > >   { "MYID0001", (kernel_ulong_t)_platform_data1 }
>> > >   { "MYID0002", (kernel_ulong_t)_platform_data2 }
>> > >   ...
>> > >   { },
>> 
>> We definitely don't want per-board match entries, that does not scale.
>> The driver should be reasonably generic and get all the necessary data 
>> out of well-defined tables. You can have different IDs when there are 
>> only a few cases that are actually relevant, but it has to be 
>> conceivable that the same driver get used on future hardware without 
>> changes.
>
>Yes, I meant that when the platform data information is not available in ACPI 
>namespace, then (and only then) pass the data by means of different IDs.
>
>Preferably this information is included in the ACPI namespace.

The idea is use the single platform_data struct  and initiaze it accordingly 
from the driver's __init call based on the board/platform.

Thanks for your inputs.
Ram.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

>> In non ACPI environment I used to initialize the platform_data under 
>> board or platforms files. Under ACPI how do I do that?
>
>If you can't extract that information from ACPI namespace, then one option is 
>to pass platform data along with the device ACPI ID:
>
>static const struct acpi_device_id my_acpi_match[] = {
>   { "MYID0001", (kernel_ulong_t)_platform_data }
>   ...
>   { },
>};

Thanks for the Quick reply.

So If I  want to use different platform_data for different boards can I do 
something like below?
And initialize the platform data in either driver or in separate module which 
gets compiled along with driver?

static const struct acpi_device_id my_acpi_match[] = {
{ "MYID0001", (kernel_ulong_t)_platform_data1 }
{ "MYID0002", (kernel_ulong_t)_platform_data2 }
...
{ },

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

Hi All,

I am trying to enable a i2c client driver under ACPI. The device is being 
enumerated behind adapter device and I am getting IRQ resource as well.

The problem I have now is, how do I pass the platform data to driver?

struct i2c_board_info {
chartype[I2C_NAME_SIZE];
unsigned short  flags;
unsigned short  addr;
void*platform_data; ===> how can I initialize this 
filed.
struct dev_archdata *archdata;
struct device_node *of_node;
struct acpi_dev_node acpi_node;
int irq;
};
 
In non ACPI environment I used to initialize the platform_data under board or 
platforms files. Under ACPI how do I do that?

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [Linaro-acpi] How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

   And initialize the platform data in either driver or in separate 
   module which gets compiled along with driver?
  
  Typically it has been done in the same driver but I don't see any 
  problems having a separate module as well.
  
   static const struct acpi_device_id my_acpi_match[] = {
 { MYID0001, (kernel_ulong_t)my_platform_data1 }
 { MYID0002, (kernel_ulong_t)my_platform_data2 }
 ...
 { },
 
 We definitely don't want per-board match entries, that does not scale.
 The driver should be reasonably generic and get all the necessary data 
 out of well-defined tables. You can have different IDs when there are 
 only a few cases that are actually relevant, but it has to be 
 conceivable that the same driver get used on future hardware without 
 changes.

Yes, I meant that when the platform data information is not available in ACPI 
namespace, then (and only then) pass the data by means of different IDs.

Preferably this information is included in the ACPI namespace.

The idea is use the single platform_data struct  and initiaze it accordingly 
from the driver's __init call based on the board/platform.

Thanks for your inputs.
Ram.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

Hi All,

I am trying to enable a i2c client driver under ACPI. The device is being 
enumerated behind adapter device and I am getting IRQ resource as well.

The problem I have now is, how do I pass the platform data to driver?

struct i2c_board_info {
chartype[I2C_NAME_SIZE];
unsigned short  flags;
unsigned short  addr;
void*platform_data; === how can I initialize this 
filed.
struct dev_archdata *archdata;
struct device_node *of_node;
struct acpi_dev_node acpi_node;
int irq;
};
 
In non ACPI environment I used to initialize the platform_data under board or 
platforms files. Under ACPI how do I do that?

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to pass I2C platform_data under ACPI

[Pallala, Ramakrishna]

 In non ACPI environment I used to initialize the platform_data under 
 board or platforms files. Under ACPI how do I do that?

If you can't extract that information from ACPI namespace, then one option is 
to pass platform data along with the device ACPI ID:

static const struct acpi_device_id my_acpi_match[] = {
   { MYID0001, (kernel_ulong_t)my_platform_data }
   ...
   { },
};

Thanks for the Quick reply.

So If I  want to use different platform_data for different boards can I do 
something like below?
And initialize the platform data in either driver or in separate module which 
gets compiled along with driver?

static const struct acpi_device_id my_acpi_match[] = {
{ MYID0001, (kernel_ulong_t)my_platform_data1 }
{ MYID0002, (kernel_ulong_t)my_platform_data2 }
...
{ },

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

> > Mika, _BIF method returns battery data/struct which is defined Table. 
> > 10-233.
> 
> Yes, it was just an example. You can have your own custom method for that, 
> lets
> say:
> 
>   BLAH()
> 
> that then returns whatever you need.
> 
> > If I want to pass some device specific data seems like I can use
> > _DSM(9.14.1) method and pass custom /device specific data to the driver.
> 
> _DSM is also an option, yes.
> 
> > And I believe this data format can be anything and private to the device?
> > Can you confirm?
> 
> Yes.

Gr8. Appreciate your prompt response and help.
Have nice day :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

> > Mika, I want to populate this characterization data as device
> > specific/custom data which could be anything And may not be entirely
> > related to battery. Is this is possible?
> 
> Yes, for example you could have a custom ACPI method with your device which
> then returns this information.
> 
> See for example chapter 10.2.2.1 from the ACPI spec. It describes _BIF method
> that returns some battery data to the caller.

Mika, _BIF method returns battery data/struct which is defined Table. 10-233.

If I want to pass some device specific data seems like I can use _DSM(9.14.1) 
method and pass custom /device specific data to the driver.
And I believe this data format can be anything and private to the device? Can 
you confirm?

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

> > > > > > I am trying to populate battery related information through
> > > > > > ACPI tables and do battery management through non acpi
> > > > > > drivers.  Can you tell or point me on how to populate the ACPI
> > > > > > tables in FW/BIOS and get them in OS? I am new to ACPI world.
> > > > >
> > > > > You should start by reading Documentation/acpi/enumeration.txt.
> > > > >
> > > > > Also can you describe with bit more details, what you are trying
> > > > > to do? A new battery driver that is enumerated from ACPI namespace,
> perhaps?
> > > > >
> > > > > Do you have a DSDT table in hand which we could take a peek?
> > > >
> > > > Thanks Mika for the pointers. I have looked at the documentation.
> > > >
> > > > we already have i2c driver for battery monitoring but the chip
> > > > needs initialized with characterization data. We wanted to
> > > > populate/pass this data through ACPI table to the i2c slave driver.
> > > >
> > > > How can I do that? Can I pass characterization data through ACPI
> > > > table to the i2c slave device?
> > >
> > > Is the characterization data available in DSDT (or SSDT) table? If
> > > yes, then you can just use the fact that
> > > ACPI_HANDLE(_client->dev) returns a valid ACPI handle for your
> > > device. You can use this handle to call some ACPI method or whaterver is
> needed to extract that information.
> >
> > As of now this data is not there in any table. I want to put this data in 
> > DSDT
> table.
> > The data I want to put is specific to the device  and will be around 1K 
> > bytes.
> >
> > One question, How the table and device are linked? Is it by device name/id?
> 
> In principle, yes. We create devices based on the IDs on the DSDT table.
> 
> > Next, how to put this characterization data into the DSDT table?
> 
> Typically this is done by BIOS team but you can experiment yourself just by
> taking an existing DSDT, disassembling it with iasl, do the changes and
> reassembling it again. You can then include this with your kernel or initrd 
> image.
> 
> I have no idea how that data is supposed to look like or be represented in the
> DSDT.
> 
> You should also familiarize yourself with ACPI by reading the ACPI 5.0 spec 
> from
> acpi.info.

Mika, I want to populate this characterization data as device specific/custom 
data which could be anything
And may not be entirely related to battery. Is this is possible?

I will also go through the spec but this info would help me to resolve some 
initial hurdles in my work.

Note: I am not going to use ACPI based battery monitoring but I need this data 
from acpi tables.

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

> > > On Tue, Feb 19, 2013 at 03:07:57PM +, Pallala, Ramakrishna wrote:
> > > > I am trying to populate battery related information through ACPI
> > > > tables and do battery management through non acpi drivers.  Can
> > > > you tell or point me on how to populate the ACPI tables in FW/BIOS
> > > > and get them in OS? I am new to ACPI world.
> > >
> > > You should start by reading Documentation/acpi/enumeration.txt.
> > >
> > > Also can you describe with bit more details, what you are trying to
> > > do? A new battery driver that is enumerated from ACPI namespace, perhaps?
> > >
> > > Do you have a DSDT table in hand which we could take a peek?
> >
> > Thanks Mika for the pointers. I have looked at the documentation.
> >
> > we already have i2c driver for battery monitoring but the chip needs
> > initialized with characterization data. We wanted to populate/pass
> > this data through ACPI table to the i2c slave driver.
> >
> > How can I do that? Can I pass characterization data through ACPI table
> > to the i2c slave device?
> 
> Is the characterization data available in DSDT (or SSDT) table? If yes, then 
> you
> can just use the fact that ACPI_HANDLE(_client->dev) returns a valid ACPI
> handle for your device. You can use this handle to call some ACPI method or
> whaterver is needed to extract that information.

As of now this data is not there in any table. I want to put this data in DSDT 
table.
The data I want to put is specific to the device  and will be around 1K bytes.

One question, How the table and device are linked? Is it by device name/id?
Next, how to put this characterization data into the DSDT table?

Thanks,
Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

> On Tue, Feb 19, 2013 at 03:07:57PM +0000, Pallala, Ramakrishna wrote:
> > I am trying to populate battery related information through ACPI
> > tables and do battery management through non acpi drivers.  Can you
> > tell or point me on how to populate the ACPI tables in FW/BIOS and get
> > them in OS? I am new to ACPI world.
> 
> You should start by reading Documentation/acpi/enumeration.txt.
> 
> Also can you describe with bit more details, what you are trying to do? A new
> battery driver that is enumerated from ACPI namespace, perhaps?
> 
> Do you have a DSDT table in hand which we could take a peek?

Thanks Mika for the pointers. I have looked at the documentation.

we already have i2c driver for battery monitoring but the chip needs 
initialized with
characterization data. We wanted to populate/pass this data through ACPI table 
to the
i2c slave driver.

How can I do that? Can I pass characterization data through ACPI table to the 
i2c slave device?

Thanks,
Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

 On Tue, Feb 19, 2013 at 03:07:57PM +, Pallala, Ramakrishna wrote:
  I am trying to populate battery related information through ACPI
  tables and do battery management through non acpi drivers.  Can you
  tell or point me on how to populate the ACPI tables in FW/BIOS and get
  them in OS? I am new to ACPI world.
 
 You should start by reading Documentation/acpi/enumeration.txt.
 
 Also can you describe with bit more details, what you are trying to do? A new
 battery driver that is enumerated from ACPI namespace, perhaps?
 
 Do you have a DSDT table in hand which we could take a peek?

Thanks Mika for the pointers. I have looked at the documentation.

we already have i2c driver for battery monitoring but the chip needs 
initialized with
characterization data. We wanted to populate/pass this data through ACPI table 
to the
i2c slave driver.

How can I do that? Can I pass characterization data through ACPI table to the 
i2c slave device?

Thanks,
Ram

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

   On Tue, Feb 19, 2013 at 03:07:57PM +, Pallala, Ramakrishna wrote:
I am trying to populate battery related information through ACPI
tables and do battery management through non acpi drivers.  Can
you tell or point me on how to populate the ACPI tables in FW/BIOS
and get them in OS? I am new to ACPI world.
  
   You should start by reading Documentation/acpi/enumeration.txt.
  
   Also can you describe with bit more details, what you are trying to
   do? A new battery driver that is enumerated from ACPI namespace, perhaps?
  
   Do you have a DSDT table in hand which we could take a peek?
 
  Thanks Mika for the pointers. I have looked at the documentation.
 
  we already have i2c driver for battery monitoring but the chip needs
  initialized with characterization data. We wanted to populate/pass
  this data through ACPI table to the i2c slave driver.
 
  How can I do that? Can I pass characterization data through ACPI table
  to the i2c slave device?
 
 Is the characterization data available in DSDT (or SSDT) table? If yes, then 
 you
 can just use the fact that ACPI_HANDLE(i2c_client-dev) returns a valid ACPI
 handle for your device. You can use this handle to call some ACPI method or
 whaterver is needed to extract that information.

As of now this data is not there in any table. I want to put this data in DSDT 
table.
The data I want to put is specific to the device  and will be around 1K bytes.

One question, How the table and device are linked? Is it by device name/id?
Next, how to put this characterization data into the DSDT table?

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

  I am trying to populate battery related information through
  ACPI tables and do battery management through non acpi
  drivers.  Can you tell or point me on how to populate the ACPI
  tables in FW/BIOS and get them in OS? I am new to ACPI world.

 You should start by reading Documentation/acpi/enumeration.txt.

 Also can you describe with bit more details, what you are trying
 to do? A new battery driver that is enumerated from ACPI namespace,
 perhaps?

 Do you have a DSDT table in hand which we could take a peek?
   
Thanks Mika for the pointers. I have looked at the documentation.
   
we already have i2c driver for battery monitoring but the chip
needs initialized with characterization data. We wanted to
populate/pass this data through ACPI table to the i2c slave driver.
   
How can I do that? Can I pass characterization data through ACPI
table to the i2c slave device?
  
   Is the characterization data available in DSDT (or SSDT) table? If
   yes, then you can just use the fact that
   ACPI_HANDLE(i2c_client-dev) returns a valid ACPI handle for your
   device. You can use this handle to call some ACPI method or whaterver is
 needed to extract that information.
 
  As of now this data is not there in any table. I want to put this data in 
  DSDT
 table.
  The data I want to put is specific to the device  and will be around 1K 
  bytes.
 
  One question, How the table and device are linked? Is it by device name/id?
 
 In principle, yes. We create devices based on the IDs on the DSDT table.
 
  Next, how to put this characterization data into the DSDT table?
 
 Typically this is done by BIOS team but you can experiment yourself just by
 taking an existing DSDT, disassembling it with iasl, do the changes and
 reassembling it again. You can then include this with your kernel or initrd 
 image.
 
 I have no idea how that data is supposed to look like or be represented in the
 DSDT.
 
 You should also familiarize yourself with ACPI by reading the ACPI 5.0 spec 
 from
 acpi.info.

Mika, I want to populate this characterization data as device specific/custom 
data which could be anything
And may not be entirely related to battery. Is this is possible?

I will also go through the spec but this info would help me to resolve some 
initial hurdles in my work.

Note: I am not going to use ACPI based battery monitoring but I need this data 
from acpi tables.

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

  Mika, I want to populate this characterization data as device
  specific/custom data which could be anything And may not be entirely
  related to battery. Is this is possible?
 
 Yes, for example you could have a custom ACPI method with your device which
 then returns this information.
 
 See for example chapter 10.2.2.1 from the ACPI spec. It describes _BIF method
 that returns some battery data to the caller.

Mika, _BIF method returns battery data/struct which is defined Table. 10-233.

If I want to pass some device specific data seems like I can use _DSM(9.14.1) 
method and pass custom /device specific data to the driver.
And I believe this data format can be anything and private to the device? Can 
you confirm?

Thanks,
Ram
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: How to populate Battery information through ACPI tables

[Pallala, Ramakrishna]

  Mika, _BIF method returns battery data/struct which is defined Table. 
  10-233.
 
 Yes, it was just an example. You can have your own custom method for that, 
 lets
 say:
 
   BLAH()
 
 that then returns whatever you need.
 
  If I want to pass some device specific data seems like I can use
  _DSM(9.14.1) method and pass custom /device specific data to the driver.
 
 _DSM is also an option, yes.
 
  And I believe this data format can be anything and private to the device?
  Can you confirm?
 
 Yes.

Gr8. Appreciate your prompt response and help.
Have nice day :-)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

> > > > > +struct power_supply_charger_control {
> > > > > + const char *name;
> > > > > + /* get charging status */
> > > > > + int (*is_charging_enabled)(void);
> > > > > + int (*is_charger_enabled)(void);
> > > > > +
> > > > > + /* set charging parameters */
> > > > > + int (*set_in_current_limit)(int uA);
> > > > > + int (*set_charge_current)(int uA);
> > > > > + int (*set_charge_voltage)(int uV);
> > > > > +
> > > > > + /* control battery charging */
> > > > > + int (*enable_charging)(void);
> > > > > + int (*disable_charging)(void);
> > > > > +
> > > > > + /* control VSYS or system supply */
> > > > > + int (*turnon_charger)(void);
> > > > > + int (*turnoff_charger)(void);
> > > > > +};
> > > > > +
> > > >
> > > > I'm all for this patch, but why do you need to place it into
> > > > power_supply.h and power_supply_core.c? :) I see nothing generic
> > > > here, it's pure charger-manager stuff. So, place everything into
> > > > charger-
> > > manager.{c,h}.
> > >
> > > Hi Anton,
> > >
> > > The main reason for keeping this stuff in power_supply.h and
> > > power_supply_core.c is to make these interfaces uniform Across
> > > multiple charger frameworks and to avoid each charger framework
> > > define it's own interfaces. If there is need for new callback They
> > > can add to the existing struct defined above and it will available
> > > to all the frameworks. Also the work required to support a new
> > > Framework will be reduced if the driver already support any one of the
> existing frameworks.
> > >
> >
> > Rama,
> >
> > The similar functionalities are exposed by patch
> https://lkml.org/lkml/2012/10/18/219.
> > As per Anton's review comments on this patch, I'll be moving the macros to
> power_supply.h.
> > Wouldn't that be enough ?
> 
> Btw, how do these two sets relate to each other? Both seem to control chargers
> in some way... But yours approach uses properties, which I like more.
> 
> I guess you should coordinate on this?

Sure Anton we will sync up on this and get back to you. 
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

> > > > +struct power_supply_charger_control {
> > > > +   const char *name;
> > > > +   /* get charging status */
> > > > +   int (*is_charging_enabled)(void);
> > > > +   int (*is_charger_enabled)(void);
> > > > +
> > > > +   /* set charging parameters */
> > > > +   int (*set_in_current_limit)(int uA);
> > > > +   int (*set_charge_current)(int uA);
> > > > +   int (*set_charge_voltage)(int uV);
> > > > +
> > > > +   /* control battery charging */
> > > > +   int (*enable_charging)(void);
> > > > +   int (*disable_charging)(void);
> > > > +
> > > > +   /* control VSYS or system supply */
> > > > +   int (*turnon_charger)(void);
> > > > +   int (*turnoff_charger)(void);
> > > > +};
> > > > +
> > >
> > > I'm all for this patch, but why do you need to place it into
> > > power_supply.h and power_supply_core.c? :) I see nothing generic
> > > here, it's pure charger-manager stuff. So, place everything into
> > > charger-
> > manager.{c,h}.
> >
> > Hi Anton,
> >
> > The main reason for keeping this stuff in power_supply.h and
> > power_supply_core.c is to make these interfaces uniform Across
> > multiple charger frameworks and to avoid each charger framework define
> > it's own interfaces. If there is need for new callback They can add to
> > the existing struct defined above and it will available to all the
> > frameworks. Also the work required to support a new Framework will be
> > reduced if the driver already support any one of the existing frameworks.
> >
> 
> Rama,
> 
> The similar functionalities are exposed by patch
> https://lkml.org/lkml/2012/10/18/219.
> As per Anton's review comments on this patch, I'll be moving the macros to
> power_supply.h.
> Wouldn't that be enough ?

Though the macros seem to be fine but I would still think that call back way of 
interfaces would be
More flexible and straightforward.

Thanks,
Ram

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

> > +struct power_supply_charger_control {
> > +   const char *name;
> > +   /* get charging status */
> > +   int (*is_charging_enabled)(void);
> > +   int (*is_charger_enabled)(void);
> > +
> > +   /* set charging parameters */
> > +   int (*set_in_current_limit)(int uA);
> > +   int (*set_charge_current)(int uA);
> > +   int (*set_charge_voltage)(int uV);
> > +
> > +   /* control battery charging */
> > +   int (*enable_charging)(void);
> > +   int (*disable_charging)(void);
> > +
> > +   /* control VSYS or system supply */
> > +   int (*turnon_charger)(void);
> > +   int (*turnoff_charger)(void);
> > +};
> > +
> 
> I'm all for this patch, but why do you need to place it into power_supply.h 
> and
> power_supply_core.c? :) I see nothing generic here, it's pure charger-manager
> stuff. So, place everything into charger-manager.{c,h}.

Hi Anton,

The main reason for keeping this stuff in power_supply.h and 
power_supply_core.c is to make these interfaces uniform
Across multiple charger frameworks and to avoid each charger framework define 
it's own interfaces. If there is need for new callback
They can add to the existing struct defined above and it will available to all 
the frameworks. Also the work required to support a new
Framework will be reduced if the driver already support any one of the existing 
frameworks.
 
Thanks,
Ram

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

  +struct power_supply_charger_control {
  +   const char *name;
  +   /* get charging status */
  +   int (*is_charging_enabled)(void);
  +   int (*is_charger_enabled)(void);
  +
  +   /* set charging parameters */
  +   int (*set_in_current_limit)(int uA);
  +   int (*set_charge_current)(int uA);
  +   int (*set_charge_voltage)(int uV);
  +
  +   /* control battery charging */
  +   int (*enable_charging)(void);
  +   int (*disable_charging)(void);
  +
  +   /* control VSYS or system supply */
  +   int (*turnon_charger)(void);
  +   int (*turnoff_charger)(void);
  +};
  +
 
 I'm all for this patch, but why do you need to place it into power_supply.h 
 and
 power_supply_core.c? :) I see nothing generic here, it's pure charger-manager
 stuff. So, place everything into charger-manager.{c,h}.

Hi Anton,

The main reason for keeping this stuff in power_supply.h and 
power_supply_core.c is to make these interfaces uniform
Across multiple charger frameworks and to avoid each charger framework define 
it's own interfaces. If there is need for new callback
They can add to the existing struct defined above and it will available to all 
the frameworks. Also the work required to support a new
Framework will be reduced if the driver already support any one of the existing 
frameworks.
 
Thanks,
Ram

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

+struct power_supply_charger_control {
+   const char *name;
+   /* get charging status */
+   int (*is_charging_enabled)(void);
+   int (*is_charger_enabled)(void);
+
+   /* set charging parameters */
+   int (*set_in_current_limit)(int uA);
+   int (*set_charge_current)(int uA);
+   int (*set_charge_voltage)(int uV);
+
+   /* control battery charging */
+   int (*enable_charging)(void);
+   int (*disable_charging)(void);
+
+   /* control VSYS or system supply */
+   int (*turnon_charger)(void);
+   int (*turnoff_charger)(void);
+};
+
  
   I'm all for this patch, but why do you need to place it into
   power_supply.h and power_supply_core.c? :) I see nothing generic
   here, it's pure charger-manager stuff. So, place everything into
   charger-
  manager.{c,h}.
 
  Hi Anton,
 
  The main reason for keeping this stuff in power_supply.h and
  power_supply_core.c is to make these interfaces uniform Across
  multiple charger frameworks and to avoid each charger framework define
  it's own interfaces. If there is need for new callback They can add to
  the existing struct defined above and it will available to all the
  frameworks. Also the work required to support a new Framework will be
  reduced if the driver already support any one of the existing frameworks.
 
 
 Rama,
 
 The similar functionalities are exposed by patch
 https://lkml.org/lkml/2012/10/18/219.
 As per Anton's review comments on this patch, I'll be moving the macros to
 power_supply.h.
 Wouldn't that be enough ?

Though the macros seem to be fine but I would still think that call back way of 
interfaces would be
More flexible and straightforward.

Thanks,
Ram

RE: [PATCH 1/2] power_supply: Add charge control struct in power supply class

[Pallala, Ramakrishna]

 +struct power_supply_charger_control {
 + const char *name;
 + /* get charging status */
 + int (*is_charging_enabled)(void);
 + int (*is_charger_enabled)(void);
 +
 + /* set charging parameters */
 + int (*set_in_current_limit)(int uA);
 + int (*set_charge_current)(int uA);
 + int (*set_charge_voltage)(int uV);
 +
 + /* control battery charging */
 + int (*enable_charging)(void);
 + int (*disable_charging)(void);
 +
 + /* control VSYS or system supply */
 + int (*turnon_charger)(void);
 + int (*turnoff_charger)(void);
 +};
 +
   
I'm all for this patch, but why do you need to place it into
power_supply.h and power_supply_core.c? :) I see nothing generic
here, it's pure charger-manager stuff. So, place everything into
charger-
   manager.{c,h}.
  
   Hi Anton,
  
   The main reason for keeping this stuff in power_supply.h and
   power_supply_core.c is to make these interfaces uniform Across
   multiple charger frameworks and to avoid each charger framework
   define it's own interfaces. If there is need for new callback They
   can add to the existing struct defined above and it will available
   to all the frameworks. Also the work required to support a new
   Framework will be reduced if the driver already support any one of the
 existing frameworks.
  
 
  Rama,
 
  The similar functionalities are exposed by patch
 https://lkml.org/lkml/2012/10/18/219.
  As per Anton's review comments on this patch, I'll be moving the macros to
 power_supply.h.
  Wouldn't that be enough ?
 
 Btw, how do these two sets relate to each other? Both seem to control chargers
 in some way... But yours approach uses properties, which I like more.
 
 I guess you should coordinate on this?

Sure Anton we will sync up on this and get back to you. 
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i