Re: [PATCH 3/3] fpga: xilinx-selectmap: add new driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/01/2024 23:56, Charles Perry wrote:
> Xilinx 7 series FPGA can be programmed using a slave parallel port named
> the SelectMAP interface in the datasheet. This slave interface is
> compatible with the i.MX6 EIM bus controller but other types of external
> memory mapped parallel bus might work.
> 
> xilinx-selectmap currently only supports the x8 mode where data is loaded
> at one byte per rising edge of the clock, with the MSb of each byte
> presented to the D0 pin.
> 
> The following DT fragment shows a valid configuration on a custom i.MX6
> board (pinctrl not shown for readability):
> 
> &weim {
>     status = "okay";
>     ranges = <0 0 0x08000000 0x04000000>;
> 
>     fpga_mgr: fpga_programmer@0,0 {
>         compatible = "xlnx,fpga-slave-selectmap";
>         reg = <0 0 0x4000000>;
>         fsl,weim-cs-timing = <0x00070031 0x00000142
>                               0x00020000 0x00000000
>                               0x0c000645 0x00000000>;
>         prog_b-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>;
>         init-b-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
>         done-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>;
>         csi-b-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>;
>         rdwr-b-gpios = <&gpio3 10 GPIO_ACTIVE_LOW>;
>     };
> };

Drop that example. First, it is not correct. Second, a correct one in
bindings is enough.

> 
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---

...

> +static int xilinx_selectmap_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_selectmap_conf *conf;
> +	struct resource *r;
> +	void __iomem *base;
> +
> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(base), "ioremap error\n");
> +	conf->base = base;
> +
> +	/* CSI_B is active low */
> +	conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi-b", GPIOD_OUT_HIGH);
> +	if (IS_ERR(conf->csi_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
> +				     "Failed to get CSI_B gpio\n");
> +
> +	/* RDWR_B is active low */
> +	conf->rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr-b", GPIOD_OUT_HIGH);
> +	if (IS_ERR(conf->rdwr_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
> +				     "Failed to get RDWR_B gpio\n");
> +
> +	return xilinx_core_probe(&conf->core, &pdev->dev,
> +							xilinx_selectmap_write,
> +							xilinx_selectmap_apply_padding);

Totally messed indentation. Please run scripts/checkpatch.pl and fix
reported warnings. Some warnings can be ignored, but the code here looks
like it needs a fix. Feel free to get in touch if the warning is not clear.

> +}
> +
> +static const struct of_device_id xlnx_selectmap_of_match[] = {
> +		{ .compatible = "xlnx,fpga-slave-selectmap", },
> +		{}
> +};
> +MODULE_DEVICE_TABLE(of, xlnx_selectmap_of_match);
> +
> +static struct platform_driver xilinx_slave_selectmap_driver = {
> +	.driver = {
> +		.name = "xilinx-slave-selectmap",
> +		.of_match_table = of_match_ptr(xlnx_selectmap_of_match),

Drop of_match_ptr, it leads to warnings.

> +	},
> +	.probe  = xilinx_selectmap_probe,
> +};

Best regards,
Krzysztof