2013-03-04 19:01:05

by Rhyland Klein

[permalink] [raw]
Subject: [PATCH v2 0/4] Add support for tps65090-charger

This patchset adds support for the TPS65090-charger. This charger is
registered as a subdevice of the TPS65090 PMIC.

This series includes a fix for the tps65090 header file where all
the interrupts were shifted by 1.

Changes since v1:
- cleaned up tps65090 driver per comments (mostly var naming)
- flushed out commit description for driver
- reordered probe so irq is registered after power_supply and cleaned up 1st

Rhyland Klein (4):
mfd: tps65090: Fix enum in header file
mfd: tps65090: Add resources for charger
power_supply: tps65090-charger: Add binding doc
power: tps65090: Add support for tps65090-charger

.../devicetree/bindings/power_supply/tps65090.txt | 23 ++
drivers/mfd/tps65090.c | 10 +
drivers/power/Kconfig | 7 +
drivers/power/Makefile | 1 +
drivers/power/tps65090-charger.c | 316 ++++++++++++++++++++
include/linux/mfd/tps65090.h | 6 +
6 files changed, 363 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt
create mode 100644 drivers/power/tps65090-charger.c

--
1.7.9.5


2013-03-04 19:01:07

by Rhyland Klein

[permalink] [raw]
Subject: [PATCH v2 1/4] mfd: tps65090: Fix enum in header file

The enum is missing the definition for the first bit, which makes all
the rest off by one. Add definition for the TPS65090_IRQ_INTERRUPT bit
which at 0.

Signed-off-by: Rhyland Klein <[email protected]>
---
v2:
- no changes since v1

include/linux/mfd/tps65090.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 6694cf4..9ce231f 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -27,6 +27,7 @@

