Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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, &reg);
>>>> +	else
>>>> +		clear_bit(bit, &reg);
>>>> +	iowrite32(reg, dev->mmio_cfg.addr);
>>>
>>> Ditto (bitops and related things).
>>>
>>>> +}
>