Re: [PATCH V3 2/5] platform: arm64: Add driver for EC found on Qualcomm reference devices

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 3/9/26 11:04 AM, Sibi Sankar wrote:
> 
> On 3/9/2026 2:33 PM, Stephan Gerhold wrote:
>> On Mon, Mar 09, 2026 at 05:06:43AM +0530, Sibi Sankar wrote:
>>> Add Embedded controller driver support for Hamoa/Purwa/Glymur qualcomm
>>> reference boards. It handles fan control, temperature sensors, access
>>> to EC state changes and supports reporting suspend entry/exit to the
>>> EC.
>>>
>>> Co-developed-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>> Signed-off-by: Sibi Sankar <sibi.sankar@oss.qualcomm.com>
>>> ---
>>>   MAINTAINERS                            |   7 +
>>>   drivers/platform/arm64/Kconfig         |  12 +
>>>   drivers/platform/arm64/Makefile        |   1 +
>>>   drivers/platform/arm64/qcom-hamoa-ec.c | 462 +++++++++++++++++++++++++
>>>   4 files changed, 482 insertions(+)
>>>   create mode 100644 drivers/platform/arm64/qcom-hamoa-ec.c
>>>
>>> [...]
>>> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
>>> new file mode 100644
>>> index 000000000000..83aa869fad8f
>>> --- /dev/null
>>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
>>> @@ -0,0 +1,462 @@
>>> [...]
>>> +static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
>>> +{
>>> +    int ret;
>>> +
>>> +    mutex_lock(&ec->lock);
>>> +    ret = i2c_smbus_read_i2c_block_data(ec->client, cmd, resp_len, resp);
>>> +    mutex_unlock(&ec->lock);
>> This mutex looks redundant to me for the current implementation. You
>> don't have any read-modify-write sequences and I think the I2C core
>> already has internal locking for the bus itself.
> 
> Hey Stephan,
> Thanks for taking time to review the series :)
> 
> Will remove this in the next re-spin.
> 
>>
>>> [...]
>>> +/*
>>> + * Fan Debug control command:
>>> + *
>>> + * Command Payload:
>>> + * ------------------------------------------------------------------------------
>>> + * | Offset    | Name        | Description                    |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x00    | Command    | Fan control command                |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x01    | Fan ID    | 0x1 : Fan 1                    |
>>> + * |        |        | 0x2 : Fan 2                    |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x02    | Byte count = 4| Size of data to set fan speed            |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x03    | Mode        | Bit 0: Debug Mode On/Off (0 - OFF, 1 - ON )    |
>>> + * |        |        | Bit 1: Fan On/Off (0 - Off, 1 - ON)        |
>>> + * |        |        | Bit 2: Debug Type (0 - RPM, 1 - PWM)        |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x04 (LSB)    | Speed in RPM    | RPM value, if mode selected is RPM        |
>>> + * | 0x05    |        |                        |
>>> + * ------------------------------------------------------------------------------
>>> + * | 0x06    | Speed in PWM    | PWM value, if mode selected is PWM (0 - 255)    |
>>> + * ______________________________________________________________________________
>>> + *
>>> + */
>>> +static int qcom_ec_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
>>> +{
>>> +    struct qcom_ec_cooling_dev *ec_cdev = cdev->devdata;
>>> +    struct device *dev = ec_cdev->parent_dev;
>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +
>>> +    u8 request[6] = { ec_cdev->fan_id, EC_FAN_SPEED_DATA_SIZE,
>>> +              EC_FAN_DEBUG_MODE_ON | EC_FAN_ON | EC_FAN_DEBUG_TYPE_PWM,
>>> +              0, 0, state };
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_write_i2c_block_data(client, EC_FAN_DBG_CONTROL_CMD,
>>> +                         sizeof(request), request);
>> I think it's nice to provide users a way to override the fan speed, but
>> is this really the main interface of the EC that we want to use for
>> influencing the fan speed?
>>
>> As the name of the command suggests, this is a debug command that
>> essentially overrides the internal fan control algorithm of the EC. If
>> you use this to turn the fan off and then Linux hangs, I would expect
>> that the fan stays off until the device will eventually overheat.
>>
>> I think it would be more reliable if:
>>
>>   (1) The default mode of operation does not make use of the "debug mode"
>>       command and instead sends the internal SoC temperatures to the EC
>>       to help optimize the fan control. (This is what Windows does on
>>       Hamoa, not sure if this is still needed on Glymur?)
> 
> That's true, Glymur already has a way to access average SoC
> temperature and even on Hamoa it can still be functional without
> SoC temperature i.e. with thermistors it has access to.
> 
> The aim of the series is to expose fans as a cooling device so
> that linux has a way of fan control independent to the algorithm
> running on the EC.

I suppose the main question here is "what happens if i set the fan to zero
and put the laptop in my backpack"

The driver for M-series Macs for example, 785205fd8139 ("hwmon: Add Apple
Silicon SMC hwmon driver") hides that behind a cmdline param, since they
have no certainty. I would *assume* that if the CPU hits thermal junction
temperatures, our boards will reset, but we should be able to get a definitive
answer here.

Konrad