| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aLglJoqBDap_eMIj@smile.fi.intel.com>
Date: Wed, 3 Sep 2025 14:23:18 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Marcelo Schmitt <marcelo.schmitt@...log.com>,
Javier Carrasco <javier.carrasco.cruz@...il.com>,
Tobias Sperling <tobias.sperling@...ting.com>,
Antoniu Miclaus <antoniu.miclaus@...log.com>,
Trevor Gamblin <tgamblin@...libre.com>,
Esteban Blanc <eblanc@...libre.com>,
Ramona Alexandra Nechita <ramona.nechita@...log.com>,
Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
Hans de Goede <hansg@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:
> On 02/09/2025 17:15, Andy Shevchenko wrote:
> > On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
> > > + unsigned int vref_mv;
> >
> > Perhaps _mV to follow the actual unit spelling?
> > (and yes, I know that both variants are present in the kernel)
>
> The 'mv' is a deliberate choise. I still remember our previous discussion
> about this same topic [1].
Rings a bell. And I still think the proper spelling is more important when
we talk about physics.
...
> TLDR; I can combine the conditions in one if (), thanks.
Works for me.
...
> > > +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
> > > + int dir)
> > > +{
> > > + unsigned int set_reg, clear_reg, bit;
> > > + int ret;
> > > +
> > > + bit = GET_GPIO_BIT(offset);
> > > +
> > > + if (dir == GPIO_LINE_DIRECTION_IN) {
> > > + set_reg = GET_GPI_EN_REG(offset);
> > > + clear_reg = GET_GPO_EN_REG(offset);
> > > + } else {
> > > + set_reg = GET_GPO_EN_REG(offset);
> > > + clear_reg = GET_GPI_EN_REG(offset);
> > > + }
> >
> > > + ret = regmap_set_bits(data->map, set_reg, bit);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regmap_clear_bits(data->map, clear_reg, bit);
> >
> > I believe the order depends on the out-in or in-out switch.
> > Otherwise it might be potential glitches on input (hw) buffer.
> > Right now when it's not an interrupt it may be okay to don't
> > bother, but in general I see a potential issues with that.
>
> Can you please explain what you mean. I am not sure I can follow you here.
If we set out first with enabled input, input can get a value from the output.
If the value before was different, it will be an event condition (of course
it all depends on the HW and I haven't read datasheet for this one). So when
1) turn out on
2) ...possible event condition... (if external bias, or similar)
3) turn in off
Compare to
1) turn in off
2) ...possible glitch on the wire... (if no external bias, or similar)
3) turn out on
> > > +}
...
> > > +static int bd79112_probe(struct spi_device *spi)
> > > +{
> > > + /* ADC channels as named in the data-sheet */
> > > + static const char * const chan_names[] = {
> > > + "AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
> > > + "AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
> > > + "AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
> > > + "AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
> > > + "AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
> > > + "AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
> >
> > Can you make all of the lines to be the same in terms of amount of entries?
>
> Maybe :) I would like to know why? As you know, I prefer to keep lines short
> to fit multiple terminals in parallel, so this will probably make the entry
> to consume more rows. Thus, I would like to have a solid reason.
Sure, the array above is unindexed. It's prone to errors and typos.
Moreover, it's really hard to follow in case one needs to debug such
a typo and see which value needs to be fixed (imagine you typed twice
the same name).
Recommended way is to use power-of-two per line (and even add a comment
at the end), like
static const char * const chan_names[] = {
"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", /* 0 - 3 */
"AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A", /* 4 - 7 */
"AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A", /* 8 - 11 */
...
(or hexadecimal offsets, whatever is better and more in accordance with
the SW / data sheet).
> > > + };
...
> > > + data->vref_mv = ret / 1000;
> >
> > (MICRO / MILLI)
>
> I find this much more confusing than plain 1000. (I know we had this type of
> discussion before. See [1] again).
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a
compromise between creating a new bunch of unit (V) related definitions.
> > > + ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
> > > + BD79112_MAX_NUM_CHANNELS - 1, &cs);
> >
> > Hmm... Indentation can be amended.
>
> Sorry but I am not sure I understand what you mean by amended? Can you
> please go an extra mile and explain :)
ret = devm_iio_adc_device_alloc_chaninfo_se(dev,
&bd79112_chan_template,
BD79112_MAX_NUM_CHANNELS - 1,
&cs);
...
> > > + gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > > + iio_dev->num_channels);
> >
> > > +
> >
> > Instead of leaving this rather unneeded blank line I would move above...
> >
> > > + /* We're done if all channels are reserved for ADC. */
> >
> > ...to be here
> >
> > gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> > iio_dev->num_channels);
>
> I suppose you mean something like:
>
> register_gpios:
> /* We're done if all channels are reserved for ADC. */
> gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> iio_dev->num_channels);
> if (!gpio_pins)
> return 0;
>
> right?
Yes.
> I don't like this because now the comment suggests we do call
> bd79112_get_gpio_pins() only to see if all channels were for ADCs. This,
> however, is not THE reason for this call, only an optimization. I think:
> having:
>
> /* We're done if all channels are reserved for ADC. */
Then you can amend the comment
/* If all channels are reserved for ADC, we are done. */
> if (!gpio_pins)
> return 0;
>
> is clearer.
Which makes my approach sustainable.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists