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

[Pengyu Luo <mitltlatltl@gmail.com>]

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.

some examples:

drivers/platform/x86/thinkpad_acpi.c
> static struct attribute *tpacpi_battery_attrs[] = {
> 	&dev_attr_charge_control_start_threshold.attr,
> 	&dev_attr_charge_control_end_threshold.attr,
> 	&dev_attr_charge_start_threshold.attr,
> 	&dev_attr_charge_stop_threshold.attr,
> 	&dev_attr_charge_behaviour.attr,
> 	NULL,
> };
>
> ATTRIBUTE_GROUPS(tpacpi_battery);
>
> /* ACPI battery hooking */
>
> static int tpacpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> {
> 	int batteryid = tpacpi_battery_get_id(battery->desc->name);
>
> 	if (tpacpi_battery_probe(batteryid))
> 		return -ENODEV;
> 	if (device_add_groups(&battery->dev, tpacpi_battery_groups))
> 		return -ENODEV;
> 	return 0;
> }

drivers/platform/x86/dell/dell-laptop.c
> static struct attribute *dell_battery_attrs[] = {
> 	&dev_attr_charge_control_start_threshold.attr,
> 	&dev_attr_charge_control_end_threshold.attr,
> 	&dev_attr_charge_types.attr,
> 	NULL,
> };
> ATTRIBUTE_GROUPS(dell_battery);
>
> static bool dell_battery_supported(struct power_supply *battery)
> {
> 	/* We currently only support the primary battery */
> 	return strcmp(battery->desc->name, "BAT0") == 0;
> }
>
> static int dell_battery_add(struct power_supply *battery,
> 		struct acpi_battery_hook *hook)
> {
> 	/* Return 0 instead of an error to avoid being unloaded */
> 	if (!dell_battery_supported(battery))
> 		return 0;
>
> 	return device_add_groups(&battery->dev, dell_battery_groups);
> }

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

OK, if you insist, I will explain it in v2.

Best wishes,
Pengyu