On 9/18/2024 15:28, Andy Shevchenko wrote:
> On Tue, Sep 17, 2024 at 11:47:00PM +0530, Shyam Sundar S K wrote:
>> On 9/14/2024 00:38, Andy Shevchenko wrote:
>>> On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:
>
> ...
>
>>>> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
>>>
>>> I didn't get the naming, the above using IO port with _bits, and this is MMIO
>>> with _bytes. Are you sure the naming schema is correct?
>>
>> Thinking to merge both the functions into one, something like this:
>>
>> enum io_type {
>> IO_PORT,
>> MMIO
>> };
>>
>> static void amd_asf_update_target(struct amd_asf_dev *dev, enum
>> io_type type, u8 bit, bool set)
>> {
>>
>> ...
>>
>> }
>
> I'm not talking about merging them (and merged variant seems less readable
> to me), but about naming. I.o.w. it's not obvious what the difference _bits vs.
> _bytes.
Alright. I will stop merging them. I'll rename them as follows:
/* updates target using memory-mapped I/O */
amd_asf_update_mmio_target(...)
/* updates target using I/O port access */
amd_asf_update_ioport_target(...)
Thanks,
Shyam
>
>>>> +{
>>>> + unsigned long reg;
>>>> +
>>>> + reg = ioread32(dev->mmio_cfg.addr);
>>>> + if (set)
>>>> + set_bit(bit, ®);
>>>> + else
>>>> + clear_bit(bit, ®);
>>>> + iowrite32(reg, dev->mmio_cfg.addr);
>>>
>>> Ditto (bitops and related things).
>>>
>>>> +}
>
