Hi Andy,
On 6/28/2017 4:58 AM, Andy Shevchenko wrote:
> On Wed, Jun 28, 2017 at 3:46 AM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
>>
>> echo dma0chan0 > /sys/module/dmatest/parameters/channel
>> echo 2 > /sys/module/dmatest/parameters/dmatest
>> echo 2000 > /sys/module/dmatest/parameters/timeout
>> echo 10 > /sys/module/dmatest/parameters/iterations
>> echo 1 > /sys/module/dmatest/parameters/run
>
> Good someone is developing tests!
> My comments below.
Yeah, no problem.
>
>> static void dmatest_init_srcs(u8 **bufs, unsigned int start, unsigned int len,
>> + unsigned int buf_size, bool is_memset)
>> {
>> unsigned int i;
>> u8 *buf;
>>
>> for (; (buf = *bufs); bufs++) {
>> + for (i = 0; i < start; i++) {
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> + buf[i] = PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> buf[i] = PATTERN_SRC | PATTERN_COPY
>> + | (~val & PATTERN_COUNT_MASK);
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> + buf[i] = PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
>
> Looks like a candidate now for a helper function
>
> static inline u8 gen_src_value(u8 index, bool is_memset)
> {
> u8 val = is_memset ? PATTERN_MEMSET_IDX : i;
> return PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
> }
>
> buf[i] = gen_src_value(i, is_memset);
> buf[i] = gen_src_value(i, is_memset) | PATTERN_COPY;
> buf[i] = gen_src_value(i, is_memset);
will change as you recommended.
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> + buf[i] = PATTERN_DST | (~val & PATTERN_COUNT_MASK);
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> buf[i] = PATTERN_DST | PATTERN_OVERWRITE
>> + | (~val & PATTERN_COUNT_MASK);
>
>> + unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> + buf[i] = PATTERN_DST | (~val & PATTERN_COUNT_MASK);
>
> Ditto for DST.
done.
>
>> + unsigned int counter, bool is_srcbuf, bool is_memset)
>
> This is a signal to replace it with
>
> unsigned int flags
>
> #define DMATEST_FL_SRCBUF BIT(0)
> #define DMATEST_FL_MEMSET BIT(1)
>
> or alike.
>
> I'm not insisting, just consider an opportunity.
OK. I'm not touching it for the moment. Maybe, we can have another patch later
to get rid of flags.
>
>> + bool is_srcbuf, bool is_memset)
>
> Ditto.
>
>> start = ktime_get();
>> +
>> pr_debug("%s: verifying source buffer...\n", current->comm);
>
> The change doesn't belong to the patch.
>
>> +
>> error_count += dmatest_verify(thread->dsts, dst_off + len,
>
> Ditto.
>
Posted V2 with the change now.
Sinan
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
