lkml.org 
[lkml]   [2021]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v13 5/9] thermal: sy7636a: Add thermal driver for sy7636a
    From
    Date
    On 15/10/2021 14:25, Alistair Francis wrote:
    > Add thermal driver to enable kernel based polling
    > and shutdown of device for temperatures out of spec

    As it is an initial submission, could you give a brief description of
    the sensor even if it is very simple ?

    > Signed-off-by: Alistair Francis <alistair@alistair23.me>
    > ---
    > drivers/thermal/Kconfig | 6 ++
    > drivers/thermal/Makefile | 1 +
    > drivers/thermal/sy7636a_thermal.c | 94 +++++++++++++++++++++++++++++++
    > 3 files changed, 101 insertions(+)
    > create mode 100644 drivers/thermal/sy7636a_thermal.c
    >
    > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
    > index d7f44deab5b1..6ee0e7de1b37 100644
    > --- a/drivers/thermal/Kconfig
    > +++ b/drivers/thermal/Kconfig
    > @@ -450,6 +450,12 @@ depends on (ARCH_STI || ARCH_STM32) && OF
    > source "drivers/thermal/st/Kconfig"
    > endmenu
    >
    > +config SY7636A_THERMAL
    > + tristate "SY7636A thermal management"

    no deps ?

    > + help
    > + Enable the sy7636a thermal driver, which supports the
    > + temperature sensor embedded in Silabs SY7636A IC.
    > +
    > source "drivers/thermal/tegra/Kconfig"
    >
    > config GENERIC_ADC_THERMAL
    > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
    > index 82fc3e616e54..2e1aca8a0a09 100644
    > --- a/drivers/thermal/Makefile
    > +++ b/drivers/thermal/Makefile
    > @@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
    > obj-y += intel/
    > obj-$(CONFIG_TI_SOC_THERMAL) += ti-soc-thermal/
    > obj-y += st/
    > +obj-$(CONFIG_SY7636A_THERMAL) += sy7636a_thermal.o
    > obj-$(CONFIG_QCOM_TSENS) += qcom/
    > obj-y += tegra/
    > obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o
    > diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
    > new file mode 100644
    > index 000000000000..9e58305ca3ce
    > --- /dev/null
    > +++ b/drivers/thermal/sy7636a_thermal.c
    > @@ -0,0 +1,94 @@
    > +// SPDX-License-Identifier: GPL-2.0+
    > +/*
    > + * Functions to access SY3686A power management chip temperature
    > + *
    > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/

    2021

    > + *
    > + * Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
    > + * Alistair Francis <alistair@alistair23.me>
    > + */

    From Documentation/process/submitting-drivers.rst

    """
    Copyright:
    The copyright owner must agree to use of GPL.
    It's best if the submitter and copyright owner
    are the same person/entity. If not, the name of
    the person/entity authorizing use of GPL should be
    listed in case it's necessary to verify the will of
    the copyright owner.
    """

    [Cc'ed Lars Ivar Miljeteig]

    > +#include <linux/module.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/thermal.h>
    > +
    > +#include <linux/mfd/sy7636a.h>
    > +
    > +static int sy7636a_get_temp(void *arg, int *res)

    ...(struct thermal_zone_device *tz, int *temp)


    > +{
    > + unsigned int mode_ctr;
    > + int ret, reg_val;
    > + struct regmap *regmap = arg;
    > + bool isVoltageActive;

    Looks like c++ code

    > + ret = regmap_read(regmap,
    > + SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
    > + if (ret)
    > + return ret;
    > +
    > + isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
    > +
    > + /* If operation mode isn't set to control, then let's set it. */
    > + if (!isVoltageActive) {
    > + ret = regmap_write(regmap,
    > + SY7636A_REG_OPERATION_MODE_CRL,
    > + mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);
    > + if (ret)
    > + return ret;
    > + }

    Who is turnning off the 'control' outside of this driver?

    > + ret = regmap_read(regmap,
    > + SY7636A_REG_TERMISTOR_READOUT, &reg_val);
    > + if (ret)
    > + return ret;
    > +
    > + /* Restore the operation mode if it wasn't set */
    > + if (!isVoltageActive) {
    > + ret = regmap_write(regmap,
    > + SY7636A_REG_OPERATION_MODE_CRL,
    > + mode_ctr);
    > + if (ret)
    > + return ret;
    > + }

    IIUC, this is a mfd, so if a component outside of this driver is
    touching this SY7636A_REG_OPERATION_MODE_CRL, there is no guarantee
    these operations will stay consistent.

    Is that correct ?

    > + *res = reg_val * 1000;
    > +
    > + return ret;
    > +}
    > +
    > +static const struct thermal_zone_of_device_ops ops = {
    > + .get_temp = sy7636a_get_temp,
    > +};
    > +
    > +static int sy7636a_thermal_probe(struct platform_device *pdev)
    > +{
    > + struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
    > + struct thermal_zone_device *thermal_zone_dev;

    regmap return value check, please

    > + thermal_zone_dev = devm_thermal_zone_of_sensor_register(
    > + pdev->dev.parent,
    > + 0,
    > + regmap,
    > + &ops);

    Fix indent please

    > +
    > + return PTR_ERR_OR_ZERO(thermal_zone_dev);
    > +}
    > +
    > +static const struct platform_device_id sy7636a_thermal_id_table[] = {
    > + { "sy7636a-thermal", },
    > + { }
    > +};
    > +MODULE_DEVICE_TABLE(platform, sy7636a_thermal_id_table);
    > +
    > +static struct platform_driver sy7636a_thermal_driver = {
    > + .driver = {
    > + .name = "sy7636a-thermal",
    > + },
    > + .probe = sy7636a_thermal_probe,
    > + .id_table = sy7636a_thermal_id_table,
    > +};
    > +module_platform_driver(sy7636a_thermal_driver);
    > +
    > +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
    > +MODULE_DESCRIPTION("SY7636A thermal driver");
    > +MODULE_LICENSE("GPL v2");
    >


    --
    <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

    Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
    <http://twitter.com/#!/linaroorg> Twitter |
    <http://www.linaro.org/linaro-blog/> Blog

    \
     
     \ /
      Last update: 2021-10-16 12:33    [W:4.090 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site