Message-ID: <94930302-571a-6712-c1a2-109a04ced418@amd.com>
Date:   Tue, 21 Mar 2023 16:35:26 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Content-Language: en-US
To:     Tom Rix <trix@redhat.com>, Xu Yilun <yilun.xu@intel.com>
Cc:     mdf@kernel.org, hao.wu@intel.com, michal.simek@xilinx.com,
        nathan@kernel.org, ndesaulniers@google.com,
        linux-fpga@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
References: <20230317230617.1673923-1-trix@redhat.com>
 <c8eaefed-e1fd-e0c1-7e8f-561c20632646@amd.com>
 <ZBhqutJChvRkUsRL@yilunxu-OptiPlex-7050>
 <2dc1f25d-d621-ec04-6de5-f731f2a8bb41@redhat.com>
 <5fb2fa11-5a58-1856-7cb9-9687637d0741@redhat.com>
From:   Michal Simek <michal.simek@amd.com>
Subject: Re: [PATCH] fpga: xilinx-pr-decoupler: remove unused
 xlnx_pr_decouple_read function
In-Reply-To: <5fb2fa11-5a58-1856-7cb9-9687637d0741@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?STJ2NUtKSEJMTTNjdm45ZFk2UWtETUVGSkd4cGp1bi84cDg3cmxVRU42alYw?=
 =?utf-8?B?VnpmL2dIc0dQclVPdHgwcmgyL3pqVXRtejgwNGQvd24zeE92T0cyT05oVzhm?=
 =?utf-8?B?N013cnN0RXVPZXMzV2FHbFBPSS90OTNyVDJlWStuZ09HVEpxbVRiUjdHUjFx?=
 =?utf-8?B?aWNPeUtteGhnb2cvMmk0Z293R29ha1dqM0FhUUcwa2RxREl2QmZldm9Hemx4?=
 =?utf-8?B?WGNKWThRN3RURVlVYWZ4cm8xQmptOThNNWt4RWxGTWtHRHNLanZoNGViWGVT?=
 =?utf-8?B?RWVMNGVWcVhRTE1Mek9lOUNUSnJ2cEJqd1N2NUhmNHc1T0R6MTVtUDBuWFBz?=
 =?utf-8?B?NWY5V3FYTUJGaWhoWVBsYkkrYlNtbHVhV09GT2NPYytDSmo0dmVkeDFra3pW?=
 =?utf-8?B?NGFxaDJUdzdVblhSMDIwcTlyVUU3OUxTenBhazVvcC9KVlYzcHlNNzZZNFpo?=
 =?utf-8?B?em1CaDkzVTFWbmJqbnRvUW5xYnBNbFlhc3VwMjcwekFDRWlzZ3JkR3ZwU2pi?=
 =?utf-8?B?c2Voa3cxWHJNL1JhUGlXZnRWNHpPOHMwM1AvQStYcWlmTUtkYU5uK2h1eGVV?=
 =?utf-8?B?T0gzY1ZzQjRmeUVRd3pjQWxYbzQrM1V5czB0c1lKMkU1NXp0UkEyOVZya1Y5?=
 =?utf-8?B?VDZEdzJEbUFMbVArSXVxUFFwT0pybW52L2l4MStZYTFHajZaNjZCRlZoTk5C?=
 =?utf-8?B?NFZyaSsyV3p5U1RVS25WQmV0Vy9KUVNmcDk1c05CbkZwa0ZwWVBlYmZiM0tw?=
 =?utf-8?B?cWxGRy9IWWRTUkVrc0VwdHpuOERWRktzbStjTGpDVm83Y2FPYUt6U0xPMlNY?=
 =?utf-8?B?cU5qSzlNWVVGbFdBcHd2c2FQamFkYWUzMTlKUFlmNWJFZ082Skwvd01pL0xi?=
 =?utf-8?B?SGJuNkFHeGNhUUhiSGV2YjN1YVV4SDBxUlZqUCt4TUt0NDF2UWY5cWRYZGh0?=
 =?utf-8?B?YlRhc2xpTy9PeFkyUEdMZ0w4U21Sdmp6SlY1QnNubnROeE1zQTJUMWlHdUdY?=
 =?utf-8?B?d3dpV3lkSExIR3oydjd4NmhTWWpMMVFEcXJUTncwNitnVmgzZXBSZTdzaWI4?=
 =?utf-8?B?cGQyNFVVWC9ibGpmdmk3TDhlL3VlMHJjZWlqRDlDZ3ZyVzB2anRqbXlpVUFS?=
 =?utf-8?B?TkZrVE1zMkNTQW9heVZEWG1Zck1BU3NTZlRVZnQ0SDhEQVFJcXQwb2I1Vm9Q?=
 =?utf-8?B?cWNUZ2doNjRUZHFmdGZFRWk1RmZkTnYybEl1Z052aHBnUTV0ZCt0eEFZS1M0?=
 =?utf-8?B?ZUM3VEdTcTNmV0JVanM1Zm1DcVRsNjlVL0hicTNDTFp5bVJKbEhjRmN4b0NV?=
 =?utf-8?B?TVdZSmdFUnBkUGdOLy9RcW94YUdJclYwQ0VWZzBSMlBLZWRjRmROTk9tT2hw?=
 =?utf-8?B?aEtFYW1NN1VVamtoZW16bDdGTlhLZnZ1OWs1Qk16dlk1RGdERU95RktjSDBD?=
 =?utf-8?B?eDFuc001RFdPdjlTS2RXV1NRMkJjVGJwd0trVXVic01XMzJUMGR3QllWSHRr?=
 =?utf-8?B?MkZaMlhPYlZvWDRXWEUzM3JTRHRKbFlTQmxPSTBZRXN5QXNCWlo5MkpOWWJF?=
 =?utf-8?B?UjZCZThaQ2tEaTFUMG9DNld5NUMrVTE5Z3grWGw4a1BVRDFnampVRnFFbm52?=
 =?utf-8?B?NWJ3MkJmVmp4OHFWSmsrbGhWejQyZUI2RmVLcmQ1VDhMZVlHWHpNNWY5aFVQ?=
 =?utf-8?B?bWhPMktNZnFMR0pmMng0WnVEZWlMb21COEtOdVZnaStXc0tudzVOVXpkTmtC?=
 =?utf-8?B?L3M3d25oOENFSktuZG1LTDBFY2YzekdOOTNhcTUrYlpmMGNRcDBQS3pwUExK?=
 =?utf-8?B?dWtnYk53eTN2NGFvamo2eE9MZEdjOHN0ekRxeEx5QjhlZnVVTVVuQld1aS91?=
 =?utf-8?B?Q2FFTWJZN1lvTStLa1RCMHVEYWRYV3lycWYwNUV2dEk0UUttbnVER1dOYi9h?=
 =?utf-8?B?Tlp4dlR0TjFnSXcxOEkzMFJYY0w4bEpFWWZTUUxrak9aU2xOVGJFYXZueFFZ?=
 =?utf-8?B?Y01YOUJOTEpCNDd6dXZuK3IwOTJ2U2QvajRoNFU1c29FNGl0VnZvdW1HWWN0?=
 =?utf-8?B?Q1VCazZNQ3p1UGszdzkvL1A3djU5Y09rK1ppbzlmcnMwSXlCdXZkbDEyRERJ?=
 =?utf-8?Q?sjtrhswdf9GGTjzHL4iTmRjgo?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0c5066c9-9f0b-4943-cbd8-08db2a21ee93
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB4758.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2023 15:35:43.0507
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JK/4hNXn7R31E7Xu9TiEtP/jkAQnY/bqx9R+YHrBsH0sa62Jb2yUxu944ZdZaSMX
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB8193
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/20/23 21:05, Tom Rix wrote:
> 
> On 3/20/23 9:24 AM, Tom Rix wrote:
>>
>> On 3/20/23 7:16 AM, Xu Yilun wrote:
>>> On 2023-03-20 at 08:40:22 +0100, Michal Simek wrote:
>>>>
>>>> On 3/18/23 00:06, Tom Rix wrote:
>>>>> CAUTION: This message has originated from an External Source. Please use 
>>>>> proper judgment and caution when opening attachments, clicking links, or 
>>>>> responding to this email.
>>>>>
>>>>>
>>>>> clang with W=1 reports
>>>>> drivers/fpga/xilinx-pr-decoupler.c:37:19: error: unused function 
>>>>> 'xlnx_pr_decouple_read' [-Werror,-Wunused-function]
>>>>> static inline u32 xlnx_pr_decouple_read(const struct xlnx_pr_decoupler_data 
>>>>> *d,
>>>>>                     ^
>>>>> This static function is not used, so remove it.
>>>>>
>>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>>> ---
>>>>>    drivers/fpga/xilinx-pr-decoupler.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/fpga/xilinx-pr-decoupler.c 
>>>>> b/drivers/fpga/xilinx-pr-decoupler.c
>>>>> index 2d9c491f7be9..b6f18c07c752 100644
>>>>> --- a/drivers/fpga/xilinx-pr-decoupler.c
>>>>> +++ b/drivers/fpga/xilinx-pr-decoupler.c
>>>>> @@ -34,12 +34,6 @@ static inline void xlnx_pr_decoupler_write(struct 
>>>>> xlnx_pr_decoupler_data *d,
>>>>>           writel(val, d->io_base + offset);
>>>>>    }
>>>>>
>>>>> -static inline u32 xlnx_pr_decouple_read(const struct 
>>>>> xlnx_pr_decoupler_data *d,
>>>>> -                                       u32 offset)
>>>>> -{
>>>>> -       return readl(d->io_base + offset);
>>>>> -}
>>>>> -
>>>>>    static int xlnx_pr_decoupler_enable_set(struct fpga_bridge *bridge, bool 
>>>>> enable)
>>>>>    {
>>>>>           int err;
>>>>> -- 
>>>>> 2.27.0
>>>>>
>>>> It should be fixed like this instead.
>>>>
>>>> Thanks,
>>>> Michal
>>>>
>>>> diff --git a/drivers/fpga/xilinx-pr-decoupler.c 
>>>> b/drivers/fpga/xilinx-pr-decoupler.c
>>>> index 2d9c491f7be9..58508f44cd49 100644
>>>> --- a/drivers/fpga/xilinx-pr-decoupler.c
>>>> +++ b/drivers/fpga/xilinx-pr-decoupler.c
>>>> @@ -69,7 +69,7 @@ static int xlnx_pr_decoupler_enable_show(struct
>>>> fpga_bridge *bridge)
>>>>          if (err)
>>>>                  return err;
>>>>
>>>> -       status = readl(priv->io_base);
>>>> +       status = xlnx_pr_decouple_read(priv);
>>> OK, I'll drop the previous fix, and waiting for the new one.
>>
>> Michal,
>>
>> Will you make this change ?
> 
> Let me provide some context.
> 
> I am cleaning up about 70 similar unused functions all over the tree.
> 
> I have removed a lot of one liner wrappers that look like this.
> 
> My opinion, to be useful the wrapper needs to be used multiple places and/or do 
> something non trival otherwise we will bloat the codebase with with 5x lines of 
> code to do a simple readl.
> 
> But this is subjection. If you want this change, you should make it.

I understand your position. From my perspective when drivers use these wrappers 
around io functions they are pretty much prepared to be easily extended for 
other access method. And also it helps with debugging.

I sent the patch here.
https://lore.kernel.org/r/0381e4e8061c2fee182a104768e84feff3a82d25.1679412800.git.michal.simek@amd.com

Thanks,
Michal