Message-ID: <df512124-af4d-2528-d752-04337957fa71@metux.net>
Date:   Mon, 28 Jun 2021 19:28:37 +0200
From:   "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To:     Moriis Ku <saumah@...il.com>, lee.jones@...aro.org
Cc:     linux-kernel@...r.kernel.org, jason_lee@...ix.com,
        taian.chen@...ix.com, morris_ku@...ix.com
Subject: Re: [PATCH] SUNIX SDC PCIe multi-function card core driver

On 25.06.21 10:55, Moriis Ku wrote:

Hi,


first of all: put all patches into one queue, so we know these things
belong together.



> From: Morris Ku <saumah@...il.com>
> 
> Add support for SUNIX SDC PCIe multi-function card
> 
> Cc: Jason Lee <jason_lee@...ix.com>
> Cc: Taian Chen <taian.chen@...ix.com>
> Cc: Morris Ku <morris_ku@...ix.com>
> Signed-off-by: Morris Ku <saumah@...il.com>
> ---
>   sunix-sdc.c | 606 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 606 insertions(+)
>   create mode 100644 sunix-sdc.c
> 
> diff --git a/sunix-sdc.c b/sunix-sdc.c
> new file mode 100644
> index 0000000..5e724a3
> --- /dev/null
> +++ b/sunix-sdc.c
> @@ -0,0 +1,606 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SUNIX SDC PCIe multi-function card core support.
> + *
> + * Copyright (C) 2021, SUNIX Co., Ltd.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/io.h>
> +#include <linux/property.h>
> +#include <linux/debugfs.h>
> +#include <linux/idr.h>
> +#include <linux/mfd/core.h>
> +#include "sunix-sdc.h"
> +
> +#define DRIVER_NAME		"sunix-sdc"
> +
> +enum cib_type {
> +	TYPE_CONFIG = 0,
> +	TYPE_UART,
> +	TYPE_DIO,
> +	TYPE_CAN
> +};

What is a "CIB" ?

> +struct cib_config {
> +	u32 mem_offset;
> +	u32 mem_size;
> +	u8 ic_brand;
> +	u8 ic_model;
> +};
> +
> +struct cib_uart {
> +	u32 io_offset;
> +	u8 io_size;
> +	u32 mem_offset;
> +	u32 mem_size;
> +	u16 tx_fifo_size;
> +	u16 rx_fifo_size;
> +	u32 significand;
> +	u8 exponent;
> +	u32 capability;
> +};
> +
> +struct cib_dio_bank {
> +	u8 number_of_io;
> +	char number_of_io_name[32];
> +	u8 capability;
> +	char capability_name[32];
> +};
> +
> +struct cib_dio {
> +	u32 mem_offset;
> +	u32 mem_size;
> +	u8 number_of_bank;
> +	u8 capability;
> +
> +	struct cib_dio_bank *banks;
> +};
> +
> +struct cib_can {
> +	u32 mem_offset;
> +	u32 mem_size;
> +	u32 significand;
> +	u8 exponent;
> +	u8 number_of_device;
> +	u8 device_type;
> +	u8 gpio_input;
> +	u8 gpio_output;
> +};
> +
> +struct cib_info {
> +	u8 number;
> +	enum cib_type type;
> +	u8 version;
> +	u8 total_length;
> +	u8 resource_cap;
> +	u8 event_type;
> +
> +	struct cib_config *config;
> +	struct cib_uart *uart;
> +	struct cib_dio *dio;
> +	struct cib_can *can;
> +};
> +
> +struct sdc_channel {
> +	struct cib_info info;
> +
> +	struct property_entry *property;
> +	struct resource *resource;
> +	struct mfd_cell *cell;
> +};
> +
> +struct sdc_board {
> +	struct sunix_sdc_platform_info *info;
> +
> +	u8 major_version;
> +	u8 minor_version;
> +	u8 available_chls;
> +	u8 total_length;
> +	char model_name[16];
> +
> +	struct sdc_channel *channels;
> +	struct device *dev;
> +	int id;
> +
> +	struct debugfs_blob_wrapper debugfs_model_name;
> +};

