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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 29/12/2024 11:28, Pengyu Luo wrote:
> On Sun, Dec 29, 2024 at 5:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 28/12/2024 12:34, Pengyu Luo wrote:
>>>> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.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.
>>
> 
> I agree with you, but device_add_groups is used to add sysfs interface
> everywhere, device_add_groups are wrapped in acpi_battery_hook, they
> handle charge_control_thresholds like this, since qcom arm64 do not
> support acpi on linux, we do not use acpi_battery_hook to implement it,
> so it is reasonable to implement it in PSY drivers.


OK, then do not make it a platform driver sysfs ABI but power supply one
and document the ABI (see Documentation/ABI/)

> 
> some examples:
> 



Best regards,
Krzysztof