On Sat 29 Jun 2024 at 20:40, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Mon, 24 Jun 2024 19:31:03 +0200 > Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote: > >> Add support for the HW found in most Amlogic SoC dedicated to measure >> system clocks. >> >> This drivers aims to replace the one found in >> drivers/soc/amlogic/meson-clk-measure.c with following improvements: >> >> * Access to the measurements through the IIO API: >> Easier re-use of the results in userspace and other drivers >> * Controllable scale with raw measurements >> * Higher precision with processed measurements >> >> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx> > Interesting device and the driver is pretty clean. > > Needs a new channel type though as altvoltage is in volts not hz. > > Various minor comments inline. > > Thanks, Thanks for the feedback Jonathan. Just a couple of things, David expressed concerns with having both IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED for a channel and we did not reach a conclusion on this. My idea here is to: * Give full control over the scale to the consumer with IIO_CHAN_INFO_RAW * Give an easy/convenient way to get an Hz result with IIO_CHAN_INFO_PROCESSED. There is a bit more than just 'raw * scale' in the implementation of this info to figure out the most appropriate scale for the measurement. They idea is also to avoid code duplication in consumers. David is definitely more familiar than me with IIO but we did not really know how to move forward on this. Is it OK to have both IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED, or should I ditch IIO_CHAN_INFO_RAW ? > > Jonathan [...] >> + indio_dev->name = "amlogic-clk-msr"; >> + indio_dev->info = &cmsr_info; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->num_channels = CLK_MSR_MAX; > > Superficially looks like the number of channels depends on the compatible. > Ideally we shouldn't provide channels to userspace that aren't useful. Not exactly. All SoCs have 128 inputs, Some may not be connected indeed but some are, and the name is just not documented (yet). > > You could search the name arrays to see how far they go, but that is bit ugly. > Probably wrap those in a structure with a num_channels parameter as well. > I've been doodling with this idea while writing this driver. Technically, there is no problem. The legacy driver this one will be replacing used to expose all 128 inputs. I thought it was more important to have continuity with the legacy driver than filtering out possibly useless channels. Another benefit of keeping all 128 is that the channel index (both in sysfs and more crucially in DT) matches the one in the SoC documentation. That makes things easier. Would it be acceptable to keep all 128 channels then or do you still prefer that I filter out the undocumented ones ? >> + indio_dev->channels = cmsr_populate_channels(dev, conf); >> + if (IS_ERR(indio_dev->channels)) >> + return PTR_ERR(indio_dev->channels); >> + >> + return devm_iio_device_register(dev, indio_dev); >> +} Thanks for you help. -- Jerome
