linux-kernel - Re: [PATCH v4 2/2] staging: iio: frequency: ad9832: Refactor powerdown control

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfnAkgP7sPs2E5i2hjcNib96mjyXz-w=jgFzF7F839yTw@mail.gmail.com>
Date: Fri, 25 Apr 2025 07:25:03 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Gabriel Shahrouzi <gshahrouzi@...il.com>
Cc: andy@...nel.org, dlechner@...libre.com, gregkh@...uxfoundation.org, 
	jic23@...nel.org, lars@...afoo.de, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev, 
	Michael.Hennerich@...log.com, nuno.sa@...log.com, skhan@...uxfoundation.org, 
	linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH v4 2/2] staging: iio: frequency: ad9832: Refactor
 powerdown control

On Fri, Apr 25, 2025 at 1:34 AM Gabriel Shahrouzi <gshahrouzi@...il.com> wrote:
>
> Replace custom implementation with out_altvoltage_powerdown ABI. The
> attribute's logic is inverted (1 now enables powerdown) to match the
> standard.


> +static ssize_t ad9832_write_powerdown(struct iio_dev *indio_dev, uintptr_t private,
> +                                     const struct iio_chan_spec *chan,
> +                                     const char *buf, size_t len)
> +{
> +       struct ad9832_state *st = iio_priv(indio_dev);
> +       int ret;
> +       bool val;
> +
> +       ret = kstrtobool(buf, &val);
> +       if (ret)
> +               return ret;
> +
> +       guard(mutex)(&st->lock);

> +       if (val)
> +               st->ctrl_src |= AD9832_SLEEP;
> +       else
> +               st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> +                                AD9832_CLR);

Why out of a sudden this went backwards in style and format (no
FIELD_PREP(), two lines, ...)?

> +
> +       st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
> +                                         st->ctrl_src);
> +       ret = spi_sync(st->spi, &st->msg);
> +
> +       return ret ? ret : len;
> +}

Use Elvis?

...

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);

This should be done in a separate patch for all locking.

...

> -                       st->ctrl_src |= FIELD_PREP(AD9832_SLEEP, 1);

Btw, I do not see this in v6.15-tcX, is this part of Linux Next?

...


>         &iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
>         &iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
>         &iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
> -       &iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,

>         NULL,

Also drop the trailing comma at some point.

...

> +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> +       {
> +               .name = "powerdown",
> +               .shared = IIO_SEPARATE,
> +               .write = ad9832_write_powerdown,
> +       },

> +       { },

Ditto.

> +};

-- 
With Best Regards,
Andy Shevchenko