rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
@ 2021-08-17  1:23 Antonio Martorana
  2021-08-17 16:43 ` Miguel Ojeda
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Antonio Martorana @ 2021-08-17  1:23 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho
  Cc: Antonio Martorana, rust-for-linux, Trilok Soni, Elliot Berman

Add initial work for Rust port of qcom/socinfo.c  driver. Socinfo driver
reads QCOM SMEM to learn current device information (e.g. SM8250).

 - Added several C libraries to bindings_helper.h for FFI support from  bindgen.
 - Implemented PlatformDriver and KernelModule traits for SocInfoDriver
    - Register with platform device framework
    - Add devicetree compatibles
 - Did not port functions referenced from SMEM, wanted to show a
   working proof of concept for Rust leaf driver with FFI code
 - Register the discovered SMEM data with socinfo core.

---

This patch relies on Mateusz Jabłoński's try_format! macro patch from GitHub:
https://github.com/Rust-for-Linux/linux/pull/478

Signed-off-by: Antonio Martorana <amartora@codeaurora.org>
---
 drivers/soc/qcom/Kconfig         |   9 +
 drivers/soc/qcom/Makefile        |   1 +
 drivers/soc/qcom/socinfo_rust.rs | 464 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/bindings_helper.h    |   7 +
 4 files changed, 481 insertions(+)
 create mode 100644 drivers/soc/qcom/socinfo_rust.rs

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..3154b1a 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -190,6 +190,15 @@ config QCOM_SOCINFO
 	 Say yes here to support the Qualcomm socinfo driver, providing
 	 information about the SoC to user space.
 
+config QCOM_SOCINFO_RUST
+	tristate "Rust implementation of Qualcomm socinfo driver"
+	depends on HAS_RUST && QCOM_SMEM
+	help
+	  This driver provides alternative Rust-based kernel-side support
+	  for the socinfo driver.
+
+	  If unsure, say N.
+
 config QCOM_WCNSS_CTRL
 	tristate "Qualcomm WCNSS control driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ad675a6..340ac65 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
+obj-$(CONFIG_QCOM_SOCINFO_RUST) += socinfo_rust.o
 obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
diff --git a/drivers/soc/qcom/socinfo_rust.rs b/drivers/soc/qcom/socinfo_rust.rs
new file mode 100644
index 0000000..8af697a
--- /dev/null
+++ b/drivers/soc/qcom/socinfo_rust.rs
@@ -0,0 +1,464 @@
+//! Qualcomm support for socinfo.c
+
+#![no_std]
+#![feature(allocator_api,global_asm)]
+
+use kernel::{
+    {c_str,},
+    bindings,
+    bindings::*,
+    c_types,
+    linked_list::Wrapper,
+    of::ConstOfMatchTable,
+    platdev,
+    platdev::PlatformDriver,
+    prelude::*,
+    str,
+    str::CStr
+};
+
+use alloc::{boxed::Box,try_format};
+use core::{pin::Pin};
+
+module! {
+    type: SocInfoDriver,
+    name: b"socinfo_rust",
+    author: b"Antonio Martorana",
+    description: b"QCOM socinfo rust implementation",
+    license: b"GPL v2",
+}
+
+/*
+ * SoC version type with major number in the upper 16 bits and minor
+ * number in the lower 16 bits.
+ */
+const fn socinfo_major(ver: u32) -> u32{
+    let major: u32 = (ver >> 16) & 0xFFFF;
+    return major;
+}
+
+const fn socinfo_minor(ver: u32) -> u32{
+    let minor: u32 = ver & 0xFFFF;
+    return minor;
+}
+
+const fn socinfo_version(maj: u32, min: u32) -> u32{
+    let version: u32 = ((maj & 0xFFFF) << 16) | (min & 0xFFFF);
+    return version;
+}
+
+const SMEM_SOCINFO_BUILD_ID_LENGTH: usize = 32;
+const SMEM_SOCINFO_CHIP_ID_LENGTH: usize = 32;
+
+/*
+ * SMEM item id, used to acquire handles to respective
+ * SMEM region.
+ */
+const SMEM_HW_SW_BUILD_ID: u32 = 137;
+
+/* C code has #ifdef */
+const SMEM_IMAGE_VERSION_BLOCKS_COUNT: usize = 32;
+const SMEM_IMAGE_VERSION_SIZE: usize = 4096;
+const SMEM_IMAGE_VERSION_NAME_SIZE: usize = 75;
+const SMEM_IMAGE_VERSION_VARIANT_SIZE: usize = 20;
+const SMEM_IMAGE_VERSION_OEM_SIZE: usize = 32;
+
+/*
+ * SMEM Image table indices
+ */
+const SMEM_IMAGE_TABLE_BOOT_INDEX: u32 = 0;
+const SMEM_IMAGE_TABLE_TZ_INDEX: u32 = 1;
+const SMEM_IMAGE_TABLE_RPM_INDEX: u32 = 3;
+const SMEM_IMAGE_TABLE_APPS_INDEX: u32 = 10;
+const SMEM_IMAGE_TABLE_MPSS_INDEX: u32 = 11;
+const SMEM_IMAGE_TABLE_ADSP_INDEX: u32 = 12;
+const SMEM_IMAGE_TABLE_CNSS_INDEX: u32 = 13;
+const SMEM_IMAGE_TABLE_VIDEO_INDEX: u32 = 14;
+const SMEM_IMAGE_VERSION_TABLE: u32 = 496;
+
+const PMIC_MODELS: [&str;37] = [
+    "Unknown PMIC model",
+    "PM8941",
+    "PM8841",
+    "PM8019",
+    "PM8226",
+    "PM8110",
+    "PMA8084",
+    "PMI8962",
+    "PMD9635",
+    "PM8994",
+    "PMI8994",
+    "PM8916",
+    "PM8004",
+    "PM8909/PM8058",
+    "PM8028",
+    "PM8901",
+    "PM8950/PM8027",
+    "PMI8950/ISL9519",
+    "PM8921",
+    "PM8018",
+    "PM8998/PM8015",
+    "PMI8998/PM8014",
+    "PM8821",
+    "PM8038",
+    "PM8005/PM8922",
+    "PM8917",
+    "PM660L",
+    "PM660",
+    "",
+    "",
+    "PM8150",
+    "PM8150L",
+    "PM8150B",
+    "PMK8002",
+    "",
+    "",
+    "PM8009",
+];
+
+
+struct SocInfo{
+    fmt: bindings::__le32,
+    id: bindings::__le32,
+    ver: bindings::__le32,
+    build_id: [c_types::c_char; SMEM_SOCINFO_BUILD_ID_LENGTH],
+    raw_id: bindings::__le32,
+    raw_ver: bindings::__le32,
+    hw_plat: bindings::__le32,
+    plat_ver: bindings::__le32,
+    accessory_chip: bindings::__le32,
+    hw_plat_subtype: bindings::__le32,
+    pmic_model: bindings::__le32,
+    pmic_die_rev: bindings::__le32,
+    pmic_model_1: bindings::__le32,
+    pmic_die_rev_1: bindings::__le32,
+    pmic_model_2: bindings::__le32,
+    pmic_die_rev_2: bindings::__le32,
+    foundry_id: bindings::__le32,
+    serial_num: bindings::__le32,
+    num_pmics: bindings::__le32,
+    pmic_array_offset: bindings::__le32,
+    chip_family: bindings::__le32,
+    raw_device_family: bindings::__le32,
+    raw_device_num: bindings::__le32,
+    nproduct_id: bindings::__le32,
+    chip_id: [c_types::c_char; SMEM_SOCINFO_CHIP_ID_LENGTH],
+    num_cluster: bindings::__le32,
+    ncluster_array_offset: bindings::__le32,
+    num_defective_parts: bindings::__le32,
+    nmodem_supported: bindings::__le32,
+}
+
+#[derive(Default)]
+struct SocInfoParams{
+    raw_device_family: u32,
+    hw_plat_subtype: u32,
+    accessory_chip: u32,
+    raw_device_num: u32,
+    chip_family: u32,
+    foundry_id: u32,
+    plat_ver: u32,
+    raw_ver: u32,
+    hw_plat: u32,
+    fmt: u32,
+    nproduct_id: u32,
+    num_clusters: u32,
+    ncluster_array_offset: u32,
+    num_defective_parts: u32,
+    ndefective_parts_array_offset: u32,
+    nmodem_supported: u32,
+}
+
+struct SmemImageVersion{
+    name: [c_types::c_char; SMEM_IMAGE_VERSION_NAME_SIZE],
+    variant: [c_types::c_char; SMEM_IMAGE_VERSION_VARIANT_SIZE],
+    pad: c_types::c_char,
+    oem: [c_types::c_char; SMEM_IMAGE_VERSION_OEM_SIZE],
+}
+
+const SOC_ID: [SocId;105] = [
+    SocId { id:87, name:c_str!("MSM8960")},
+	SocId{ id:109, name:c_str!("APQ8064") },
+	SocId{ id:122, name:c_str!("MSM8660A") },
+	SocId{ id:123, name:c_str!("MSM8260A") },
+	SocId{ id:124, name:c_str!("APQ8060A") },
+	SocId{ id:126, name:c_str!("MSM8974") },
+	SocId{ id:130, name:c_str!("MPQ8064") },
+	SocId{ id:138, name:c_str!("MSM8960AB") },
+	SocId{ id:139, name:c_str!("APQ8060AB") },
+	SocId{ id:140, name:c_str!("MSM8260AB") },
+	SocId{ id:141, name:c_str!("MSM8660AB") },
+	SocId{ id:145, name:c_str!("MSM8626") },
+	SocId{ id:147, name:c_str!("MSM8610") },
+	SocId{ id:153, name:c_str!("APQ8064AB") },
+	SocId{ id:158, name:c_str!("MSM8226") },
+	SocId{ id:159, name:c_str!("MSM8526") },
+	SocId{ id:161, name:c_str!("MSM8110") },
+	SocId{ id:162, name:c_str!("MSM8210") },
+	SocId{ id:163, name:c_str!("MSM8810") },
+	SocId{ id:164, name:c_str!("MSM8212") },
+	SocId{ id:165, name:c_str!("MSM8612") },
+	SocId{ id:166, name:c_str!("MSM8112") },
+	SocId{ id:168, name:c_str!("MSM8225Q") },
+	SocId{ id:169, name:c_str!("MSM8625Q") },
+	SocId{ id:170, name:c_str!("MSM8125Q") },
+	SocId{ id:172, name:c_str!("APQ8064AA") },
+	SocId{ id:178, name:c_str!("APQ8084") },
+	SocId{ id:184, name:c_str!("APQ8074") },
+	SocId{ id:185, name:c_str!("MSM8274") },
+	SocId{ id:186, name:c_str!("MSM8674") },
+	SocId{ id:194, name:c_str!("MSM8974PRO") },
+	SocId{ id:198, name:c_str!("MSM8126") },
+	SocId{ id:199, name:c_str!("APQ8026") },
+	SocId{ id:200, name:c_str!("MSM8926") },
+	SocId{ id:205, name:c_str!("MSM8326") },
+	SocId{ id:206, name:c_str!("MSM8916") },
+	SocId{ id:207, name:c_str!("MSM8994") },
+	SocId{ id:208, name:c_str!("APQ8074-AA") },
+	SocId{ id:209, name:c_str!("APQ8074-AB") },
+	SocId{ id:210, name:c_str!("APQ8074PRO") },
+	SocId{ id:211, name:c_str!("MSM8274-AA") },
+	SocId{ id:212, name:c_str!("MSM8274-AB") },
+	SocId{ id:213, name:c_str!("MSM8274PRO") },
+	SocId{ id:214, name:c_str!("MSM8674-AA") },
+	SocId{ id:215, name:c_str!("MSM8674-AB") },
+	SocId{ id:216, name:c_str!("MSM8674PRO") },
+	SocId{ id:217, name:c_str!("MSM8974-AA") },
+	SocId{ id:218, name:c_str!("MSM8974-AB") },
+	SocId{ id:219, name:c_str!("APQ8028") },
+	SocId{ id:220, name:c_str!("MSM8128") },
+	SocId{ id:221, name:c_str!("MSM8228") },
+	SocId{ id:222, name:c_str!("MSM8528") },
+	SocId{ id:223, name:c_str!("MSM8628") },
+	SocId{ id:224, name:c_str!("MSM8928") },
+	SocId{ id:225, name:c_str!("MSM8510") },
+	SocId{ id:226, name:c_str!("MSM8512") },
+	SocId{ id:233, name:c_str!("MSM8936") },
+	SocId{ id:239, name:c_str!("MSM8939") },
+	SocId{ id:240, name:c_str!("APQ8036") },
+	SocId{ id:241, name:c_str!("APQ8039") },
+	SocId{ id:246, name:c_str!("MSM8996") },
+	SocId{ id:247, name:c_str!("APQ8016") },
+	SocId{ id:248, name:c_str!("MSM8216") },
+	SocId{ id:249, name:c_str!("MSM8116") },
+	SocId{ id:250, name:c_str!("MSM8616") },
+	SocId{ id:251, name:c_str!("MSM8992") },
+	SocId{ id:253, name:c_str!("APQ8094") },
+	SocId{ id:290, name:c_str!("MDM9607") },
+	SocId{ id:291, name:c_str!("APQ8096") },
+	SocId{ id:292, name:c_str!("MSM8998") },
+	SocId{ id:293, name:c_str!("MSM8953") },
+	SocId{ id:296, name:c_str!("MDM8207") },
+	SocId{ id:297, name:c_str!("MDM9207") },
+	SocId{ id:298, name:c_str!("MDM9307") },
+	SocId{ id:299, name:c_str!("MDM9628") },
+	SocId{ id:304, name:c_str!("APQ8053") },
+	SocId{ id:305, name:c_str!("MSM8996SG") },
+	SocId{ id:310, name:c_str!("MSM8996AU") },
+	SocId{ id:311, name:c_str!("APQ8096AU") },
+	SocId{ id:312, name:c_str!("APQ8096SG") },
+	SocId{ id:317, name:c_str!("SDM660") },
+	SocId{ id:318, name:c_str!("SDM630") },
+	SocId{ id:319, name:c_str!("APQ8098") },
+	SocId{ id:321, name:c_str!("SDM845") },
+	SocId{ id:322, name:c_str!("MDM9206") },
+	SocId{ id:324, name:c_str!("SDA660") },
+	SocId{ id:325, name:c_str!("SDM658") },
+	SocId{ id:326, name:c_str!("SDA658") },
+	SocId{ id:327, name:c_str!("SDA630") },
+	SocId{ id:338, name:c_str!("SDM450") },
+	SocId{ id:341, name:c_str!("SDA845") },
+	SocId{ id:345, name:c_str!("SDM636") },
+	SocId{ id:346, name:c_str!("SDA636") },
+	SocId{ id:349, name:c_str!("SDM632") },
+	SocId{ id:350, name:c_str!("SDA632") },
+	SocId{ id:351, name:c_str!("SDA450") },
+	SocId{ id:356, name:c_str!("SM8250") },
+	SocId{ id:394, name:c_str!("SM6125") },
+	SocId{ id:402, name:c_str!("IPQ6018") },
+	SocId{ id:403, name:c_str!("IPQ6028") },
+	SocId{ id:421, name:c_str!("IPQ6000") },
+	SocId{ id:422, name:c_str!("IPQ6010") },
+	SocId{ id:425, name:c_str!("SC7180") },
+	SocId{ id:453, name:c_str!("IPQ6005") },
+	SocId{ id:455, name:c_str!("QRB5165") },
+];
+
+
+struct SocId{
+    id: u32,
+    name: &'static str::CStr
+}
+
+fn socinfo_machine(id: &u32) -> Option<&'static str::CStr>{
+    for idx in SOC_ID {
+        if idx.id == *id{
+            return Some(idx.name);
+        }
+    }
+    return None;
+}
+
+
+fn qcom_show_build_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
+
+    let seq_private= seq.private as *mut SocInfo;
+    unsafe {let ref_mut_seq_priv = seq_private.as_mut().unwrap(); }
+
+    let mut format = c_str!("%s\n");
+    let format = format.as_char_ptr();
+
+    unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,(*seq_private).build_id)};
+
+    return 0;
+}
+
+fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> i32{
+    let socinfo= seq.private as *mut SocInfo;
+    let mut model;
+
+    let mut format = c_str!("%s\n");
+    let format = format.as_char_ptr();
+
+    let mut secondary_format = c_str!("unknown (%d)\n");
+    let secondary_format = secondary_format.as_char_ptr();
+
+    unsafe {
+        let mut_socinfo = socinfo.as_mut().unwrap();
+        let test_model = socinfo_minor((*socinfo).pmic_model);
+        model = test_model;
+    }
+
+    if model < 0{
+        return -22; //EINVAL
+    }
+
+    let mut check_valid_model: bool = false;
+    if PMIC_MODELS[model as usize] != ""{
+        check_valid_model = true;
+    }
+
+    if model < PMIC_MODELS.len() as u32 && check_valid_model {
+        unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,PMIC_MODELS[model as usize])};
+    }
+    else{
+        unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,secondary_format,model)};
+    }
+
+    return 0;
+}
+/*
+fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
+
+}
+
+fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
+
+}
+
+fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
+
+}
+
+*/
+
+struct QcomSocInfo{
+    soc_dev: *mut bindings::soc_device,
+    attr: bindings::soc_device_attribute,
+    dbg_root: *mut bindings::dentry,
+    info: SocInfoParams
+}
+
+impl QcomSocInfo{
+    fn socinfo_debugfs_init(&self, info: &mut SocInfo, info_size: usize) -> Option<c_types::c_void>{
+        None
+    }
+
+    fn socinfo_debugfs_exit(&self) -> Option<c_types::c_void>{
+        None
+    }
+
+    fn new() -> Self {
+        QcomSocInfo{
+            soc_dev: core::ptr::null_mut(),
+            attr: bindings::soc_device_attribute::default(),
+            dbg_root: core::ptr::null_mut(),
+            info: <SocInfoParams as Default>::default(),
+        }
+    }
+}
+
+struct SocInfoDriver {
+    _pdev: Pin<Box<platdev::Registration>>,
+}
+
+impl PlatformDriver for SocInfoDriver {
+    type DrvData = Pin<Box<QcomSocInfo>>;
+    fn probe(device_id: i32) -> Result<Self::DrvData>{
+        pr_info!("Just checking if probe works, with device id {}\n", device_id);
+
+        let mut drv_data= Pin::from(Box::try_new(QcomSocInfo::new())?);
+
+        let item_size: *mut usize = core::ptr::null_mut();
+
+        let mut info = unsafe {
+            bindings::qcom_smem_get(
+                QCOM_SMEM_HOST_ANY,
+                SMEM_HW_SW_BUILD_ID,
+                item_size,
+            ) as *mut SocInfo
+        };
+
+        let info_mut: &mut SocInfo;
+        unsafe { info_mut = info.as_mut().unwrap(); }
+
+        drv_data.as_mut().attr.family = c_str!("Snapdragon").as_char_ptr();
+        drv_data.as_mut().attr.machine = socinfo_machine(&info_mut.id).unwrap().as_char_ptr();
+
+        drv_data.as_mut().attr.soc_id = CStr::from_bytes_with_nul(
+            try_format!(
+                "{}\0",
+                info_mut.id
+            )?.as_bytes()
+        ).unwrap().as_char_ptr();
+
+        drv_data.as_mut().attr.revision = CStr::from_bytes_with_nul(
+            try_format!(
+                "{}.{}\0",
+                socinfo_major(info_mut.ver),
+                socinfo_minor(info_mut.ver)
+            )?.as_bytes()
+        ).unwrap().as_char_ptr();
+
+
+        unsafe{
+            drv_data.as_mut().soc_dev = bindings::soc_device_register(&mut drv_data.as_mut().attr );
+            drv_data.as_mut().socinfo_debugfs_init(info.as_mut().unwrap(),*item_size);
+            //socinfo_debugfs_init(/*&mut drv_data.as_mut(),*/ info.as_mut().unwrap(),*item_size) ;
+            bindings::add_device_randomness(info as *mut c_types::c_void,*item_size as u32)
+        };
+
+        Ok(drv_data)
+    }
+    fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
+        pr_info!("removing socinfo_rust now with device id {}\n", device_id);
+
+        unsafe {bindings::soc_device_unregister(_drv_data.soc_dev) };
+        _drv_data.as_ref().socinfo_debugfs_exit();
+        //socinfo_debugfs_exit(&_drv_data.as_ref());
+        Ok(())
+    }
+}
+
+impl KernelModule for SocInfoDriver {
+    fn init() -> Result<Self> {
+        const OF_MATCH_TBL: ConstOfMatchTable<1> = ConstOfMatchTable::new_const([c_str!("qcom-socinfo")]);
+
+        let pdev = platdev::Registration::new_pinned::<SocInfoDriver>(
+            c_str!("qcom-socinfo"),
+            Some(&OF_MATCH_TBL),
+            &THIS_MODULE,
+        )?;
+        Ok(SocInfoDriver {_pdev: pdev} )
+    }
+}
\ No newline at end of file
diff --git a/rust/kernel/bindings_helper.h b/rust/kernel/bindings_helper.h
index c64a630..a8c73450 100644
--- a/rust/kernel/bindings_helper.h
+++ b/rust/kernel/bindings_helper.h
@@ -18,6 +18,13 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>
 #include <linux/security.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/random.h>
 
 // `bindgen` gets confused at certain things
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
  2021-08-17  1:23 [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Antonio Martorana
@ 2021-08-17 16:43 ` Miguel Ojeda
  2021-08-18  9:36 ` Wei Liu
  2021-08-20 19:38 ` [RFC v2] " Antonio Martorana
  2 siblings, 0 replies; 6+ messages in thread
From: Miguel Ojeda @ 2021-08-17 16:43 UTC (permalink / raw)
  To: Antonio Martorana
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, rust-for-linux,
	Trilok Soni, Elliot Berman, agross, bjorn.andersson,
	linux-arm-msm

Hi Antonio,

Thanks a lot for giving Rust in the kernel a try!

Cc'ing the socinfo maintainers.

A few comments inline. In general, you should seek to avoid `unsafe`
in the Rust module as much as possible by providing safe abstractions
inside the `kernel` crate instead, which are the ones dealing with the
C-Rust interface / bindings.

On Tue, Aug 17, 2021 at 3:24 AM Antonio Martorana
<amartora@codeaurora.org> wrote:
>
> +       depends on HAS_RUST && QCOM_SMEM

This should be `RUST`, not `HAS_RUST` (the former is whether it is
actually enabled, the latter whether the toolchain was found).

> @@ -0,0 +1,464 @@

The SPDX identifier is missing at the top.

> +#![feature(allocator_api,global_asm)]

Please format the code (you can use `make rustfmt` or manually call
the `rustfmt` tool yourself).

Also, please pass the linter too if you did not do it (`CLIPPY=1`).

> +module! {
> +    type: SocInfoDriver,
> +    name: b"socinfo_rust",
> +    author: b"Antonio Martorana",
> +    description: b"QCOM socinfo rust implementation",
> +    license: b"GPL v2",
> +}

This is a proof of concept, so it is OK, but I am not sure if we
should say "rust" in the description for actual non-proof-of-concept
modules. My guess is that most maintainers will only want to
maintainer a single module, whether in C or Rust.

> +/*
> + * SMEM item id, used to acquire handles to respective
> + * SMEM region.
> + */
> +const SMEM_HW_SW_BUILD_ID: u32 = 137;

In general, we do not use `/*`-style comments. In addition, this
should be a documentation comment, i.e. `///`.

Same for other places.

> +/* C code has #ifdef */
> +const SMEM_IMAGE_VERSION_BLOCKS_COUNT: usize = 32;
> +const SMEM_IMAGE_VERSION_SIZE: usize = 4096;
> +const SMEM_IMAGE_VERSION_NAME_SIZE: usize = 75;
> +const SMEM_IMAGE_VERSION_VARIANT_SIZE: usize = 20;
> +const SMEM_IMAGE_VERSION_OEM_SIZE: usize = 32;

We have support for conditional compilation based on the kernel
configuration via e.g. an attribute like `#[cfg(CONFIG_X)]`.

Ideally you can put this inside a module, which would allow you to
have a single condition compilation avoid having to repeat the
attribute, as well as avoiding repeated prefixes like
`SMEM_IMAGE_VERSION`.

