On 28/12/2024 12:34, Pengyu Luo wrote: >> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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