I think it might be better splitting this big thing apart into several
drivers for each sub device and have frontend driver(s) that does the
probing / instantiation of the individual components. Note that not
everybody has all the corresponding subsystems enabled.


> +static int sdc_board_id = 1;
> +static int sdc_uart_id = 1;
> +static int sdc_dio_id = 1;
> +static int sdc_can_id = 1;

What exactly are these global singletons for ?
Usually device specific data belongs into the device's private data,
except there's a damn good reason.

<snip>

> +static void sdc_get_dio_info(struct cib_dio *dio, void __iomem *base,
> +				u16 ptr)
> +{
> +	u32 temp;

what exactly is an "dio" ? do you mean gpio ?

<snip>

> +int sunix_sdc_probe(struct device *dev, struct sunix_sdc_platform_info *info)
> +{

Doesn't really look like some device probe function. Who calls that and
who fills this struct ?

> +	struct sdc_channel *chl;
> +	struct sdc_board *sdc;
> +	struct cib_dio *dio;
> +	struct cib_can *can;
> +	void __iomem *mem_base;
> +	u16 chl_offset_backup;
> +	u16 chl_offset;
> +	u32 temp;
> +	u8 type;
> +	int index;
> +	int ret;
> +	int i;
> +	int j;
> +
> +	if (!info || !info->b1_io || !info->b2_mem || info->irq <= 0)
> +		return -EINVAL;
> +
> +	sdc = devm_kzalloc(dev, sizeof(*sdc), GFP_KERNEL);
> +	if (!sdc)
> +		return -ENOMEM;
> +
> +	mem_base = devm_ioremap(dev, info->b2_mem->start, resource_size(info->b2_mem));
> +	if (!mem_base)
> +		return -ENOMEM;
> +
> +	sdc->info = info;
> +	sdc->dev = dev;
> +	sdc->id = sdc_board_id++;
> +
> +	temp = sdc_readl(mem_base, 0, 0);
> +	sdc->major_version = temp & GENMASK(7, 0);
> +	sdc->minor_version = (temp & GENMASK(15, 8)) >> 8;
> +	sdc->available_chls = (temp & GENMASK(23, 16)) >> 16;
> +	sdc->total_length = (temp & GENMASK(31, 24)) >> 24;
> +
> +	temp = sdc_readl(mem_base, 0, 1);
> +	chl_offset = chl_offset_backup = temp & GENMASK(15, 0);
> +
> +	j = 0;
> +	for (i = 0; i < 4; i++) {
> +		temp = sdc_readl(mem_base, 0, 2 + i);
> +		sdc->model_name[j++] = temp & GENMASK(7, 0);
> +		sdc->model_name[j++] = (temp & GENMASK(15, 8)) >> 8;
> +		sdc->model_name[j++] = (temp & GENMASK(23, 16)) >> 16;
> +		sdc->model_name[j++] = (temp & GENMASK(31, 24)) >> 24;
> +	}
> +	sdc->model_name[strlen(sdc->model_name)] = '\n';
> +
> +	sdc->channels = devm_kcalloc(dev, sdc->available_chls,
> +		sizeof(struct sdc_channel), GFP_KERNEL);
> +	if (!sdc->channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sdc->available_chls; i++) {
> +		chl = &sdc->channels[i];
> +		chl_offset_backup = chl_offset;
> +
> +		temp = sdc_readl(mem_base, chl_offset, 0);
> +		chl->info.number = temp & GENMASK(7, 0);
> +		type = (temp & GENMASK(15, 8)) >> 8;
> +		switch (type) {
> +		case 0x00:
> +		case 0x01:
> +		case 0x02:
> +		case 0x03:
> +			chl->info.type = (enum cib_type)type;
> +			break;
> +		default:
> +			break;
> +		}
> +		chl->info.version = (temp & GENMASK(23, 16)) >> 16;
> +		chl->info.total_length = (temp & GENMASK(31, 24)) >> 24;
> +
> +		temp = sdc_readl(mem_base, chl_offset, 1);
> +		chl_offset = temp & GENMASK(15, 0);
> +		chl->info.resource_cap = (temp & GENMASK(23, 16)) >> 16;
> +		chl->info.event_type = (temp & GENMASK(31, 24)) >> 24;
> +
> +		switch (chl->info.type) {
> +		case TYPE_CONFIG:
> +			chl->info.config = devm_kzalloc(dev,
> +				sizeof(*chl->info.config), GFP_KERNEL);
> +			if (!chl->info.config)
> +				return -ENOMEM;
> +			sdc_get_config_info(chl->info.config, mem_base,
> +				chl_offset_backup);
> +			break;
> +
> +		case TYPE_UART:
> +			chl->info.uart = devm_kzalloc(dev,
> +				sizeof(*chl->info.uart), GFP_KERNEL);
> +			if (!chl->info.uart)
> +				return -ENOMEM;
> +			sdc_get_uart_info(chl->info.uart, mem_base,
> +				chl_offset_backup);
> +
> +			chl->property = devm_kcalloc(dev, 8,
> +				sizeof(struct property_entry), GFP_KERNEL);
> +			if (!chl->property)
> +				return -ENOMEM;
> +			index = 0;
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"board_id", sdc->id);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"chl_number", chl->info.number);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"version", chl->info.version);
> +			chl->property[index++] = PROPERTY_ENTRY_U16(
> +				"tx_fifo_size", chl->info.uart->tx_fifo_size);
> +			chl->property[index++] = PROPERTY_ENTRY_U16(
> +				"rx_fifo_size", chl->info.uart->rx_fifo_size);
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"significand", chl->info.uart->significand);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"exponent", chl->info.uart->exponent);
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"capability", chl->info.uart->capability);
> +
> +			chl->resource = devm_kcalloc(dev, 2,
> +				sizeof(struct resource), GFP_KERNEL);
> +			if (!chl->resource)
> +				return -ENOMEM;
> +			chl->resource[0].start = info->b1_io->start +
> +				chl->info.uart->io_offset;
> +			chl->resource[0].end = chl->resource[0].start +
> +				chl->info.uart->io_size - 1;
> +			chl->resource[0].name = "8250_sdc";
> +			chl->resource[0].flags = IORESOURCE_IO;
> +			chl->resource[0].desc = IORES_DESC_NONE;
> +			chl->resource[1].start = 0;
> +			chl->resource[1].end =  0;
> +			chl->resource[1].name = "irq";
> +			chl->resource[1].flags = IORESOURCE_IRQ;
> +			chl->resource[1].desc = IORES_DESC_NONE;

