Messages in this thread | | | From | "Zulkifli, Muhammad Husaini" <> | Subject | RE: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC | Date | Wed, 7 Oct 2020 13:28:22 +0000 |
| |
Hi Andy,
Thanks for the feedback. I replied inline
>-----Original Message----- >From: Andy Shevchenko <andy.shevchenko@gmail.com> >Sent: Wednesday, October 7, 2020 4:56 PM >To: Michal Simek <michal.simek@xilinx.com> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >Hunter, Adrian <adrian.hunter@intel.com>; Sudeep Holla ><sudeep.holla@arm.com>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc ><linux-mmc@vger.kernel.org>; linux-arm Mailing List <linux-arm- >kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- >kernel@vger.kernel.org>; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad >Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Arnd Bergmann ><arnd@arndb.de> >Subject: Re: [PATCH v3 1/2] mmc: sdhci-of-arasan: Enable UHS-1 support for >Keem Bay SOC > >On Wed, Oct 7, 2020 at 11:38 AM Michal Simek <michal.simek@xilinx.com> >wrote: >> On 06. 10. 20 17:55, muhammad.husaini.zulkifli@intel.com wrote: > >... > >> > + /* >> > + * This is like final gatekeeper. Need to ensure >> > + changed voltage > >like a final Noted. Done the changes > >> > + * is settled before and after turn on this bit. >> > + */ > >... > >> > + /* >> > + * This is like final gatekeeper. Need to ensure >> > + changed voltage > >Likewise. Noted. Done the changes > >> > + * is settled before and after turn on this bit. >> > + */ > >... > >> > + struct device *dev = &pdev->dev; >> >> nit: I got this but as I see 3 lines below maybe would be better to >> use it everywhere but it can be done in separate patch. > >In that case I think it would be better to have that patch first. It make follow up >code cleaner. I want to get some clarification here. Do I need a separate patch for this struct device *dev = &pdev->dev;? Can I embedded together with UHS patch? > >... > >> > + if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { >> > + struct gpio_desc *uhs; >> > + >> > + uhs = devm_gpiod_get_optional(dev, "uhs", >> > + GPIOD_OUT_HIGH); >> >> I can't see change in dt binding to record uhs gpio. >> >> >> Better >> sdhci_arasan->uhs_gpio = devm_gpiod_get_optional(dev, "uhs", >> GPIOD_OUT_HIGH); >> >> then you can avoid uhs variable. > >Actually it's readability vs. additional variable. It was my suggestion to have a >variable to make readability better. >Are you insisting on this change? > >-- >With Best Regards, >Andy Shevchenko
| |