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: 3211 bytes --]

On Fri, 16 Oct 2026, Ilpo Järvinen wrote:

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

This patch 2 has been applied to the review-ilpo-next branch. The rest of 
series will have to go through some other tree.

Thanks.

-- 
 i.