by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v4 01/15] scripts: checkpatch: Add __aligned to the list of attribute notes

On Sat, 2023-12-16 at 14:45 -0300, Marcelo Schmitt wrote:
> Checkpatch presumes attributes marked with __aligned(alignment) are part
> of a function declaration and throws a warning stating that those
> compiler attributes should have an identifier name which is not correct.
> Add __aligned compiler attributes to the list of attribute notes
> so they don't cause warnings anymore.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> Any expression that evaluates to an integer that is a power of 2 can be
> within __aligned parenthesis.
> I can't see how we could use a regex to check code meets such constraint (if possible).

You can't because if a #define is used for the alignment
value it is not necessarily available to a patch fragment
or even a file if the #define is in an #include.

> Some additional exotic uses of __aligned are:
> drivers/net/wireless/quantenna/qtnfmac/bus.h:72: char bus_priv[] __aligned(sizeof(void *));
> include/linux/netdevice.h:225:} __aligned(4 * sizeof(unsigned long));

Right, then there are random uses like:

drivers/firmware/qcom/qcom_qseecom_uefisecapp.c: size_t __aligned; \
drivers/firmware/qcom/qcom_qseecom_uefisecapp.c: *__offset = __aligned; \

so there's always some false positive/negative issue
with checkpatch.

Anyway:

Acked-by: Joe Perches <[email protected]>

>
> The regex
> __aligned\s*$.*$
> seems to match all use cases.
>
> We might not catch invalid arguments to __aligned, but it looks like making
> checkpath confidently report those wouldn't be feasible anyway.
>
> The patch that would trigger the mentioned warning in now
> patch number 13 (iio: adc: Add support for AD7091R-8).
>
> scripts/checkpatch.pl | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 25fdb7fda112..d56c98146da3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -512,6 +512,7 @@ our $Attribute = qr{
> __ro_after_init|
> __kprobes|
> $InitAttribute|
> + __aligned\s*$.*$|
> ____cacheline_aligned|
> ____cacheline_aligned_in_smp|
> ____cacheline_internodealigned_in_smp|

2023-12-16 23:55:19

2023-12-20 14:14:47

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v4 09/15] iio: adc: ad7091r: Enable internal vref if external vref is not supplied

On Mon, 18 Dec 2023 14:35:27 -0300
Marcelo Schmitt <[email protected]> wrote:

> On 12/17, Jonathan Cameron wrote:
> > On Sat, 16 Dec 2023 14:49:07 -0300
> > Marcelo Schmitt <[email protected]> wrote:
> >
> > > The ADC needs a voltage reference to work correctly.
> > > Enable AD7091R internal voltage reference if no external vref is supplied.
> > >
> > > Signed-off-by: Marcelo Schmitt <[email protected]>
> > This one sounds to me like it should have a fixes tag and be
> > much earlier in the set to perhaps simplify backports.
>
> Could be. If we stick to the fact that the dt-binding does not require a voltage
> regulator then this can be seen as a fix.
> Though, if users can provide an external reference this patch makes no
> difference them.
> I am using the internal reference for testing so having this one makes a
> difference for me.
The binding has it as optional, though usually when not having an
external reference leads to use of an internal one, we call it out
in the description.

Meh, can backport it as a fix if anyone asks for it.

Jonathan

>
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/adc/ad7091r-base.c | 7 +++++++
> > > drivers/iio/adc/ad7091r-base.h | 1 +
> > > 2 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> > > index aead72ef55b6..9d0b489966f5 100644
> > > --- a/drivers/iio/adc/ad7091r-base.c
> > > +++ b/drivers/iio/adc/ad7091r-base.c
> > > @@ -217,7 +217,14 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,
> > > if (IS_ERR(st->vref)) {
> > > if (PTR_ERR(st->vref) == -EPROBE_DEFER)
> > > return -EPROBE_DEFER;
> > > +
> > > st->vref = NULL;
> > > + /* Enable internal vref */
> > > + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> > > + AD7091R_REG_CONF_INT_VREF, BIT(0));
> > > + if (ret)
> > > + return dev_err_probe(st->dev, ret,
> > > + "Error on enable internal reference\n");
> > > } else {
> > > ret = regulator_enable(st->vref);
> > > if (ret)
> > > diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> > > index 81b8a4bbb929..9cfb362a00a4 100644
> > > --- a/drivers/iio/adc/ad7091r-base.h
> > > +++ b/drivers/iio/adc/ad7091r-base.h
> > > @@ -20,6 +20,7 @@
> > > #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
> > >
> > > /* AD7091R_REG_CONF */
> > > +#define AD7091R_REG_CONF_INT_VREF BIT(0)
> > > #define AD7091R_REG_CONF_ALERT_EN BIT(4)
> > > #define AD7091R_REG_CONF_AUTO BIT(8)
> > > #define AD7091R_REG_CONF_CMD BIT(10)
> >
> >