RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-09 Thread Jun Li

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: 2018年4月5日 4:13
> To: Mats Karrman <mats.dev.l...@gmail.com>; Jun Li <jun...@nxp.com>;
> gre...@linuxfoundation.org; robh...@kernel.org; mark.rutl...@arm.com;
> heikki.kroge...@linux.intel.com
> Cc: li...@roeck-us.net; rmf...@gmail.com; yueyao@gmail.com;
> linux-usb@vger.kernel.org; devicet...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi,
> 
> On 04-04-18 14:06, Mats Karrman wrote:
> > Hi Li,
> >
> > On 2018-03-23 15:58, Li Jun wrote:
> >
> >> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> >> variable PDO for sink config.
> >>
> >> Signed-off-by: Li Jun <jun...@nxp.com>
> >> ---
> >>   drivers/usb/typec/fusb302/fusb302.c | 51
> >> +++--
> >>   1 file changed, 37 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/fusb302/fusb302.c
> >> b/drivers/usb/typec/fusb302/fusb302.c
> >> index 7036171..db4d9cd 100644
> >> --- a/drivers/usb/typec/fusb302/fusb302.c
> >> +++ b/drivers/usb/typec/fusb302/fusb302.c
> >> @@ -120,6 +120,7 @@ struct fusb302_chip {
> >>   enum typec_cc_polarity cc_polarity;
> >>   enum typec_cc_status cc1;
> >>   enum typec_cc_status cc2;
> >> +    u32 snk_pdo[PDO_MAX_OBJECTS];
> >>   #ifdef CONFIG_DEBUG_FS
> >>   struct dentry *dentry;
> >> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >>   static const struct tcpc_config fusb302_tcpc_config = {
> >>   .src_pdo = src_pdo,
> >>   .nr_src_pdo = ARRAY_SIZE(src_pdo),
> >> -    .snk_pdo = snk_pdo,
> >> -    .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> >> -    .max_snk_mv = 5000,
> >> -    .max_snk_ma = 3000,
> >> -    .max_snk_mw = 15000,
> >>   .operating_snk_mw = 2500,
> >>   .type = TYPEC_PORT_DRP,
> >>   .data = TYPEC_PORT_DRD,
> >> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip
> >> *chip)
> >>   return 0;
> >>   }
> >> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip
> >> +*chip) {
> >> +    struct device *dev = chip->dev;
> >> +    u32 mv, ma, mw, min_mv;
> >> +    unsigned int i;
> >> +
> >> +    /* Copy the static snk pdo */
> >> +    for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> >> +    chip->snk_pdo[i] = snk_pdo[i];
> >> +
> >> +    if (device_property_read_u32(dev, "fcs,max-sink-microvolt", )
> >> +||
> >> +    device_property_read_u32(dev, "fcs,max-sink-microamp", )
> >> +||
> >> +    device_property_read_u32(dev, "fcs,max-sink-microwatt",
> >> +))
> >> +    return i;
> >> +
> >> +    mv = mv / 1000;
> >> +    ma = ma / 1000;
> >> +    mw = mw / 1000;
> >> +
> >> +    min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
> >> +    if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
> >
> > You've got the parentheses wrong.
> >
> > Apart from that I don't like/understand why the PDO's should be fixed.
> > Every product should be able to have its own settings, including the
> > first PDO (e.g. it might need to specify a higher current and/or set the 
> > "Higher
> Capability" bit).
> >
> > I think this would be best solved the same way as in your TCPCI driver
> > patch series [1] with a list freely specified by a property.
> >
> > [1]
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> > .spinics.net%2Flists%2Flinux-usb%2Fmsg167398.html=02%7C01%7Cjun.l
> >
> i%40nxp.com%7C678fb35afff542581d3808d59a68708b%7C686ea1d3bc2b4c6fa92c
> d
> >
> 99c5c301635%7C0%7C0%7C636584695710093111=ZyAJQUR8ZbAq8VWsZkF
> PGLg
> > F9P5j5QpvF3yeGyNoyH8%3D=0
> 
> Hmm, interesting, for the x86 use-case that would require updating the
> properties for the fusb302 defined in:
> 
> drivers/platform/x86/intel_cht_int33fe.c
> 
> In tandem, which is easily doable.
> 
> But what about other users of the fusb302 driver? Since this driver was added
> before I started using it for some x86 boards, I assume there are some non x86
> users, so we should preserve compatibility for DTB files which don't define 
> any
> sink PDOs in their properties, I gue

RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-09 Thread Jun Li
Hi
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年4月4日 20:07
> To: Jun Li <jun...@nxp.com>; gre...@linuxfoundation.org; robh...@kernel.org;
> mark.rutl...@arm.com; heikki.kroge...@linux.intel.com;
> hdego...@redhat.com
> Cc: li...@roeck-us.net; rmf...@gmail.com; yueyao@gmail.com;
> linux-usb@vger.kernel.org; devicet...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi Li,
> 
> On 2018-03-23 15:58, Li Jun wrote:
> 
> > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> > variable PDO for sink config.
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >   drivers/usb/typec/fusb302/fusb302.c | 51
> +++--
> >   1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c
> > b/drivers/usb/typec/fusb302/fusb302.c
> > index 7036171..db4d9cd 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -120,6 +120,7 @@ struct fusb302_chip {
> > enum typec_cc_polarity cc_polarity;
> > enum typec_cc_status cc1;
> > enum typec_cc_status cc2;
> > +   u32 snk_pdo[PDO_MAX_OBJECTS];
> >
> >   #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >   static const struct tcpc_config fusb302_tcpc_config = {
> > .src_pdo = src_pdo,
> > .nr_src_pdo = ARRAY_SIZE(src_pdo),
> > -   .snk_pdo = snk_pdo,
> > -   .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> > -   .max_snk_mv = 5000,
> > -   .max_snk_ma = 3000,
> > -   .max_snk_mw = 15000,
> > .operating_snk_mw = 2500,
> > .type = TYPEC_PORT_DRP,
> > .data = TYPEC_PORT_DRD,
> > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
> > return 0;
> >   }
> >
> > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> > +{
> > +   struct device *dev = chip->dev;
> > +   u32 mv, ma, mw, min_mv;
> > +   unsigned int i;
> > +
> > +   /* Copy the static snk pdo */
> > +   for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> > +   chip->snk_pdo[i] = snk_pdo[i];
> > +
> > +   if (device_property_read_u32(dev, "fcs,max-sink-microvolt", ) ||
> > +   device_property_read_u32(dev, "fcs,max-sink-microamp", ) ||
> > +   device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
> > +   return i;
> > +
> > +   mv = mv / 1000;
> > +   ma = ma / 1000;
> > +   mw = mw / 1000;
> > +
> > +   min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
> > +   if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
> 
> You've got the parentheses wrong.

Good catch.

> 
> Apart from that I don't like/understand why the PDO's should be fixed.

This is only for legacy setting(a fixed PDO) and properties(fcs,max-sink-*),
to make it can still work after using the new PDO matching policy.

> Every product should be able to have its own settings, including the first 
> PDO (e.g.
> it might need to specify a higher current and/or set the "Higher Capability" 
> bit).
> 
> I think this would be best solved the same way as in your TCPCI driver patch
> series [1] with a list freely specified by a property.
> 
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.s
> pinics.net%2Flists%2Flinux-usb%2Fmsg167398.html=02%7C01%7Cjun.li%40
> nxp.com%7C94bfdd71fa87479f54c808d59a24873a%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C636584404043415663=RbOoMtJM9XtmvhHYNQ9a
> nhPMn%2BJpdqwY3UagRPCgM9k%3D=0

Agreed, new code(dt) should use[1] to describe the power properties.
Fusb302 driver just need pass a fwnode at init, tcpm will read and parse
all the properties when register port.

Thanks
Jun
> 
> > +   min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
> > +   else
> > +   min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
> > +   ma = min(ma, 1000 * mw / min_mv);
> > +
> > +   /* Insert the new pdo to the end */
> > +   chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
> > +
> > +   return i+1;
> > +}
> > +
> >   static int fusb302_probe(struct i2c_client *client,
> >  const struct i2c_device_id *id)
> >   {
> > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
> > chip->tcpc_dev.config = >tcpc_co

RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-09 Thread Jun Li
Hi
> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: 2018年4月3日 23:26
> To: Jun Li <jun...@nxp.com>; gre...@linuxfoundation.org; robh...@kernel.org;
> mark.rutl...@arm.com; heikki.kroge...@linux.intel.com
> Cc: li...@roeck-us.net; rmf...@gmail.com; yueyao@gmail.com;
> linux-usb@vger.kernel.org; devicet...@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi,
> 
> On 23-03-18 15:58, Li Jun wrote:
> > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> > variable PDO for sink config.
> >
> > Signed-off-by: Li Jun <jun...@nxp.com>
> > ---
> >   drivers/usb/typec/fusb302/fusb302.c | 51
> +++--
> >   1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c
> > b/drivers/usb/typec/fusb302/fusb302.c
> > index 7036171..db4d9cd 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -120,6 +120,7 @@ struct fusb302_chip {
> > enum typec_cc_polarity cc_polarity;
> > enum typec_cc_status cc1;
> > enum typec_cc_status cc2;
> > +   u32 snk_pdo[PDO_MAX_OBJECTS];
> >
> >   #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >   static const struct tcpc_config fusb302_tcpc_config = {
> > .src_pdo = src_pdo,
> > .nr_src_pdo = ARRAY_SIZE(src_pdo),
> > -   .snk_pdo = snk_pdo,
> > -   .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> > -   .max_snk_mv = 5000,
> > -   .max_snk_ma = 3000,
> > -   .max_snk_mw = 15000,
> > .operating_snk_mw = 2500,
> > .type = TYPEC_PORT_DRP,
> > .data = TYPEC_PORT_DRD,
> > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
> > return 0;
> >   }
> >
> > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> > +{
> > +   struct device *dev = chip->dev;
> > +   u32 mv, ma, mw, min_mv;
> > +   unsigned int i;
> > +
> > +   /* Copy the static snk pdo */
> > +   for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> > +   chip->snk_pdo[i] = snk_pdo[i];
> 
> The static snk PDO is constant and only contains:
>   PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> 
> So you can just do:
> 
>   chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);
> 
> Here.
> 
> > +
> > +   if (device_property_read_u32(dev, "fcs,max-sink-microvolt", ) ||
> > +   device_property_read_u32(dev, "fcs,max-sink-microamp", ) ||
> > +   device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
> 
> You can drop the reading of "fcs,max-sink-microwatt" here, it was only ever
> added because the tcpm code used to depend on max_mw being set, now that
> that is no longer the case, we don't need it.
> 
> So this entire function can be simplified to:
> 
> static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) {
>   struct device *dev = chip->dev;
>   u32 max_uv, max_ua;
> 
>   chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);
> 
>   if (device_property_read_u32(dev, "fcs,max-sink-microvolt", _uv) ||
>   device_property_read_u32(dev, "fcs,max-sink-microamp", _ua))
>   return 1;
> 
>   chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000);
>   return 2;
> }

OK, I will change in v3.

Thanks
Jun

> 
> > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
> > chip->tcpc_dev.config = >tcpc_config;
> > mutex_init(>lock);
> >
> > -   if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", ))
> > -   chip->tcpc_config.max_snk_mv = v / 1000;
> > -
> > -   if (!device_property_read_u32(dev, "fcs,max-sink-microamp", ))
> > -   chip->tcpc_config.max_snk_ma = v / 1000;
> > -
> > -   if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
> > -   chip->tcpc_config.max_snk_mw = v / 1000;
> > -
> > if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", ))
> > chip->tcpc_config.operating_snk_mw = v / 1000;
> >
> > +   /* Composite sink PDO */
> > +   chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> > +   chip->tcpc_config.snk_pdo = chip->snk_pdo;
> > +
> > /*
> >  * Devicetree platforms should get extcon via phandle (not yet
> >  * supported). On ACPI platforms, we get the name from a device prop.
> >
> 
> Regards,
> 
> Hans


Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-04 Thread Hans de Goede

Hi,

On 04-04-18 14:06, Mats Karrman wrote:

Hi Li,

On 2018-03-23 15:58, Li Jun wrote:


Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
variable PDO for sink config.

Signed-off-by: Li Jun 
---
  drivers/usb/typec/fusb302/fusb302.c | 51 +++--
  1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c 
b/drivers/usb/typec/fusb302/fusb302.c
index 7036171..db4d9cd 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -120,6 +120,7 @@ struct fusb302_chip {
  enum typec_cc_polarity cc_polarity;
  enum typec_cc_status cc1;
  enum typec_cc_status cc2;
+    u32 snk_pdo[PDO_MAX_OBJECTS];
  #ifdef CONFIG_DEBUG_FS
  struct dentry *dentry;
@@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
  static const struct tcpc_config fusb302_tcpc_config = {
  .src_pdo = src_pdo,
  .nr_src_pdo = ARRAY_SIZE(src_pdo),
-    .snk_pdo = snk_pdo,
-    .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-    .max_snk_mv = 5000,
-    .max_snk_ma = 3000,
-    .max_snk_mw = 15000,
  .operating_snk_mw = 2500,
  .type = TYPEC_PORT_DRP,
  .data = TYPEC_PORT_DRD,
@@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
  return 0;
  }
+static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
+{
+    struct device *dev = chip->dev;
+    u32 mv, ma, mw, min_mv;
+    unsigned int i;
+
+    /* Copy the static snk pdo */
+    for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
+    chip->snk_pdo[i] = snk_pdo[i];
+
+    if (device_property_read_u32(dev, "fcs,max-sink-microvolt", ) ||
+    device_property_read_u32(dev, "fcs,max-sink-microamp", ) ||
+    device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
+    return i;
+
+    mv = mv / 1000;
+    ma = ma / 1000;
+    mw = mw / 1000;
+
+    min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
+    if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))


You've got the parentheses wrong.

Apart from that I don't like/understand why the PDO's should be fixed.
Every product should be able to have its own settings, including the first PDO 
(e.g. it
might need to specify a higher current and/or set the "Higher Capability" bit).

I think this would be best solved the same way as in your TCPCI driver patch 
series [1]
with a list freely specified by a property.

[1] https://www.spinics.net/lists/linux-usb/msg167398.html


Hmm, interesting, for the x86 use-case that would require updating
the properties for the fusb302 defined in:

drivers/platform/x86/intel_cht_int33fe.c

In tandem, which is easily doable.

But what about other users of the fusb302 driver? Since this driver
was added before I started using it for some x86 boards, I assume
there are some non x86 users, so we should preserve compatibility
for DTB files which don't define any sink PDOs in their properties,
I guess we could fall to a default fixed 5V sink pdo for those.

Regards,

Hans






+    min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
+    else
+    min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
+    ma = min(ma, 1000 * mw / min_mv);
+
+    /* Insert the new pdo to the end */
+    chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
+
+    return i+1;
+}
+
  static int fusb302_probe(struct i2c_client *client,
   const struct i2c_device_id *id)
  {
@@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
  chip->tcpc_dev.config = >tcpc_config;
  mutex_init(>lock);
-    if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", ))
-    chip->tcpc_config.max_snk_mv = v / 1000;
-
-    if (!device_property_read_u32(dev, "fcs,max-sink-microamp", ))
-    chip->tcpc_config.max_snk_ma = v / 1000;
-
-    if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
-    chip->tcpc_config.max_snk_mw = v / 1000;
-
  if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", ))
  chip->tcpc_config.operating_snk_mw = v / 1000;
+    /* Composite sink PDO */
+    chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
+    chip->tcpc_config.snk_pdo = chip->snk_pdo;
+
  /*
   * Devicetree platforms should get extcon via phandle (not yet
   * supported). On ACPI platforms, we get the name from a device prop.


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


Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-04 Thread Mats Karrman

Hi Li,

On 2018-03-23 15:58, Li Jun wrote:


Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
variable PDO for sink config.

Signed-off-by: Li Jun 
---
  drivers/usb/typec/fusb302/fusb302.c | 51 +++--
  1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c 
b/drivers/usb/typec/fusb302/fusb302.c
index 7036171..db4d9cd 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -120,6 +120,7 @@ struct fusb302_chip {
enum typec_cc_polarity cc_polarity;
enum typec_cc_status cc1;
enum typec_cc_status cc2;
+   u32 snk_pdo[PDO_MAX_OBJECTS];
  
  #ifdef CONFIG_DEBUG_FS

struct dentry *dentry;
@@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
  static const struct tcpc_config fusb302_tcpc_config = {
.src_pdo = src_pdo,
.nr_src_pdo = ARRAY_SIZE(src_pdo),
-   .snk_pdo = snk_pdo,
-   .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-   .max_snk_mv = 5000,
-   .max_snk_ma = 3000,
-   .max_snk_mw = 15000,
.operating_snk_mw = 2500,
.type = TYPEC_PORT_DRP,
.data = TYPEC_PORT_DRD,
@@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
return 0;
  }
  
+static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)

+{
+   struct device *dev = chip->dev;
+   u32 mv, ma, mw, min_mv;
+   unsigned int i;
+
+   /* Copy the static snk pdo */
+   for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
+   chip->snk_pdo[i] = snk_pdo[i];
+
+   if (device_property_read_u32(dev, "fcs,max-sink-microvolt", ) ||
+   device_property_read_u32(dev, "fcs,max-sink-microamp", ) ||
+   device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
+   return i;
+
+   mv = mv / 1000;
+   ma = ma / 1000;
+   mw = mw / 1000;
+
+   min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
+   if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))