/* TPS65090 IRQs */
enum {
+ TPS65090_IRQ_INTERRUPT,
TPS65090_IRQ_VAC_STATUS_CHANGE,
TPS65090_IRQ_VSYS_STATUS_CHANGE,
TPS65090_IRQ_BAT_STATUS_CHANGE,
--
1.7.9.5

2013-03-04 19:01:31

by Rhyland Klein

[permalink] [raw]
Subject: [PATCH v2 3/4] power_supply: tps65090-charger: Add binding doc

This change adds the binding documentation for the tps65090-charger.

Signed-off-by: Rhyland Klein <[email protected]>
---
v2:
- no changes since v1

.../devicetree/bindings/power_supply/tps65090.txt | 23 ++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt

diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
new file mode 100644
index 0000000..56370c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/tps65090.txt
@@ -0,0 +1,23 @@
+TPS65090 Frontend PMU with Switchmode Charger
+
+Required Properties:
+-compatible: "ti,tps65090"
+-reg: I2C slave address
+-interrupts: the interrupt output to which this device connects
+
+Optional Properties:
+-ti,enable-low-current-chrg: Enables charging when a low current is detected
+ while the default logic is to stop charging.
+
+Example:
+
+ tps65090@48 {
+ compatible = "ti,tps65090";
+ reg = <0x48>;
+ interrupts = <0 88 0x4>;
+
+ ti,enable-low-current-chrg;
+
+ regulators {
+ ...
+ };
--
1.7.9.5

2013-03-04 19:01:38

by Rhyland Klein

[permalink] [raw]
Subject: [PATCH v2 4/4] power: tps65090: Add support for tps65090-charger

This patch adds support for the tps65090 charger driver. This driver is
responsible for controlling the charger aspect of the tps65090 mfd.
Currently, this mainly consists of turning on and off the charger, but
some features of the charger can be supported through this driver
including:

Enable Auto Recharge based on Battery voltage
Fast Charge Safety Timer
Maximum battery discharge current
Maximum battery adapter current
Enable External Charge
Disable charging termination based on low charger current (supported)

Once the driver is accepted, later patches can add support for the
features above which are not yet supported.

Based on work by:
Syed Rafiuddin <[email protected]>
Laxman Dewangan <[email protected]>

Signed-off-by: Rhyland Klein <[email protected]>
---
v2:
- updated commit description with more info
- cleaned up misc comments (var names, spacing, etc)
- reordered probe around irq so it is cleaner

drivers/power/Kconfig | 7 +
drivers/power/Makefile | 1 +
drivers/power/tps65090-charger.c | 316 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/tps65090.h | 5 +
4 files changed, 329 insertions(+)
create mode 100644 drivers/power/tps65090-charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 9e00c38..0c5d336 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -340,6 +340,13 @@ config CHARGER_SMB347
Say Y to include support for Summit Microelectronics SMB347
Battery Charger.

+config CHARGER_TPS65090
+ tristate "TPS65090 battery charger driver"
+ depends on MFD_TPS65090
+ help
+ Say Y here to enable support for battery charging with TPS65090
+ PMIC chips.
+
config AB8500_BM
bool "AB8500 Battery Management Driver"
depends on AB8500_CORE && AB8500_GPADC
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 3f66436..8720987 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -53,4 +53,5 @@ obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o
obj-$(CONFIG_POWER_AVS) += avs/
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
+obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
obj-$(CONFIG_POWER_RESET) += reset/
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
new file mode 100644
index 0000000..408ea65
--- /dev/null
+++ b/drivers/power/tps65090-charger.c
@@ -0,0 +1,316 @@
+/*
+ * Battery charger driver for TI's tps65090
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION. All rights reserved.
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope 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.
+
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/mfd/tps65090.h>
+
+#define TPS65090_REG_INTR_STS 0x00
+#define TPS65090_REG_CG_CTRL0 0x04
+#define TPS65090_REG_CG_CTRL1 0x05
+#define TPS65090_REG_CG_CTRL2 0x06
+#define TPS65090_REG_CG_CTRL3 0x07
+#define TPS65090_REG_CG_CTRL4 0x08
+#define TPS65090_REG_CG_CTRL5 0x09
+#define TPS65090_REG_CG_STATUS1 0x0a
+#define TPS65090_REG_CG_STATUS2 0x0b
+
+#define TPS65090_CHARGER_ENABLE BIT(0)
+#define TPS65090_VACG BIT(1)
+#define TPS65090_NOITERM BIT(5)
+
+struct tps65090_charger {
+ struct device *dev;
+ int ac_online;
+ int prev_ac_online;
+ int irq;
+ struct power_supply ac;
+ struct tps65090_platform_data *pdata;
+};
+
+static enum power_supply_property tps65090_ac_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int tps65090_low_chrg_current(struct tps65090_charger *charger)
+{
+ int ret;
+
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
+ TPS65090_NOITERM);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL5);
+ return ret;
+ }
+ return 0;
+}
+
+static int tps65090_enable_charging(struct tps65090_charger *charger,
+ uint8_t enable)
+{
+ int ret;
+ uint8_t ctrl0 = 0;
+
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+ &ctrl0);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+
+ ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+ (ctrl0 | TPS65090_CHARGER_ENABLE));
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+ __func__, TPS65090_REG_CG_CTRL0);
+ return ret;
+ }
+ return 0;
+}
+
+static int tps65090_config_charger(struct tps65090_charger *charger)
+{
+ int ret;
+
+ if (charger->pdata->enable_low_current_chrg) {
+ ret = tps65090_low_chrg_current(charger);
+ if (ret < 0) {
+ dev_err(charger->dev,
+ "error configuring low charge current\n");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int tps65090_ac_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct tps65090_charger *charger = container_of(psy,
+ struct tps65090_charger, ac);
+
+ if (psp == POWER_SUPPLY_PROP_ONLINE) {
+ val->intval = charger->ac_online;
+ charger->prev_ac_online = charger->ac_online;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
+{
+ struct tps65090_charger *charger = dev_id;
+ int ret;
+ uint8_t status1 = 0;
+ uint8_t intrsts = 0;
+
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_STATUS1,
+ &status1);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+ __func__, TPS65090_REG_CG_STATUS1);
+ return IRQ_HANDLED;
+ }
+ msleep(75);
+ ret = tps65090_read(charger->dev->parent, TPS65090_REG_INTR_STS,
+ &intrsts);
+ if (ret < 0) {
+ dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+ __func__, TPS65090_REG_INTR_STS);
+ return IRQ_HANDLED;
+ }
+
+
+ if (intrsts & TPS65090_VACG) {
+ ret = tps65090_enable_charging(charger, 1);
+ if (ret < 0)
+ return IRQ_HANDLED;
+ charger->ac_online = 1;
+ } else {
+ charger->ac_online = 0;
+ }
+
+ if (charger->prev_ac_online != charger->ac_online)
+ power_supply_changed(&charger->ac);
+
+ return IRQ_HANDLED;
+}
+
+#if defined(CONFIG_OF)
+
+#include <linux/of_device.h>
+
+static struct tps65090_platform_data *
+ tps65090_parse_dt_charger_data(struct platform_device *pdev)
+{
+ struct tps65090_platform_data *pdata;
+ struct device_node *np = pdev->dev.parent->of_node;
+ unsigned int prop;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(&pdev->dev, "Memory alloc for tps65090_pdata failed\n");
+ return NULL;
+ }
+
+ prop = of_property_read_bool(np, "ti,enable-low-current-chrg");
+ pdata->enable_low_current_chrg = prop;
+
+ pdata->irq_base = -1;
+
+ return pdata;
+
+}
+#else
+static struct tps65090_platform_data *
+ tps65090_parse_dt_charger_data(struct platform_device *pdev)
+{
+ return NULL;
+}
+#endif
+
+static int tps65090_charger_probe(struct platform_device *pdev)
+{
+ struct tps65090 *tps65090_mfd = dev_get_drvdata(pdev->dev.parent);
+ struct tps65090_charger *cdata;
+ struct tps65090_platform_data *pdata;
+ uint8_t status1 = 0;
+ int ret;
+ int irq;
+
+ pdata = dev_get_platdata(pdev->dev.parent);
+
+ if (!pdata && tps65090_mfd->dev->of_node)
+ pdata = tps65090_parse_dt_charger_data(pdev);
+
+ if (!pdata) {
+ dev_err(&pdev->dev, "%s():no platform data available\n",
+ __func__);
+ return -ENODEV;
+ }
+
+ cdata = devm_kzalloc(&pdev->dev, sizeof(*cdata), GFP_KERNEL);
+ if (!cdata) {
+ dev_err(&pdev->dev, "failed to allocate memory status\n");
+ return -ENOMEM;
+ }
+
+ dev_set_drvdata(&pdev->dev, cdata);
+
+ cdata->dev = &pdev->dev;
+ cdata->pdata = pdata;
+
+ cdata->ac.name = "tps65090-ac";
+ cdata->ac.type = POWER_SUPPLY_TYPE_MAINS;
+ cdata->ac.get_property = tps65090_ac_get_property;
+ cdata->ac.properties = tps65090_ac_props;
+ cdata->ac.num_properties = ARRAY_SIZE(tps65090_ac_props);
+ cdata->ac.supplied_to = pdata->supplied_to;
+ cdata->ac.num_supplicants = pdata->num_supplicants;
+
+ ret = power_supply_register(&pdev->dev, &cdata->ac);
+ if (ret) {
+ dev_err(&pdev->dev, "failed: power supply register\n");
+ return ret;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
+ ret = irq;
+ goto fail_unregister_supply;
+ }
+
+ cdata->irq = irq;
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ tps65090_charger_isr, 0, "tps65090-charger", cdata);
+ if (ret) {
+ dev_err(cdata->dev, "Unable to register irq %d err %d\n", irq,
+ ret);
+ goto fail_unregister_supply;
+ }
+
+ ret = tps65090_config_charger(cdata);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
+ goto fail_unregister_supply;
+ }
+
+ /* Check for charger presence */
+ ret = tps65090_read(cdata->dev->parent, TPS65090_REG_CG_STATUS1,
+ &status1);
+ if (ret < 0) {
+ dev_err(cdata->dev, "%s(): Error in reading reg 0x%x", __func__,
+ TPS65090_REG_CG_STATUS1);
+ goto fail_unregister_supply;
+ }
+
+ if (status1 != 0) {
+ ret = tps65090_enable_charging(cdata, 1);
+ if (ret < 0) {
+ dev_err(cdata->dev, "error enabling charger\n");
+ goto fail_unregister_supply;
+ }
+ cdata->ac_online = 1;
+ power_supply_changed(&cdata->ac);
+ }
+
+ return 0;
+
+fail_free_irq:
+ devm_free_irq(irq);
+fail_unregister_supply:
+ power_supply_unregister(&cdata->ac);
+
+ return ret;
+}
+
+static int tps65090_charger_remove(struct platform_device *pdev)
+{
+ struct tps65090_charger *cdata = dev_get_drvdata(&pdev->dev);
+
+ devm_free_irq(cdata->irq);
+ power_supply_unregister(&cdata->ac);
+
+ return 0;
+}
+
+static struct platform_driver tps65090_charger_driver = {
+ .driver = {
+ .name = "tps65090-charger",
+ .owner = THIS_MODULE,
+ },
+ .probe = tps65090_charger_probe,
+ .remove = tps65090_charger_remove,
+};
+module_platform_driver(tps65090_charger_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Syed Rafiuddin <[email protected]>");
+MODULE_DESCRIPTION("tps65090 battery charger driver");
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 9ce231f..3f43069 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -87,6 +87,11 @@ struct tps65090_regulator_plat_data {

struct tps65090_platform_data {
int irq_base;
+
+ char **supplied_to;
+ size_t num_supplicants;
+ int enable_low_current_chrg;
+
struct tps65090_regulator_plat_data *reg_pdata[TPS65090_REGULATOR_MAX];
};

--
1.7.9.5

2013-03-04 19:01:36

by Rhyland Klein

[permalink] [raw]
Subject: [PATCH v2 2/4] mfd: tps65090: Add resources for charger

Add irq resources to pass to the charger mfd sub dev so
the charger can listen for interrupts.

Signed-off-by: Rhyland Klein <[email protected]>
---
v2:
- no changes since v1

drivers/mfd/tps65090.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index 98edb5be..88846ae 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -56,12 +56,22 @@
#define TPS65090_INT2_MASK_OVERLOAD_FET6 6
#define TPS65090_INT2_MASK_OVERLOAD_FET7 7

+static struct resource charger_resources[] = {
+ {
+ .start = TPS65090_IRQ_VAC_STATUS_CHANGE,
+ .end = TPS65090_IRQ_VAC_STATUS_CHANGE,
+ .flags = IORESOURCE_IRQ,
+ }
+};
+
static struct mfd_cell tps65090s[] = {
{
.name = "tps65090-pmic",
},
{
.name = "tps65090-charger",
+ .num_resources = ARRAY_SIZE(charger_resources),
+ .resources = &charger_resources[0],
},
};

--
1.7.9.5

2013-03-05 18:15:44

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] power_supply: tps65090-charger: Add binding doc

On 03/04/2013 12:01 PM, Rhyland Klein wrote:
> This change adds the binding documentation for the tps65090-charger.

> diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
...
> +Example:
> +
> + tps65090@48 {
...
> + regulators {
> + ...
> + };
>

The "regulators" node in the example isn't mentioned in the list of
properties/nodes that's above. What goes in there? You probably want to
include text similar to what I've quoted below from
Documentation/devicetree/bindings/regulator/tps6586x.txt:

> - regulators: A node that houses a sub-node for each regulator within the
> device. Each sub-node is identified using the node's name (or the deprecated
> regulator-compatible property if present), with valid values listed below.
> The content of each sub-node is defined by the standard binding for
> regulators; see regulator.txt.
> sys, sm[0-2], ldo[0-9] and ldo_rtc

2013-03-05 19:12:43

by Rhyland Klein

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] power_supply: tps65090-charger: Add binding doc

On 3/5/2013 1:15 PM, Stephen Warren wrote:
> On 03/04/2013 12:01 PM, Rhyland Klein wrote:
>> This change adds the binding documentation for the tps65090-charger.
>> diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
> ...
>> +Example:
>> +
>> + tps65090@48 {
> ...
>> + regulators {
>> + ...
>> + };
>>
> The "regulators" node in the example isn't mentioned in the list of
> properties/nodes that's above. What goes in there? You probably want to
> include text similar to what I've quoted below from
> Documentation/devicetree/bindings/regulator/tps6586x.txt:
>
>> - regulators: A node that houses a sub-node for each regulator within the
>> device. Each sub-node is identified using the node's name (or the deprecated
>> regulator-compatible property if present), with valid values listed below.
>> The content of each sub-node is defined by the standard binding for
>> regulators; see regulator.txt.
>> sys, sm[0-2], ldo[0-9] and ldo_rtc
The reason I didn't bother documenting the regulators node was that
since this is a child device
driver of an mfd device, there is already a child driver for the
regulators with its own documentation

https://patchwork.kernel.org/patch/2051381/

I wasn't sure how I should handle this, as splitting the bindings to
make logic sense in the binding
layout (charger under power_supply, and regulators under regulator) or
combine them somehow
into a single documentation entry common to the device. The latter seems
to make more sense to me,
but since there aren't any dt specific entries for the core mfd part
currently, it doesn't have its own
documentation, and sticking the charger info under the regulators seemed
backwards to me.

I thought doing it this way would be a good way of starting a discussion
around how to handle this.

thanks for starting it Stephen :)

