Re: [PATCH 1/1] rtc: atcrtc100: The first release of the atcrtc100 driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/05/2024 15:37, CL Wang wrote:
> The following features are included in this driver.
> 1. Basic Function of the RTC.
> 
> 2. The Andes internal implementation of the RTC on FPGA cannot count more
>    than 32 days because the day counter is 5 bits long, so use a variable
>    to hold the date in the situation where the size of the day is
>    insufficient to represent the date.
> 
> 3. The maximum day counter available for the ATCRTC100 hardware is 15
>    bits long and can count up to 32767 days. This means that the ATCRTC100
>    hardware can count up to about 89 years, so we set range_min to 2000
>    and range_max to the end of the year 2089.
> 
> Signed-off-by: CL Wang <cl634@andestech.com>
> ---
>  drivers/rtc/Kconfig         |  10 +
>  drivers/rtc/Makefile        |   1 +
>  drivers/rtc/rtc-atcrtc100.c | 463 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 474 insertions(+)
>  create mode 100644 drivers/rtc/rtc-atcrtc100.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2a95b05982ad..cea1750c368d 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1442,6 +1442,16 @@ config RTC_DRV_OMAP
>  	  This driver can also be built as a module, if so, module
>  	  will be called rtc-omap.
>  
> +config RTC_DRV_ATCRTC100
> +	tristate "Andes Real Time Clock"
> +	depends on RISCV
> +	help
> +	  If you say Y here you will get access to the real time clock
> +	  built into your AE350.
> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called rtc-atcrtc100.
> +
>  config RTC_DRV_S3C
>  	tristate "Samsung S3C series SoC RTC"
>  	depends on ARCH_EXYNOS || ARCH_S3C64XX || ARCH_S5PV210 || \
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 3004e372f25f..cc41cd5d3017 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_RTC_DRV_ASM9260)	+= rtc-asm9260.o
>  obj-$(CONFIG_RTC_DRV_ASPEED)	+= rtc-aspeed.o
>  obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
>  obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
> +obj-$(CONFIG_RTC_DRV_ATCRTC100) += rtc-atcrtc100.o
>  obj-$(CONFIG_RTC_DRV_AU1XXX)	+= rtc-au1xxx.o
>  obj-$(CONFIG_RTC_DRV_BBNSM)	+= rtc-nxp-bbnsm.o
>  obj-$(CONFIG_RTC_DRV_BD70528)	+= rtc-bd70528.o
> diff --git a/drivers/rtc/rtc-atcrtc100.c b/drivers/rtc/rtc-atcrtc100.c
> new file mode 100644
> index 000000000000..505caf04e948
> --- /dev/null
> +++ b/drivers/rtc/rtc-atcrtc100.c
> @@ -0,0 +1,463 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2008-2024 Andes Technology Corporation
> + * Andes RTC Support
> + *
> + * Copyright (C) 2006, 2007, 2008  Paul Mundt

I can see that... you need to fix and improve the driver to look like
something recent. Start from scratch and take the newest driver as template.

> +	RTC_WRITE32(RTC_READ32(RTC_CR) | RTC_EN, RTC_CR);
> +
> +	return 0;
> +
> +err_unmap:
> +	iounmap(rtc->regbase);
> +err_ioremap1:
> +	release_resource(rtc->res);
> +err_request_region:
> +	free_irq(rtc->interrupt_irq, rtc);
> +err_interrupt_irq:
> +	free_irq(rtc->alarm_irq, rtc);
> +err_exit:

Why you do not use devm?

> +	return ret;
> +}
> +

You miss bindings patch.

> +static int atc_rtc_remove(struct platform_device *pdev)
> +{
> +	struct atc_rtc *rtc = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * Because generic rtc will not execute rtc_device_release()
> +	 * when call rtc_device_unregister(),
> +	 * rtc id will increase when unloading a rtc driver.
> +	 * This can work around to recycle rtc id.
> +	 * But if kernel fix this issue, it shell be removed away.
> +	 */

No, that's just confusing... and does not look correct. Anyway, fix the
bug in the kernel (if there is such...) instead of introducing weird
workarounds.

> +	rtc->rtc_dev->dev.release(&rtc->rtc_dev->dev);
> +	RTC_WRITE32(RTC_READ32(RTC_CR) & ~(RTC_EN | SEC_INT | ALARM_INT), RTC_CR);
> +	free_irq(rtc->alarm_irq, rtc);
> +	free_irq(rtc->interrupt_irq, rtc);
> +	iounmap(rtc->regbase);
> +	release_resource(rtc->res);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}


> +module_platform_driver(atc_rtc_platform_driver);
> +MODULE_DESCRIPTION("Andes ATCRTC100 driver");
> +MODULE_AUTHOR("Paul Mundt <lethal@linux-sh.org>, Jamie Lenehan <lenehan@twibble.org>, Angelo Castello <angelo.castello@st.com>, Nick Hu <nickhu@andestech.com>, CL Wang <cl634@andestech.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.



Best regards,
Krzysztof