You've got the parentheses wrong.

Apart from that I don't like/understand why the PDO's should be fixed.
Every product should be able to have its own settings, including the first PDO 
(e.g. it
might need to specify a higher current and/or set the "Higher Capability" bit).

I think this would be best solved the same way as in your TCPCI driver patch 
series [1]
with a list freely specified by a property.

[1] https://www.spinics.net/lists/linux-usb/msg167398.html


+   min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
+   else
+   min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
+   ma = min(ma, 1000 * mw / min_mv);
+
+   /* Insert the new pdo to the end */
+   chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
+
+   return i+1;
+}
+
  static int fusb302_probe(struct i2c_client *client,
 const struct i2c_device_id *id)
  {
@@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
chip->tcpc_dev.config = >tcpc_config;
mutex_init(>lock);
  
-	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", ))

-   chip->tcpc_config.max_snk_mv = v / 1000;
-
-   if (!device_property_read_u32(dev, "fcs,max-sink-microamp", ))
-   chip->tcpc_config.max_snk_ma = v / 1000;
-
-   if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
-   chip->tcpc_config.max_snk_mw = v / 1000;
-
if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", ))
chip->tcpc_config.operating_snk_mw = v / 1000;
  
+	/* Composite sink PDO */

+   chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
+   chip->tcpc_config.snk_pdo = chip->snk_pdo;
+
/*
 * Devicetree platforms should get extcon via phandle (not yet
 * supported). On ACPI platforms, we get the name from a device prop.


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


Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

2018-04-03 Thread Hans de Goede

Hi,

On 23-03-18 15:58, Li Jun wrote:

Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
variable PDO for sink config.

Signed-off-by: Li Jun 
---
  drivers/usb/typec/fusb302/fusb302.c | 51 +++--
  1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c 
b/drivers/usb/typec/fusb302/fusb302.c
index 7036171..db4d9cd 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -120,6 +120,7 @@ struct fusb302_chip {
enum typec_cc_polarity cc_polarity;
enum typec_cc_status cc1;
enum typec_cc_status cc2;
+   u32 snk_pdo[PDO_MAX_OBJECTS];
  
  #ifdef CONFIG_DEBUG_FS

struct dentry *dentry;
@@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
  static const struct tcpc_config fusb302_tcpc_config = {
.src_pdo = src_pdo,
.nr_src_pdo = ARRAY_SIZE(src_pdo),
-   .snk_pdo = snk_pdo,
-   .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-   .max_snk_mv = 5000,
-   .max_snk_ma = 3000,
-   .max_snk_mw = 15000,
.operating_snk_mw = 2500,
.type = TYPEC_PORT_DRP,
.data = TYPEC_PORT_DRD,
@@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
return 0;
  }
  
+static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)

+{
+   struct device *dev = chip->dev;
+   u32 mv, ma, mw, min_mv;
+   unsigned int i;
+
+   /* Copy the static snk pdo */
+   for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
+   chip->snk_pdo[i] = snk_pdo[i];


The static snk PDO is constant and only contains:
PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),

