On 16/03/2022 04:01, Li-hao Kuo wrote:
> Add thermal driver for Sunplus SP7021.
>
> Signed-off-by: Li-hao Kuo <[email protected]>
> ---
> Changes in v5:
> - Modify yaml file remove reg name and change nvmem name
> - Addressed comments from Mr. Daniel Lezcano
>
> MAINTAINERS | 6 ++
> drivers/thermal/Kconfig | 10 +++
> drivers/thermal/Makefile | 1 +
> drivers/thermal/sunplus_thermal.c | 140 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 157 insertions(+)
> create mode 100644 drivers/thermal/sunplus_thermal.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e127c2f..96d5218 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18542,6 +18542,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/rtc/sunplus,sp7021-rtc.yaml
> F: drivers/rtc/rtc-sunplus.c
>
> +SUNPLUS THERMAL DRIVER
> +M: Li-hao Kuo <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/thermal/sunplus_thermal.c
> +
> SUPERH
> M: Yoshinori Sato <[email protected]>
> M: Rich Felker <[email protected]>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e37691e..98647c7 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -502,4 +502,14 @@ config KHADAS_MCU_FAN_THERMAL
> If you say yes here you get support for the FAN controlled
> by the Microcontroller found on the Khadas VIM boards.
>
> +config SUNPLUS_THERMAL
> + tristate "Sunplus thermal drivers"
> + depends on SOC_SP7021
|| COMPILE_TEST
(and test if it compiles on other archs)
> + help
> + This the Sunplus SP7021 thermal driver, which supports the primitive
> + temperature sensor embedded in Sunplus SP7021 SoC.
> +
> + If you have a Sunplus SP7021 platform say Y here and enable this option
> + to have support for thermal management
> +
> endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index f0c36a1..2f7417a 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -61,3 +61,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL) += uniphier_thermal.o
> obj-$(CONFIG_AMLOGIC_THERMAL) += amlogic_thermal.o
> obj-$(CONFIG_SPRD_THERMAL) += sprd_thermal.o
> obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL) += khadas_mcu_fan.o
> +obj-$(CONFIG_SUNPLUS_THERMAL) += sunplus_thermal.o
> \ No newline at end of file
Patch error here.
> diff --git a/drivers/thermal/sunplus_thermal.c b/drivers/thermal/sunplus_thermal.c
> new file mode 100644
> index 0000000..e2e955e
> --- /dev/null
> +++ b/drivers/thermal/sunplus_thermal.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Sunplus Inc.
> + * Author: Li-hao Kuo <[email protected]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/thermal.h>
> +
> +#define DISABLE_THERMAL (BIT(31) | BIT(15))
> +#define ENABLE_THERMAL BIT(31)
> +#define SP_THERMAL_MASK GENMASK(10, 0)
> +#define SP_TCODE_HIGH_MASK GENMASK(10, 8)
> +#define SP_TCODE_LOW_MASK GENMASK(7, 0)
> +
> +#define TEMP_RATE 608
> +#define TEMP_BASE 3500
> +#define TEMP_OTP_BASE 1518
> +
> +#define SP_THERMAL_CTL0_REG 0x0000
> +#define SP_THERMAL_STS0_REG 0x0030
> +
> +/* common data structures */
> +struct sp_thermal_data {
> + struct thermal_zone_device *pcb_tz;
> + struct platform_device *pdev;
> + enum thermal_device_mode mode;
> + void __iomem *regs;
> + int otp_temp0;
> + u32 id;
> +};
> +
> +static char sp7021_get_otp_temp_coef(struct sp_thermal_data *sp_data, struct device *dev)
Why does it return char?
> +{
> + struct nvmem_cell *cell;
> + ssize_t otp_l;
> + char *otp_v;
> +
> + cell = nvmem_cell_get(dev, "calib");
> + if (IS_ERR(cell))
> + return ERR_CAST(cell);
> +
> + otp_v = nvmem_cell_read(cell, &otp_l);
> + nvmem_cell_put(cell);
> +
> + if (otp_l < 3)
> + return -EINVAL;
> + sp_data->otp_temp0 = FIELD_PREP(SP_TCODE_LOW_MASK, otp_v[0]) |
> + FIELD_PREP(SP_TCODE_HIGH_MASK, otp_v[1]);
> + if (sp_data->otp_temp0 == 0)
> + sp_data->otp_temp0 = TEMP_OTP_BASE;
> + return 0;
> +}
> +
> +static int sp_thermal_get_sensor_temp(void *data, int *temp)
> +{
> + struct sp_thermal_data *sp_data = data;
> + int t_code;
> +
> + t_code = readl(sp_data->regs + SP_THERMAL_STS0_REG);
> + t_code = FIELD_GET(SP_THERMAL_MASK, t_code);
> + *temp = ((sp_data->otp_temp0 - t_code) * 10000 / TEMP_RATE) + TEMP_BASE;
> + *temp *= 10;
> + return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops sp_of_thermal_ops = {
This should be const.
> + .get_temp = sp_thermal_get_sensor_temp,
> +};
> +
> +static int sp_thermal_register_sensor(struct platform_device *pdev,
> + struct sp_thermal_data *data, int index)
> +{
> + data->id = index;
> + data->pcb_tz = devm_thermal_zone_of_sensor_register(&pdev->dev,
> + data->id,
> + data, &sp_of_thermal_ops);
> + if (IS_ERR_OR_NULL(data->pcb_tz))
> + return PTR_ERR(data->pcb_tz);
> + return 0;
> +}
> +
> +static int sp7021_thermal_probe(struct platform_device *pdev)
> +{
> + struct sp_thermal_data *sp_data;
> + struct resource *res;
> + int ret;
> +
> + sp_data = devm_kzalloc(&pdev->dev, sizeof(*sp_data), GFP_KERNEL);
> + if (!sp_data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res))
> + return dev_err_probe(&pdev->dev, PTR_ERR(res), "resource get fail\n");
> +
> + sp_data->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + if (IS_ERR(sp_data->regs))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sp_data->regs), "mas_base get fail\n");
Use devm_platform_ioremap_resource() instead.
> +
> + writel(ENABLE_THERMAL, sp_data->regs + SP_THERMAL_CTL0_REG);
> +
> + platform_set_drvdata(pdev, sp_data);
> + ret = sp7021_get_otp_temp_coef(sp_data, &pdev->dev);
Ignored return code.
> + ret = sp_thermal_register_sensor(pdev, sp_data, 0);
> +
> + return ret;
> +}
> +
> +static int sp7021_thermal_remove(struct platform_device *pdev)
> +{
> + return 0;
No need for empty code.
> +}
> +
> +static const struct of_device_id of_sp7021_thermal_ids[] = {
> + { .compatible = "sunplus,sp7021-thermal" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_sp7021_thermal_ids);
> +
> +static struct platform_driver sp7021_thermal_driver = {
> + .probe = sp7021_thermal_probe,
> + .remove = sp7021_thermal_remove,
> + .driver = {
> + .name = "sp7021-thermal",
> + .of_match_table = of_match_ptr(of_sp7021_thermal_ids),
of_match_ptr looks incorrect - of_device_id is always present. If you
want to handle such case - build !OF - then you need maybe_unused to
of_device_id.
Best regards,
Best regards,
Krzysztof
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (IS_ERR(res))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(res), "resource get
> > +fail\n");
> > +
> > + sp_data->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > + if (IS_ERR(sp_data->regs))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sp_data->regs), "mas_base
> > +get fail\n");
>
> Use devm_platform_ioremap_resource() instead.
>
Other drivers must also access these registers.
Warning when using devm_platform_ioremap_resource
Can I keep the original settings?
On 22/03/2022 03:55, Lh Kuo 郭力豪 wrote:
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (IS_ERR(res))
>>> + return dev_err_probe(&pdev->dev, PTR_ERR(res), "resource get
>>> +fail\n");
>>> +
>>> + sp_data->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> + if (IS_ERR(sp_data->regs))
>>> + return dev_err_probe(&pdev->dev, PTR_ERR(sp_data->regs), "mas_base
>>> +get fail\n");
>>
>> Use devm_platform_ioremap_resource() instead.
>>
>
> Other drivers must also access these registers.
> Warning when using devm_platform_ioremap_resource
> Can I keep the original settings?
You should not map one region twice. How do you guarantee
synchronization during for example updates of specific registers? In
such case you need to use regmap and share it via syscon (although this
does not solve synchronization on higher level - avoiding conflicting
changes to same registers)
Best regards,
Krzysztof