On Sun, 17 May 2026 19:37:50 +0100 Rodrigo Alencar via B4 Relay <[email protected]> wrote:
> From: Rodrigo Alencar <[email protected]> > > Add the core AD9910 DDS driver infrastructure with single tone mode > support. This includes SPI register access, profile management via GPIO > pins, PLL/DAC configuration from firmware properties, and single tone > frequency/phase/amplitude control through IIO attributes. > > Signed-off-by: Rodrigo Alencar <[email protected]> Hi Rodrigo A couple of potential nice to haves. Jonathan > + > +static int ad9910_parse_fw(struct ad9910_state *st) > +{ > + static const char * const refclk_out_drv0[] = { > + "disabled", "low", "medium", "high", > + }; > + struct device *dev = &st->spi->dev; > + u32 tmp[2]; > + int ret; > + > + st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable"); > + if (st->data.pll_enabled) { > + tmp[0] = AD9910_ICP_MIN_uA; > + device_property_read_u32(dev, > "adi,charge-pump-current-microamp", &tmp[0]); Might be a good idea to move to the pattern that seems to be becoming the preferred way to do this and do if (device_property_present()) { ret = device_property_read_u32()... ... } else { ... } That is slightly nicer ad picks up malformed DT. I know I was the advocate for the set a default and don't check ret but I'm learning! > + if (tmp[0] < AD9910_ICP_MIN_uA || tmp[0] > AD9910_ICP_MAX_uA) > + return dev_err_probe(dev, -ERANGE, > + "invalid charge pump current > %u\n", tmp[0]); > + st->data.pll_charge_pump_current = tmp[0]; > + > + ret = device_property_match_property_string(dev, > + > "adi,refclk-out-drive-strength", > + refclk_out_drv0, > + > ARRAY_SIZE(refclk_out_drv0)); > + if (ret < 0) Similarly good to know if failure to match actually means wasn't there or not. > + st->data.refclk_out_drv = > AD9910_REFCLK_OUT_DRV_DISABLED; > + else > + st->data.refclk_out_drv = ret; > + } > + > + tmp[1] = AD9910_DAC_IOUT_DEFAULT_uA; And similar again. > + device_property_read_u32_array(dev, "output-range-microamp", tmp, > + ARRAY_SIZE(tmp)); > + if (tmp[1] < AD9910_DAC_IOUT_MIN_uA || tmp[1] > AD9910_DAC_IOUT_MAX_uA) > + return dev_err_probe(dev, -ERANGE, > + "Invalid DAC output current %u uA\n", > tmp[1]); > + st->data.dac_output_current = tmp[1]; > + > + return 0; > +} > +static const struct spi_device_id ad9910_id[] = { > + { "ad9910" }, Request to simplify what Uwe is busy doing (assuming he'll get to spi at somepoint). Please use a named initializer like we always do for of_device_id. > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ad9910_id);
