lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SG2PR06MB37425EDBA90850FD5ABB0214F9679@SG2PR06MB3742.apcprd06.prod.outlook.com>
Date:   Tue, 25 Apr 2023 15:28:14 +0000
From:   Larry Lai <larry.lai@...jingtech.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     "lee@...nel.org" <lee@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "pavel@....cz" <pavel@....cz>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "GaryWang@...on.com.tw" <GaryWang@...on.com.tw>,
        Musa Lin <musa.lin@...jingtech.com>,
        Jack Chang <jack.chang@...jingtech.com>,
        Noah Hung <noah.hung@...jingtech.com>,
        Michael Wang <michael.wang@...jingtech.com>
Subject: 回覆: [RFC 1/3] mfd: Add support for UP board CPLD/FPGA

Dear Andy, 
 
        Thank you for spending time to review this code, please kindly check the following comments with “>>>” beginning.
        Some of issues we will fix in new RFC_230426, some may need you give us more examples or comments.
 
Best Regards,
Larry Lai
 
寄件者: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
日期: 星期四, 2022年12月8日 上午5:10
收件者: Larry Lai <larry.lai@...jingtech.com>
副本: lee@...nel.org <lee@...nel.org>, linus.walleij@...aro.org <linus.walleij@...aro.org>, pavel@....cz <pavel@....cz>, linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>, linux-gpio@...r.kernel.org <linux-gpio@...r.kernel.org>, linux-leds@...r.kernel.org <linux-leds@...r.kernel.org>, GaryWang@...on.com.tw <GaryWang@...on.com.tw>, Musa Lin <musa.lin@...jingtech.com>, Jack Chang <jack.chang@...jingtech.com>, Noah Hung <noah.hung@...jingtech.com>, Javier Arteaga <javier@...tex.com>, Nicola Lunghi <nicola.lunghi@...tex.com>
主旨: Re: [RFC 1/3] mfd: Add support for UP board CPLD/FPGA
On Thu, Dec 08, 2022 at 12:33:57AM +0800, larry.lai wrote:
> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control, onboard LEDs or CEC) through an on-board CPLD/FPGA.
> 
> This mfd driver implements the line protocol to read and write registers
> from the FPGA through regmap. The register address map is also included.
> 
> The UP Boards provide a few I/O pin headers (for both GPIO and
> functions), including a 40-pin Raspberry Pi compatible header.
> 
> This patch implements support for the FPGA-based pin controller that
> manages direction and enable state for those header pins.
> 
> Partial support UP boards:
> * UP core + CREX
> * UP core + CRST02

...

> +#include <linux/acpi.h>
> +#include <linux/dmi.h>

> +#include <linux/gpio.h>

I'm not sure if you read my previous emails regarding the topic.
This header must not be in the new code.
>>> fixed.

> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/upboard-fpga.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>


Missing bits.h and err.h at least.
>>> fixed.
...

> +struct upboard_fpga_data {
> +     const struct regmap_config *regmapconf;

No need to repeat regmap twice.
>>> fixed.

> +     const struct mfd_cell *cells;
> +     size_t ncells;
> +};

...

> +#define MENUFACTURER_ID_MASK         0xFF

GENMASK()?
>>> fixed.
...

> +#define FIRMWARE_ID_MASK             0xF

Ditto.
>>> fixed.
...

> +/* Apollo Lake GPIO pin number mapping to FPGA LED */
> +#define APL_GPIO_218                 507

No way. It should be addressed as GPIO chip reference and relative pin
(or GPIO, whichever suits better for your purposes) number.
>>> This section has been removed in new RFC patch, please ignore it.
...

> +/* For UP Board Series FPGA register read/write protocols                   */
> +/* EMUTEX specs:                                                            */
> +/* D0   D1  D2  D3  D4  D5  D6  D7  D8  D9 .... D22  D23                      */
> +/* [RW][        address           ][     DATA        ]                     */
> +
> +/* Read Sequence:                                                             */
> +/*      ___   ____________________________________________________   _________*/
> +/* clr:    \_/ <--low-pulse does start the write-readback         \_/<--start */
> +/*             sequence with partital reset of internal          new sequence*/
> +/*             registers but the CONF-REG.                                     */
> +/*        ____________________________________________________________________*/
> +/* rst: _/       _   _   _        _   _   _   __       __   __   _            */
> +/* stb: STB#1->_/1\_/2\_/3\_...._/7\_/8\_/9\_/10\_..../23\_/24\_/<-STB#25 edge*/
> +/*                                                              is needed  */
> +/*                                                                to ACK     */
> +/*             (D0 - D7 stb rising latch)                                     */
> +/* data_in:     D0  D1  D2  .... D6  D7  don't ........ care(DC)              */
> +/* data_out:    don't ...........care(DC)  D8   D9 ....  D22  D23           */
> +/*                                     (D8 - D23 stb falling latch)          */
> +/* flag_Read:                                  _________...._________              */
> +/*      __DC_   ____________...._________/                      \_            */
> +/* counter:                                                                */
> +/*    [00]DC[00][01][02] ............[08][9][10]............[24][00]       */
> +/* CONF-REG:                                                               */
> +/*    [00] [                         CONF-REG               ]              */
> +/* wreg:                                                                    */
> +/*    [00]DC[00][  wreg=SHFT(wreg)  ][ADR][DATA][wreg=SHFT(wreg]           */
> +/* wreg2:                                                                    */
> +/*                                              [         (COPY)=ADDR ]       */