So you can just do:

chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);

Here.


+
+   if (device_property_read_u32(dev, "fcs,max-sink-microvolt", ) ||
+   device_property_read_u32(dev, "fcs,max-sink-microamp", ) ||
+   device_property_read_u32(dev, "fcs,max-sink-microwatt", ))


You can drop the reading of "fcs,max-sink-microwatt" here, it was only
ever added because the tcpm code used to depend on max_mw being set,
now that that is no longer the case, we don't need it.

So this entire function can be simplified to:

static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
{
struct device *dev = chip->dev;
u32 max_uv, max_ua;

chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);

if (device_property_read_u32(dev, "fcs,max-sink-microvolt", _uv) ||
device_property_read_u32(dev, "fcs,max-sink-microamp", _ua))
return 1;

chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000);
return 2;
}


@@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
chip->tcpc_dev.config = >tcpc_config;
mutex_init(>lock);
  
-	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", ))

-   chip->tcpc_config.max_snk_mv = v / 1000;
-
-   if (!device_property_read_u32(dev, "fcs,max-sink-microamp", ))
-   chip->tcpc_config.max_snk_ma = v / 1000;
-
-   if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
-   chip->tcpc_config.max_snk_mw = v / 1000;
-
if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", ))
chip->tcpc_config.operating_snk_mw = v / 1000;
  
+	/* Composite sink PDO */

+   chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
+   chip->tcpc_config.snk_pdo = chip->snk_pdo;
+
/*
 * Devicetree platforms should get extcon via phandle (not yet
 * supported). On ACPI platforms, we get the name from a device prop.



Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html