Since this thing is initialized from somewhere else, why not directly 
passing in struct resource instances ?

> +			chl->cell = devm_kzalloc(dev,
> +				sizeof(struct mfd_cell), GFP_KERNEL);

MFD ? Seriously ?

> +			if (!chl->cell)
> +				return -ENOMEM;
> +			chl->cell->name = "8250_sdc";
> +			chl->cell->id = sdc_uart_id++;
> +			chl->cell->properties = chl->property;
> +			chl->cell->num_resources = 2;
> +			chl->cell->resources = chl->resource;
> +			break;
> +
> +		case TYPE_DIO:
> +			chl->info.dio = devm_kzalloc(dev,
> +				sizeof(*chl->info.dio), GFP_KERNEL);
> +			if (!chl->info.dio)
> +				return -ENOMEM;
> +			dio = chl->info.dio;
> +			sdc_get_dio_info(dio, mem_base, chl_offset_backup);
> +			if (dio->number_of_bank) {
> +				dio->banks =
> +					devm_kcalloc(dev, dio->number_of_bank,
> +					sizeof(*dio->banks), GFP_KERNEL);
> +				if (!dio->banks)
> +					return -ENOMEM;
> +
> +				sdc_get_dio_banks_info(dio, mem_base,
> +					chl_offset_backup);
> +			}
> +
> +			chl->property = devm_kcalloc(dev, 5 + dio->number_of_bank * 2,
> +				sizeof(struct property_entry), GFP_KERNEL);
> +			if (!chl->property)
> +				return -ENOMEM;
> +			index = 0;
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"board_id", sdc->id);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"chl_number", chl->info.number);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"version", chl->info.version);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"number_of_bank", dio->number_of_bank);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"capability", dio->capability);
> +			for (j = 0; j < dio->number_of_bank; j++) {
> +				snprintf(dio->banks[j].number_of_io_name,
> +					sizeof(dio->banks[j].number_of_io_name),
> +					"b%d_number_of_io", j);
> +				chl->property[index++] = PROPERTY_ENTRY_U8(
> +				dio->banks[j].number_of_io_name, dio->banks[j].number_of_io);
> +				snprintf(dio->banks[j].capability_name,
> +					sizeof(dio->banks[j].capability_name),
> +					"b%d_capability", j);
> +				chl->property[index++] = PROPERTY_ENTRY_U8(
> +				dio->banks[j].capability_name, dio->banks[j].capability);
> +			}
> +
> +			chl->resource = devm_kcalloc(dev, 4,
> +				sizeof(struct resource), GFP_KERNEL);
> +			if (!chl->resource)
> +				return -ENOMEM;
> +			chl->resource[0].start = info->b2_mem->start +
> +				chl->info.dio->mem_offset;
> +			chl->resource[0].end = chl->resource[0].start +
> +				chl->info.dio->mem_size - 1;
> +			chl->resource[0].name = "gpio_sdc";
> +			chl->resource[0].flags = IORESOURCE_MEM;
> +			chl->resource[0].desc = IORES_DESC_NONE;
> +			chl->resource[1].start = 0;
> +			chl->resource[1].end =  0;
> +			chl->resource[1].name = "irq";
> +			chl->resource[1].flags = IORESOURCE_IRQ;
> +			chl->resource[1].desc = IORES_DESC_NONE;
> +			chl->resource[2].start = info->b0_mem->start;
> +			chl->resource[2].end = chl->resource[2].start + 32 - 1;
> +			chl->resource[2].name = "sdc_irq_vector";
> +			chl->resource[2].flags = IORESOURCE_MEM;
> +			chl->resource[2].desc = IORES_DESC_NONE;
> +			chl->resource[3].start = info->b0_mem->start + 32 +
> +				(chl->info.number * 4);
> +			chl->resource[3].end = chl->resource[3].start + 4 - 1;
> +			chl->resource[3].name = "gpio_sdc_event_header";
> +			chl->resource[3].flags = IORESOURCE_MEM;
> +			chl->resource[3].desc = IORES_DESC_NONE;
> +
> +			chl->cell = devm_kzalloc(dev,
> +				sizeof(struct mfd_cell), GFP_KERNEL);
> +			if (!chl->cell)
> +				return -ENOMEM;
> +			chl->cell->name = "gpio_sdc";
> +			chl->cell->id = sdc_dio_id++;
> +			chl->cell->properties = chl->property;
> +			chl->cell->num_resources = 4;
> +			chl->cell->resources = chl->resource;
> +			break;
> +
> +		case TYPE_CAN:
> +			chl->info.can = devm_kzalloc(dev,
> +				sizeof(*chl->info.can), GFP_KERNEL);
> +			if (!chl->info.can)
> +				return -ENOMEM;
> +			can = chl->info.can;
> +			sdc_get_can_info(can, mem_base,	chl_offset_backup);
> +
> +			if (can->number_of_device != 1 && can->device_type != 0x03)
> +				break;
> +
> +			chl->property = devm_kcalloc(dev, 7,
> +				sizeof(struct property_entry), GFP_KERNEL);
> +			if (!chl->property)
> +				return -ENOMEM;
> +			index = 0;
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"board_id", sdc->id);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"chl_number", chl->info.number);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"version", chl->info.version);
> +			chl->property[index++] = PROPERTY_ENTRY_U32(
> +				"significand", can->significand);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"exponent", can->exponent);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"gpio_input", can->gpio_input);
> +			chl->property[index++] = PROPERTY_ENTRY_U8(
> +				"gpio_output", can->gpio_output);
> +
> +			chl->resource = devm_kcalloc(dev, 2,
> +				sizeof(struct resource), GFP_KERNEL);
> +			if (!chl->resource)
> +				return -ENOMEM;
> +			chl->resource[0].start = info->b2_mem->start +
> +				chl->info.can->mem_offset;
> +			chl->resource[0].end = chl->resource[0].start +
> +				chl->info.can->mem_size - 1;
> +			chl->resource[0].name = "sx2010_can";
> +			chl->resource[0].flags = IORESOURCE_MEM;
> +			chl->resource[0].desc = IORES_DESC_NONE;
> +			chl->resource[1].start = 0;
> +			chl->resource[1].end =  0;
> +			chl->resource[1].name = "irq";
> +			chl->resource[1].flags = IORESOURCE_IRQ;
> +			chl->resource[1].desc = IORES_DESC_NONE;
> +
> +			chl->cell = devm_kzalloc(dev,
> +				sizeof(struct mfd_cell), GFP_KERNEL);
> +			if (!chl->cell)
> +				return -ENOMEM;
> +			chl->cell->name = "sx2010_can";
> +			chl->cell->id = sdc_can_id++;
> +			chl->cell->properties = chl->property;
> +			chl->cell->num_resources = 2;
> +			chl->cell->resources = chl->resource;
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
> +	dev_set_drvdata(dev, sdc);
> +	sdc_debugfs_add(sdc);
> +
> +	for (i = 0; i < sdc->available_chls; i++) {
> +		chl = &sdc->channels[i];
> +
> +		if (chl->cell) {
> +			ret = mfd_add_devices(dev, sdc->id, chl->cell, 1,
> +						NULL, sdc->info->irq, NULL);
> +			if (ret)
> +				goto err_remove_sdc;
> +		}
> +	}
> +
> +	dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
> +	return 0;
> +
> +err_remove_sdc:
> +	sdc_debugfs_remove(sdc);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sunix_sdc_probe);

I really wonder what this functions is actually for. Doesn't seem to be
some driver probe function. Doesn't seem to create any devices.

> +void sunix_sdc_remove(struct device *dev)
> +{
> +	struct sdc_board *sdc = dev_get_drvdata(dev);
> +
> +	mfd_remove_devices(dev);
> +	sdc_debugfs_remove(sdc);
> +}
> +EXPORT_SYMBOL_GPL(sunix_sdc_remove);
> +
> +static int __init sunix_sdc_init(void)
> +{
> +	sdc_debugfs_init();
> +	return 0;
> +}
> +module_init(sunix_sdc_init);
> +
> +static void __exit sunix_sdc_exit(void)
> +{
> +	sdc_debugfs_exit();
> +}
> +module_exit(sunix_sdc_exit);
> +
> +MODULE_AUTHOR("Jason Lee <jason_lee@...ix.com>");
> +MODULE_DESCRIPTION("SUNIX SDC PCIe multi-function card core driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> 

Sorry, but it's totally unclear to me what this code is actually
supposed to do.

Please submit a complete queue that does something actually useful and
give some more explainations.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287

Powered by blists - more mailing lists