This has too many /* */ and TABs vs space mix... Please, fix it.
>>> fixed.
Is it SPI 24-bit bit-banging? Why spi-gpio can't be utilized for it?
>>> The UP board FPGA protocol (stb, clr, rst, in, out) is different with SPI 24-bit banging (CS, CLK, MOST, MOSO).
...

> +/* Write Sequence:                                                          */
> +/*      ___   ____________________________________________________   _________*/
> +/* clr:    \_/ <--low-pulse does start the write-readback         \_/<--start */
> +/*             sequence with partital reset of internal          new sequence*/
> +/*             registers but the CONF-REG.                                     */
> +/*        ____________________________________________________________________*/
> +/* rst: _/       _   _   _        _   _   _   __       __   __   _            */
> +/* stb: STB#1->_/1\_/2\_/3\_...._/7\_/8\_/9\_/10\_..../23\_/24\_/<-STB#25 edge*/
> +/*                                                              is needed  */
> +/*                                                              to ACK     */
> +/*             (D0 - D23 stb rising latch)                                    */
> +/* data_in:     D0  D1  D2  .... D6  D7  D8  D9 ....  D22  D23                */
> +/* data_out:    don't ................................care (DC)               */
> +/* flag_Read:                                                                */
> +/*      __DC_   ____________....__________________________________            */
> +/* counter:                                                                */
> +/*    [00]DC[00][01][02] ............[08][9][10]............[24][00]          */
> +/* wreg:                                                                   */
> +/*    [00]DC[00][wreg=SHFT(wreg)&dat_in ][SHFT(wreg)&dat_in][DAT]             */
> +/* wreg2:                                                                  */
> +/*                                             [     (COPY)=ADDR     ]       */
> +/* CONF-REG:                                                               */
> +/*    [00] [      CONF-REG = OLD VALUE                       ][CONF-REG=DAT]*/


Same comments as per above.
>>> fixed.
...

> +             gpiod_set_value(fpga->datain_gpio, (reg >> i) & 0x1);

!!(reg & BIT(i))
>>> fixed.
...

> +             gpiod_set_value(fpga->datain_gpio, (val >> i) & 0x1);

Ditto.
>>> fixed.
But see above.

...

> +static struct gpio_led upboard_gpio_leds[] = {
> +     {
> +             .name = "upboard:blue:",
> +             .gpio = APL_GPIO_218,

You must understand that it won't work with dynamic GPIO bases which will be
enabled in v6.2-rc1. And even in general it must not be like this.
>>> fixed.

> +             .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> +     },
> +};

...

> +     enum gpiod_flags flags;
> +
> +     flags = fpga->uninitialised ? GPIOD_OUT_LOW : GPIOD_ASIS;

Can be united.
>>> fixed.
...

> +     /*
> +      * The SoC pinctrl driver may not support reserving the GPIO line for
> +      * FPGA reset without causing an undesired reset pulse. This will clear
> +      * any settings on the FPGA, so only do it if we must.
> +      * Reset gpio defaults HIGH, get gpio and set to LOW, then set back to
> +      * HIGH as a pulse.
> +      */
> +     if (fpga->uninitialised) {
> +             fpga->reset_gpio = devm_gpiod_get(fpga->dev, "reset", GPIOD_OUT_LOW);
> +             if (IS_ERR(fpga->reset_gpio))
> +                     return PTR_ERR(fpga->reset_gpio);

No sleep for the hardware to be really reset?
>>> This is hardware reset pin, so there's no need the sleep wait.

> +             gpiod_set_value(fpga->reset_gpio, 1);
> +     }

> +/*
> + * MFD upboard-fpga is acpi driver and can recognize the AANT ID from different

ACPI
>>> fixed.
> + * kind of upboards. We get the led gpio initialized information from this

LED GPIO
>>> fixed.
> + * then add led-upboard driver.
> + */

...