> +/*
> + * SMEM Image table indices
> + */
> +const SMEM_IMAGE_TABLE_BOOT_INDEX: u32 = 0;
> +const SMEM_IMAGE_TABLE_TZ_INDEX: u32 = 1;
> +const SMEM_IMAGE_TABLE_RPM_INDEX: u32 = 3;
> +const SMEM_IMAGE_TABLE_APPS_INDEX: u32 = 10;
> +const SMEM_IMAGE_TABLE_MPSS_INDEX: u32 = 11;
> +const SMEM_IMAGE_TABLE_ADSP_INDEX: u32 = 12;
> +const SMEM_IMAGE_TABLE_CNSS_INDEX: u32 = 13;
> +const SMEM_IMAGE_TABLE_VIDEO_INDEX: u32 = 14;
> +const SMEM_IMAGE_VERSION_TABLE: u32 = 496;

Same here -- this could be a `mod` and a doc comment on it.

> +struct SocInfo{
> +    fmt: bindings::__le32,

Rust modules should not access `bindings` in general -- abstractions
should be provided in `rust/`.

Also, `__le32` is for `sparse` -- to do something similar, we could
use a wrapper type that encodes the endianness.

> +    unsafe {let ref_mut_seq_priv = seq_private.as_mut().unwrap(); }

Unsafe blocks must have a `SAFETY: ` annotation, justifying all the
requirements of `as_mut()` (alignment, no aliasing, etc.).

They should also be as small as possible (to clearly indicate the part
that is unsafe), without using a single block for several unsafe
operations.

> +    if model < 0{
> +        return -22; //EINVAL
> +    }

We have the constants available. Also, the function should return a
`Result`, not a naked integer.

> +        unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,PMIC_MODELS[model as usize])};

`seq_printf` should likely be a macro like `pr_info!` etc. that we have.

> +/*
> +fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +*/

