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

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

 



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.

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

Type-c -> Type-C

Simple -> simple


> +	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?

...

> +/**

Is it kernel doc?!

> + * switch-simple.c - typec switch simple control.

Remove file name from the file.

> + *
> + * Copyright 2020 NXP
> + * Author: Jun Li <jun.li@xxxxxxx>

> + *

Redundant blank line.

> + */

...

> +#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.


...

> +	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.

> +			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.

...

> +	/* 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.

> +	if (IS_ERR(typec_sw->sel_gpio))
> +		return PTR_ERR(typec_sw->sel_gpio);

-- 
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux