linux-kernel - Re: [PATCH v4 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

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] [day] [month] [year] [list]

Message-ID: <CAHp75Vdw5X1Y057fpGjdvVGwKq0x0UBdm8py+m+55RbzXi1PJw@mail.gmail.com>
Date: Thu, 21 Aug 2025 10:41:03 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Dixit Parmar <dixitparmar19@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.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>, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 1/2] iio: magnetometer: add support for Infineon
 TLV493D 3D Magentic sensor

On Thu, Aug 21, 2025 at 6:02 AM Dixit Parmar <dixitparmar19@...il.com> wrote:
> On Wed, Aug 20, 2025 at 05:08:38PM +0300, Andy Shevchenko wrote:

...

> > >  st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
> > >  obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
> > >  obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> > >
> > > +obj-$(CONFIG_INFINEON_TLV493D)             += tlv493d.o
> > > +
> > >  obj-$(CONFIG_SENSORS_HMC5843)              += hmc5843_core.o
> > >  obj-$(CONFIG_SENSORS_HMC5843_I2C)  += hmc5843_i2c.o
> > >  obj-$(CONFIG_SENSORS_HMC5843_SPI)  += hmc5843_spi.o
> >
> > I haven't got the ordering rules here and in Kconfig. Can it be alphabetical?
> From what I can see, the order is alphabetical based on the CONFIG option in the
> Makefile and Kconfig, and I kept CONFIG_INFINEO_TLV493D after CONFIG_IIO_ST*.
> Isn't it in correct order? or my understanding is incorrect?

I dunno, The file name there is with the vendor prefix, in many cases
the configuration option is with vendor prefix as well, but the file.

...

> > > +   switch (ch) {
> > > +   case TLV493D_AXIS_X:
> > > +           val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> > > +                   FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> >
> > Wrong indentation, make both 'F':s to be in the same column.
> To have 'F' in the same column, it will need spaces after tab (I think its not
> advisable to mix spaces and tabs).

No, mixing tabs and spaces basically means spaces inside TABs or
before TABs. The TABS followed by solely spaces is okay and the
correct way to indent.

> With just tabs the second FIELD_GET will be
> align to 'v' of val. What will be the correct indentation?

...

> > > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > > +                           s16 *z, s16 *t)
> >
> > Indentation issue. Please, check fully the code for such issues.
> I followed the single tab after default as suggested. At which column the s16 *z should be align to?

s16 starts with the same column as struct.

...

> > > +   ret = read_poll_timeout(i2c_master_recv, err, err ||
> > > +                   FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> >
> > Please, resplit logically, i.e leave the condition on the single line.
> > Also to make it shorter you can use '!' instead of ' == 0'.
> Having both conditions in same line will go out of 80 char length limit, even with !.
> Is it fine?

In _this_ case yes. I expect something like this to see

   ret = read_poll_timeout(i2c_master_recv, err,
                   err || !FIELD_GET(TLV493D_TEMP_CHANNEL,
buff[TLV493D_RD_REG_TEMP]),

...

> > Just wondering if you have tested for negative coordinates, does it propagate
> > correctly?
> Yes I have validated, it reports correct negative coordinate values.

OK!

> > > +   iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> > > +                           pf->timestamp);
> >
> > Interestingly that you have used 100 limit and suddenly don't do it here
> > and maybe elsewhere. Why inconsistent style? Please, go through the whole
> > file and make sure the style is consistent in all of the aspects:
> > - C style used
> > - comments style (one-line and multi-line)
> > - indentation
> > - etc.
> I tried to follow 80 limit(except few places where it was just on border or not
> clear to read). I belive the standard is to use 80 limit(correct me if I referred
> wrong place) and I will recheck to meet that.

There are two standards, the old and strict one -- 80 characters, and
this subsystem _tries_ to follow it and relaxed with 100 limit.
The exceptions are possible when it affects readability.

--
With Best Regards,
Andy Shevchenko