Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver

[Pengyu Luo <mitltlatltl@gmail.com>]

On Sun, Dec 29, 2024 at 12:08 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Sat, Dec 28, 2024 at 07:34:37PM +0800, 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?
> > 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.
>
> Please push the smart_* to the PSY driver. At least it makes sense to
> move those. I'm not sure about the fn_lock one. If you have a separate
> EC-based input device, it should go to it. If not, let's keep it in the
> base device.
>

I see, so can I fix it in v2 like this?
- Using device_add_groups to register smart_* sysfs in PSY
- Using hwmon_device_register_with_groups to register thermal related sysfs in base driver

> >
> > >
> > > > 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
>
> Please don't pollute header files with extra dependencies. It's usually
> better to just forware-declare the struct instead of adding unnecessary
> include.
>

Both of you are resonable. So how?

BTW, Krzysztof said

> > You need kerneldoc, in the C file, for all exported functions.

So Dmitry is here, I want to check again, should I add kerneldoc for all
exported functions? C630 one never added all kerneldocs. In my driver,
some function names have already indicated many things, some complex
functions have been doced.


Best wishes,
Pengyu