On 10/20/2018 12:49 PM, Andy Shevchenko wrote: > On Fri, Oct 19, 2018 at 8:26 PM Dan O'Donovan <dan@xxxxxxxxxx> wrote: >> From: Javier Arteaga <javier@xxxxxxxxxx> >> >> UP Squared (UP2) is a x86 SBC from AAEON based on Intel Apollo Lake. It >> features a MAX 10 FPGA that routes lines from both SoC and on-board >> devices to two I/O headers: >> >> +------------------------+ >> | 40-pin RPi-like header | >> +------| (HAT) | >> | +------------------------+ >> +-------+ +--------+ >> | | | | +------------------------+ >> | SoC |----| FPGA |-----| Custom UP2 pin header | >> | | | | | (EXHAT) | >> +-------+ +--------+ +------------------------+ >> | >> +------* On-board devices: LED, VLS... >> >> This is intended to enable vendor-specific applications to customize I/O >> header pinout, as well as include low-latency functionality. It also >> performs voltage level translation between the SoC (1.8V) and HAT header >> (3.3V). >> >> Out of the box, this block implements a platform controller with a >> GPIO-bitbanged control interface. It's enumerated by ACPI and provides >> registers to control: >> >> - Configuration of all FPGA-routed header lines. These can be driven >> SoC-to-header, header-to-SoC or set in high impedance. >> >> - On-board LEDs and enable lines for other platform devices. >> >> Add core support for this platform controller as a MFD device, exposing >> these registers as a regmap. > Can we see a link to or an excerpt of ACPI table for this device? Sure: https://gist.github.com/Dan-Emutex/6382c25f4b8b8cdd80e6056889cdf48b I'll add this to the cover letter for v3 as well. > >> +#define set_clear(u, x) gpiod_set_value((u)->clear_gpio, (x)) >> +#define set_strobe(u, x) gpiod_set_value((u)->strobe_gpio, (x)) >> +#define set_datain(u, x) gpiod_set_value((u)->datain_gpio, (x)) >> +#define get_dataout(u) gpiod_get_value((u)->dataout_gpio) > I think these macros don't bring much value. (Up to you and Lee to decide) These were added in response to one of your review comments on the original patch-set ;) I think they do slightly improve the code readability so I'll leave them there if there are no objections. > > >> +static void __reg_io_write(const struct upboard_ddata * const ddata, >> + unsigned int size, unsigned int val) >> +{ >> + int i; >> + >> + /* >> + * DATAIN is latched on each rising edge of the STROBE signal. >> + * Data (register address or value) is sent MSb first. >> + */ >> + for (i = size - 1; i >= 0; i--) { > while (size--) Yep, much neater. I'll change that, thanks! Ditto for the other comments on this file.+ ret = upboard_init_gpio(dev); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "failed to init GPIOs: %d", ret); >> + return ret; >> + } > I don't know if probe_err() helper is going to be a part of v4.21 > (which this series targets), it would be good to use it. I left this out for now in case it causes any awkwardness with merging. Hope that's ok.
