Message-ID: <20190628081141.GJ508@localhost>
Date:   Fri, 28 Jun 2019 10:11:41 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Serge Semin <Sergey.Semin@...latforms.ru>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: cp210x: Add cp2108 GPIOs support

On Sun, Jun 16, 2019 at 01:56:36AM +0300, Serge Semin wrote:

> > > +/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
> > > +#define CP2108_IFACE_NUM		4
> > > +#define CP2108_GPIO_NUM			4
> > > +#define CP2108_PB_NUM			5
> > > +#define CP2108_GPIO_PB			1
> > 
> > Please try to give these more descriptive names; I'd prefer COUNT over
> > NUM when used as a suffix.
> > 
> > Perhaps slap an INDEX suffix on CP2108_GPIO_PB etc.
> 
> Ok. Added CNT and IDX suffixes.

Please spell out COUNT, no need to be that terse here.

> > > +/*
> > > + * CP2108 default pins state. There are five PBs. Each one is with its specific
> > > + * pins-set (see USB Express SDK sources or SDK-based smt application
> > > + * https://github.com/fancer/smt-cp210x for details).
> > > + */
> > > +struct cp2108_state {
> > > +	__le16	mode[CP2108_PB_NUM];	/* 0 - Open-Drain, 1 - Push-Pull */
> > > +	__le16	low_power[CP2108_PB_NUM];
> > > +	__le16	latch[CP2108_PB_NUM];	/* 0 - Logic Low, 1 - Logic High */
> > > +} __packed;
> > 
> > Ok, so you use mode[CP2108_GPIO_PB] to get the pin modes below;
> > what are the other "PB"s used for? Why is it a "port" block, if all 16
> > pins allocated to four different ports are accessible through one block?
> > 
> > Please try to make the comment self-contained (even if you leave some
> > details out). And perhaps we shouldn't refer to proprietary code in
> > here, and in any case the link may go away.
> > 
> 
> Ok added a bit more detailed information regarding the port blocks. But
> I'd prefer to keep the link. First of all I can't provide the complete
> description of the fields here because it would take too much space and
> in fact is pointless since only a single port block with GPIOs is utilized.
> So the link and the SDK info are a good reference to find some details
> (especially due to lacking such an info in the chip datasheet).

I never said it had to be complete, just self-contained for what it's
used for here. I don't want to go browsing random vendor code to figure
out what the code is doing when reviewing or modifying kernel code.

> Secondly even though the code is distributed under the Silicon Labs
> specific license it is open-source.

But it's not necessarily GPL compatible, which is what we care about
here.

Then again, you've already used it as reference for the protocol, so
let's keep it in.

> Finally the link may go away only
> if I removed the application from my github account, which I don't
> intend to.

Ok, didn't notice it was your account.

> > > +/*
> > > + * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
> > > + * Reset/Suspend latches describe default states after reset/suspend of the
> > > + * pins. The rest are responsible for alternate functions settings of the
> > > + * chip pins (see USB Express SDK sources or SDK-based smt application
> > > + * https://github.com/fancer/smt-cp210x for details).
> > > + */
> > > +struct cp2108_config {
> > > +	struct cp2108_state reset_latch;
> > > +	struct cp2108_state suspend_latch;
> > > +	u8	ip_delay[CP2108_IFACE_NUM];
> > > +	u8	enhanced_fxn[CP2108_IFACE_NUM];
> > > +	u8	enhanced_fxn_dev;
> > > +	u8	ext_clock_freq[CP2108_IFACE_NUM];
> > > +} __packed;
> > > +
> > > +/* CP2108 port alternate functions fields */

So it's really the enhanced_fxn field above right (and not about pin
alternate functions only).

> > > +#define CP2108_GPIO_TXLED_MODE		BIT(0)
> > > +#define CP2108_GPIO_RXLED_MODE		BIT(1)
> > > +#define CP2108_GPIO_RS485_MODE		BIT(2)
> > > +#define CP2108_GPIO_RS485_LOGIC		BIT(3)
> > > +#define CP2108_GPIO_CLOCK_MODE		BIT(4)
> > > +#define CP2108_DYNAMIC_SUSPEND		BIT(5)
> > 
> > No GPIO infix?
> 
> No, because this refers to all pins state when the chip being suspended
> (whether the suspend_latch state is pushed to the pins at the suspend USB
> state), while the rest of the macros are defined for the GPIOs alternative
> functions. I'll add the MODE suffix though.

Ok, does the pins maintain their state if DYNAMIC_SUSPEND is set or cleared?

> > > +	int result, bufsize = sizeof(buf.single);
> > 
> > One declaration per line please, especially when initialising.
> 
> Done. Though I can't remember this being requirement in the kernel
> coding style. Could you give me a link to the corresponding statement?

It's actually mentioned in passing in the coding style, but with a
different motivation.

The general idea is just to avoid unnecessarily terse code.

> > > +
> > > +	/*
> > > +	 * Move the GPIO clock alternative function bit value to the fourth bit
> > > +	 * as the corresponding GPIO pin reside. It shall make the generic
> > > +	 * cp210x GPIO request method being suitable for cp2108 as well.
> > > +	 */
> > 
> > This isn't entirely clear, please try to rephrase.
> > 
> > Which functions are available on which pins? Do the function defines
> > need to be renamed to reflect the pin numbers as for CP2104?
> 
> I wouldn't rename the macros'es, since they describe the functions of
> enhanced_fxn bits. BTW CP2104 doesn't do any renaming anyway, while it
> is doing the same thing as I am here. Instead I added a more descriptive
> comment, so the code hackers would see the reason of the BITs copy-pasting.

My point was that the corresponding macros for cp2104 includes the pin
number in the name (e.g. CP2104_GPIO1_RXLED_MODE), which makes it clear
on which pin each alternate function is available.

> > > +	priv->gpio_altfunc &= ~BIT(3);
> > > +	if (priv->gpio_altfunc & CP2108_GPIO_CLOCK_MODE)
> > > +		priv->gpio_altfunc |= BIT(3);
> > > +
> > > +	/*
> > > +	 * Open-drain mode in combination with a high latch value is used
> > > +	 * to emulate the GPIO input pin.
> > > +	 */
> > > +	priv->gpio_input &= ~priv->gpio_pushpull;
> > 
> > Input mode should only be set when the reset latch value is 1 (see
> > cp2104).
> 
> Yes, and this code does the same thing as the loop in cp2104, but in a single
> line. BTW the cp2104 cp2104_gpioconf_init() method can be simplified in the
> same way.stash@...stash@{0}

Not in a single line, you initialised gpio_input to the latch values
several lines above. Please keep the logic in one place, and use
temporaries where appropriate to make the code self-documenting.

I'd prefer a comment here along the lines of those in the other gpio
init functions (e.g. "set input mode iff open-drain and high latch
value").

> It should be also noted that in v2 I fixed a cp2108-related bug in the
> cp210x_gpio_get()/cp210x_gpio_set() methods. So aside from the requested
> alterations the logic of the cp2108 GPIOs setter/getter is bit different
> now.

Ah, guess you had only tested the first port's pins? Possibly another
reason for adding a dedicated cp2108 callback.

I'll comment on the patch itself.

Johan

Powered by blists - more mailing lists