On Mon, 24 Feb 2025 08:14:23 +0200 Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote: > On 23/02/2025 18:28, Jonathan Cameron wrote: > > On Wed, 19 Feb 2025 14:30:43 +0200 > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote: > > > >> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports > >> an automatic measurement mode, with an alarm interrupt for out-of-window > >> measurements. The window is configurable for each channel. > >> > >> The I2C protocol for manual start of the measurement and data reading is > >> somewhat peculiar. It requires the master to do clock stretching after > >> sending the I2C slave-address until the slave has captured the data. > >> Needless to say this is not well suopported by the I2C controllers. > >> > >> Thus the driver does not support the BD79124's manual measurement mode > >> but implements the measurements using automatic measurement mode relying > >> on the BD79124's ability of storing latest measurements into register. > >> > >> The driver does also support configuring the threshold events for > >> detecting the out-of-window events. > >> > >> The BD79124 keeps asserting IRQ for as long as the measured voltage is > >> out of the configured window. Thus the driver masks the received event > >> for a fixed duration (1 second) when an event is handled. This prevents > >> the user-space from choking on the events > >> > >> The ADC input pins can be also configured as general purpose outputs. > >> Those pins which don't have corresponding ADC channel node in the > >> device-tree will be controllable as GPO. > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx> > >> > > Hi Matti, > > > > Some fairly superficial review follows. I'm travelling for next few weeks > > so not sure when I'll get time to take a more thorough look. > > Yeah, unfortunately people are allowed to have other life beyond the > ROHM drivers :D > Enjoy your journey(s) ;) So far so good. Hi from Shenzhen. Obligatory pilgrimage to SEG market done ;) > >> + /* Set no channels to be manually measured */ > >> + ret = regmap_write(data->map, BD79124_REG_MANUAL_CHANNELS, 0x0); > >> + if (ret) > >> + return ret; > >> + > >> + /* Set the measurement interval to 0.75 mS */ > >> + regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075); > >> + ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG, > >> + BD79124_MASK_AUTO_INTERVAL, regval); > > > > Where it doesn't make any other difference, align after ( > > > > If you are going shorter, single tab only. > > Single tab only? You mean like: > > ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG, > BD79124_MASK_AUTO_INTERVAL, regval); > > Do you prefer that even if the variable holding the return value was > longer than 8 chars? To me it looks odd if arguments on the next line > begin earlier than the function on previous line: > > longvariable = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG, > BD79124_MASK_AUTO_INTERVAL, regval); > > (Just ensuring I understood your preference). It's hard to come up with an absolute policy / preference on this but whilst I agree it looks a bit odd, I think it's easier to say one tab as 'default' choice. Obviously if it's really hideous for some reason feel free to do something else ;) Jonathan
