> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Monday, October 19, 2020 8:31 PM
> To: Jun Li <jun.li@xxxxxxx>
> Cc: heikki.krogerus@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> rafael@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; hdegoede@xxxxxxxxxx;
> lee.jones@xxxxxxxxxx; mika.westerberg@xxxxxxxxxxxxxxx;
> dmitry.torokhov@xxxxxxxxx; prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx;
> laurent.pinchart+renesas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Peter Chen
> <peter.chen@xxxxxxx>
> 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@xxxxxxx>
>
> > + *
>
> 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
>