> +     int blue_gpio = -1, yellow_gpio = -1, green_gpio = -1, red_gpio = -1;

NAK.
>>> fixed.
...

> +             blue_gpio = desc_to_gpio(desc);

NAK.
>>> fixed.
...

> +             yellow_gpio = desc_to_gpio(desc);

NAK.
>>> fixed.
...

> +             green_gpio = desc_to_gpio(desc);

NAK.
>>> fixed.
...

> +             red_gpio = desc_to_gpio(desc);

NAK.
>>> fixed.
...

> +/*
> + *   Refer https://www.kernel.org/doc/htmldocs/writing_musb_glue_layer/device-platform-data.html,
> + *   the id field could be set to -1 (equivalent to PLATFORM_DEVID_NONE),
> + *  -2 (equivalent to PLATFORM_DEVID_AUTO) or start with 0 for the first
> + *   device of this kind if we want a specific id number.
> + */

Useless comment. Just use the proper definition.
>>> remove useless comment and use PLATFORM_DEVID_AUTO proper definition.

> +     if (devm_mfd_add_devices(fpga->dev, 0,
> +                              upboard_gpio_led_cells,
> +                              ARRAY_SIZE(upboard_gpio_led_cells),
> +                              NULL, 0, NULL)) {
> +             dev_info(fpga->dev, "Failed to add GPIO leds");
> +     }


        ret = ...(...);
        if (ret)
                dev_warn(...);
>>> fixed.

...

> +     /* get fpga/EC protocol hardware version */
> +     acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev), "_HRV", NULL, &hrv);

No error check?
>>> This section has been removed in new RFC patch, please ignore it.
...

> +     system_id = dmi_first_match(upboard_dmi_table);
> +     if (system_id)
> +             quirks = (unsigned long)system_id->driver_data;
> +
> +     if (hrv == UPFPGA_PROTOCOL_V1_HRV &&
> +         (quirks & UPFPGA_QUIRK_HRV1_IS_PROTO2))
> +             hrv = UPFPGA_PROTOCOL_V2_HRV;

Maybe it's easier to provide driver data?
>>> This section has been removed in new RFC patch, please ignore it.
...

> +     fpga_data = (const struct upboard_fpga_data *) id->driver_data;


Use device_get_match_data().
>>> This section has been removed in new RFC patch, please ignore it.
...

> +     if (quirks & UPFPGA_QUIRK_UNINITIALISED) {
> +             dev_info(&pdev->dev, "FPGA not initialised by this BIOS");

dev_warn()?
>>> This section has been removed in new RFC patch, please ignore it.
> +             fpga->uninitialised = true;
> +     }

...

> +     dev_set_drvdata(&pdev->dev, fpga);

platform_set_drvdata().
>>> fixed.
> +     fpga->dev = &pdev->dev;
> +     fpga->regmap = devm_regmap_init(&pdev->dev, NULL,
> +                                     fpga, fpga_data->regmapconf);

Can be one line and you can actually have

        struct device *dev = &pdev->dev;

at the top of the function.
>>> fixed.
> +     fpga->regmapconf = fpga_data->regmapconf;

Why is it done if you know that error might happen?
>>> This section has been removed in new RFC patch, please ignore it.
> +     if (IS_ERR(fpga->regmap))
> +             return PTR_ERR(fpga->regmap);

...

> +     /* gpio leds initialize */

GPIO LEDs
>>> fixed.
...

> +             ret =  devm_mfd_add_devices(&pdev->dev, 0,

Use proper definition.
>>> This section has been removed in new RFC patch, please ignore it.
> +                                         upboard_gpio_led_cells,
> +                                         ARRAY_SIZE(upboard_gpio_led_cells),
> +                                         NULL, 0, NULL);

> +                     dev_err(&pdev->dev, "Failed to add GPIO leds");
> +                     return ret;

        return dev_err_probe();
>>> This section has been removed in new RFC patch, please ignore it.
> +             }
> +     }

+ blank line.
>>> fixed.
> +     return devm_mfd_add_devices(&pdev->dev, 0,

Use proper definition.
>>> fixed by using PLATFORM_DEVID_AUTO definition.
> +                                 fpga_data->cells,
> +                                 fpga_data->ncells,
> +                                 NULL, 0, NULL);
> +}

...

Move ACPI ID table here, it's not needed to have it upper.
>>> Could you kindly explain more and give some examples?
> +static struct platform_driver upboard_fpga_driver = {
> +     .driver = {
> +             .name = "upboard-fpga",
> +             .acpi_match_table = upboard_fpga_acpi_match,
> +     },
> +};

...

The header file missing several forward declarations and inclusions, like
>>> fixed.
#include <linux/types.h>

struct regmap;

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