linux-kernel - Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0fc279b2-fa58-404a-af8e-ed18d4e2f514@kernel.org>
Date: Sun, 29 Dec 2024 10:43:32 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Pengyu Luo <mitltlatltl@...il.com>
Cc: andersson@...nel.org, bryan.odonoghue@...aro.org, conor+dt@...nel.org,
 devicetree@...r.kernel.org, dmitry.baryshkov@...aro.org,
 gregkh@...uxfoundation.org, hdegoede@...hat.com,
 heikki.krogerus@...ux.intel.com, ilpo.jarvinen@...ux.intel.com,
 konradybcio@...nel.org, krzk+dt@...nel.org, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
 linux-usb@...r.kernel.org, nikita@...n.ru,
 platform-driver-x86@...r.kernel.org, robh@...nel.org, sre@...nel.org
Subject: Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC

On 28/12/2024 12:34, Pengyu Luo wrote:
>> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>> On 27/12/2024 18:13, Pengyu Luo wrote:
>>> +
>>> +#include <linux/platform_data/huawei-gaokun-ec.h>
>>> +
>>> +#define EC_EVENT             0x06
>>> +
>>> +/* Also can be found in ACPI specification 12.3 */
>>> +#define EC_READ                      0x80
>>> +#define EC_WRITE             0x81
>>> +#define EC_BURST             0x82
>>> +#define EC_QUERY             0x84
>>> +
>>> +
>>> +#define EC_EVENT_LID         0x81
>>> +
>>> +#define EC_LID_STATE         0x80
>>> +#define EC_LID_OPEN          BIT(1)
>>> +
>>> +#define UCSI_REG_SIZE                7
>>> +
>>> +/* for tx, command sequences are arranged as
>>
>> Use Linux style comments, see coding style.
>>
> 
> Agree
> 
>>> + * {master_cmd, slave_cmd, data_len, data_seq}
>>> + */
>>> +#define REQ_HDR_SIZE         3
>>> +#define INPUT_SIZE_OFFSET    2
>>> +#define INPUT_DATA_OFFSET    3
>>> +
>>> +/* for rx, data sequences are arranged as
>>> + * {status, data_len(unreliable), data_seq}
>>> + */
>>> +#define RESP_HDR_SIZE                2
>>> +#define DATA_OFFSET          2
>>> +
>>> +
>>> +struct gaokun_ec {
>>> +     struct i2c_client *client;
>>> +     struct mutex lock;
>>
>> Missing doc. Run Checkpatch --strict, so you will know what is missing here.
>>
> 
> I see. A comment for mutex lock.
> 
>>> +     struct blocking_notifier_head notifier_list;
>>> +     struct input_dev *idev;
>>> +     bool suspended;
>>> +};
>>> +
>>
>>
>>
>> ...
>>
>>> +
>>> +static DEVICE_ATTR_RO(temperature);
>>> +
>>> +static struct attribute *gaokun_wmi_features_attrs[] = {
>>> +     &dev_attr_charge_control_thresholds.attr,
>>> +     &dev_attr_smart_charge_param.attr,
>>> +     &dev_attr_smart_charge.attr,
>>> +     &dev_attr_fn_lock_state.attr,
>>> +     &dev_attr_temperature.attr,
>>> +     NULL,
>>> +};
>>
>>
>> No, don't expose your own interface. Charging is already exposed by
>> power supply framework. Temperature by hwmon sensors. Drop all these and
>> never re-implement existing kernel user-space interfaces.
>>
> 
> I don't quite understand what you mean. You mean I should use hwmon
> interface like hwmon_device_register_with_groups to register it, right?

You added sysfs interface, I think. My comment is: do not. We have
existing interfaces.

> As for battery, get/set_propery allow us to handle charging thresholds
> things, but there are smart_charge_param, smart_charge and fn_lock to handle.

So where is the ABI documentation? Where is any explanation why existing
interfaces are not enough?



Best regards,
Krzysztof