Hi Michal, Thanks for the feedback. I replied inline
>-----Original Message----- >From: Michal Simek <michal.si...@xilinx.com> >Sent: Wednesday, October 7, 2020 4:20 PM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>; >Hunter, Adrian <adrian.hun...@intel.com>; michal.si...@xilinx.com; >sudeep.ho...@arm.com; ulf.hans...@linaro.org; linux-...@vger.kernel.org; >linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org >Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subraman...@intel.com>; >Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.moha...@intel.com>; a...@arndb.de >Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >Hi, > >1. Keem Bay: in subject is wrong. Tools are working with it and you should just >use keembay: instead. Are you saying like this ? Keem Bay: Add support for Arm Trusted Firmware Service call > >2. This should come first before actual change to keep the tree bisectable. Noted. Done the changes > >On 06. 10. 20 17:55, muhammad.husaini.zulki...@intel.com wrote: >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulki...@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.zulki...@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.zulki...@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 Noted. Done the changes > >> +#define KEEMBAY_SET_3V3_VOLT 0x00 > >0x00 is just 0 Noted. Done the changes > >> + >> +#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. Yeah I changed to below line of codes. Is this Ok? Tested working. int keembay_sd_voltage_selection(int volt) { struct arm_smccc_res res; arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res); if ((int)res.a0 < 0) return -EINVAL; return 0; } > > >> +} >> + >> +int keembay_sd_voltage_selection(int volt) > >as was reported by robot Added the func prototype. int keembay_sd_voltage_selection(int volt); No error observed after that. > >> +{ >> + 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