Re: [PATCH v4 2/4] platform: arm64: Add a driver for the EC found on ASUS Glymur machines

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]

On Thu, 15 Oct 2026, Konrad Dybcio wrote:

> On 10/15/26 4:24 PM, Ilpo Järvinen wrote:
> > On Thu, 15 Oct 2026, Konrad Dybcio wrote:
> > 
> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>
> >> Some Qualcomm Snapdragon X2 Elite (Extreme)-based ASUS laptops (at
> >> least the Zenbook A14 and A16) expose a custom, somewhat
> >> Qualcomm reference-derived EC interface. Introduce a driver to
> >> communicate with said EC, allowing for fan speed reporting, keyboard
> >> backlight setting and receiving of a number of miscellaneous sideband
> >> events.
> 
> [...]
> 
> >> +/* RAM addresses are sent MSB first, while returned data is LSB first. */
> >> +struct asus_qcom_ec_ram_request {
> >> +	__be16 addr;
> >> +	u8 count;
> >> +	u8 reserved[2];
> >> +} __packed;
> >> +
> >> +struct asus_qcom_ec_ram_response {
> >> +	__le16 data;
> >> +	u8 unused[3];
> >> +} __packed;
> > 
> > Endianness stuff looks sane now but __packed also needs an include.
> 
> Some fun statistics:
> 
> $ rg compiler_attributes.h drivers -l | wc -l
> 30

You didn't consider compiler.h -> compiler_types.h -> 
compiler_attributes.h that seems a guaranteed include path to me. The 
numbers would be slightly better with that.

> $ rg __packed drivers -l | wc -l
> 1228
> 
> [...]

So lets not add another mine to what is already a scary minefield. :-)

The indirect include paths are a minefield if somebody ever wants to 
refactor a header. I know some maintainers may be more lax on this but 
here we try to follow include-what-you-use principle to extent that is 
reasonable (when the include path is guaranteed such as 
platform_device.h -> device.h or linux/x.h -> asm.h you can only include 
the former ones).

> >> +static int asus_qcom_ec_read_ram(struct asus_ec *ec, u16 addr, u8 count, u16 *out)
> >> +{
> >> +	struct asus_qcom_ec_ram_request req = {
> >> +		.addr = cpu_to_be16(addr),
> >> +		.count = count,
> >> +		.reserved = { 0 },
> > 
> > 0 is the default init value so generally = {} should be enough but I think 
> > it's not necessary to add the reserved member at all because the entire 
> > struct should be initialized anyway by the surrounding construct.
> > 
> > With these fixed,
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Yeah I know. I still left it in deliberately so that in case anyone
> wants to audit the format here and compare it to the DSDT it's more
> obvious that this wasn't an oversight. Do you still prefer I resend
> (or maybe you'd like to fix that up while applying, which I don't
> mind either)?

We can keep the member there but I'll change it to {}.

I can try to remember to fix these while applying (I'm good in 
forgetting though :-)). But I at least try to check all the comments made 
to the last version.

-- 
 i.