Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Oct 2022 15:23:43 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     chengwei <foxfly.lai.tw@gmail.com>
Cc:     pavel@ucw.cz, lee@kernel.org, linux-kernel@vger.kernel.org,
        linux-leds@vger.kernel.org, GaryWang@aaeon.com.tw,
        musa.lin@yunjingtech.com, jack.chang@yunjingtech.com,
        chengwei <larry.lai@yunjingtech.com>,
        Javier Arteaga <javier@emutex.com>,
        Nicola Lunghi <nicola.lunghi@emutex.com>
Subject: Re: [PATCH] mfd: Add support for UP board CPLD/FPGA
Message-ID: <YzrUT9JlIEjIfTMG@smile.fi.intel.com>
References: <20221001090547.24899-1-larry.lai@yunjingtech.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221001090547.24899-1-larry.lai@yunjingtech.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Sat, Oct 01, 2022 at 05:05:47PM +0800, chengwei wrote:
> The UP Squared board <http://www.upboard.com> implements certain
> features (pin control, onboard LEDs or CEC) through an on-board 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 come with a few FPGA-controlled onboard LEDs:
> * UP Board: yellow, green, red
> * UP Squared: blue, yellow, green, red
> 
> 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

...

> +config LEDS_UPBOARD
> +	tristate "LED support for the UP board"
> +	depends on LEDS_CLASS
> +	depends on MFD_UPBOARD_FPGA
> +	help
> +	  This option enables support for the UP board LEDs. The UP boards come
> +	  with a few FPGA-controlled onboard LEDs. The UP Squared includes 4 LEDs
> +	  (yellow, green, red and blue) on the underside of the board which are
> +	  controlled by the pin control FPGA on the board

What would be the module name?

Also take care in every comment and text of the English grammar and
punctuation. (Here, for example, the trailing period is missed)

...

> +// SPDX-License-Identifier: GPL-2.0-only

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

This text is redundant as far as SPDX is provided.

...

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

Instead of implying the headers by kernel.h, please revisit this list and add
what you are using (like container_of.h, types.h, etc).

...

> +struct upboard_led {
> +	struct regmap_field *field;
> +	struct led_classdev cdev;
> +};

If you put cdev as a first member it may decrease a code size, but check this
with bloat-o-meter.

...

> +static enum led_brightness upboard_led_brightness_get(struct led_classdev
> +						      *cdev)

Wrong indentation. Why not to put on one line?
Same Q to other similar places.

...

> +static int __init upboard_led_probe(struct platform_device *pdev)

__init ?! Haven't your linker issued a warning about section mismatches?

...

> +	struct upboard_fpga * const fpga = dev_get_drvdata(pdev->dev.parent);

fpga is a confusing name since we have the fpga framework in the Linux kernel.
I believe you are talking about cpld, which suits here better.

...

> +	struct upboard_led_data * const pdata = pdev->dev.platform_data;

dev_get_platdata()

...

> +	if (!fpga || !pdata)
> +		return -EINVAL;

When is this true?

...

> +	led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "upboard:%s:",
> +					pdata->colour);

> +

Redundant blank line.

> +	if (!led->cdev.name)
> +		return -ENOMEM;

...

> +static struct platform_driver upboard_led_driver = {
> +	.driver = {
> +		.name = "upboard-led",
> +	},
> +};

> +

Ditto.

> +module_platform_driver_probe(upboard_led_driver, upboard_led_probe);

> +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>");
> +MODULE_DESCRIPTION("UP Board LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:upboard-led");

> +

Why is this?

...

> +config MFD_UPBOARD_FPGA
> +	tristate "Support for the UP board FPGA"
> +	select MFD_CORE
> +	help
> +	  Select this option to enable the UP and UP^2 on-board FPGA. The UP
> +	  board implements certain features (pin control, onboard LEDs or CEC)
> +	  through an on-board FPGA. This mfd driver implements the line protocol
> +	  to read and write registers from the FPGA through regmap.

Module name?

...

And seems so on as per LED driver...

I stop my review here. This submission needs more work.

-- 
With Best Regards,
Andy Shevchenko