Messages in this thread | | | Subject | Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call | From | Michal Simek <> | Date | Wed, 7 Oct 2020 10:20:21 +0200 |
| |
Hi,
1. Keem Bay: in subject is wrong. Tools are working with it and you should just use keembay: instead.
2. This should come first before actual change to keep the tree bisectable.
On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > > Add header file to handle API function for device driver to communicate > with Arm Trusted Firmware. > > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > .../linux/firmware/intel/keembay_firmware.h | 46 +++++++++++++++++++ > 1 file changed, 46 insertions(+) > create mode 100644 include/linux/firmware/intel/keembay_firmware.h > > diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h > new file mode 100644 > index 000000000000..9adb8c87b788 > --- /dev/null > +++ b/include/linux/firmware/intel/keembay_firmware.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Intel Keembay SOC Firmware API Layer > + * > + * Copyright (C) 2020-2021, Intel Corporation > + * > + * Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com> > + */ > + > +#ifndef __FIRMWARE_KEEMBAY_SMC_H__ > +#define __FIRMWARE_KEEMBAY_SMC_H__ > + > +#include <linux/arm-smccc.h> > + > +/**
This is not a kernel doc comment. Just use /*
> + * This file defines API function that can be called by device driver in order to > + * communicate with Arm Trusted Firmware. > + */ > + > +/* Setting for Keem Bay IO Pad Line Voltage Selection */ > +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID 0x8200ff26
Sudeep: Don't we have any macros for composing these IDs? nit: IMHO composing these IDs from macros would make more sense to me.
> +#define KEEMBAY_SET_1V8_VOLT 0x01
0x01 is just 1
> +#define KEEMBAY_SET_3V3_VOLT 0x00
0x00 is just 0
> + > +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) > +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_1_1_invoke(func_id, arg0, arg1, &res); > + > + return res.a0;
I am not big fan of this error propagation in case of failure.
If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is defined as -1 which is based on errno-base.h defined as EPERM.
That driver which Sudeep pointed you to is using EINVAL instead.
It means I would add a code to check it.
> +} > + > +int keembay_sd_voltage_selection(int volt)
as was reported by robot
> +{ > + return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); > +} > +#else > +static inline int keembay_sd_voltage_selection(int volt) > +{ > + return -ENODEV; > +} > +#endif > + > +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */ >
M
| |