RE: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver

[Jun Li <jun.li@nxp.com>]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, October 19, 2020 8:31 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; robh+dt@kernel.org;
> rafael@kernel.org; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> lee.jones@linaro.org; mika.westerberg@linux.intel.com;
> dmitry.torokhov@gmail.com; prabhakar.mahadev-lad.rj@bp.renesas.com;
> laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
> 
> On Mon, Oct 19, 2020 at 05:03:15PM +0800, Li Jun wrote:
> > This patch adds a simple typec switch driver for cases which only
> > needs some simple operations but a dedicated driver is required,
> > current driver only supports GPIO toggle to switch the super speed
> > active channel according to typec orientation.
> 
> ...
> 
> >  	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
> >  	  control the USB role switch and also the multiplexer/demultiplexer
> >  	  switches used with USB Type-C Alternate Modes.
> 
> Missed blank line.

Will add.

> 
> > +config TYPEC_SWITCH_SIMPLE
> > +	tristate "Type-c orientation switch Simple driver"
> 
> Type-c -> Type-C
> 
> Simple -> simple

Will change.

> 
> 
> > +	depends on GPIOLIB
> > +	help
> > +	  Say Y or M if your system need a simple driver for typec switch
> > +	  control, like use GPIO to select active channel.
> 
> Driver name?

Will add the driver module name.

> 
> ...
> 
> > +/**
> 
> Is it kernel doc?!

Will change to "/*"

> 
> > + * switch-simple.c - typec switch simple control.
> 
> Remove file name from the file.

Will remove it.
> 
> > + *
> > + * Copyright 2020 NXP
> > + * Author: Jun Li <jun.li@nxp.com>
> 
> > + *
> 
> Redundant blank line.

Will remove.

> 
> > + */
> 
> ...
> 
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> 
> No evidence of use of these headers, but mod_devicetable.h along with
> gpio/consumer.h are missed.

Thanks, will change.

> 
> 
> ...
> 
> > +	switch (orientation) {
> > +	case TYPEC_ORIENTATION_NORMAL:
> > +		if (typec_sw->sel_gpio)
> 
> Optional GPIO does not require these checks. Drop them and learn what
> "optional" means.

Checked, will drop those checks.

> 
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> > +		break;
> > +	case TYPEC_ORIENTATION_REVERSE:
> > +		if (typec_sw->sel_gpio)
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> > +		break;
> > +	case TYPEC_ORIENTATION_NONE:
> > +		break;
> > +	}
> 
> ...
> 
> > +	struct typec_switch_simple	*typec_sw;
> > +	struct device			*dev = &pdev->dev;
> > +	struct typec_switch_desc sw_desc;
> 
> Be consistent with indentation.

Wil change.

> 
> ...
> 
> > +	/* Get the super speed active channel selection GPIO */
> > +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> > +						     GPIOD_OUT_LOW);
> 
> It can be one line.

Will change.

Thanks
Li Jun
> 
> > +	if (IS_ERR(typec_sw->sel_gpio))
> > +		return PTR_ERR(typec_sw->sel_gpio);
> 
> --
> With Best Regards,
> Andy Shevchenko
>