Re: [PATCH] New module to add NCT6692D watchdog funtionality

[Christophe JAILLET]

Le 02/06/2023 à 18:30, David Ober a écrit :
> The new module adds in the basic functionality of the NCT6692D
> watchdog driver. This functionality is added to support the
> Lenovo SE30 device
>
> Signed-off-by: David Ober <dober6023-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
> ---
>   drivers/watchdog/Kconfig       |  12 +
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/nct6692_wdt.c | 690 +++++++++++++++++++++++++++++++++
>   3 files changed, 703 insertions(+)
>   create mode 100644 drivers/watchdog/nct6692_wdt.c

[...]

> +static int nct6692_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nct6692_data_t *data = NULL;
> +	struct nct6692_sio_data *sio_data = dev->platform_data;
> +	struct resource *res;
> +
> +	pr_info("Probe NCT6692 called\n");
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +
> +	data = kzalloc(sizeof(struct nct6692_data_t), GFP_KERNEL);

Is it released somewhere?
Should this be devm_kzalloc()?

> +	if (!data)
> +		return -ENOMEM;
> +
> +	// init value
> +	data->shm.base_addr = 0;
> +	data->shm.report_addr = 0;

Harmless, but useless (kzalloc() above)

> +
> +	data->shm.base_phys = sio_data->base_phys;
> +	data->shm.base_addr = (u_char *)ioremap_cache(data->shm.base_phys, 256);
> +
> +	data->shm.offset_mod = SHM_WIN_MOD_OFFSET;
> +	data->shm.offset_cmd = SHM_WIN_CMD_OFFSET;
> +	data->shm.offset_sel = SHM_WIN_SEL_OFFSET;
> +	data->shm.offset_ctl = SHM_WIN_CTL_OFFSET;
> +	data->shm.offset_id = SHM_WIN_ID_OFFSET;
> +	data->shm.offset_dat = SHM_WIN_DAT_OFFSET;
> +
> +	// Base for REPORT Channel
> +	data->shm.report_phys = sio_data->report_phys;
> +	data->shm.report_addr = (u_char *)ioremap_cache(data->shm.report_phys, 256);
> +
> +	data->cfg.channel = NCT6692_CHANNEL_DEFAULT;
> +	data->cfg.mod = 0x10;
> +	data->cfg.cmd = 0;
> +	data->cfg.sel = 0;
> +	data->cfg.idx = 0x15;
> +	data->cnt.channel = NCT6692_CHANNEL_DEFAULT;
> +	data->cnt.mod = 0x10;
> +	data->cnt.cmd = 0;
> +	data->cnt.sel = 0;
> +	data->cnt.idx = 0x16;

All these "= 0" are harmless, but useless (kzalloc() above). Maybe it 
makes sense to keep them.

> +
> +	data->wdt.ops = &nct6692_wdt_ops;
> +	data->wdt.info = &nct6692_wdt_info;
> +
> +	data->wdt.timeout = WATCHDOG_TIMEOUT; /* Set default timeout */
> +	data->wdt.min_timeout = MIN_TIMEOUT;
> +	data->wdt.max_timeout = MAX_TIMEOUT;
> +	data->wdt.parent = &pdev->dev;
> +
> +	watchdog_init_timeout(&data->wdt, timeout, &pdev->dev);
> +	watchdog_set_nowayout(&data->wdt, nowayout);
> +	watchdog_set_drvdata(&data->wdt, data);
> +
> +	watchdog_stop_on_unregister(&data->wdt);
> +
> +	return devm_watchdog_register_device(dev, &data->wdt);
> +}

[...]

> +static int __init nct6692_init(void)
> +{
> +	struct nct6692_sio_data sio_data;
> +	int sioaddr[2] = { 0x2e, 0x4e };
> +	struct resource res;
> +	int err;
> +	int address;
> +	bool found = false;
> +	u_long base_phys = 0;
> +	u_long report_phys = 0;
> +
> +	platform_driver_register(&nct6692_driver);
> +
> +	/*
> +	 * initialize sio_data->kind and sio_data->sioreg.
> +	 *
> +	 * when Super-I/O functions move to a separate file, the Super-I/O
> +	 * driver will probe 0x2e and 0x4e and auto-detect the presence of a
> +	 * nct6692 hardware monitor, and call probe()
> +	 */
> +	err = nct6692_find(sioaddr[0], &base_phys, &report_phys);
> +	if (err) {
> +		err = nct6692_find(sioaddr[1], &base_phys, &report_phys);
> +		if (err)
> +			return -ENODEV;

goto exit_unregister; ?

> +	}
> +	found = true;
> +	sio_data.base_phys = base_phys;
> +	sio_data.report_phys = report_phys;
> +

[...]