> -----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", &mv)
> >> +||
> >> +        device_property_read_u32(dev, "fcs,max-sink-microamp", &ma)
> >> +||
> >> +        device_property_read_u32(dev, "fcs,max-sink-microwatt",
> >> +&mw))
> >> +        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&data=02%7C01%7Cjun.l
> >
> i%40nxp.com%7C678fb35afff542581d3808d59a68708b%7C686ea1d3bc2b4c6fa92c
> d
> >
> 99c5c301635%7C0%7C0%7C636584695710093111&sdata=ZyAJQUR8ZbAq8VWsZkF
> PGLg
> > F9P5j5QpvF3yeGyNoyH8%3D&reserved=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 guess we could fall to a default fixed 5V 
> sink pdo
> for those.

If we can't elaborate all existing users, we have to change the driver
like this to preserve compatibility.

Thanks
Jun
> 
> 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 = &chip->tcpc_config;
> >>       mutex_init(&chip->lock);
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microvolt",
> >> &v))
> >> -        chip->tcpc_config.max_snk_mv = v / 1000;
> >> -
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> >> -        chip->tcpc_config.max_snk_ma = v / 1000;
> >> -
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microwatt",
> >> &v))
> >> -        chip->tcpc_config.max_snk_mw = v / 1000;
> >> -
> >>       if (!device_property_read_u32(dev,
> >> "fcs,operating-sink-microwatt", &v))
> >>           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.
> >>
N�����r��y����b�X��ǧv�^�)޺{.n�+����{������^n�r���z���h�����&���G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�

Reply via email to