https://lwn.net/Articles/863459/ LWN.net Logo LWN .net News from the source LWN * Content + Weekly Edition + Archives + Search + Kernel + Security + Distributions + Events calendar + Unread comments + ------------------------------------------------------------- + LWN FAQ + Write for us User: [ ] Password: [ ] [Log in] | [Subscribe] | [Register] Subscribe / Log in / New account A GPIO driver in Rust [Posted July 19, 2021 by corbet] As an example of what a "real" device driver in Rust would look like, Wedson Almeida Filho has posted a translation of the PL061 GPIO driver alongside the original. For ease of reading, the resulting HTML has been reformatted a bit and placed below; viewing in a wide window is recommended. C version Rust version 1 // SPDX-License-Identifier: GPL-2.0-only 1 // SPDX-License-Identifier: GPL-2.0 2 /* 2 3 * Copyright (C) 2008, 2009 Provigent Ltd. 3 //! Driver for the ARM PrimeCell(tm) General Purpose Input/Output (PL061). 4 * 4 //! 5 * Author: Baruch Siach 5 //! Based on the C driver written by Baruch Siach . 6 * 6 7 * Driver for the ARM PrimeCell(tm) General Purpose Input/Output (PL061) 7 #![no_std] 8 * 8 #![feature(global_asm, allocator_api)] 9 * Data sheet: ARM DDI 0190B, September 2000 9 10 */ 11 #include 10 use core::ops::DerefMut; 12 #include 11 use kernel::{ 13 #include 12 amba, bit, declare_id_table, device, gpio, 14 #include 13 io_mem::IoMem, 15 #include 14 irq::{self, IrqData, LockedIrqData}, 16 #include 15 power, 17 #include 16 prelude::*, 18 #include 17 sync::{IrqDisableSpinLock, Ref}, 19 #include 18 }; 20 #include 19 21 #include 22 #include 23 #include 24 #include 25 #include 26 #include 27 28 #define GPIODIR 0x400 20 const GPIODIR: usize = 0x400; 29 #define GPIOIS 0x404 21 const GPIOIS: usize = 0x404; 30 #define GPIOIBE 0x408 22 const GPIOIBE: usize = 0x408; 31 #define GPIOIEV 0x40C 23 const GPIOIEV: usize = 0x40C; 32 #define GPIOIE 0x410 24 const GPIOIE: usize = 0x410; 33 #define GPIORIS 0x414 25 const GPIOMIS: usize = 0x418; 34 #define GPIOMIS 0x418 26 const GPIOIC: usize = 0x41C; 35 #define GPIOIC 0x41C 27 const GPIO_SIZE: usize = 0x1000; 36 28 37 #define PL061_GPIO_NR 8 29 const PL061_GPIO_NR: u16 = 8; 38 30 39 #ifdef CONFIG_PM 31 #[derive(Default)] 40 struct pl061_context_save_regs { 32 struct ContextSaveRegs { 41 u8 gpio_data; 33 gpio_data: u8, 42 u8 gpio_dir; 34 gpio_dir: u8, 43 u8 gpio_is; 35 gpio_is: u8, 44 u8 gpio_ibe; 36 gpio_ibe: u8, 45 u8 gpio_iev; 37 gpio_iev: u8, 46 u8 gpio_ie; 38 gpio_ie: u8, 47 }; 39 } 48 #endif 40 49 41 #[derive(Default)] 50 struct pl061 { 42 struct PL061Data { 51 raw_spinlock_t lock; 43 csave_regs: ContextSaveRegs, 52 44 } 53 void __iomem *base; 45 54 struct gpio_chip gc; 46 struct PL061Resources { 55 struct irq_chip irq_chip; 47 base: IoMem, 56 int parent_irq; 48 parent_irq: u32, 57 49 } 58 #ifdef CONFIG_PM 50 59 struct pl061_context_save_regs csave_regs; 51 struct PL061Registrations { 60 #endif 52 gpio_chip: gpio::ChipRegistration, 61 }; 53 } 62 54 55 type DeviceData = device::Data>; 56 57 struct PL061Device; 58 59 impl gpio::Chip for PL061Device { 60 type IrqChip = Self; 61 type Data = Ref; 62 63 static int pl061_get_direction(struct gpio_chip *gc, unsigned offset) 63 fn get_direction(data: &Ref, offset: u32) -> Result { 64 { 64 let pl061 = data.resources().ok_or(Error::ENXIO)?; 65 struct pl061 *pl061 = gpiochip_get_data(gc); 65 Ok(if pl061.base.readb(GPIODIR) & bit(offset) != 0 { 66 66 gpio::LineDirection::Out 67 if (readb(pl061->base + GPIODIR) & BIT(offset)) 67 } else { 68 return GPIO_LINE_DIRECTION_OUT; 68 gpio::LineDirection::In 69 69 }) 70 return GPIO_LINE_DIRECTION_IN; 70 } 71 } 71 72 73 static int pl061_direction_input(struct gpio_chip *gc, unsigned offset) 72 fn direction_input(data: &Ref, offset: u32) -> Result { 74 { 73 let _guard = data.lock(); 75 struct pl061 *pl061 = gpiochip_get_data(gc); 74 let pl061 = data.resources().ok_or(Error::ENXIO)?; 76 unsigned long flags; 75 let mut gpiodir = pl061.base.readb(GPIODIR); 77 unsigned char gpiodir; 76 gpiodir &= !bit(offset); 78 77 pl061.base.writeb(gpiodir, GPIODIR); 79 raw_spin_lock_irqsave(&pl061->lock, flags); 78 Ok(()) 80 gpiodir = readb(pl061->base + GPIODIR); 79 } 81 gpiodir &= ~(BIT(offset)); 80 82 writeb(gpiodir, pl061->base + GPIODIR); 81 83 raw_spin_unlock_irqrestore(&pl061->lock, flags); 84 85 return 0; 86 } 87 88 static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, fn direction_output(data: &Ref, offset: u32, value: bool) -> Result { 89 int value) 82 let woffset = bit(offset + 2).into(); 90 { 83 let _guard = data.lock(); 91 struct pl061 *pl061 = gpiochip_get_data(gc); 84 let pl061 = data.resources().ok_or(Error::ENXIO)?; 92 unsigned long flags; 85 pl061.base.try_writeb((value as u8) << offset, woffset)?; 93 unsigned char gpiodir; 86 let mut gpiodir = pl061.base.readb(GPIODIR); 94 87 gpiodir |= bit(offset); 95 raw_spin_lock_irqsave(&pl061->lock, flags); 88 pl061.base.writeb(gpiodir, GPIODIR); 96 writeb(!!value << offset, pl061->base + (BIT(offset + 2))); 89 97 gpiodir = readb(pl061->base + GPIODIR); 90 // gpio value is set again, because pl061 doesn't allow to set value of a gpio pin before 98 gpiodir |= BIT(offset); 91 // configuring it in OUT mode. 99 writeb(gpiodir, pl061->base + GPIODIR); 92 pl061.base.try_writeb((value as u8) << offset, woffset)?; 100 93 Ok(()) 101 /* 94 } 102 * gpio value is set again, because pl061 doesn't allow to set value of 95 103 * a gpio pin before configuring it in OUT mode. 104 */ 105 writeb(!!value << offset, pl061->base + (BIT(offset + 2))); 106 raw_spin_unlock_irqrestore(&pl061->lock, flags); 107 108 return 0; 109 } 110 111 static int pl061_get_value(struct gpio_chip *gc, unsigned offset) 96 fn get(data: &Ref, offset: u32) -> Result { 112 { 97 let pl061 = data.resources().ok_or(Error::ENXIO)?; 113 struct pl061 *pl061 = gpiochip_get_data(gc); 98 Ok(pl061.base.try_readb(bit(offset + 2).into())? != 0) 114 99 } 115 return !!readb(pl061->base + (BIT(offset + 2))); 100 116 } 117 118 static void pl061_set_value(struct gpio_chip *gc, unsigned offset, int value) 101 fn set(data: &Ref, offset: u32, value: bool) { 119 { 102 if let Some(pl061) = data.resources() { 120 struct pl061 *pl061 = gpiochip_get_data(gc); 103 let woffset = bit(offset + 2).into(); 121 104 let _ = pl061.base.try_writeb((value as u8) << offset, woffset); 122 writeb(!!value << offset, pl061->base + (BIT(offset + 2))); 105 } 123 } 106 } 124 107 125 static void pl061_irq_handler(struct irq_desc *desc) 108 fn irq_route(data: &Ref, router: &gpio::IrqRouter) { 126 { 109 if let Some(pl061) = data.resources() { 127 unsigned long pending; 110 let pending = pl061.base.readb(GPIOMIS); 128 int offset; 111 if pending != 0 { 129 struct gpio_chip *gc = irq_desc_get_handler_data(desc); 112 for offset in 0..PL061_GPIO_NR { 130 struct pl061 *pl061 = gpiochip_get_data(gc); 113 if pending & bit(offset) != 0 { 131 struct irq_chip *irqchip = irq_desc_get_chip(desc); 114 router.deliver(offset.into()); 132 115 } 133 chained_irq_enter(irqchip, desc); 116 } 134 117 } 135 pending = readb(pl061->base + GPIOMIS); 118 } 136 if (pending) { 119 } 137 for_each_set_bit(offset, &pending, PL061_GPIO_NR) 120 } 138 generic_handle_irq(irq_find_mapping(gc->irq.domain, 121 139 offset)); 122 impl irq::Chip for PL061Device { 140 } 123 type Data = Ref; 141 124 142 chained_irq_exit(irqchip, desc); 143 } 144 145 static int pl061_irq_type(struct irq_data *d, unsigned trigger) 125 fn set_type(data: &Ref, irq_data: &mut LockedIrqData, trigger: u32) -> Result { 146 { 126 let offset = irq_data.hwirq(); 147 struct gpio_chip *gc = irq_data_get_irq_chip_data(d); 127 let bit = bit(offset); 148 struct pl061 *pl061 = gpiochip_get_data(gc); 128 149 int offset = irqd_to_hwirq(d); 129 if offset >= PL061_GPIO_NR.into() { 150 unsigned long flags; 130 return Err(Error::EINVAL); 151 u8 gpiois, gpioibe, gpioiev; 131 } 152 u8 bit = BIT(offset); 132 153 133 if trigger & (irq::TYPE_LEVEL_HIGH | irq::TYPE_LEVEL_LOW) != 0 154 if (offset < 0 || offset >= PL061_GPIO_NR) 134 && trigger & (irq::TYPE_EDGE_RISING | irq::TYPE_EDGE_FALLING) != 0 155 return -EINVAL; 135 { 156 136 pr_err!( 157 if ((trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) && 137 "trying to configure line {} for both level and edge detection, choose one!\n", 158 (trigger & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))) 138 offset 159 { 139 ); 160 dev_err(gc->parent, 140 return Err(Error::EINVAL); 161 "trying to configure line %d for both level and edge " 141 } 162 "detection, choose one!\n", 142 163 offset); 143 let _guard = data.lock(); 164 return -EINVAL; 144 let pl061 = data.resources().ok_or(Error::ENXIO)?; 165 } 145 166 146 let mut gpioiev = pl061.base.readb(GPIOIEV); 167 147 let mut gpiois = pl061.base.readb(GPIOIS); 168 raw_spin_lock_irqsave(&pl061->lock, flags); 148 let mut gpioibe = pl061.base.readb(GPIOIBE); 169 149 170 gpioiev = readb(pl061->base + GPIOIEV); 150 if trigger & (irq::TYPE_LEVEL_HIGH | irq::TYPE_LEVEL_LOW) != 0 { 171 gpiois = readb(pl061->base + GPIOIS); 151 let polarity = trigger & irq::TYPE_LEVEL_HIGH != 0; 172 gpioibe = readb(pl061->base + GPIOIBE); 152 173 153 // Disable edge detection. 174 if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) { 154 gpioibe &= !bit; 175 bool polarity = trigger & IRQ_TYPE_LEVEL_HIGH; 155 // Enable level detection. 176 156 gpiois |= bit; 177 /* Disable edge detection */ 157 // Select polarity. 178 gpioibe &= ~bit; 158 if polarity { 179 /* Enable level detection */ 159 gpioiev |= bit; 180 gpiois |= bit; 160 } else { 181 /* Select polarity */ 161 gpioiev &= !bit; 182 if (polarity) 162 } 183 gpioiev |= bit; 163 irq_data.set_level_handler(); 184 else 164 pr_debug!( 185 gpioiev &= ~bit; 165 "line {}: IRQ on {} level\n", 186 irq_set_handler_locked(d, handle_level_irq); 166 offset, 187 dev_dbg(gc->parent, "line %d: IRQ on %s level\n", 167 if polarity { "HIGH" } else { "LOW" } 188 offset, 168 ); 189 polarity ? "HIGH" : "LOW"); 169 } else if (trigger & irq::TYPE_EDGE_BOTH) == irq::TYPE_EDGE_BOTH { 190 } else if ((trigger & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) { 170 // Disable level detection. 191 /* Disable level detection */ 171 gpiois &= !bit; 192 gpiois &= ~bit; 172 // Select both edges, settings this makes GPIOEV be ignored. 193 /* Select both edges, setting this makes GPIOEV be ignored */ 173 gpioibe |= bit; 194 gpioibe |= bit; 174 irq_data.set_edge_handler(); 195 irq_set_handler_locked(d, handle_edge_irq); 175 pr_debug!("line {}: IRQ on both edges\n", offset); 196 dev_dbg(gc->parent, "line %d: IRQ on both edges\n", offset); 176 } else if trigger & (irq::TYPE_EDGE_RISING | irq::TYPE_EDGE_FALLING) != 0 { 197 } else if ((trigger & IRQ_TYPE_EDGE_RISING) || 177 let rising = trigger & irq::TYPE_EDGE_RISING != 0; 198 (trigger & IRQ_TYPE_EDGE_FALLING)) { 178 199 bool rising = trigger & IRQ_TYPE_EDGE_RISING; 179 // Disable level detection. 200 180 gpiois &= !bit; 201 /* Disable level detection */ 181 // Clear detection on both edges. 202 gpiois &= ~bit; 182 gpioibe &= !bit; 203 /* Clear detection on both edges */ 183 // Select edge. 204 gpioibe &= ~bit; 184 if rising { 205 /* Select edge */ 185 gpioiev |= bit; 206 if (rising) 186 } else { 207 gpioiev |= bit; 187 gpioiev &= !bit; 208 else 188 } 209 gpioiev &= ~bit; 189 irq_data.set_edge_handler(); 210 irq_set_handler_locked(d, handle_edge_irq); 190 pr_debug!( 211 dev_dbg(gc->parent, "line %d: IRQ on %s edge\n", 191 "line {}: IRQ on {} edge\n", 212 offset, 192 offset, 213 rising ? "RISING" : "FALLING"); 193 if rising { "RISING" } else { "FALLING}" } 214 } else { 194 ); 215 /* No trigger: disable everything */ 195 } else { 216 gpiois &= ~bit; 196 // No trigger: disable everything. 217 gpioibe &= ~bit; 197 gpiois &= !bit; 218 gpioiev &= ~bit; 198 gpioibe &= !bit; 219 irq_set_handler_locked(d, handle_bad_irq); 199 gpioiev &= !bit; 220 dev_warn(gc->parent, "no trigger selected for line %d\n", 200 irq_data.set_bad_handler(); 221 offset); 201 pr_warn!("no trigger selected for line {}\n", offset); 222 } 202 } 223 203 224 writeb(gpiois, pl061->base + GPIOIS); 204 pl061.base.writeb(gpioiev, GPIOIEV); 225 writeb(gpioibe, pl061->base + GPIOIBE); 205 pl061.base.writeb(gpiois, GPIOIS); 226 writeb(gpioiev, pl061->base + GPIOIEV); 206 pl061.base.writeb(gpioibe, GPIOIBE); 227 207 228 raw_spin_unlock_irqrestore(&pl061->lock, flags); 208 Ok(()) 229 209 } 230 return 0; 210 231 } 232 233 static void pl061_irq_mask(struct irq_data *d) 211 fn mask(data: &Ref, irq_data: &IrqData) { 234 { 212 let mask = bit(irq_data.hwirq() % u64::from(PL061_GPIO_NR)); 235 struct gpio_chip *gc = irq_data_get_irq_chip_data(d); 213 let _guard = data.lock(); 236 struct pl061 *pl061 = gpiochip_get_data(gc); 214 if let Some(pl061) = data.resources() { 237 u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR); 215 let gpioie = pl061.base.readb(GPIOIE) & !mask; 238 u8 gpioie; 216 let _ = pl061.base.try_writeb(gpioie, GPIOIE); 239 217 } 240 raw_spin_lock(&pl061->lock); 218 } 241 gpioie = readb(pl061->base + GPIOIE) & ~mask; 219 242 writeb(gpioie, pl061->base + GPIOIE); 243 raw_spin_unlock(&pl061->lock); 244 } 245 246 static void pl061_irq_unmask(struct irq_data *d) 220 fn unmask(data: &Ref, irq_data: &IrqData) { 247 { 221 let mask = bit(irq_data.hwirq() % u64::from(PL061_GPIO_NR)); 248 struct gpio_chip *gc = irq_data_get_irq_chip_data(d); 222 let _guard = data.lock(); 249 struct pl061 *pl061 = gpiochip_get_data(gc); 223 if let Some(pl061) = data.resources() { 250 u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR); 224 let gpioie = pl061.base.readb(GPIOIE) | mask; 251 u8 gpioie; 225 let _ = pl061.base.try_writeb(gpioie, GPIOIE); 252 226 } 253 raw_spin_lock(&pl061->lock); 227 } 254 gpioie = readb(pl061->base + GPIOIE) | mask; 228 255 writeb(gpioie, pl061->base + GPIOIE); 256 raw_spin_unlock(&pl061->lock); 257 } 258 259 /** 260 * pl061_irq_ack() - ACK an edge IRQ 261 * @d: IRQ data for this IRQ 262 * 263 * This gets called from the edge IRQ handler to ACK the edge IRQ 229 // This gets called from the edge IRQ handler to ACK the edge IRQ in the GPIOIC 264 * in the GPIOIC (interrupt-clear) register. For level IRQs this is 230 // (interrupt-clear) register. For level IRQs this is not needed: these go away when the level 265 * not needed: these go away when the level signal goes away. 231 // signal goes away. 266 */ 232 fn ack(data: &Ref, irq_data: &IrqData) { 267 static void pl061_irq_ack(struct irq_data *d) 233 let mask = bit(irq_data.hwirq() % u64::from(PL061_GPIO_NR)); 268 { 234 let _guard = data.lock(); 269 struct gpio_chip *gc = irq_data_get_irq_chip_data(d); 235 if let Some(pl061) = data.resources() { 270 struct pl061 *pl061 = gpiochip_get_data(gc); 236 let _ = pl061.base.try_writeb(mask.into(), GPIOIC); 271 u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR); 237 } 272 238 } 273 raw_spin_lock(&pl061->lock); 239 274 writeb(mask, pl061->base + GPIOIC); 275 raw_spin_unlock(&pl061->lock); 276 } 277 278 static int pl061_irq_set_wake(struct irq_data *d, unsigned int state) 240 fn set_wake(data: &Ref, _irq_data: &IrqData, on: bool) -> Result { 279 { 241 let pl061 = data.resources().ok_or(Error::ENXIO)?; 280 struct gpio_chip *gc = irq_data_get_irq_chip_data(d); 242 irq::set_irq_wake(pl061.parent_irq, on) 281 struct pl061 *pl061 = gpiochip_get_data(gc); 243 } 282 244 } 283 return irq_set_irq_wake(pl061->parent_irq, state); 245 284 } 246 impl amba::Driver for PL061Device { 285 247 type InnerData = DeviceData; 248 type PowerOps = Self; 249 250 declare_id_table! { 251 (0x00041061, 0x000fffff), 252 } 253 286 static int pl061_probe(struct amba_device *adev, const struct amba_id *id) 254 fn probe( 287 { 255 dev: &mut amba::Device, 288 struct device *dev = &adev->dev; 256 _id: &(u32, u32, Option), 289 struct pl061 *pl061; 257 ) -> Result> { 290 struct gpio_irq_chip *girq; 258 let res = dev.take_resource().ok_or(Error::ENXIO)?; 291 int ret, irq; 259 let irq = dev.irq(0).ok_or(Error::ENXIO)?; 292 260 293 pl061 = devm_kzalloc(dev, sizeof(*pl061), GFP_KERNEL); 261 let data = Ref::try_new_and_init( 294 if (pl061 == NULL) 262 device::Data::new( 295 return -ENOMEM; 263 PL061Registrations { 296 264 gpio_chip: gpio::ChipRegistration::default(), 297 pl061->base = devm_ioremap_resource(dev, &adev->res); 265 }, 298 if (IS_ERR(pl061->base)) 266 PL061Resources { 299 return PTR_ERR(pl061->base); 267 base: IoMem::try_new(res)?, 300 268 parent_irq: irq, 301 raw_spin_lock_init(&pl061->lock); 269 }, 302 pl061->gc.request = gpiochip_generic_request; 270 // SAFETY: We call `irqsave_spinlock_init` below. 303 pl061->gc.free = gpiochip_generic_free; 271 unsafe { IrqDisableSpinLock::new(PL061Data::default()) }, 304 pl061->gc.base = -1; 272 ), 305 pl061->gc.get_direction = pl061_get_direction; 273 |mut data| { 306 pl061->gc.direction_input = pl061_direction_input; 274 // SAFETY: General part of the data is pinned when `data` is. 307 pl061->gc.direction_output = pl061_direction_output; 275 let gen = unsafe { data.as_mut().map_unchecked_mut(|d| d.deref_mut()) }; 308 pl061->gc.get = pl061_get_value; 276 kernel::irqdisable_spinlock_init!(gen, "PL061::General"); 309 pl061->gc.set = pl061_set_value; 277 }, 310 pl061->gc.ngpio = PL061_GPIO_NR; 278 )?; 311 pl061->gc.label = dev_name(dev); 279 312 pl061->gc.parent = dev; 280 data.resources().ok_or(Error::ENXIO)?.base.writeb(0, GPIOIE); // disable irqs 313 pl061->gc.owner = THIS_MODULE; 281 data.registrations() 314 282 .ok_or(Error::ENXIO)? 315 /* 283 .gpio_chip 316 * irq_chip support 284 .register_with_irq(PL061_GPIO_NR, None, dev, data.clone(), irq)?; 317 */ 285 318 pl061->irq_chip.name = dev_name(dev); 286 pr_info!("PL061 GPIO chip registered\n"); 319 pl061->irq_chip.irq_ack = pl061_irq_ack; 287 320 pl061->irq_chip.irq_mask = pl061_irq_mask; 288 Ok(data) 321 pl061->irq_chip.irq_unmask = pl061_irq_unmask; 289 } 322 pl061->irq_chip.irq_set_type = pl061_irq_type; 290 } 323 pl061->irq_chip.irq_set_wake = pl061_irq_set_wake; 291 324 292 impl power::Operations for PL061Device { 325 writeb(0, pl061->base + GPIOIE); /* disable irqs */ 293 type Data = Ref; 326 irq = adev->irq[0]; 294 327 if (!irq) 328 dev_warn(&adev->dev, "IRQ support disabled\n"); 329 pl061->parent_irq = irq; 330 331 girq = &pl061->gc.irq; 332 girq->chip = &pl061->irq_chip; 333 girq->parent_handler = pl061_irq_handler; 334 girq->num_parents = 1; 335 girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), 336 GFP_KERNEL); 337 if (!girq->parents) 338 return -ENOMEM; 339 girq->parents[0] = irq; 340 girq->default_type = IRQ_TYPE_NONE; 341 girq->handler = handle_bad_irq; 342 343 ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061); 344 if (ret) 345 return ret; 346 347 amba_set_drvdata(adev, pl061); 348 dev_info(dev, "PL061 GPIO chip registered\n"); 349 350 return 0; 351 } 352 353 #ifdef CONFIG_PM 354 static int pl061_suspend(struct device *dev) 295 fn suspend(data: &Ref) -> Result { 355 { 296 let mut inner = data.lock(); 356 struct pl061 *pl061 = dev_get_drvdata(dev); 297 let pl061 = data.resources().ok_or(Error::ENXIO)?; 357 int offset; 298 inner.csave_regs.gpio_data = 0; 358 299 inner.csave_regs.gpio_dir = pl061.base.readb(GPIODIR); 359 pl061->csave_regs.gpio_data = 0; 300 inner.csave_regs.gpio_is = pl061.base.readb(GPIOIS); 360 pl061->csave_regs.gpio_dir = readb(pl061->base + GPIODIR); 301 inner.csave_regs.gpio_ibe = pl061.base.readb(GPIOIBE); 361 pl061->csave_regs.gpio_is = readb(pl061->base + GPIOIS); 302 inner.csave_regs.gpio_iev = pl061.base.readb(GPIOIEV); 362 pl061->csave_regs.gpio_ibe = readb(pl061->base + GPIOIBE); 303 inner.csave_regs.gpio_ie = pl061.base.readb(GPIOIE); 363 pl061->csave_regs.gpio_iev = readb(pl061->base + GPIOIEV); 304 364 pl061->csave_regs.gpio_ie = readb(pl061->base + GPIOIE); 305 for offset in 0..PL061_GPIO_NR { 365 306 if inner.csave_regs.gpio_dir & bit(offset) != 0 { 366 for (offset = 0; offset < PL061_GPIO_NR; offset++) { 307 if let Ok(v) = ::get(data, offset.into()) { 367 if (pl061->csave_regs.gpio_dir & (BIT(offset))) 308 inner.csave_regs.gpio_data |= (v as u8) << offset; 368 pl061->csave_regs.gpio_data |= 309 } 369 pl061_get_value(&pl061->gc, offset) << offset; 310 } 370 } 311 } 371 312 372 return 0; 313 Ok(()) 373 } 314 } 374 315 375 static int pl061_resume(struct device *dev) 316 fn resume(data: &Ref) -> Result { 376 { 317 let inner = data.lock(); 377 struct pl061 *pl061 = dev_get_drvdata(dev); 318 let pl061 = data.resources().ok_or(Error::ENXIO)?; 378 int offset; 319 379 320 for offset in 0..PL061_GPIO_NR { 380 for (offset = 0; offset < PL061_GPIO_NR; offset++) { 321 if inner.csave_regs.gpio_dir & bit(offset) != 0 { 381 if (pl061->csave_regs.gpio_dir & (BIT(offset))) 322 let value = inner.csave_regs.gpio_data & bit(offset) != 0; 382 pl061_direction_output(&pl061->gc, offset, 323 let _ = ::direction_output(data, offset.into(), value); 383 pl061->csave_regs.gpio_data & 324 } else { 384 (BIT(offset))); 325 let _ = ::direction_input(data, offset.into()); 385 else 326 } 386 pl061_direction_input(&pl061->gc, offset); 327 } 387 } 328 388 329 pl061.base.writeb(inner.csave_regs.gpio_is, GPIOIS); 389 writeb(pl061->csave_regs.gpio_is, pl061->base + GPIOIS); 330 pl061.base.writeb(inner.csave_regs.gpio_ibe, GPIOIBE); 390 writeb(pl061->csave_regs.gpio_ibe, pl061->base + GPIOIBE); 331 pl061.base.writeb(inner.csave_regs.gpio_iev, GPIOIEV); 391 writeb(pl061->csave_regs.gpio_iev, pl061->base + GPIOIEV); 332 pl061.base.writeb(inner.csave_regs.gpio_ie, GPIOIE); 392 writeb(pl061->csave_regs.gpio_ie, pl061->base + GPIOIE); 333 393 334 Ok(()) 394 return 0; 335 } 395 } 336 396 337 fn freeze(data: &Ref) -> Result { 397 static const struct dev_pm_ops pl061_dev_pm_ops = { 338 Self::suspend(data) 398 .suspend = pl061_suspend, 339 } 399 .resume = pl061_resume, 340 400 .freeze = pl061_suspend, 341 fn restore(data: &Ref) -> Result { 401 .restore = pl061_resume, 342 Self::resume(data) 402 }; 343 } 403 #endif 344 } 404 345 405 static const struct amba_id pl061_ids[] = { 406 { 407 .id = 0x00041061, 408 .mask = 0x000fffff, 409 }, 410 { 0, 0 }, 411 }; 412 MODULE_DEVICE_TABLE(amba, pl061_ids); 413 414 static struct amba_driver pl061_gpio_driver = { 415 .drv = { 416 .name = "pl061_gpio", 417 #ifdef CONFIG_PM 418 .pm = &pl061_dev_pm_ops, 419 #endif 420 }, 421 .id_table = pl061_ids, 422 .probe = pl061_probe, 423 }; 424 module_amba_driver(pl061_gpio_driver); 346 module_amba_driver! { 425 347 type: PL061Device, 426 MODULE_LICENSE("GPL v2"); 348 name: b"pl061_gpio", 349 author: b"Wedson Almeida Filho", 350 license: b"GPL v2", 351 } ----------------------------------------- (Log in to post comments) A GPIO driver in Rust Posted Jul 19, 2021 16:15 UTC (Mon) by bredelings (subscriber, # 53082) [Link] Nice! Also 75 lines shorter. -BenRI [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 16:18 UTC (Mon) by NHO (subscriber, #104320) [ Link] Doesn't actually calls `irqsave_spinlock_init` below [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 17:10 UTC (Mon) by pbonzini ( supporter , # 60935) [Link] Looks like a typo, there's an irqdisable_spinlock_init below. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 16:55 UTC (Mon) by logang (subscriber, #127618) [ Link] But a lot of the line count difference appears to be due to style... Linux style requires the open brace of the function to be on it's own line, there be a blank line after a function's variable declarations, and often a blank line before the return statement. The Rust code is also using 4 char indents and ignores the line length limit which allows a few lines to require less wrapping. Not to mention the Rust code mixes variable declarations in the body of the code which saves a few more lines and is also against the style guide. If the Rust code didn't ignore the style guide (or the C code was written in a similar compressed style) the line difference would be far less pronounced. IMO all this makes the rust code harder to read than the C code. I suspect the Rust developers would have less resistance if they followed kernel coding style more closely instead of assuming a new language allows them to define their own style. The biggest line savings seem to be in the include list (where rust can put multiple includes on a single line) and the initialization of the gpiochip and irqchip methods (which Rust gets away without needing by adding an additional indent on the definition of those methods). [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 18:04 UTC (Mon) by mathstuf (subscriber, #69389) [Link] > there be a blank line after a function's variable declarations In Rust, one can do `let` declarations at the top of the function, but I find `let result = { /* block which computes */ };` instead of `int ret; /* decls and setup code */; ret = computed_result;` and not have `ret` be `const` is *far* worse. Rust can do the latter *and* have it be `const`, but...why do the hoist? > Not to mention the Rust code mixes variable declarations in the body of the code which saves a few more lines and is also against the style guide. In Rust, this can be *very* important if the lifetime of a variable is not available until the middle of the function. Declaring it at the top of the function can make its lifetime "too long" to pass off to some narrower lifetime API. > I suspect the Rust developers would have less resistance if they followed kernel coding style more closely instead of assuming a new language allows them to define their own style. Some of the rules come from C being C. Those are fine to let lie on the floor. Determining which is which however is where the flamewars then lie... Variable declarations needing to be hoisted (and the "Christmas tree" correlary) are such rules. Indentation, extra newlines, and brace placement? I like hoisted braces for vertical savings (which allows for more spacing between "sections" of code too. Indentation is something I've come to care less about as long as it's "enough" to be able to find a matching section by eyeball. 2 is...fine, but not ideal. 4 is great. 8 is OK (though I find people's consistency for using spaces-for-alignment in tabulator-using source lacking). Tabs also break alignment in diffs (my main pet peeve with them). [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 19:06 UTC (Mon) by jezuch (subscriber, #52988) [ Link] > If the Rust code didn't ignore the style guide (or the C code was written in a similar compressed style) Rust is not ignoring the style guide, it's just using a different one (Rust's). In contrast to C and almost all languages before it, Rust has an official style guide. You deviate from it at your own peril! [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 19:15 UTC (Mon) by kay (subscriber, #1362) [Link] > But a lot of the line count difference appears to be due to style... [explanation why kernel style has more lines] OTH the rust code always use braces for if and else statements, the C code does not in case of a single instruction if or else. Nevertheless it's not the line count that counts ;-) [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 20:44 UTC (Mon) by marcH (subscriber, #57642) [ Link] > Not to mention the Rust code mixes variable declarations in the body of the code which saves a few more lines and is also against the style guide. BTW it is remarkably ignorant to believe C99 and every other programming language combined declarations and initializations for pure "code style" reasons. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 20:48 UTC (Mon) by marcH (subscriber, #57642) [ Link] > But a lot of the line count difference appears to be due to style... Plus 75 lines saved out of 425 is not the kind of difference that really matters in practice anyway, especially not considering all the other big differences that actually matter. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 22:27 UTC (Mon) by Paf (subscriber, #91811) [Link ] I would say a 20% difference is not insignificant, and it's interesting to ask why at least. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 22:40 UTC (Mon) by marcH (subscriber, #57642) [ Link] Sorry to insist but I really think a 20% lines difference has a negligible impact on most practical aspects of software engineering. If anything the number of characters would be a much better measure. But even that could be deceiving because one language or its code style could be "too terse" requiring more comments which would make the code longer in the end. > it's interesting to ask why at least. Comparing the lengths of two solutions in the same language has some value but for two totally different languages it's apples versus oranges because pretty much every language difference can affect the end total. These differences are worth discussing individually, not as an aggregate basket of disparate things. If the main discussion topic that comes up when comparing equivalent C and Rust code is a 20% difference in the number of lines then maybe it was not very useful to have them side by side. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 17:07 UTC (Mon) by jgg (subscriber, #55211) [Link ] Is all the type erasure a good idea? ... data: &Ref ... let _guard = data.lock(); Quick! Tell me if this code is now in an atomic context? Should it use GFP_KERNEL or GFP_ATOMIC? Does rust check this kind of stuff at compile time? This is not an improvement (again, what type is this, what do the values mean?): declare_id_table! { (0x00041061, 0x000fffff), } vs static const struct amba_id pl061_ids[] = { { .id = 0x00041061, .mask = 0x000fffff, }, The CONFIG_PM memory consumption optimization seems to have been lost? The trick with the drvdata is... interesting.. I wonder how that works with something like sysfs files that often will read the drvdata? The ordering of the set will be wrong. Could rust tell at compile time? [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 17:16 UTC (Mon) by pbonzini ( supporter , # 60935) [Link] > Quick! Tell me if this code is now in an atomic context? Should it use GFP_KERNEL or GFP_ATOMIC? You trade that for not having to remember, for every spinlock, whether it's the irqsave kind or the "normal" one. It's probably possible to improve on both the Rust problem (too much type erasure) and the C problem. > Does rust check this kind of stuff at compile time? In some cases, it's probably possible to pass in a guard that ensures that a given function is called with disabled IRQ or with a given spinlock taken. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 18:36 UTC (Mon) by jgg (subscriber, #55211) [Link ] The use of spinlock irqsave/irq/bh/etc is very situational depending on the design of the system. The irqsave version is supposed to be irqsave in normal processes, but just spin_lock() in the IRQ (and thus should have an acquire on an IRQ path). Further if you know you are not already in an irq disabled region you should be using just spin_lock_irq() Collapsing everything to a irqsave is a simplification, but not necessarily an improvement.. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 21:31 UTC (Mon) by pbonzini ( supporter , # 60935) [Link] In the Rust bindings, you choose at declaration time the kind of your spinlock, i.e. normal/irqsave. Then it's type safe and you won't ever have a bug because one lock/unlock pair uses the wrong call. > if you know you are not already in an irq disabled region you should be using just spin_lock_irq() That really only matters in 1% of the calls, probably. It should be possible to implement it in Rust in such a way that the type system validates it, though. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 17:45 UTC (Mon) by farnz (subscriber, #17727) [ Link] > declare_id_table! { > (0x00041061, 0x000fffff), > } This declares a table of IDs (hence the macro name); it would be a simple change to make it: declare_amba_id_table! { (id = 0x00041061, mask = 0x000fffff), } if the Rust binding authors want you to, so that it's (a) clear this is an AMBA ID table, and (b) the id and mask elements are clearly demarcated. That's a stylistic choice that Linus et al can definitely weigh in on. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 17:08 UTC (Mon) by rvolgers (subscriber, #63218) [Link] It's funny, seeing them side by side like this seems to activate the "reading C" part of my brain while looking at the Rust code. I notice I get annoyed at the lack of explicit types on variables and things like ".into()" (which likewise doesn't offer much clue as to typing, just that a conversion is happening). In C, it's relatively important to be aware of types. On one hand due to various traps related to UB and integer arithmetic, and on the other hand because there are no guard rails on most APIs. Rust really encourages you to embed restrictions in the API, so that the use site can be a lot more terse. Also with "C brain" engaged, some of the nested control flow like in `Chip::get_direction` seems weird. But objectively, I think the Rust version is quicker to read. I do notice that the indentation is 4 spaces in the Rust version. Pretty sure that's because they're using Rust defaults for formatting and also pretty sure there has been a lot of discussion about this already and I really don't want to start that again. But just mentioning it since it does make nesting look a little worse in the C case. [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 22:14 UTC (Mon) by roc (subscriber, #30627) [Link ] IDE support, e.g. rust-analyzer, will tell you the inferred type of a variable by hovering over it with your mouse. I find this incredibly useful. rust-analyzer with VSCode can also be configured to display the type annotations inline (non-editable) in the text, though I prefer not to do that. For me, this is basically the best of both worlds: I can read the types when I want to, but I don't have to write them and they're not cluttering up the code when I don't need them. But I can see why the kernel might not want to assume code readers have such capabilities available. [Reply to this comment] Machine code Posted Jul 19, 2021 19:52 UTC (Mon) by ikm (subscriber, #493) [Link] Did anyone try comparing the resulting machine code for both versions? [Reply to this comment] Machine code Posted Jul 19, 2021 20:51 UTC (Mon) by marcH (subscriber, #57642) [ Link] BTW https://diffoscope.org/ supports recursive readelf, objdump etc. in case anyone has been living under a rock :-) [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 21:31 UTC (Mon) by pm215 (subscriber, #98099) [ Link] QEMU's emulation isn't necessarily going to be much use for anything beyond basic-smoke-test, though -- although various boards have a PL061, mostly they are put there just to keep the kernel happy when it prods them as it boots up, and the GPIO lines don't get wired to anything. A few boards use them for inputs (eg the power-key on the 'virt' board), but use of outputs is much more limited (only the M-profile stellaris boards and the secure-world-only PL061 on 'virt' use it), and in fact output handling was pretty broken until I fixed some bugs a few weeks ago... [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 22:16 UTC (Mon) by roc (subscriber, #30627) [Link ] It looks to me like get_direction and a number of other methods can return ENXIO in the Rust version where no error can be returned in the C version. Is that because the C version is lacking error handling paths that should exist, or are these possible error returns in the Rust driver spurious? [Reply to this comment] A GPIO driver in Rust Posted Jul 19, 2021 22:47 UTC (Mon) by mathstuf (subscriber, #69389) [Link] I can't seem to find the `kernel::device` module to be able to tell what that `resources()` method is doing. The C call is just `return gc->gpiodev->data;`, so I imagine that the Rust API is just guarding against `NULL` here where C is assuming things. Err, I mean, the `ENXIO` is just hidden in the UB fog of a `NULL` `gc->gpiodev`, right? ;) [Reply to this comment] Copyright (c) 2021, Eklektix, Inc. Comments and public postings are copyrighted by their creators. Linux is a registered trademark of Linus Torvalds