On Tue, 5 Jul 2022 13:01:43 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Tue, Jul 5, 2022 at 12:33 PM ChiYuan Huang <u0084500@xxxxxxxxx> wrote:
> > ChiYuan Huang <u0084500@xxxxxxxxx> 於 2022年7月5日 週二 下午5:30寫道：
>
> ...
>
> > Sorry to bother you by the personal mail.
>
> No problem asking this in ML, it's normal development process.
> Moreover it's good that others may browse mail archives and find the
> answers. Private mails won't help in that sense. Hence Cc'ing mailing
> list again.
>
> > I try to put the 'align' in timestamp member.
> > checkpatch.pl show me the below warning.
> >
> > WARNING: Missing a blank line after declarations
> > #448: FILE: drivers/iio/adc/rtq6056.c:448:
> > + u16 vals[RTQ6056_MAX_CHANNEL];
> > + int64_t timestamp __aligned(8);
> >
> > WARNING: externs should be avoided in .c files
> > #448: FILE: drivers/iio/adc/rtq6056.c:448:
> > + int64_t timestamp __aligned(8);
> >
> > total: 0 errors, 2 warnings, 651 lines checked
>
> You may run `git grep -n 'align(8)'
>
> > Does it mean that I only can put the align for the whole struct?
>
> It means that checkpatch is full of false positives.
I fixed some of those in the past by fixing checkpatch, but I guess
not all of them...
>
> Btw, use s64 as a type for timestamp as it's more common.
>
> 1. Jonathan, shan't we unify this across the IIO drivers? It seems
> they are using (besides s64): int64_t, u64.
Should all be s64. Tidying that up would be good (be careful, there
are a few hardware timestamps where the format is defined by the
hardware though!)
>
> 2. Also:
>
> drivers/iio/light/vcnl4000.c:911: u16 buffer[8] __aligned(8)
For these, what are you proposing changing? Moving the __aligned earlier?
The oddity of all this lot is that the timestamp isn't at a fixed
location. It depends on what channels are enabled. Hence we can't
enforce things via a more readable structure.
>
> drivers/iio/light/vcnl4035.c:106: u8 buffer[ALIGN(sizeof(u16),
> sizeof(s64)) + sizeof(s64)] __aligned(8);
>
> drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);
>
> // misplaced aligned()
> drivers/iio/adc/mt6360-adc.c-265- u16 values[MT6360_CHAN_MAX];
> drivers/iio/adc/mt6360-adc.c-266- int64_t timestamp;
> drivers/iio/adc/mt6360-adc.c:267: } data __aligned(8);
This one is indeed a bug.
>
> drivers/iio/adc/stm32-adc.c:250: u16
> buffer[STM32_ADC_MAX_SQ + 4] __aligned(8);
>
> drivers/iio/adc/mxs-lradc-adc.c:119: u32
> buffer[10] __aligned(8);
>
> drivers/iio/adc/ti-adc0832.c:37: u8 data[24] __aligned(8);
>
> drivers/iio/adc/ti-ads124s08.c:108: u32
> buffer[ADS124S08_MAX_CHANNELS + sizeof(s64)/sizeof(u32)] __aligned(8);
>
> drivers/iio/amplifiers/ada4250.c:304: u8 data[2] __aligned(8) = {};
>
> drivers/iio/chemical/atlas-sensor.c:95: __be32 buffer[6] __aligned(8);
>
> drivers/iio/health/afe4403.c:79: s32 buffer[8] __aligned(8);
>
> drivers/iio/health/afe4404.c:95: s32 buffer[10] __aligned(8);
>
> drivers/iio/imu/adis16475.c:107: __be16
> data[ADIS16475_MAX_SCAN_DATA] __aligned(8);
>
> drivers/iio/imu/adis16480.c:174: __be16
> data[ADIS16495_BURST_MAX_DATA] __aligned(8);
>
> drivers/iio/imu/bmi160/bmi160.h:19: __le16 buf[12] __aligned(8);
>
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c:568: u8
> iio_buff[ST_LSM6DSX_IIO_BUFF_SIZE] __aligned(8);
>
> // ??? haven't checked context
> drivers/iio/light/acpi-als.c:65: s32
> evt_buffer[ACPI_ALS_EVT_BUFFER_SIZE / sizeof(s32)] __aligned(8);
>
> drivers/iio/light/si1145.c:190: u8 buffer[24] __aligned(8);
>
> drivers/iio/magnetometer/rm3100-core.c:82: u8
> buffer[RM3100_SCAN_BYTES] __aligned(8);
>
> drivers/iio/potentiostat/lmp91000.c:75: u32 buffer[4] __aligned(8);
>
> drivers/iio/pressure/mpl3115.c:160: u8 buffer[16] __aligned(8);
>
> drivers/iio/proximity/isl29501.c:941: u32 buffer[4] __aligned(8) =
> {}; /* 1x16-bit + naturally aligned ts */
>
>
> need conversion.
>
> 3. Why do we need explicit garbage in the code? :-)
If I recall correctly, because the hardware explicitly writes garbage into
that location during the read (it probably claims it's status flags
of some such, but as far as we are concerned it's garbage).
Note there is a nice comment just above this structure explaining why :)
Jonathan
>
> drivers/iio/light/rpr0521.c-204- __le16 channels[3];
> drivers/iio/light/rpr0521.c-205- u8 garbage;
> drivers/iio/light/rpr0521.c:206: s64 ts __aligned(8);
>