-rhyland

--
nvpublic

2013-03-05 19:25:35

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] power_supply: tps65090-charger: Add binding doc

On 03/05/2013 12:12 PM, Rhyland Klein wrote:
> On 3/5/2013 1:15 PM, Stephen Warren wrote:
>> On 03/04/2013 12:01 PM, Rhyland Klein wrote:
>>> This change adds the binding documentation for the tps65090-charger.
>>> diff --git
>>> a/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>> b/Documentation/devicetree/bindings/power_supply/tps65090.txt
>> ...
>>> +Example:
>>> +
>>> + tps65090@48 {
>> ...
>>> + regulators {
>>> + ...
>>> + };
>>
>> The "regulators" node in the example isn't mentioned in the list of
>> properties/nodes that's above. What goes in there? You probably want to
>> include text similar to what I've quoted below from
>> Documentation/devicetree/bindings/regulator/tps6586x.txt:
>>
>>> - regulators: A node that houses a sub-node for each regulator within the
>>> device. Each sub-node is identified using the node's name (or the deprecated
>>> regulator-compatible property if present), with valid values listed below.
>>> The content of each sub-node is defined by the standard binding for
>>> regulators; see regulator.txt.
>>> sys, sm[0-2], ldo[0-9] and ldo_rtc
>
> The reason I didn't bother documenting the regulators node was that
> since this is a child device
> driver of an mfd device, there is already a child driver for the
> regulators with its own documentation
>
> https://patchwork.kernel.org/patch/2051381/

Ah, I see.

> I wasn't sure how I should handle this, as splitting the bindings to
> make logic sense in the binding
> layout (charger under power_supply, and regulators under regulator) or
> combine them somehow
> into a single documentation entry common to the device. The latter seems
> to make more sense to me,

Yes, given we're talking about properties in the same node, rather than
a binding for a new child node that could be plugged into arbitrary
parent nodes, I think everything should be documented in a single file.

> but since there aren't any dt specific entries for the core mfd part
> currently, it doesn't have its own
> documentation, and sticking the charger info under the regulators seemed
> backwards to me.

Hmmm. That's a good question. I'm not really sure where the best
location for that file would be. Admittedly regulators does seem
slightly over-specific, but short of creating a new bindings/mfd/
directory, it doesn't seem to unreasonable to just put the whole binding
in the existing file in bindings/regulators/.

Perhaps Grant or Rob can comment on what their preference would be.