From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH 3/6] soc/tegra: pmc: Add support for IO pads power state and voltage Date: Tue, 3 May 2016 13:55:17 +0100 Message-ID: <57289FB5.5040705@nvidia.com> References: <1462191434-28933-1-git-send-email-ldewangan@nvidia.com> <1462191434-28933-4-git-send-email-ldewangan@nvidia.com> <57289AC0.4090604@nvidia.com> <57289A2B.7040501@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <57289A2B.7040501-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Laxman Dewangan , swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org, linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-gpio@vger.kernel.org On 03/05/16 13:31, Laxman Dewangan wrote: > > On Tuesday 03 May 2016 06:04 PM, Jon Hunter wrote: >> On 02/05/16 13:17, Laxman Dewangan wrote: >>> + >>> + return tegra_io_rail_power_on(dpd_bit); >> From a readability standpoint the above seems weird because >> tegra_io_pads_power_enable() takes an ID as the argument, translates it >> to a bit value and passes it to tegra_io_rail_power_on() which also >> takes an ID for the argument. > > Yaah, I did not want to duplicate the implementation and hence it is there. > We will get rid of the older APIs once this is IN and new mechanism there. > > > >>> + >>> + bval = (io_volt_uv == 3300000) ? BIT(pwr_bit) : 0; >>> + >>> + mutex_lock(&pmc->powergates_lock); >>> + tegra_pmc_read_modify_write(PMC_PWR_DET, BIT(pwr_bit), >>> BIT(pwr_bit)); >>> + tegra_pmc_read_modify_write(PMC_PWR_DET_VAL, BIT(pwr_bit), bval); >>> + mutex_unlock(&pmc->powergates_lock); >> There are 4 instances of BIT(pwr_bit). May be we should do this once or >> have tegra_io_pads_to_power_val() return the bit? > > OK, will re-write this part. > > >> >>> +int tegra_io_pads_power_disable(int io_pad_id); >>> +int tegra_io_pads_power_is_enabled(int io_pad_id); >> What I don't like here, is now we have two public APIs to do the same >> job because tegra_io_pads_power_enable/disable() calls >> tegra_io_rail_power_off/on() internally. Furthermore, the two APIs use >> different ID definitions to accomplish the same job. This shouldn't be >> necessary. > > Currently SOR driver is using the tegra_io_rail_power_off/on() APIs. > Once the proper interface available then I will move sor driver to use > new method and then we can full get rid of older APIs and macros. > > Till that, we need to have this. I prefer it is done before this series. In other words, if we need a proper enum for the rail/pad IDs then add one and convert any existing drivers over to use any new APIs first. The other option is to live with the existing API names. Jon From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933392AbcECMz2 (ORCPT ); Tue, 3 May 2016 08:55:28 -0400 Received: from hqemgate16.nvidia.com ([216.228.121.65]:18392 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932687AbcECMzZ (ORCPT ); Tue, 3 May 2016 08:55:25 -0400 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Tue, 03 May 2016 05:54:23 -0700 Subject: Re: [PATCH 3/6] soc/tegra: pmc: Add support for IO pads power state and voltage To: Laxman Dewangan , , , , , , References: <1462191434-28933-1-git-send-email-ldewangan@nvidia.com> <1462191434-28933-4-git-send-email-ldewangan@nvidia.com> <57289AC0.4090604@nvidia.com> <57289A2B.7040501@nvidia.com> CC: , , , From: Jon Hunter Message-ID: <57289FB5.5040705@nvidia.com> Date: Tue, 3 May 2016 13:55:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <57289A2B.7040501@nvidia.com> X-Originating-IP: [10.21.132.133] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/05/16 13:31, Laxman Dewangan wrote: > > On Tuesday 03 May 2016 06:04 PM, Jon Hunter wrote: >> On 02/05/16 13:17, Laxman Dewangan wrote: >>> + >>> + return tegra_io_rail_power_on(dpd_bit); >> From a readability standpoint the above seems weird because >> tegra_io_pads_power_enable() takes an ID as the argument, translates it >> to a bit value and passes it to tegra_io_rail_power_on() which also >> takes an ID for the argument. > > Yaah, I did not want to duplicate the implementation and hence it is there. > We will get rid of the older APIs once this is IN and new mechanism there. > > > >>> + >>> + bval = (io_volt_uv == 3300000) ? BIT(pwr_bit) : 0; >>> + >>> + mutex_lock(&pmc->powergates_lock); >>> + tegra_pmc_read_modify_write(PMC_PWR_DET, BIT(pwr_bit), >>> BIT(pwr_bit)); >>> + tegra_pmc_read_modify_write(PMC_PWR_DET_VAL, BIT(pwr_bit), bval); >>> + mutex_unlock(&pmc->powergates_lock); >> There are 4 instances of BIT(pwr_bit). May be we should do this once or >> have tegra_io_pads_to_power_val() return the bit? > > OK, will re-write this part. > > >> >>> +int tegra_io_pads_power_disable(int io_pad_id); >>> +int tegra_io_pads_power_is_enabled(int io_pad_id); >> What I don't like here, is now we have two public APIs to do the same >> job because tegra_io_pads_power_enable/disable() calls >> tegra_io_rail_power_off/on() internally. Furthermore, the two APIs use >> different ID definitions to accomplish the same job. This shouldn't be >> necessary. > > Currently SOR driver is using the tegra_io_rail_power_off/on() APIs. > Once the proper interface available then I will move sor driver to use > new method and then we can full get rid of older APIs and macros. > > Till that, we need to have this. I prefer it is done before this series. In other words, if we need a proper enum for the rail/pad IDs then add one and convert any existing drivers over to use any new APIs first. The other option is to live with the existing API names. Jon