> -----Original Message----- > From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > Sent: 2018年4月5日 4:13 > To: Mats Karrman <mats.dev.list@xxxxxxxxx>; Jun Li <jun.li@xxxxxxx>; > gregkh@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; > heikki.krogerus@xxxxxxxxxxxxxxx > Cc: linux@xxxxxxxxxxxx; rmfrfs@xxxxxxxxx; yueyao.zhu@xxxxxxxxx; > linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx> > 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.li@xxxxxxx> > >> --- > >> 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��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