Commented out code (also in other places).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
  2021-08-17  1:23 [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Antonio Martorana
  2021-08-17 16:43 ` Miguel Ojeda
@ 2021-08-18  9:36 ` Wei Liu
  2021-08-20 19:38 ` [RFC v2] " Antonio Martorana
  2 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2021-08-18  9:36 UTC (permalink / raw)
  To: Antonio Martorana
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, rust-for-linux,
	Trilok Soni, Elliot Berman, Wei Liu

On Mon, Aug 16, 2021 at 06:23:32PM -0700, Antonio Martorana wrote:
[...]
> new file mode 100644
> index 0000000..8af697a
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo_rust.rs
> @@ -0,0 +1,464 @@
> +//! Qualcomm support for socinfo.c
> +
> +#![no_std]
> +#![feature(allocator_api,global_asm)]

Not really related to this patch, but I wish there was a more ergonomic
way to specify these crate level features. I think these two lines are
c&p in a lot of places.

[...]
> +/*
> + * SoC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.
> + */
> +const fn socinfo_major(ver: u32) -> u32{
> +    let major: u32 = (ver >> 16) & 0xFFFF;
> +    return major;

This can be simplified as

    const fn socinfo_major(ver: u32) -> u32 {
        (ver >> 16) & 0xFFFF
    }

> +}
> +
> +const fn socinfo_minor(ver: u32) -> u32{
> +    let minor: u32 = ver & 0xFFFF;
> +    return minor;
> +}
> +
> +const fn socinfo_version(maj: u32, min: u32) -> u32{
> +    let version: u32 = ((maj & 0xFFFF) << 16) | (min & 0xFFFF);
> +    return version;
> +}

Same for these two functions. They can be simplified as well.

I have one further question: why aren't these helpers implemented as
methods of SocInfo? Like:

    impl SocInfo {
        fn major(&self) -> u32 {
	    (self.ver >> 16) & 0xFFFF
	}
    }

Thanks,
Wei.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
  2021-08-17  1:23 [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Antonio Martorana
  2021-08-17 16:43 ` Miguel Ojeda
  2021-08-18  9:36 ` Wei Liu
@ 2021-08-20 19:38 ` Antonio Martorana
  2021-08-20 22:18   ` Miguel Ojeda
  2 siblings, 1 reply; 6+ messages in thread
From: Antonio Martorana @ 2021-08-20 19:38 UTC (permalink / raw)
  Cc: Antonio Martorana, rust-for-linux

To: Miguel Ojeda <ojeda@kernel.org>,
    Alex Gaynor <alex.gaynor@gmail.com>,
    Wedson Almeida Filho <wedsonaf@google.com>
Cc: rust-for-linux@vger.kernel.org,
    Trilok Soni <tsoni@codeaurora.org>,
    Elliot Berman <quic_eberman@quicinc.com>,
    linux-arm-msm <linux-arm-msm@vger.kernel.org>,
    Alex Gaynor <alex.gaynor@gmail.com>,
    Wei Liu <wei.liu@kernel.org>

Add initial work for Rust port of qcom/socinfo.c  driver. Socinfo driver
reads QCOM SMEM to learn current device information (e.g. SM8250).

 - Added several C libraries to bindings_helper.h for FFI support from  bindgen.
 - Implemented PlatformDriver and KernelModule traits for SocInfoDriver
    - Register with platform device framework
    - Add devicetree compatibles
 - Did not port functions referenced from SMEM, wanted to show a
   working proof of concept for Rust leaf driver with FFI code
 - Register the discovered SMEM data with socinfo core.

---

This patch relies on Mateusz Jabłoński's try_format! macro patch from GitHub:
https://github.com/Rust-for-Linux/linux/pull/478

V2: Addressed formating issues, added conditional compilation to debug_fs 
variables, fixed Kconfig, and still working on removing references to bindings:: 
by abstracting FFI functions through rust/ .

Signed-off-by: Antonio Martorana <amartora@codeaurora.org>
---
 drivers/soc/qcom/Kconfig         |   9 +
 drivers/soc/qcom/Makefile        |   1 +
 drivers/soc/qcom/socinfo_rust.rs | 794 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/bindings_helper.h    |   7 +
 4 files changed, 811 insertions(+)
 create mode 100644 drivers/soc/qcom/socinfo_rust.rs

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..d1b23c1 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -190,6 +190,15 @@ config QCOM_SOCINFO
 	 Say yes here to support the Qualcomm socinfo driver, providing
 	 information about the SoC to user space.
 
+config QCOM_SOCINFO_RUST
+	tristate "Rust implementation of Qualcomm socinfo driver"
+	depends on RUST && QCOM_SMEM
+	help
+	  This driver provides alternative Rust-based kernel-side support
+	  for the socinfo driver.
+
+	  If unsure, say N.
+
 config QCOM_WCNSS_CTRL
 	tristate "Qualcomm WCNSS control driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ad675a6..340ac65 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
+obj-$(CONFIG_QCOM_SOCINFO_RUST) += socinfo_rust.o
 obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
diff --git a/drivers/soc/qcom/socinfo_rust.rs b/drivers/soc/qcom/socinfo_rust.rs
new file mode 100644
index 0000000..3c4fb4c
--- /dev/null
+++ b/drivers/soc/qcom/socinfo_rust.rs
@@ -0,0 +1,794 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Qualcomm support for socinfo.c
+
+#![no_std]
+#![feature(allocator_api, global_asm)]
+
+use kernel::{
+    bindings, bindings::*, c_str, c_types, linked_list::Wrapper, of::ConstOfMatchTable, platdev,
+    platdev::PlatformDriver, prelude::*, str, str::CStr, sys_soc, Error,
+};
+
+use alloc::{boxed::Box, try_format};
+use core::{convert::TryInto, pin::Pin};
+
+module! {
+    type: SocInfoDriver,
+    name: b"socinfo_rust",
+    author: b"Antonio Martorana",
+    description: b"QCOM socinfo rust implementation",
+    license: b"GPL v2",
+}
+
+/// SoC version type with major number in the upper 16 bits and minor
+/// number in the lower 16 bits.
+
+const fn socinfo_major(ver: u32) -> u32 {
+    (ver >> 16) & 0xFFFF
+}
+
+const fn socinfo_minor(ver: u32) -> u32 {
+    ver & 0xFFFF
+}
+
+const fn socinfo_version(maj: u32, min: u32) -> u32 {
+    ((maj & 0xFFFF) << 16) | (min & 0xFFFF)
+}
+
+const SMEM_SOCINFO_BUILD_ID_LENGTH: usize = 32;
+const SMEM_SOCINFO_CHIP_ID_LENGTH: usize = 32;
+
+///  SMEM item id, used to acquire handles to respective
+///  SMEM region.
+const SMEM_HW_SW_BUILD_ID: u32 = 137;
+
+#[cfg(CONFIG_DEBUG_FS)]
+mod config_debug_fs {
+    pub const SMEM_IMAGE_VERSION_BLOCKS_COUNT: usize = 32;
+    pub const SMEM_IMAGE_VERSION_SIZE: usize = 4096;
+    pub const SMEM_IMAGE_VERSION_NAME_SIZE: usize = 75;
+    pub const SMEM_IMAGE_VERSION_VARIANT_SIZE: usize = 20;
+    pub const SMEM_IMAGE_VERSION_OEM_SIZE: usize = 32;
+
+    ///
+    /// SMEM Image table indices
+    ///
+    pub const SMEM_IMAGE_TABLE_BOOT_INDEX: u32 = 0;
+    pub const SMEM_IMAGE_TABLE_TZ_INDEX: u32 = 1;
+    pub const SMEM_IMAGE_TABLE_RPM_INDEX: u32 = 3;
+    pub const SMEM_IMAGE_TABLE_APPS_INDEX: u32 = 10;
+    pub const SMEM_IMAGE_TABLE_MPSS_INDEX: u32 = 11;
+    pub const SMEM_IMAGE_TABLE_ADSP_INDEX: u32 = 12;
+    pub const SMEM_IMAGE_TABLE_CNSS_INDEX: u32 = 13;
+    pub const SMEM_IMAGE_TABLE_VIDEO_INDEX: u32 = 14;
+    pub const SMEM_IMAGE_VERSION_TABLE: u32 = 496;
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+const PMIC_MODELS: [&str; 37] = [
+    "Unknown PMIC model",
+    "PM8941",
+    "PM8841",
+    "PM8019",
+    "PM8226",
+    "PM8110",
+    "PMA8084",
+    "PMI8962",
+    "PMD9635",
+    "PM8994",
+    "PMI8994",
+    "PM8916",
+    "PM8004",
+    "PM8909/PM8058",
+    "PM8028",
+    "PM8901",
+    "PM8950/PM8027",
+    "PMI8950/ISL9519",
+    "PM8921",
+    "PM8018",
+    "PM8998/PM8015",
+    "PMI8998/PM8014",
+    "PM8821",
+    "PM8038",
+    "PM8005/PM8922",
+    "PM8917",
+    "PM660L",
+    "PM660",
+    "",
+    "",
+    "PM8150",
+    "PM8150L",
+    "PM8150B",
+    "PMK8002",
+    "",
+    "",
+    "PM8009",
+];
+
+#[cfg(CONFIG_DEBUG_FS)]
+#[derive(Default)]
+struct SocInfoParams {
+    raw_device_family: u32,
+    hw_plat_subtype: u32,
+    accessory_chip: u32,
+    raw_device_num: u32,
+    chip_family: u32,
+    foundry_id: u32,
+    plat_ver: u32,
+    raw_ver: u32,
+    hw_plat: u32,
+    fmt: u32,
+    nproduct_id: u32,
+    num_clusters: u32,
+    ncluster_array_offset: u32,
+    num_defective_parts: u32,
+    ndefective_parts_array_offset: u32,
+    nmodem_supported: u32,
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+struct SmemImageVersion {
+    name: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_NAME_SIZE],
+    variant: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_VARIANT_SIZE],
+    pad: c_types::c_char,
+    oem: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_OEM_SIZE],
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+fn qcom_show_build_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
+    let seq_private = seq.private as *mut SocInfo;
+    unsafe {
+        let ref_mut_seq_priv = seq_private.as_mut().unwrap();
+    }
+
+    let mut format = c_str!("%s\n");
+    let format = format.as_char_ptr();
+
+    unsafe {
+        bindings::seq_printf(
+            seq as *mut bindings::seq_file,
+            format,
+            (*seq_private).build_id,
+        )
+    };
+
+    Ok(())
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
+    let socinfo = seq.private as *mut SocInfo;
+    let mut model;
+
+    let mut format = c_str!("%s\n");
+    let format = format.as_char_ptr();
+
+    let mut secondary_format = c_str!("unknown (%d)\n");
+    let secondary_format = secondary_format.as_char_ptr();
+
+    // SAFETY: `socinfo` is guaranteed to be:
+    // - A valid (and instalized), non-null pointer
+    // - Is properly aligned
+    // - Adheres to aliasing rules
+    unsafe {
+        let mut_socinfo = socinfo.as_mut().unwrap();
+        let test_model = socinfo_minor((*socinfo).pmic_model);
+        model = test_model;
+    }
+
+    let mut check_valid_model: bool = false;
+    if !PMIC_MODELS[model as usize].is_empty() {
+        check_valid_model = true;
+    }
+
+    if model < PMIC_MODELS.len() as u32 && check_valid_model {
+        unsafe {
+            bindings::seq_printf(
+                seq as *mut bindings::seq_file,
+                format,
+                PMIC_MODELS[model as usize],
+            )
+        };
+    } else {
+        unsafe { bindings::seq_printf(seq as *mut bindings::seq_file, secondary_format, model) };
+    }
+
+    Ok(())
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
+    Ok(())
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
+    Ok(())
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
+    Ok(())
+}
+
+struct SocInfo {
+    fmt: bindings::__le32,
+    id: bindings::__le32,
+    ver: bindings::__le32,
+    build_id: [c_types::c_char; SMEM_SOCINFO_BUILD_ID_LENGTH],
+    raw_id: bindings::__le32,
+    raw_ver: bindings::__le32,
+    hw_plat: bindings::__le32,
+    plat_ver: bindings::__le32,
+    accessory_chip: bindings::__le32,
+    hw_plat_subtype: bindings::__le32,
+    pmic_model: bindings::__le32,
+    pmic_die_rev: bindings::__le32,
+    pmic_model_1: bindings::__le32,
+    pmic_die_rev_1: bindings::__le32,
+    pmic_model_2: bindings::__le32,
+    pmic_die_rev_2: bindings::__le32,
+    foundry_id: bindings::__le32,
+    serial_num: bindings::__le32,
+    num_pmics: bindings::__le32,
+    pmic_array_offset: bindings::__le32,
+    chip_family: bindings::__le32,
+    raw_device_family: bindings::__le32,
+    raw_device_num: bindings::__le32,
+    nproduct_id: bindings::__le32,
+    chip_id: [c_types::c_char; SMEM_SOCINFO_CHIP_ID_LENGTH],
+    num_cluster: bindings::__le32,
+    ncluster_array_offset: bindings::__le32,
+    num_defective_parts: bindings::__le32,
+    nmodem_supported: bindings::__le32,
+}
+
+const SOC_ID: [SocId; 105] = [
+    SocId {
+        id: 87,
+        name: c_str!("MSM8960"),
+    },
+    SocId {
+        id: 109,
+        name: c_str!("APQ8064"),
+    },
+    SocId {
+        id: 122,
+        name: c_str!("MSM8660A"),
+    },
+    SocId {
+        id: 123,
+        name: c_str!("MSM8260A"),
+    },
+    SocId {
+        id: 124,
+        name: c_str!("APQ8060A"),
+    },
+    SocId {
+        id: 126,
+        name: c_str!("MSM8974"),
+    },
+    SocId {
+        id: 130,
+        name: c_str!("MPQ8064"),
+    },
+    SocId {
+        id: 138,
+        name: c_str!("MSM8960AB"),
+    },
+    SocId {
+        id: 139,
+        name: c_str!("APQ8060AB"),
+    },
+    SocId {
+        id: 140,
+        name: c_str!("MSM8260AB"),
+    },
+    SocId {
+        id: 141,
+        name: c_str!("MSM8660AB"),
+    },
+    SocId {
+        id: 145,
+        name: c_str!("MSM8626"),
+    },
+    SocId {
+        id: 147,
+        name: c_str!("MSM8610"),
+    },
+    SocId {
+        id: 153,
+        name: c_str!("APQ8064AB"),
+    },
+    SocId {
+        id: 158,
+        name: c_str!("MSM8226"),
+    },
+    SocId {
+        id: 159,
+        name: c_str!("MSM8526"),
+    },
+    SocId {
+        id: 161,
+        name: c_str!("MSM8110"),
+    },
+    SocId {
+        id: 162,
+        name: c_str!("MSM8210"),
+    },
+    SocId {
+        id: 163,
+        name: c_str!("MSM8810"),
+    },
+    SocId {
+        id: 164,
+        name: c_str!("MSM8212"),
+    },
+    SocId {
+        id: 165,
+        name: c_str!("MSM8612"),
+    },
+    SocId {
+        id: 166,
+        name: c_str!("MSM8112"),
+    },
+    SocId {
+        id: 168,
+        name: c_str!("MSM8225Q"),
+    },
+    SocId {
+        id: 169,
+        name: c_str!("MSM8625Q"),
+    },
+    SocId {
+        id: 170,
+        name: c_str!("MSM8125Q"),
+    },
+    SocId {
+        id: 172,
+        name: c_str!("APQ8064AA"),
+    },
+    SocId {
+        id: 178,
+        name: c_str!("APQ8084"),
+    },
+    SocId {
+        id: 184,
+        name: c_str!("APQ8074"),
+    },
+    SocId {
+        id: 185,
+        name: c_str!("MSM8274"),
+    },
+    SocId {
+        id: 186,
+        name: c_str!("MSM8674"),
+    },
+    SocId {
+        id: 194,
+        name: c_str!("MSM8974PRO"),
+    },
+    SocId {
+        id: 198,
+        name: c_str!("MSM8126"),
+    },
+    SocId {
+        id: 199,
+        name: c_str!("APQ8026"),
+    },
+    SocId {
+        id: 200,
+        name: c_str!("MSM8926"),
+    },
+    SocId {
+        id: 205,
+        name: c_str!("MSM8326"),
+    },
+    SocId {
+        id: 206,
+        name: c_str!("MSM8916"),
+    },
+    SocId {
+        id: 207,
+        name: c_str!("MSM8994"),
+    },
+    SocId {
+        id: 208,
+        name: c_str!("APQ8074-AA"),
+    },
+    SocId {
+        id: 209,
+        name: c_str!("APQ8074-AB"),
+    },
+    SocId {
+        id: 210,
+        name: c_str!("APQ8074PRO"),
+    },
+    SocId {
+        id: 211,
+        name: c_str!("MSM8274-AA"),
+    },
+    SocId {
+        id: 212,
+        name: c_str!("MSM8274-AB"),
+    },
+    SocId {
+        id: 213,
+        name: c_str!("MSM8274PRO"),
+    },
+    SocId {
+        id: 214,
+        name: c_str!("MSM8674-AA"),
+    },
+    SocId {
+        id: 215,
+        name: c_str!("MSM8674-AB"),
+    },
+    SocId {
+        id: 216,
+        name: c_str!("MSM8674PRO"),
+    },
+    SocId {
+        id: 217,
+        name: c_str!("MSM8974-AA"),
+    },
+    SocId {
+        id: 218,
+        name: c_str!("MSM8974-AB"),
+    },
+    SocId {
+        id: 219,
+        name: c_str!("APQ8028"),
+    },
+    SocId {
+        id: 220,
+        name: c_str!("MSM8128"),
+    },
+    SocId {
+        id: 221,
+        name: c_str!("MSM8228"),
+    },
+    SocId {
+        id: 222,
+        name: c_str!("MSM8528"),
+    },
+    SocId {
+        id: 223,
+        name: c_str!("MSM8628"),
+    },
+    SocId {
+        id: 224,
+        name: c_str!("MSM8928"),
+    },
+    SocId {
+        id: 225,
+        name: c_str!("MSM8510"),
+    },
+    SocId {
+        id: 226,
+        name: c_str!("MSM8512"),
+    },
+    SocId {
+        id: 233,
+        name: c_str!("MSM8936"),
+    },
+    SocId {
+        id: 239,
+        name: c_str!("MSM8939"),
+    },
+    SocId {
+        id: 240,
+        name: c_str!("APQ8036"),
+    },
+    SocId {
+        id: 241,
+        name: c_str!("APQ8039"),
+    },
+    SocId {
+        id: 246,
+        name: c_str!("MSM8996"),
+    },
+    SocId {
+        id: 247,
+        name: c_str!("APQ8016"),
+    },
+    SocId {
+        id: 248,
+        name: c_str!("MSM8216"),
+    },
+    SocId {
+        id: 249,
+        name: c_str!("MSM8116"),
+    },
+    SocId {
+        id: 250,
+        name: c_str!("MSM8616"),
+    },
+    SocId {
+        id: 251,
+        name: c_str!("MSM8992"),
+    },
+    SocId {
+        id: 253,
+        name: c_str!("APQ8094"),
+    },
+    SocId {
+        id: 290,
+        name: c_str!("MDM9607"),
+    },
+    SocId {
+        id: 291,
+        name: c_str!("APQ8096"),
+    },
+    SocId {
+        id: 292,
+        name: c_str!("MSM8998"),
+    },
+    SocId {
+        id: 293,
+        name: c_str!("MSM8953"),
+    },
+    SocId {
+        id: 296,
+        name: c_str!("MDM8207"),
+    },
+    SocId {
+        id: 297,
+        name: c_str!("MDM9207"),
+    },
+    SocId {
+        id: 298,
+        name: c_str!("MDM9307"),
+    },
+    SocId {
+        id: 299,
+        name: c_str!("MDM9628"),
+    },
+    SocId {
+        id: 304,
+        name: c_str!("APQ8053"),
+    },
+    SocId {
+        id: 305,
+        name: c_str!("MSM8996SG"),
+    },
+    SocId {
+        id: 310,
+        name: c_str!("MSM8996AU"),
+    },
+    SocId {
+        id: 311,
+        name: c_str!("APQ8096AU"),
+    },
+    SocId {
+        id: 312,
+        name: c_str!("APQ8096SG"),
+    },
+    SocId {
+        id: 317,
+        name: c_str!("SDM660"),
+    },
+    SocId {
+        id: 318,
+        name: c_str!("SDM630"),
+    },
+    SocId {
+        id: 319,
+        name: c_str!("APQ8098"),
+    },
+    SocId {
+        id: 321,
+        name: c_str!("SDM845"),
+    },
+    SocId {
+        id: 322,
+        name: c_str!("MDM9206"),
+    },
+    SocId {
+        id: 324,
+        name: c_str!("SDA660"),
+    },
+    SocId {
+        id: 325,
+        name: c_str!("SDM658"),
+    },
+    SocId {
+        id: 326,
+        name: c_str!("SDA658"),
+    },
+    SocId {
+        id: 327,
+        name: c_str!("SDA630"),
+    },
+    SocId {
+        id: 338,
+        name: c_str!("SDM450"),
+    },
+    SocId {
+        id: 341,
+        name: c_str!("SDA845"),
+    },
+    SocId {
+        id: 345,
+        name: c_str!("SDM636"),
+    },
+    SocId {
+        id: 346,
+        name: c_str!("SDA636"),
+    },
+    SocId {
+        id: 349,
+        name: c_str!("SDM632"),
+    },
+    SocId {
+        id: 350,
+        name: c_str!("SDA632"),
+    },
+    SocId {
+        id: 351,
+        name: c_str!("SDA450"),
+    },
+    SocId {
+        id: 356,
+        name: c_str!("SM8250"),
+    },
+    SocId {
+        id: 394,
+        name: c_str!("SM6125"),
+    },
+    SocId {
+        id: 402,
+        name: c_str!("IPQ6018"),
+    },
+    SocId {
+        id: 403,
+        name: c_str!("IPQ6028"),
+    },
+    SocId {
+        id: 421,
+        name: c_str!("IPQ6000"),
+    },
+    SocId {
+        id: 422,
+        name: c_str!("IPQ6010"),
+    },
+    SocId {
+        id: 425,
+        name: c_str!("SC7180"),
+    },
+    SocId {
+        id: 453,
+        name: c_str!("IPQ6005"),
+    },
+    SocId {
+        id: 455,
+        name: c_str!("QRB5165"),
+    },
+];
+
+struct SocId {
+    id: u32,
+    name: &'static str::CStr,
+}
+
+fn socinfo_machine(id: &u32) -> Option<&'static str::CStr> {
+    for idx in SOC_ID {
+        if idx.id == *id {
+            return Some(idx.name);
+        }
+    }
+    None
+}
+
+struct QcomSocInfo {
+    soc_dev: *mut bindings::soc_device,
+    attr: bindings::soc_device_attribute,
+    dbg_root: *mut bindings::dentry,
+    info: SocInfoParams,
+}
+
+impl QcomSocInfo {
+    fn socinfo_debugfs_init(
+        &self,
+        info: &mut SocInfo,
+        info_size: usize,
+    ) -> Option<c_types::c_void> {
+        None
+    }
+
+    fn socinfo_debugfs_exit(&self) -> Option<c_types::c_void> {
+        None
+    }
+
+    fn new() -> Self {
+        QcomSocInfo {
+            soc_dev: core::ptr::null_mut(),
+            attr: bindings::soc_device_attribute::default(),
+            dbg_root: core::ptr::null_mut(),
+            info: <SocInfoParams as Default>::default(),
+        }
+    }
+}
+
+struct SocInfoDriver {
+    _pdev: Pin<Box<platdev::Registration>>,
+}
+
+impl PlatformDriver for SocInfoDriver {
+    type DrvData = Pin<Box<QcomSocInfo>>;
+
+    fn probe(device_id: i32) -> Result<Self::DrvData> {
+        pr_info!(
+            "Just checking if probe works, with device id {}\n",
+            device_id
+        );
+
+        let mut drv_data = Pin::from(Box::try_new(QcomSocInfo::new())?);
+
+        let item_size: *mut usize = core::ptr::null_mut();
+
+        let mut info = unsafe {
+            bindings::qcom_smem_get(
+                QCOM_SMEM_HOST_ANY.try_into().unwrap(),
+                SMEM_HW_SW_BUILD_ID,
+                item_size,
+            ) as *mut SocInfo
+        };
+
+        let info_mut: &mut SocInfo;
+        unsafe {
+            info_mut = info.as_mut().unwrap();
+        }
+
+        drv_data.as_mut().attr.family = c_str!("Snapdragon").as_char_ptr();
+        drv_data.as_mut().attr.machine = socinfo_machine(&info_mut.id).unwrap().as_char_ptr();
+
+        drv_data.as_mut().attr.soc_id =
+            CStr::from_bytes_with_nul(try_format!("{}\0", info_mut.id)?.as_bytes())
+                .unwrap()
+                .as_char_ptr();
+
+        drv_data.as_mut().attr.revision = CStr::from_bytes_with_nul(
+            try_format!(
+                "{}.{}\0",
+                socinfo_major(info_mut.ver),
+                socinfo_minor(info_mut.ver)
+            )?
+            .as_bytes(),
+        )
+        .unwrap()
+        .as_char_ptr();
+
+        unsafe {
+            drv_data.as_mut().soc_dev = bindings::soc_device_register(&mut drv_data.as_mut().attr);
+            drv_data
+                .as_mut()
+                .socinfo_debugfs_init(info.as_mut().unwrap(), *item_size);
+            bindings::add_device_randomness(info as *mut c_types::c_void, *item_size as u32)
+        };
+
+        Ok(drv_data)
+    }
+    fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
+        pr_info!("removing socinfo_rust now with device id {}\n", device_id);
+
+        unsafe { bindings::soc_device_unregister(_drv_data.soc_dev) };
+        _drv_data.as_ref().socinfo_debugfs_exit();
+        Ok(())
+    }
+}
+
+impl KernelModule for SocInfoDriver {
+    fn init() -> Result<Self> {
+        const OF_MATCH_TBL: ConstOfMatchTable<1> =
+            ConstOfMatchTable::new_const([c_str!("qcom-socinfo")]);
+
+        let pdev = platdev::Registration::new_pinned::<SocInfoDriver>(
+            c_str!("qcom-socinfo"),
+            Some(&OF_MATCH_TBL),
+            &THIS_MODULE,
+        )?;
+        Ok(SocInfoDriver { _pdev: pdev })
+    }
+}
diff --git a/rust/kernel/bindings_helper.h b/rust/kernel/bindings_helper.h
index c64a630..a8c73450 100644
--- a/rust/kernel/bindings_helper.h
+++ b/rust/kernel/bindings_helper.h
@@ -18,6 +18,13 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>
 #include <linux/security.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/random.h>
 
 // `bindgen` gets confused at certain things
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
  2021-08-20 19:38 ` [RFC v2] " Antonio Martorana
@ 2021-08-20 22:18   ` Miguel Ojeda
  2021-08-20 23:05     ` amartora
  0 siblings, 1 reply; 6+ messages in thread
From: Miguel Ojeda @ 2021-08-20 22:18 UTC (permalink / raw)
  To: Antonio Martorana; +Cc: rust-for-linux

Hi Antonio,

Thanks again for working on this.

On Fri, Aug 20, 2021 at 9:39 PM Antonio Martorana
<amartora@codeaurora.org> wrote:
>
> V2: Addressed formating issues, added conditional compilation to debug_fs
> variables, fixed Kconfig, and still working on removing references to bindings::
> by abstracting FFI functions through rust/ .

Normally the changelog goes outside the commit message, i.e. after `---`.

> +/// SoC version type with major number in the upper 16 bits and minor
> +/// number in the lower 16 bits.
> +
> +const fn socinfo_major(ver: u32) -> u32 {

Doc comments should "touch" the function they document. But using
`///` like shown here would document only `socinfo_major`, and what
the comment says is not really the function documentation. What I
meant in v1 is that perhaps you can find a better way to document all
of them, or none.

Given `socinfo_version` is not used, I would remove that one, and put
"// The upper 16 bits are the major." and "// The lower 16 bits are
the minor." as normal comments inside the function body. Even better,
you could create a `SocInfoVersion` type.

> +    ///
> +    /// SMEM Image table indices
> +    ///

No empty comment lines before/after, please. In addition, `///` is a
doc comment, not a normal comment. You are trying to give a heading
for a set of `const`s, not document the first one.

We should add a lint for the former, and probably we could also try
one for the latter, too...

> +#[cfg(CONFIG_DEBUG_FS)]
> +const PMIC_MODELS: [&str; 37] = [

This is fine, but since you have created a `config_debug_fs` module,
perhaps you could move everything inside it.

Related: perhaps the name for that module could be something simpler,
like `debugfs`.

> +    unsafe {
> +        let ref_mut_seq_priv = seq_private.as_mut().unwrap();
> +    }

The `// SAFETY: ` comments in several places are missing.

> +fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {

Several of these functions cannot fail -- so why `Result`?

...ah, I see, you are panicking now -- but then you don't need the
`Result`. If this was userspace and you were not expecting a failure,
then yes, panic might have been a good idea. However, here we are
supposed to return the error like you did in v1. When I mentioned you
should use `Result`, it means you should return the `EINVAL` as such
(using the pre-defined error constants we have in
`rust/kernel/error.rs`, which are in the prelude for ease-of-use, see
e.g. https://rust-for-linux.github.io/docs/kernel/prelude/struct.Error.html).

> +    // SAFETY: `socinfo` is guaranteed to be:
> +    // - A valid (and instalized), non-null pointer
> +    // - Is properly aligned
> +    // - Adheres to aliasing rules

The `// SAFETY: ` comment should explain why the callee's
preconditions hold, not just state them (which does not help much a
future reader searching for UB).

> +    unsafe {
> +        let mut_socinfo = socinfo.as_mut().unwrap();

As mentioned in v1, `unsafe` blocks should be split and reduced as
much as possible.

> +        let test_model = socinfo_minor((*socinfo).pmic_model);
> +        model = test_model;

Why is `test_model` there?

> +    let mut check_valid_model: bool = false;
> +    if !PMIC_MODELS[model as usize].is_empty() {
> +        check_valid_model = true;
> +    }

Try to avoid mutability where possible, e.g.:

    let valid_model = !PMIC_MODELS[model as usize].is_empty();

> +    if model < PMIC_MODELS.len() as u32 && check_valid_model {

Something looks odd -- we are checking whether the model is within
`PMIC_MODELS`, but you already used it to index (which is safe, but
because it will panic if wrong -- which we do not want!).

> +#[cfg(CONFIG_DEBUG_FS)]
> +fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
> +    Ok(())
> +}
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
> +    Ok(())
> +}
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result {
> +    Ok(())
> +}

Even if not commented out, this is still dead code -- it is best to
remove it until you implement them.

> +    fmt: bindings::__le32,

Same thing about `bindings::` from v1 etc. -- if this were not a proof
of concept, it should be properly abstracted.

> +const SOC_ID: [SocId; 105] = [

For lots of data, it is usually better to use `static` to have a
single instance somewhere in memory (vs. e.g. inlining).

> +    SocId {
> +        id: 87,
> +        name: c_str!("MSM8960"),
> +    },

For repetitive things like this (and especially if the automatic
formatting makes it look bad!), a local "macro by example" is usually
a good approach, e.g.:

    soc_ids!(3,
         87 "MSM8960"
        109 "APQ8064"
        122 "MSM8660A etc etc"
    );

or any other syntax you like for the particular case. It looks much
better, and separates the details of the type from the data table.

The macro can be something like:

    macro_rules! soc_ids(
        ($length:literal, $($id:literal $name:literal)*) => {
            static SOC_ID: [SocID; $length] = [
                $(SocID { id: $id, name: $name },)*
            ];
        }
    );

https://godbolt.org/z/dWofxYn19 to play with it.

> +struct SocId {
> +    id: u32,
> +    name: &'static str::CStr,
> +}

Even if Rust allows otherwise, I would still try to put declarations
before they are used.

> +fn socinfo_machine(id: &u32) -> Option<&'static str::CStr> {

It is simpler to pass an integer as-is, rather than a pointer to it.

> +    for idx in SOC_ID {

You probably want `&SOC_ID` here (see the `static` vs. `const`
discussion above).

> +        if idx.id == *id {
> +            return Some(idx.name);
> +        }
> +    }
> +    None

Loops like this are good candidates for a functional style -- something like:

    SOC_ID.iter().find(|x| x.id == id).map(|x| x.name)

Whether this looks better or not, of course, is up for debate ;)

> +        Ok(drv_data)
> +    }
> +    fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {

Newline between functions, please.

Normally we only prefix by underscore if unused.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
  2021-08-20 22:18   ` Miguel Ojeda
@ 2021-08-20 23:05     ` amartora
  0 siblings, 0 replies; 6+ messages in thread
From: amartora @ 2021-08-20 23:05 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: rust-for-linux, Alex Gaynor, Wedson Almeida Filho, Trilok Soni,
	Elliot Berman, linux-arm-msm, Wei Liu

Just CC'ing the recipients who were missed, and thank you for all the 
feedback
  its much appreciated.

Cheers,
Antonio Martorana

On 2021-08-20 15:18, Miguel Ojeda wrote:
> Hi Antonio,
> 
> Thanks again for working on this.
> 
> On Fri, Aug 20, 2021 at 9:39 PM Antonio Martorana
> <amartora@codeaurora.org> wrote:
>> 
>> V2: Addressed formating issues, added conditional compilation to 
>> debug_fs
>> variables, fixed Kconfig, and still working on removing references to 
>> bindings::
>> by abstracting FFI functions through rust/ .
> 
> Normally the changelog goes outside the commit message, i.e. after 
> `---`.
> 
>> +/// SoC version type with major number in the upper 16 bits and minor
>> +/// number in the lower 16 bits.
>> +
>> +const fn socinfo_major(ver: u32) -> u32 {
> 
> Doc comments should "touch" the function they document. But using
> `///` like shown here would document only `socinfo_major`, and what
> the comment says is not really the function documentation. What I
> meant in v1 is that perhaps you can find a better way to document all
> of them, or none.
> 
> Given `socinfo_version` is not used, I would remove that one, and put
> "// The upper 16 bits are the major." and "// The lower 16 bits are
> the minor." as normal comments inside the function body. Even better,
> you could create a `SocInfoVersion` type.
> 
>> +    ///
>> +    /// SMEM Image table indices
>> +    ///
> 
> No empty comment lines before/after, please. In addition, `///` is a
> doc comment, not a normal comment. You are trying to give a heading
> for a set of `const`s, not document the first one.
> 
> We should add a lint for the former, and probably we could also try
> one for the latter, too...
> 
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +const PMIC_MODELS: [&str; 37] = [
> 
> This is fine, but since you have created a `config_debug_fs` module,
> perhaps you could move everything inside it.
> 
> Related: perhaps the name for that module could be something simpler,
> like `debugfs`.
> 
>> +    unsafe {
>> +        let ref_mut_seq_priv = seq_private.as_mut().unwrap();
>> +    }
> 
> The `// SAFETY: ` comments in several places are missing.
> 
>> +fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
> 
> Several of these functions cannot fail -- so why `Result`?
> 
> ...ah, I see, you are panicking now -- but then you don't need the
> `Result`. If this was userspace and you were not expecting a failure,
> then yes, panic might have been a good idea. However, here we are
> supposed to return the error like you did in v1. When I mentioned you
> should use `Result`, it means you should return the `EINVAL` as such
> (using the pre-defined error constants we have in
> `rust/kernel/error.rs`, which are in the prelude for ease-of-use, see
> e.g. 
> https://rust-for-linux.github.io/docs/kernel/prelude/struct.Error.html).
> 
>> +    // SAFETY: `socinfo` is guaranteed to be:
>> +    // - A valid (and instalized), non-null pointer
>> +    // - Is properly aligned
>> +    // - Adheres to aliasing rules
> 
> The `// SAFETY: ` comment should explain why the callee's
> preconditions hold, not just state them (which does not help much a
> future reader searching for UB).
> 
>> +    unsafe {
>> +        let mut_socinfo = socinfo.as_mut().unwrap();
> 
> As mentioned in v1, `unsafe` blocks should be split and reduced as
> much as possible.
> 
>> +        let test_model = socinfo_minor((*socinfo).pmic_model);
>> +        model = test_model;
> 
> Why is `test_model` there?
> 
>> +    let mut check_valid_model: bool = false;
>> +    if !PMIC_MODELS[model as usize].is_empty() {
>> +        check_valid_model = true;
>> +    }
> 
> Try to avoid mutability where possible, e.g.:
> 
>     let valid_model = !PMIC_MODELS[model as usize].is_empty();
> 
>> +    if model < PMIC_MODELS.len() as u32 && check_valid_model {
> 
> Something looks odd -- we are checking whether the model is within
> `PMIC_MODELS`, but you already used it to index (which is safe, but
> because it will panic if wrong -- which we do not want!).
> 
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
>> +
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
>> +
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
> 
> Even if not commented out, this is still dead code -- it is best to
> remove it until you implement them.
> 
>> +    fmt: bindings::__le32,
> 
> Same thing about `bindings::` from v1 etc. -- if this were not a proof
> of concept, it should be properly abstracted.
> 
>> +const SOC_ID: [SocId; 105] = [
> 
> For lots of data, it is usually better to use `static` to have a
> single instance somewhere in memory (vs. e.g. inlining).
> 
>> +    SocId {
>> +        id: 87,
>> +        name: c_str!("MSM8960"),
>> +    },
> 
> For repetitive things like this (and especially if the automatic
> formatting makes it look bad!), a local "macro by example" is usually
> a good approach, e.g.:
> 
>     soc_ids!(3,
>          87 "MSM8960"
>         109 "APQ8064"
>         122 "MSM8660A etc etc"
>     );
> 
> or any other syntax you like for the particular case. It looks much
> better, and separates the details of the type from the data table.
> 
> The macro can be something like:
> 
>     macro_rules! soc_ids(
>         ($length:literal, $($id:literal $name:literal)*) => {
>             static SOC_ID: [SocID; $length] = [
>                 $(SocID { id: $id, name: $name },)*
>             ];
>         }
>     );
> 
> https://godbolt.org/z/dWofxYn19 to play with it.
> 
>> +struct SocId {
>> +    id: u32,
>> +    name: &'static str::CStr,
>> +}
> 
> Even if Rust allows otherwise, I would still try to put declarations
> before they are used.
> 
>> +fn socinfo_machine(id: &u32) -> Option<&'static str::CStr> {
> 
> It is simpler to pass an integer as-is, rather than a pointer to it.
> 
>> +    for idx in SOC_ID {
> 
> You probably want `&SOC_ID` here (see the `static` vs. `const`
> discussion above).
> 
>> +        if idx.id == *id {
>> +            return Some(idx.name);
>> +        }
>> +    }
>> +    None
> 
> Loops like this are good candidates for a functional style -- something 
> like:
> 
>     SOC_ID.iter().find(|x| x.id == id).map(|x| x.name)
> 
> Whether this looks better or not, of course, is up for debate ;)
> 
>> +        Ok(drv_data)
>> +    }
>> +    fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
> 
> Newline between functions, please.
> 
> Normally we only prefix by underscore if unused.
> 
> Cheers,
> Miguel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-20 23:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-17  1:23 [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Antonio Martorana
2021-08-17 16:43 ` Miguel Ojeda
2021-08-18  9:36 ` Wei Liu
2021-08-20 19:38 ` [RFC v2] " Antonio Martorana
2021-08-20 22:18   ` Miguel Ojeda
2021-08-20 23:05     ` amartora

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).