Re: [PATCH 1/4] platform/x86: (ayn-ec) Add PWM Fan HWMON Interface

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On July 25, 2025 8:08:42 AM PDT, ALOK TIWARI <alok.a.tiwari@oracle.com> wrote:
>
>> +/*
>> + * Platform driver for Ayn x86 Handhelds.
>> + *
>> + * Implements multiple attributes provided by the EC. Fan reading and control,
>> + * as well as temperature sensor readings are exposed via hwmon sysfs. EC RGB
>> + * control is exposed via an led-class-multicolor interface.
>> + *
>> + * Fan control is provided via a pwm interface in the range [0-255]. Ayn use
>> + * [0-128] as the range in the EC, the written value is scaled to accommodate.
>> + * The EC also provides a configurable fan curve with five set points that
>> + * associate a temperature in Celcius [0-100] with a fan speed [0-128]. The
>> + * auto_point fan speeds are also scaled from the range [0-255]. Temperature
>> + * readings are scaled from degrees to millidegrees when read.
>> + *
>> + * RGB control is provided using 4 registers. One each for the colors red,
>> + * green, and blue are [0-255]. There is also a effect register that takes
>> + * switches between an EC controlled breathing that cycles through all colors
>> + * and fades in/out, and manual, which enables setting a user defined color.
>> + *
>> + * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
>> + */
>> +
>[clip]
>> +		if (val < 0 || val > 255)
>> +			return -EINVAL;
>> +		val = val >> 1; /* Max EC value is 128, scale from 255 */
>> +		break;
>> +	case 5:
>> +	case 6:
>> +	case 7:
>> +	case 8:
>> +	case 9:
>> +		if (val < 0 || val > 100)
>> +			return -EINVAL;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = write_to_ec(reg, val);
>> +	if (ret)
>> +		return ret;
>> +	return count;
>> +}
>> +
>> +/**
>> + * pwm_curve_show() - Read a fan curve speed or temperature value.
>> + *
>> + * @dev: The attribute's parent device.
>> + * @attr: The attribute to read.
>> + * @buf: Buffer to read to.
>
>"to read to" is awkward
>Output buffer.
>
>> + *
>> + * Return: Number of bytes read, or an error.
>> + */
>> +static ssize_t pwm_curve_show(struct device *dev, struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	int i, ret;
>> +	long val;
>> +	u8 reg;
>> +
>> +	i = to_sensor_dev_attr(attr)->index;
>> +	switch (i) {
>> +	case 0:
>> +		reg = AYN_SENSOR_PWM_FAN_SPEED_1_REG;
>> +		break;
>> +	case 1:
>> +		reg = AYN_SENSOR_PWM_FAN_SPEED_2_REG;
>> +		break;
>> +	case 2:
>> +		reg = AYN_SENSOR_PWM_FAN_SPEED_3_REG;
>> +		break;
>> +	case 3:
>> +		reg = AYN_SENSOR_PWM_FAN_SPEED_4_REG;
>> +		break;
>> +	case 4:
>> +		reg = AYN_SENSOR_PWM_FAN_SPEED_5_REG;
>> +		break;
>> +	case 5:
>> +		reg = AYN_SENSOR_PWM_FAN_TEMP_1_REG;
>> +		break;
>> +	case 6:
>> +		reg = AYN_SENSOR_PWM_FAN_TEMP_2_REG;
>> +		break;
>> +	case 7:
>> +		reg = AYN_SENSOR_PWM_FAN_TEMP_3_REG;
>> +		break;
>> +	case 8:
>> +		reg = AYN_SENSOR_PWM_FAN_TEMP_4_REG;
>> +		break;
>> +	case 9:
>> +		reg = AYN_SENSOR_PWM_FAN_TEMP_5_REG;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = read_from_ec(reg, 1, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (i) {
>> +	case 0:
>> +	case 1:
>> +	case 2:
>> +	case 3:
>> +	case 4:
>> +		val = val << 1; /* Max EC value is 128, scale to 255 */
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return sysfs_emit(buf, "%ld\n", val);
>> +}
>> +
>> +/* Fan curve attributes */
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_pwm, pwm_curve, 0);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_pwm, pwm_curve, 1);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point3_pwm, pwm_curve, 2);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point4_pwm, pwm_curve, 3);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point5_pwm, pwm_curve, 4);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_temp, pwm_curve, 5);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_temp, pwm_curve, 6);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point3_temp, pwm_curve, 7);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point4_temp, pwm_curve, 8);
>> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point5_temp, pwm_curve, 9);
>> +
>> +static struct attribute *ayn_sensors_attrs[] = {
>> +	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +ATTRIBUTE_GROUPS(ayn_sensors);
>> +
>> +static int ayn_ec_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device *hwdev;
>> +	int ret;
>
>ret is unused.
>
Woops, that should have been part of patch 3.

Ack all for 1/4 and 2/4 reviews. Thanks.

Derek
>> +
>> +	hwdev = devm_hwmon_device_register_with_info(dev, "aynec", NULL,
>> +						     &ayn_ec_chip_info,
>> +						     ayn_sensors_groups);
>> +	return PTR_ERR_OR_ZERO(hwdev);
>> +}
>> +
>> +static struct platform_driver ayn_ec_driver = {
>> +	.driver = {
>> +		.name = "ayn-ec",
>> +	},
>> +	.probe = ayn_ec_probe,
>> +};
>
>Thanks,
>Alok

- Derek