On Tue, 9 Jun 2020 14:20:37 +0200
Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:
> Jonathan Cameron writes:
>
> > On Tue, 9 Jun 2020 09:49:40 +0200
> > Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:
> >
> >> This patch adds a temperature sensor driver to the Sparx5 SoC.
> >>
> >> Reviewed-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> >> Signed-off-by: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
> >
> > Hi. Quick drive by review whilst waiting for coffee time...
> >
> > A few minor suggestions inline.
> >
>
> Much appreciated!
>
> > Thanks,
> >
> > Jonathan
> >
> >> ---
> >> drivers/hwmon/Kconfig | 10 +++
> >> drivers/hwmon/Makefile | 2 +-
> >> drivers/hwmon/sparx5-temp.c | 152
> >> ++++++++++++++++++++++++++++++++++++ 3 files changed, 163
> >> insertions(+), 1 deletion(-) create mode 100644
> >> drivers/hwmon/sparx5-temp.c
> >>
> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >> index 288ae9f63588c..ec6bb8b8b12df 100644
> >> --- a/drivers/hwmon/Kconfig
> >> +++ b/drivers/hwmon/Kconfig
> >> @@ -515,6 +515,16 @@ config SENSORS_I5K_AMB
> >> This driver can also be built as a module. If so, the
> >> module will be called i5k_amb.
> >>
> >> +config SENSORS_SPARX5
> >> + tristate "Sparx5 SoC temperature sensor"
> >> + depends on ARCH_SPARX5
> > Anything stop this building with COMPILE_TEST?
> >
> > That will great increase automated build coverage.
> >
> >> + help
> >> + If you say yes here you get support for temperature
> >> monitoring
> >> + with the Microchip Sparx5 SoC.
> >> +
> >> + This driver can also be built as a module. If so, the
> >> module
> >> + will be called sparx5-temp.
> >> +
> >> config SENSORS_F71805F
> >> tristate "Fintek F71805F/FG, F71806F/FG and F71872F/FG"
> >> depends on !PPC
> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >> index 3e32c21f5efe3..144f09993a3f4 100644
> >> --- a/drivers/hwmon/Makefile
> >> +++ b/drivers/hwmon/Makefile
> >> @@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_DS1621) += ds1621.o
> >> obj-$(CONFIG_SENSORS_EMC1403) += emc1403.o
> >> obj-$(CONFIG_SENSORS_EMC2103) += emc2103.o
> >> obj-$(CONFIG_SENSORS_EMC6W201) += emc6w201.o
> >> +obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> >> obj-$(CONFIG_SENSORS_F71805F) += f71805f.o
> >> obj-$(CONFIG_SENSORS_F71882FG) += f71882fg.o
> >> obj-$(CONFIG_SENSORS_F75375S) += f75375s.o
> >> @@ -193,4 +194,3 @@ obj-$(CONFIG_SENSORS_OCC) += occ/
> >> obj-$(CONFIG_PMBUS) += pmbus/
> >>
> >> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> >> -
> >> diff --git a/drivers/hwmon/sparx5-temp.c
> >> b/drivers/hwmon/sparx5-temp.c new file mode 100644
> >> index 0000000000000..2e754462b6010
> >> --- /dev/null
> >> +++ b/drivers/hwmon/sparx5-temp.c
> >> @@ -0,0 +1,152 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/* Sparx5 SoC temperature sensor driver
> >> + *
> >> + * Copyright (C) 2020 Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/init.h>
> >> +#include <linux/hwmon.h>
> >> +#include <linux/io.h>
> >> +#include <linux/of_device.h>
> > I think you only have this to define the id table?
> >
> > If so, perhaps better to include mod_devicetable.h and not include
> > the of header.
>
> Well, I ended up needed other, so it became:
>
> -#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>
> Other of these drivers seem to use "linux/of_device.h" as well?
Yeah. Lots of drivers take the lazy approach of including of_device.h
but doesn't mean we can't do better.
:)
Preference is always to include a little as possible (in terms of the
tree of headers rather than include lines in a file)
J
>
> >
> >> +
> >> +#define TEMP_CTRL 0
> >> +#define TEMP_CFG 4
> >> +#define TEMP_CFG_CYCLES GENMASK(24, 15)
> >> +#define TEMP_CFG_CYCLES_OFF 15
> >
> > Could you used FIELD_PREP etc to avoid having to have both
> > the mask and offset defined here?
> >
> >> +#define TEMP_CFG_ENA BIT(0)
> >> +#define TEMP_STAT 8
> >> +#define TEMP_STAT_VALID BIT(12)
> >> +#define TEMP_STAT_TEMP GENMASK(11, 0)
> >> +
> >> +struct s5_hwmon {
> >> + void __iomem *base;
> >> +};
> >> +
> >> +static void s5_temp_enable(struct s5_hwmon *hwmon)
> >> +{
> >> + u32 val = readl(hwmon->base + TEMP_CFG);
> >> + u32 clk = 250;
> >> +
> >> + val &= ~TEMP_CFG_CYCLES;
> >> + val |= (clk << TEMP_CFG_CYCLES_OFF);
> >> + val |= TEMP_CFG_ENA;
> >> +
> >> + writel(val, hwmon->base + TEMP_CFG);
> >> +}
> >> +
> >> +static void s5_temp_disable(void *data)
> >> +{
> >> + struct s5_hwmon *hwmon = data;
> >> + u32 val = readl(hwmon->base + TEMP_CFG);
> >> +
> >> + val &= ~TEMP_CFG_ENA;
> >> +
> >> + writel(val, hwmon->base + TEMP_CFG);
> >> +}
> >> +
> >> +static int s5_read(struct device *dev, enum hwmon_sensor_types
> >> type,
> >> + u32 attr, int channel, long *temp)
> >> +{
> >> + struct s5_hwmon *hwmon = dev_get_drvdata(dev);
> >> + int rc = 0, value;
> >> + u32 stat;
> >> +
> >> + switch (attr) {
> >> + case hwmon_temp_input:
> >> + stat = readl_relaxed(hwmon->base + TEMP_STAT);
> >> + if (!(stat & TEMP_STAT_VALID))
> >> + return -EIO;
> >> + value = stat & TEMP_STAT_TEMP;
> >> + value = DIV_ROUND_CLOSEST(value * 3522, 4096) - 1094;
> >> + value *= 100;
> >> + *temp = value;
> >> + break;
> >> + default:
> >> + rc = -EOPNOTSUPP;
> >> + break;
> >> + }
> >> +
> >> + return rc;
> >> +}
> >> +
> >> +static umode_t s5_is_visible(const void *_data, enum
> >> hwmon_sensor_types type,
> >> + u32 attr, int channel)
> >> +{
> >> + if (type != hwmon_temp)
> >> + return 0;
> >> +
> >> + switch (attr) {
> >> + case hwmon_temp_input:
> >> + return 0444;
> >> + default:
> >> + return 0;
> >> + }
> >> +}
> >> +
> >> +static const struct hwmon_channel_info *s5_info[] = {
> >> + HWMON_CHANNEL_INFO(chip,
> >> + HWMON_C_REGISTER_TZ),
> >> + HWMON_CHANNEL_INFO(temp,
> >> + HWMON_T_INPUT),
> >
> > Excess line breaks. The above 2 would be more readable on one line
> > each.
>
> Right.
>
> >> + NULL
> >> +};
> >> +
> >> +static const struct hwmon_ops s5_hwmon_ops = {
> >> + .is_visible = s5_is_visible,
> >> + .read = s5_read,
> >> +};
> >> +
> >> +static const struct hwmon_chip_info s5_chip_info = {
> >> + .ops = &s5_hwmon_ops,
> >> + .info = s5_info,
> >> +};
> >> +
> >> +static int s5_temp_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *hwmon_dev;
> >> + struct s5_hwmon *hwmon;
> >> + int err;
> >> +
> >> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
> >> + if (!hwmon)
> >> + return -ENOMEM;
> >> +
> >> + hwmon->base = devm_platform_ioremap_resource(pdev, 0);
> >> + if (IS_ERR(hwmon->base))
> >> + return PTR_ERR(hwmon->base);
> >> +
> >> + err = devm_add_action(&pdev->dev, s5_temp_disable, hwmon);
> >> + if (err)
> >> + return err;
> >
> > Probably just my linear way of thinking, but unusual to put error
> > handling / remove stuff in place _before_ the thing it's unwinding.
> >
> > We have devm_add_action_or_reset to make it safe to call this after
> > the thing it unwinds.
> >
>
> Seems I got this "backwardness" from ltq-cputemp.c. However, its
> totally unneeded here, so I'll just remove it.
>
> Thanks!
>
> ---Lars
>
> >> +
> >> + s5_temp_enable(hwmon);
> >> +
> >> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> >> + "s5_temp",
> >> + hwmon,
> >> +
> >> &s5_chip_info,
> >> + NULL);
> >> +
> >> + return PTR_ERR_OR_ZERO(hwmon_dev);
> >> +}
> >> +
> >> +const struct of_device_id s5_temp_match[] = {
> >> + { .compatible = "microchip,sparx5-temp" },
> >> + {},
> >> +};
> >> +MODULE_DEVICE_TABLE(of, s5_temp_match);
> >> +
> >> +static struct platform_driver s5_temp_driver = {
> >> + .probe = s5_temp_probe,
> >> + .driver = {
> >> + .name = "sparx5-temp",
> >> + .of_match_table = s5_temp_match,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(s5_temp_driver);
> >> +
> >> +MODULE_AUTHOR("Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>");
> >> +MODULE_DESCRIPTION("Sparx5 SoC temperature sensor driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.27.0
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
