Re: [PATCH v2 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver

[Marek Vasut <marek.vasut@gmail.com>]

On 12/07/2018 08:24 AM, masonccyang@mxic.com.tw wrote:
> 
> Hi Marek,

Hi,

>> >> >> > +
>> >> >> > +         regmap_write(rpc->regmap, RPC_SMWDR0,
>> >> >> > +                 *(u32 *)(tx_buf + pos));
>> >> >>
>> >> >> *(u32 *) cast is probably not needed , fix casts globally.
>> >> >
>> >> > It must have it!
>> >>
>> >> Why ?
>> >
>> > Get a compiler warning due to tx_bug is void *, as Geert replied.
>>
>> The compiler warning is usually an indication that this is something to
>> check, not silence with a type cast.
>>
>> > Using get_unaligned(), patched code would be
>> > -----------------------------------------------------
>> > regmap_write(rpc->regmap, RPC_SMWDR0,
>> >                  get_unaligned((u32 *)(tx_buf + pos)));                
>> > ----------------------------------------------------
>>
>> Do you need the cast if you use get_unaligned() ?
> 
> yes!

Why ? (you can drop one iteration here by explaining why right away,
since I'll ask for that explanation for sure)

>> >> >> > +
>> >> >> > +static const struct regmap_range rpc_spi_volatile_ranges[] = {
>> >> >> > +   regmap_reg_range(RPC_SMRDR0, RPC_SMRDR0),
>> >> >> > +   regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0),
>> >> >>
>> >> >> Why is SMWDR volatile ?
>> >> >
>> >> > No matter is write-back or write-through.
>> >>
>> >> Oh, do you want to assure the SMWDR value is always written directly to
>> >> the hardware ?
>> >
>> > yes,
>> >
>> >>
>> >> btw. I think SMRDR should be precious ?
>> >
>> > so, you think I should drop
>> > regmap_reg_range(RPC_SMWDR0, RPC_SMWDR0)
>>
>> No, I am asking whether SMRDR should be marked precious or not.
>>
> 
> I don't think so,
> precious_reg: the register should not be read outside of
> call from driver, i.e,. a clear on read interrupt status register.

If you read the reg outside of the call from driver, it will mess up the
internal FIFO and the whole driver will get into undefined state, right?
Maybe Mark can chime in ?

>> [...]
>>
>> > CONFIDENTIALITY NOTE:
>> >
>> > This e-mail and any attachments may contain confidential information
>> > and/or personal data, which is protected by applicable laws. Please be
>> > reminded that duplication, disclosure, distribution, or use of this
>> > e-mail (and/or its attachments) or any part thereof is prohibited. If
>> > you receive this e-mail in error, please notify us immediately and
>> > delete this mail as well as its attachment(s) from your system. In
>> > addition, please be informed that collection, processing, and/or use of
>> > personal data is prohibited unless expressly permitted by personal data
>> > protection laws. Thank you for your attention and cooperation.
>> >
>> > Macronix International Co., Ltd.
>>
>> Can you do something about this ^ please ?
>>
> 
> I submit the patch file is by another remote git-server, which
> supports git-format patch, git send-mail and so on.
> But it can't receive email.
> 
> I get/send email are by office PC w/ Notes system and
> this "CONFx NOTE:" is appended automatically by Notes mail-server.
> 
> Please just never mind it.

I am concerned MXIC can sue everyone on the CC list of this email based
solely on your suggestion to ignore this paragraph. That's not a good
position to be in.

-- 
Best regards,
Marek Vasut