RE: [PATCH 04/12] staging: typec: tcpci: support port config passed via dt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter Roeck
> Sent: Tuesday, September 26, 2017 9:33 PM
> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx
> Cc: yueyao@xxxxxxxxxx; o_leveque@xxxxxxxxx; Peter Chen
> <peter.chen@xxxxxxx>; A.s. Dong <aisheng.dong@xxxxxxx>; linux-
> usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 04/12] staging: typec: tcpci: support port config passed via
> dt
> 
> On 09/25/2017 05:45 PM, Li Jun wrote:
> > User can define the typec port properties in tcpci node to setup the
> > port config.
> >
> > Signed-off-by: Li Jun <jun.li@xxxxxxx>
> > ---
> >   drivers/staging/typec/tcpci.c | 89
> +++++++++++++++++++++++++++++++++++++++----
> >   include/linux/usb/tcpm.h      |  6 +--
> >   2 files changed, 85 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 4636804..0119453 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -425,17 +425,92 @@ static const struct regmap_config
> tcpci_regmap_config = {
> >   	.max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
> >   };
> >
> > -static const struct tcpc_config tcpci_tcpc_config = {
> > -	.type = TYPEC_PORT_DFP,
> > -	.default_role = TYPEC_SINK,
> > -};
> > -
> > +/* Populate struct tcpc_config from device-tree */
> >   static int tcpci_parse_config(struct tcpci *tcpci)
> >   {
> > +	struct tcpc_config *tcfg;
> > +	int ret = -EINVAL;
> > +
> >   	tcpci->controls_vbus = true; /* XXX */
> >
> > -	/* TODO: Populate struct tcpc_config from ACPI/device-tree */
> > -	tcpci->tcpc.config = &tcpci_tcpc_config;
> > +	tcpci->tcpc.config = devm_kzalloc(tcpci->dev, sizeof(*tcfg),
> > +					  GFP_KERNEL);
> > +	if (!tcpci->tcpc.config)
> > +		return -ENOMEM;
> > +
> > +	tcfg = tcpci->tcpc.config;
> > +
> > +	/* Get port-type */
> > +	ret = typec_get_port_type(tcpci->dev);
> > +	if (ret < 0) {
> > +		dev_err(tcpci->dev, "typec port type is NOT correct!\n");
> 
> Are all those exclamation marks necessary ?

I will remove it.

> 
> > +		return ret;
> > +	}
> > +	tcfg->type = ret;
> > +
> > +	if (tcfg->type == TYPEC_PORT_UFP)
> > +		goto sink;
> > +
> > +	/* Get source PDO */
> > +	tcfg->nr_src_pdo = device_property_read_u32_array(tcpci->dev,
> > +						"src-pdos", NULL, 0);
> 
> I personally prefer continuation line alignment to '(', but that is up to Greg to
> decide.
> 
> > +	if (tcfg->nr_src_pdo <= 0) {
> > +		dev_err(tcpci->dev, "typec source pdo is missing!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	tcfg->src_pdo = devm_kzalloc(tcpci->dev,
> > +		sizeof(*tcfg->src_pdo) * tcfg->nr_src_pdo, GFP_KERNEL);
> 
> devm_kcalloc

Will change.

> 
> > +	if (!tcfg->src_pdo)
> > +		return -ENOMEM;
> > +
> > +	ret = device_property_read_u32_array(tcpci->dev, "src-pdos",
> > +				tcfg->src_pdo, tcfg->nr_src_pdo);
> > +	if (ret) {
> > +		dev_err(tcpci->dev, "Failed to read src pdo!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (tcfg->type == TYPEC_PORT_DFP)
> > +		return 0;
> > +
> > +	/* Get the preferred power role for drp */
> > +	ret = typec_get_power_role(tcpci->dev);
> 
> Maybe this should be named typec_get_preferred_role(). The generic function
> names are a bit misleading; they suggest they would return the current role, and
> don't indicate that the data is from device properties.

Thanks,  typec_get_preferred_role() looks more precise, I will change.

> I am also not sure about the mix of using typec infra functions and direct
> device_property calls.

OK, I will try to also use typec infra functions for those device_property
calls(some may be grouped).	

> 
> > +	if (ret < 0) {
> > +		dev_err(tcpci->dev, "typec preferred role is wrong!\n");
> 
> Wrong or missing ?

Both wrong and missing will return negative error code, I will refine it.

> 
> > +		return ret;
> > +	}
> > +	tcfg->default_role = ret;
> > +sink:
> > +	/* Get sink power capability */
> > +	tcfg->nr_snk_pdo = device_property_read_u32_array(tcpci->dev,
> > +						"snk-pdos", NULL, 0);
> > +	if (tcfg->nr_snk_pdo <= 0) {
> > +		dev_err(tcpci->dev, "typec sink pdo is missing!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	tcfg->snk_pdo = devm_kzalloc(tcpci->dev,
> > +		sizeof(*tcfg->snk_pdo) * tcfg->nr_snk_pdo, GFP_KERNEL);
> > +	if (!tcfg->snk_pdo)
> > +		return -ENOMEM;
> > +
> > +	ret = device_property_read_u32_array(tcpci->dev, "snk-pdos",
> > +				tcfg->snk_pdo, tcfg->nr_snk_pdo);
> > +	if (ret) {
> > +		dev_err(tcpci->dev, "Failed to read sink pdo!\n");
> 
> There is a mix of "Failed to read" and "missing" messages. What is the
> difference ?

I will refine the error message.

> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (device_property_read_u32(tcpci->dev, "max-snk-mv",
> > +				     &tcfg->max_snk_mv) ||
> > +		device_property_read_u32(tcpci->dev, "max-snk-ma",
> > +					 &tcfg->max_snk_ma) ||
> > +		device_property_read_u32(tcpci->dev, "op-snk-mw",
> > +					 &tcfg->operating_snk_mw)) {
> > +		dev_err(tcpci->dev, "Failed to read sink capability!\n");
> > +		return -EINVAL;
> > +	}
> >
> >   	return 0;
> >   }
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> > 073197f..a67cf77 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -76,10 +76,10 @@ enum tcpm_transmit_type {
> >    * @alt_modes:	List of supported alternate modes
> >    */
> >   struct tcpc_config {
> > -	const u32 *src_pdo;
> > +	u32 *src_pdo;
> >   	unsigned int nr_src_pdo;
> >
> > -	const u32 *snk_pdo;
> > +	u32 *snk_pdo;
> >   	unsigned int nr_snk_pdo;
> >
> >   	const u32 *snk_vdo;
> > @@ -154,7 +154,7 @@ struct tcpc_mux_dev {
> >    * @mux:	Pointer to multiplexer data
> >    */
> >   struct tcpc_dev {
> > -	const struct tcpc_config *config;
> > +	struct tcpc_config *config;
> >
> >   	int (*init)(struct tcpc_dev *dev);
> >   	int (*get_vbus)(struct tcpc_dev *dev);
> >

��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux