| Message ID | 20240507-cros_ec-hwmon-v1-1-2c47c5ce8e85@weissschuh.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ChromeOS Embedded controller hwmon driver | expand |
On 5/7/2024 08:57, Thomas Weißschuh wrote: > The ChromeOS Embedded Controller exposes fan speed and temperature > readings. > Expose this data through the hwmon subsystem. > > The driver is designed to be probed via the cros_ec mfd device. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > Documentation/hwmon/cros_ec_hwmon.rst | 26 ++++ > Documentation/hwmon/index.rst | 1 + > MAINTAINERS | 8 + > drivers/hwmon/Kconfig | 11 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/cros_ec_hwmon.c | 279 ++++++++++++++++++++++++++++++++++ > 6 files changed, 326 insertions(+) > > diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst > new file mode 100644 > index 000000000000..aeb88c79d11b > --- /dev/null > +++ b/Documentation/hwmon/cros_ec_hwmon.rst > @@ -0,0 +1,26 @@ > +.. SPDX-License-Identifier: GPL-2.0-or-later > + > +Kernel driver cros_ec_hwmon > +=========================== > + > +Supported chips: > + > + * ChromeOS embedded controllers connected via LPC > + > + Prefix: 'cros_ec' > + > + Addresses scanned: - > + > +Author: > + > + - Thomas Weißschuh <linux@weissschuh.net> > + > +Description > +----------- > + > +This driver implements support for hardware monitoring commands exposed by the > +ChromeOS embedded controller used in Chromebooks and other devices. > + > +The channel labels exposed via hwmon are retrieved from the EC itself. > + > +Fan and temperature readings are supported. > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index 1ca7a4fe1f8f..355a83e66928 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers > coretemp > corsair-cpro > corsair-psu > + cros_ec_hwmon > da9052 > da9055 > dell-smm-hwmon > diff --git a/MAINTAINERS b/MAINTAINERS > index c23fda1aa1f0..aa5689169eca 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4988,6 +4988,14 @@ S: Maintained > F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml > F: sound/soc/codecs/cros_ec_codec.* > > +CHROMEOS EC HARDWARE MONITORING > +M: Thomas Weißschuh <thomas@weissschuh.net> > +L: chrome-platform@lists.linux.dev > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: Documentation/hwmon/chros_ec_hwmon.rst > +F: drivers/hwmon/cros_ec_hwmon.c > + > CHROMEOS EC SUBDRIVERS > M: Benson Leung <bleung@chromium.org> > R: Guenter Roeck <groeck@chromium.org> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 83945397b6eb..c1284d42697f 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU > This driver can also be built as a module. If so, the module > will be called corsair-psu. > > +config SENSORS_CROS_EC > + tristate "ChromeOS Embedded Controller sensors" > + depends on MFD_CROS_EC_DEV > + default MFD_CROS_EC_DEV > + help > + If you say yes here you get support for ChromeOS Embedded Controller > + sensors. > + > + This driver can also be built as a module. If so, the module > + will be called cros_ec_hwmon. > + > config SENSORS_DRIVETEMP > tristate "Hard disk drives with temperature sensors" > depends on SCSI && ATA > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 5c31808f6378..8519a6b36c00 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o > obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o > obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o > +obj-$(CONFIG_SENSORS_CROS_EC) += cros_ec_hwmon.o > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o > obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c > new file mode 100644 > index 000000000000..9588df202a74 > --- /dev/null > +++ b/drivers/hwmon/cros_ec_hwmon.c > @@ -0,0 +1,279 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * ChromesOS EC driver for hwmon > + * > + * Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net> > + */ > + > +#include <linux/device.h> > +#include <linux/hwmon.h> > +#include <linux/kernel.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/platform_data/cros_ec_commands.h> > +#include <linux/platform_data/cros_ec_proto.h> > +#include <linux/units.h> > + > +#define DRV_NAME "cros-ec-hwmon" > + > +struct cros_ec_hwmon_priv { > + struct cros_ec_device *cros_ec; > + u8 thermal_version; > + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES]; > +}; > + > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed) > +{ > + int ret; > + u16 data; > + > + if (index >= EC_FAN_SPEED_ENTRIES) > + return -ENODEV; I could see an argument that this should be -EINVAL as EC_FAN_SPEED_ENTRIES is hardcoded in the driver and "index" was passed into the function. > + > + ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data); > + if (ret < 0) > + return ret; > + > + data = le16_to_cpu(data); > + > + if (data == EC_FAN_SPEED_NOT_PRESENT) > + return -ENODEV; > + > + *speed = data; For safety purposes I think you should either do if (speed) *speed = data; Or have a check at the start of the function and return -EINVAL and throw up a warning if speed was NULL. > + return 0; > +} > + > +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version, > + u8 index, u8 *data) > +{ > + unsigned int offset; > + int ret; > + > + if (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES) > + return -ENODEV; > + > + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES) > + return -ENODEV; Like cros_ec_hwmon_read_fan_speed I have an opinion these should be -EINVAL. > + > + if (index < EC_TEMP_SENSOR_ENTRIES) > + offset = EC_MEMMAP_TEMP_SENSOR + index; > + else > + offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES; > + > + ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data); > + if (ret < 0) > + return ret; > + You should make sure that data isn't NULL before doing these checks: > + if (*data == EC_TEMP_SENSOR_NOT_PRESENT || > + *data == EC_TEMP_SENSOR_ERROR || > + *data == EC_TEMP_SENSOR_NOT_POWERED || > + *data == EC_TEMP_SENSOR_NOT_CALIBRATED) > + return -ENODEV; > + > + return 0; > +} > + > +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > + u16 speed; > + u8 temp; > + int ret = -ENODATA; I feel it is better practice to add this as a 3rd clause than initialize the variable at the start of the function. switch (type) { case hwmon_fan: //do stuff break; case hwmon_temp: // dot stuff break; default: ret = -ENODATA; } return ret; > + > + if (type == hwmon_fan) { > + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed); > + if (ret == 0) > + *val = speed; > + } else if (type == hwmon_temp) { > + ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp); > + if (ret == 0) > + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET)); > + } > + > + return ret; > +} > + > +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id, > + struct ec_response_temp_sensor_get_info *resp) > +{ > + int ret; > + struct { > + struct cros_ec_command msg; > + union { > + struct ec_params_temp_sensor_get_info req; > + struct ec_response_temp_sensor_get_info resp; > + } __packed data; > + } __packed buf = { > + .msg = { > + .version = 0, > + .command = EC_CMD_TEMP_SENSOR_GET_INFO, > + .insize = sizeof(buf.data.resp), > + .outsize = sizeof(buf.data.req), > + }, > + .data.req.id = id, > + }; > + > + ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg); > + if (ret < 0) > + return ret; > + > + *resp = buf.data.resp; Make sure that resp isn't NULL. > + return 0; > +} > + > +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > + int ret = -ENODATA; How about just drop the variable and then do below two comments: > + > + if (type == hwmon_temp && attr == hwmon_temp_label) { > + *str = priv->temp_sensor_names[channel]; > + ret = 0; return 0; > + } > + > + return ret; return -ENODATA; > +} > + > +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + const struct cros_ec_hwmon_priv *priv = data; > + u16 speed; > + > + if (type == hwmon_fan) { > + if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0) > + return 0444; > + } else if (type == hwmon_temp) { > + if (priv->temp_sensor_names[channel]) > + return 0444; > + } > + > + return 0; > +} > + > +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = { > + HWMON_CHANNEL_INFO(fan, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT), > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL, > + HWMON_T_INPUT | HWMON_T_LABEL), > + NULL > +}; > + > +static const struct hwmon_ops cros_ec_hwmon_ops = { > + .read = cros_ec_hwmon_read, > + .read_string = cros_ec_hwmon_read_string, > + .is_visible = cros_ec_hwmon_is_visible, > +}; > + > +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = { > + .ops = &cros_ec_hwmon_ops, > + .info = cros_ec_hwmon_info, > +}; > + > +static int cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv) > +{ > + struct ec_response_temp_sensor_get_info info; > + size_t i; > + u8 temp; > + int ret; > + > + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION, > + 1, &priv->thermal_version); > + if (ret < 0) > + return ret; > + > + if (!priv->thermal_version) > + return 0; I'm a bit confused; why isn't this -ENODEV? I would think that you don't want to have probe succeed and make devices if there isn't thermal support in the EC. > + > + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); i++) { > + if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0) > + continue; > + > + ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info); > + if (ret < 0) > + continue; > + > + priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s", > + (int)sizeof(info.sensor_name), > + info.sensor_name); > + } > + > + return 0; > +} > + > +static int cros_ec_hwmon_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); > + struct cros_ec_device *cros_ec = ec_dev->ec_dev; > + struct cros_ec_hwmon_priv *priv; > + struct device *hwmon_dev; > + int ret; > + > + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24); > + > + /* Not every platform supports direct reads */ > + if (!cros_ec->cmd_readmem) > + return -ENOTTY; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->cros_ec = cros_ec; > + > + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv); > + if (ret < 0) > + return ret; > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv, > + &cros_ec_hwmon_chip_info, NULL); > + > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct platform_device_id cros_ec_hwmon_id[] = { > + { DRV_NAME, 0 }, > + { } > +}; > + > +static struct platform_driver cros_ec_hwmon_driver = { > + .driver.name = DRV_NAME, > + .probe = cros_ec_hwmon_probe, > + .id_table = cros_ec_hwmon_id, > +}; > +module_platform_driver(cros_ec_hwmon_driver); > + > +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id); > +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver"); > +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net"); > +MODULE_LICENSE("GPL"); >
On Tue, May 7, 2024 at 8:30 AM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 5/7/2024 08:57, Thomas Weißschuh wrote: > > The ChromeOS Embedded Controller exposes fan speed and temperature > > readings. > > Expose this data through the hwmon subsystem. > > > > The driver is designed to be probed via the cros_ec mfd device. > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > Documentation/hwmon/cros_ec_hwmon.rst | 26 ++++ > > Documentation/hwmon/index.rst | 1 + > > MAINTAINERS | 8 + > > drivers/hwmon/Kconfig | 11 ++ > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/cros_ec_hwmon.c | 279 ++++++++++++++++++++++++++++++++++ > > 6 files changed, 326 insertions(+) > > > > diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst > > new file mode 100644 > > index 000000000000..aeb88c79d11b > > --- /dev/null > > +++ b/Documentation/hwmon/cros_ec_hwmon.rst > > @@ -0,0 +1,26 @@ > > +.. SPDX-License-Identifier: GPL-2.0-or-later > > + > > +Kernel driver cros_ec_hwmon > > +=========================== > > + > > +Supported chips: > > + > > + * ChromeOS embedded controllers connected via LPC > > + > > + Prefix: 'cros_ec' > > + > > + Addresses scanned: - > > + > > +Author: > > + > > + - Thomas Weißschuh <linux@weissschuh.net> > > + > > +Description > > +----------- > > + > > +This driver implements support for hardware monitoring commands exposed by the > > +ChromeOS embedded controller used in Chromebooks and other devices. > > + > > +The channel labels exposed via hwmon are retrieved from the EC itself. > > + > > +Fan and temperature readings are supported. > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > > index 1ca7a4fe1f8f..355a83e66928 100644 > > --- a/Documentation/hwmon/index.rst > > +++ b/Documentation/hwmon/index.rst > > @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers > > coretemp > > corsair-cpro > > corsair-psu > > + cros_ec_hwmon > > da9052 > > da9055 > > dell-smm-hwmon > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c23fda1aa1f0..aa5689169eca 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -4988,6 +4988,14 @@ S: Maintained > > F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml > > F: sound/soc/codecs/cros_ec_codec.* > > > > +CHROMEOS EC HARDWARE MONITORING > > +M: Thomas Weißschuh <thomas@weissschuh.net> > > +L: chrome-platform@lists.linux.dev > > +L: linux-hwmon@vger.kernel.org > > +S: Maintained > > +F: Documentation/hwmon/chros_ec_hwmon.rst > > +F: drivers/hwmon/cros_ec_hwmon.c > > + > > CHROMEOS EC SUBDRIVERS > > M: Benson Leung <bleung@chromium.org> > > R: Guenter Roeck <groeck@chromium.org> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 83945397b6eb..c1284d42697f 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU > > This driver can also be built as a module. If so, the module > > will be called corsair-psu. > > > > +config SENSORS_CROS_EC > > + tristate "ChromeOS Embedded Controller sensors" > > + depends on MFD_CROS_EC_DEV > > + default MFD_CROS_EC_DEV > > + help > > + If you say yes here you get support for ChromeOS Embedded Controller > > + sensors. > > + > > + This driver can also be built as a module. If so, the module > > + will be called cros_ec_hwmon. > > + > > config SENSORS_DRIVETEMP > > tristate "Hard disk drives with temperature sensors" > > depends on SCSI && ATA > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index 5c31808f6378..8519a6b36c00 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o > > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o > > obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o > > obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o > > +obj-$(CONFIG_SENSORS_CROS_EC) += cros_ec_hwmon.o > > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o > > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o > > obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c > > new file mode 100644 > > index 000000000000..9588df202a74 > > --- /dev/null > > +++ b/drivers/hwmon/cros_ec_hwmon.c > > @@ -0,0 +1,279 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * ChromesOS EC driver for hwmon > > + * > > + * Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net> > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/hwmon.h> > > +#include <linux/kernel.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/platform_data/cros_ec_commands.h> > > +#include <linux/platform_data/cros_ec_proto.h> > > +#include <linux/units.h> > > + > > +#define DRV_NAME "cros-ec-hwmon" > > + > > +struct cros_ec_hwmon_priv { > > + struct cros_ec_device *cros_ec; > > + u8 thermal_version; > > + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES]; > > +}; > > + > > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed) > > +{ > > + int ret; > > + u16 data; > > + > > + if (index >= EC_FAN_SPEED_ENTRIES) > > + return -ENODEV; > > I could see an argument that this should be -EINVAL as > EC_FAN_SPEED_ENTRIES is hardcoded in the driver and "index" was passed > into the function. > > > + > > + ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data); > > + if (ret < 0) > > + return ret; > > + > > + data = le16_to_cpu(data); > > + > > + if (data == EC_FAN_SPEED_NOT_PRESENT) > > + return -ENODEV; > > + > > + *speed = data; > > For safety purposes I think you should either do > > if (speed) > *speed = data; > NACK. This is a static function. Callers are well known. If speed is NULL this code deserves to crash. > Or have a check at the start of the function and return -EINVAL and > throw up a warning if speed was NULL. > > > + return 0; > > +} > > + > > +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version, > > + u8 index, u8 *data) > > +{ > > + unsigned int offset; > > + int ret; > > + > > + if (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES) > > + return -ENODEV; > > + > > + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES) > > + return -ENODEV; > > Like cros_ec_hwmon_read_fan_speed I have an opinion these should be -EINVAL. > Again, this is a static function. The caller is in full control of the parameter range, and those error checks should be unnecessary to start with. I am not going to review the rest of the driver. Drop all unnecessary range checks and resubmit. Guenter > > + > > + if (index < EC_TEMP_SENSOR_ENTRIES) > > + offset = EC_MEMMAP_TEMP_SENSOR + index; > > + else > > + offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES; > > + > > + ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data); > > + if (ret < 0) > > + return ret; > > + > > You should make sure that data isn't NULL before doing these checks: > > > + if (*data == EC_TEMP_SENSOR_NOT_PRESENT || > > + *data == EC_TEMP_SENSOR_ERROR || > > + *data == EC_TEMP_SENSOR_NOT_POWERED || > > + *data == EC_TEMP_SENSOR_NOT_CALIBRATED) > > + return -ENODEV; > > + > > + return 0; > > +} > > + > > +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > > + u16 speed; > > + u8 temp; > > + int ret = -ENODATA; > > I feel it is better practice to add this as a 3rd clause than initialize > the variable at the start of the function. > > switch (type) { > case hwmon_fan: > //do stuff > break; > case hwmon_temp: > // dot stuff > break; > default: > ret = -ENODATA; > } > > return ret; > > > + > > + if (type == hwmon_fan) { > > + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed); > > + if (ret == 0) > > + *val = speed; > > + } else if (type == hwmon_temp) { > > + ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp); > > + if (ret == 0) > > + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET)); > > + } > > + > > + return ret; > > +} > > + > > +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id, > > + struct ec_response_temp_sensor_get_info *resp) > > +{ > > + int ret; > > + struct { > > + struct cros_ec_command msg; > > + union { > > + struct ec_params_temp_sensor_get_info req; > > + struct ec_response_temp_sensor_get_info resp; > > + } __packed data; > > + } __packed buf = { > > + .msg = { > > + .version = 0, > > + .command = EC_CMD_TEMP_SENSOR_GET_INFO, > > + .insize = sizeof(buf.data.resp), > > + .outsize = sizeof(buf.data.req), > > + }, > > + .data.req.id = id, > > + }; > > + > > + ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg); > > + if (ret < 0) > > + return ret; > > + > > + *resp = buf.data.resp; > > Make sure that resp isn't NULL. > > > + return 0; > > +} > > + > > +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, const char **str) > > +{ > > + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > > + int ret = -ENODATA; > > How about just drop the variable and then do below two comments: > > > + > > + if (type == hwmon_temp && attr == hwmon_temp_label) { > > + *str = priv->temp_sensor_names[channel]; > > + ret = 0; > return 0; > > + } > > + > > + return ret; > return -ENODATA; > > +} > > + > > +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + const struct cros_ec_hwmon_priv *priv = data; > > + u16 speed; > > + > > + if (type == hwmon_fan) { > > + if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0) > > + return 0444; > > + } else if (type == hwmon_temp) { > > + if (priv->temp_sensor_names[channel]) > > + return 0444; > > + } > > + > > + return 0; > > +} > > + > > +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = { > > + HWMON_CHANNEL_INFO(fan, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT, > > + HWMON_F_INPUT), > > + HWMON_CHANNEL_INFO(temp, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL, > > + HWMON_T_INPUT | HWMON_T_LABEL), > > + NULL > > +}; > > + > > +static const struct hwmon_ops cros_ec_hwmon_ops = { > > + .read = cros_ec_hwmon_read, > > + .read_string = cros_ec_hwmon_read_string, > > + .is_visible = cros_ec_hwmon_is_visible, > > +}; > > + > > +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = { > > + .ops = &cros_ec_hwmon_ops, > > + .info = cros_ec_hwmon_info, > > +}; > > + > > +static int cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv) > > +{ > > + struct ec_response_temp_sensor_get_info info; > > + size_t i; > > + u8 temp; > > + int ret; > > + > > + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION, > > + 1, &priv->thermal_version); > > + if (ret < 0) > > + return ret; > > + > > + if (!priv->thermal_version) > > + return 0; > > I'm a bit confused; why isn't this -ENODEV? I would think that you > don't want to have probe succeed and make devices if there isn't thermal > support in the EC. > > > + > > + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); i++) { > > + if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0) > > + continue; > > + > > + ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info); > > + if (ret < 0) > > + continue; > > + > > + priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s", > > + (int)sizeof(info.sensor_name), > > + info.sensor_name); > > + } > > + > > + return 0; > > +} > > + > > +static int cros_ec_hwmon_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); > > + struct cros_ec_device *cros_ec = ec_dev->ec_dev; > > + struct cros_ec_hwmon_priv *priv; > > + struct device *hwmon_dev; > > + int ret; > > + > > + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24); > > + > > + /* Not every platform supports direct reads */ > > + if (!cros_ec->cmd_readmem) > > + return -ENOTTY; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->cros_ec = cros_ec; > > + > > + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv); > > + if (ret < 0) > > + return ret; > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv, > > + &cros_ec_hwmon_chip_info, NULL); > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static const struct platform_device_id cros_ec_hwmon_id[] = { > > + { DRV_NAME, 0 }, > > + { } > > +}; > > + > > +static struct platform_driver cros_ec_hwmon_driver = { > > + .driver.name = DRV_NAME, > > + .probe = cros_ec_hwmon_probe, > > + .id_table = cros_ec_hwmon_id, > > +}; > > +module_platform_driver(cros_ec_hwmon_driver); > > + > > +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id); > > +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver"); > > +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net"); > > +MODULE_LICENSE("GPL"); > > >
diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst new file mode 100644 index 000000000000..aeb88c79d11b --- /dev/null +++ b/Documentation/hwmon/cros_ec_hwmon.rst @@ -0,0 +1,26 @@ +.. SPDX-License-Identifier: GPL-2.0-or-later + +Kernel driver cros_ec_hwmon +=========================== + +Supported chips: + + * ChromeOS embedded controllers connected via LPC + + Prefix: 'cros_ec' + + Addresses scanned: - + +Author: + + - Thomas Weißschuh <linux@weissschuh.net> + +Description +----------- + +This driver implements support for hardware monitoring commands exposed by the +ChromeOS embedded controller used in Chromebooks and other devices. + +The channel labels exposed via hwmon are retrieved from the EC itself. + +Fan and temperature readings are supported. diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 1ca7a4fe1f8f..355a83e66928 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers coretemp corsair-cpro corsair-psu + cros_ec_hwmon da9052 da9055 dell-smm-hwmon diff --git a/MAINTAINERS b/MAINTAINERS index c23fda1aa1f0..aa5689169eca 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4988,6 +4988,14 @@ S: Maintained F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml F: sound/soc/codecs/cros_ec_codec.* +CHROMEOS EC HARDWARE MONITORING +M: Thomas Weißschuh <thomas@weissschuh.net> +L: chrome-platform@lists.linux.dev +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/hwmon/chros_ec_hwmon.rst +F: drivers/hwmon/cros_ec_hwmon.c + CHROMEOS EC SUBDRIVERS M: Benson Leung <bleung@chromium.org> R: Guenter Roeck <groeck@chromium.org> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 83945397b6eb..c1284d42697f 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU This driver can also be built as a module. If so, the module will be called corsair-psu. +config SENSORS_CROS_EC + tristate "ChromeOS Embedded Controller sensors" + depends on MFD_CROS_EC_DEV + default MFD_CROS_EC_DEV + help + If you say yes here you get support for ChromeOS Embedded Controller + sensors. + + This driver can also be built as a module. If so, the module + will be called cros_ec_hwmon. + config SENSORS_DRIVETEMP tristate "Hard disk drives with temperature sensors" depends on SCSI && ATA diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 5c31808f6378..8519a6b36c00 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o +obj-$(CONFIG_SENSORS_CROS_EC) += cros_ec_hwmon.o obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c new file mode 100644 index 000000000000..9588df202a74 --- /dev/null +++ b/drivers/hwmon/cros_ec_hwmon.c @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * ChromesOS EC driver for hwmon + * + * Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net> + */ + +#include <linux/device.h> +#include <linux/hwmon.h> +#include <linux/kernel.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/platform_data/cros_ec_commands.h> +#include <linux/platform_data/cros_ec_proto.h> +#include <linux/units.h> + +#define DRV_NAME "cros-ec-hwmon" + +struct cros_ec_hwmon_priv { + struct cros_ec_device *cros_ec; + u8 thermal_version; + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES]; +}; + +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed) +{ + int ret; + u16 data; + + if (index >= EC_FAN_SPEED_ENTRIES) + return -ENODEV; + + ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data); + if (ret < 0) + return ret; + + data = le16_to_cpu(data); + + if (data == EC_FAN_SPEED_NOT_PRESENT) + return -ENODEV; + + *speed = data; + return 0; +} + +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version, + u8 index, u8 *data) +{ + unsigned int offset; + int ret; + + if (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES) + return -ENODEV; + + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES) + return -ENODEV; + + if (index < EC_TEMP_SENSOR_ENTRIES) + offset = EC_MEMMAP_TEMP_SENSOR + index; + else + offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES; + + ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data); + if (ret < 0) + return ret; + + if (*data == EC_TEMP_SENSOR_NOT_PRESENT || + *data == EC_TEMP_SENSOR_ERROR || + *data == EC_TEMP_SENSOR_NOT_POWERED || + *data == EC_TEMP_SENSOR_NOT_CALIBRATED) + return -ENODEV; + + return 0; +} + +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); + u16 speed; + u8 temp; + int ret = -ENODATA; + + if (type == hwmon_fan) { + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed); + if (ret == 0) + *val = speed; + } else if (type == hwmon_temp) { + ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp); + if (ret == 0) + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET)); + } + + return ret; +} + +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id, + struct ec_response_temp_sensor_get_info *resp) +{ + int ret; + struct { + struct cros_ec_command msg; + union { + struct ec_params_temp_sensor_get_info req; + struct ec_response_temp_sensor_get_info resp; + } __packed data; + } __packed buf = { + .msg = { + .version = 0, + .command = EC_CMD_TEMP_SENSOR_GET_INFO, + .insize = sizeof(buf.data.resp), + .outsize = sizeof(buf.data.req), + }, + .data.req.id = id, + }; + + ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg); + if (ret < 0) + return ret; + + *resp = buf.data.resp; + return 0; +} + +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); + int ret = -ENODATA; + + if (type == hwmon_temp && attr == hwmon_temp_label) { + *str = priv->temp_sensor_names[channel]; + ret = 0; + } + + return ret; +} + +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + const struct cros_ec_hwmon_priv *priv = data; + u16 speed; + + if (type == hwmon_fan) { + if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0) + return 0444; + } else if (type == hwmon_temp) { + if (priv->temp_sensor_names[channel]) + return 0444; + } + + return 0; +} + +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = { + HWMON_CHANNEL_INFO(fan, + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT), + HWMON_CHANNEL_INFO(temp, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL, + HWMON_T_INPUT | HWMON_T_LABEL), + NULL +}; + +static const struct hwmon_ops cros_ec_hwmon_ops = { + .read = cros_ec_hwmon_read, + .read_string = cros_ec_hwmon_read_string, + .is_visible = cros_ec_hwmon_is_visible, +}; + +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = { + .ops = &cros_ec_hwmon_ops, + .info = cros_ec_hwmon_info, +}; + +static int cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv) +{ + struct ec_response_temp_sensor_get_info info; + size_t i; + u8 temp; + int ret; + + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION, + 1, &priv->thermal_version); + if (ret < 0) + return ret; + + if (!priv->thermal_version) + return 0; + + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); i++) { + if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0) + continue; + + ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info); + if (ret < 0) + continue; + + priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s", + (int)sizeof(info.sensor_name), + info.sensor_name); + } + + return 0; +} + +static int cros_ec_hwmon_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent); + struct cros_ec_device *cros_ec = ec_dev->ec_dev; + struct cros_ec_hwmon_priv *priv; + struct device *hwmon_dev; + int ret; + + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24); + + /* Not every platform supports direct reads */ + if (!cros_ec->cmd_readmem) + return -ENOTTY; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->cros_ec = cros_ec; + + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv); + if (ret < 0) + return ret; + + hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv, + &cros_ec_hwmon_chip_info, NULL); + + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static const struct platform_device_id cros_ec_hwmon_id[] = { + { DRV_NAME, 0 }, + { } +}; + +static struct platform_driver cros_ec_hwmon_driver = { + .driver.name = DRV_NAME, + .probe = cros_ec_hwmon_probe, + .id_table = cros_ec_hwmon_id, +}; +module_platform_driver(cros_ec_hwmon_driver); + +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id); +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver"); +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net"); +MODULE_LICENSE("GPL");
The ChromeOS Embedded Controller exposes fan speed and temperature readings. Expose this data through the hwmon subsystem. The driver is designed to be probed via the cros_ec mfd device. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- Documentation/hwmon/cros_ec_hwmon.rst | 26 ++++ Documentation/hwmon/index.rst | 1 + MAINTAINERS | 8 + drivers/hwmon/Kconfig | 11 ++ drivers/hwmon/Makefile | 1 + drivers/hwmon/cros_ec_hwmon.c | 279 ++++++++++++++++++++++++++++++++++ 6 files changed, 326 insertions(+)