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@{0}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
