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

[Pengyu Luo <mitltlatltl@gmail.com>]

> 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?
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.

>
> > diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> > new file mode 100644
> > index 000000000..a649e9ecf
> > --- /dev/null
> > +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + *
> > + */
> > +
> > +#ifndef __HUAWEI_GAOKUN_EC_H__
> > +#define __HUAWEI_GAOKUN_EC_H__
> > +
> > +#define GAOKUN_UCSI_CCI_SIZE 4
> > +#define GAOKUN_UCSI_DATA_SIZE        16
> > +#define GAOKUN_UCSI_READ_SIZE        (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> > +#define GAOKUN_UCSI_WRITE_SIZE       0x18
> > +
> > +#define GAOKUN_TZ_REG_NUM    20
> > +#define GAOKUN_SMART_CHARGE_DATA_SIZE        4 /* mode, delay, start, end */
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +struct gaokun_ec;
> > +struct notifier_block;
>
> Drop, include proper header instead.
>

I agree, I copy 'struct notifier_block;' from
include/linux/platform_data/lenovo-yoga-c630.h

> > +
> > +#define GAOKUN_MOD_NAME                      "huawei_gaokun_ec"
> > +#define GAOKUN_DEV_PSY                       "psy"
> > +#define GAOKUN_DEV_WMI                       "wmi"
> > +#define GAOKUN_DEV_UCSI                      "ucsi"
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* Common API */
> > +
> > +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> > +                           struct notifier_block *nb);
> > +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> > +                              struct notifier_block *nb);
> > +
> > +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> > +                size_t resp_len, u8 *resp);
> > +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> > +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
>
>
> You need kerneldoc, in the C file, for all exported functions.
>

I got it.

>
>
> Best regards,
> Krzysztof

Best Wishes,
Pengyu