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: <xiwaq7fapkkmohg743v36uzpxv4ib4o6upibh7fgvmfjiupy2k@zqxw53prsith>
Date: Sun, 29 Dec 2024 06:08:00 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Pengyu Luo <mitltlatltl@...il.com>
Cc: krzk@...nel.org, andersson@...nel.org, bryan.odonoghue@...aro.org, 
	conor+dt@...nel.org, devicetree@...r.kernel.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 Sat, Dec 28, 2024 at 07:34:37PM +0800, 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?
> 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.

> 
> >
> > > 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@...il.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.


-- 
With best wishes
Dmitry