Messages in this thread | | | Date | Sun, 24 Apr 2022 15:52:50 +0530 | Subject | Re: [PATCH V4] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for SDHC | From | "Sajida Bhanu (Temp)" <> |
| |
Hi,
Thanks for the review.
Please find the inline comments.
Thanks,
Sajida
On 4/21/2022 3:26 PM, Philipp Zabel wrote: > On Do, 2022-04-21 at 10:32 +0530, Sajida Bhanu (Temp) wrote: >> Hi, >> >> Thanks for the review. >> >> Please find the inline comments. >> >> Thanks, >> >> Sajida >> >> On 4/19/2022 12:52 PM, Philipp Zabel wrote: >>> Hi Sajida, >>> >>> On Di, 2022-04-19 at 11:46 +0530, Sajida Bhanu (Temp) wrote: >>> [...] >>>>>> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host) >>>>>> +{ >>>>>> + struct reset_control *reset; >>>>>> + int ret = 0; >>>>> No need to initialize ret. >>>>> >>>>>> + >>>>>> + reset = reset_control_get_optional_exclusive(dev, NULL); >>>>>> + if (IS_ERR(reset)) >>>>>> + return dev_err_probe(dev, PTR_ERR(reset), >>>>>> + "unable to acquire core_reset\n"); >>>>>> + >>>>>> + if (!reset) >>>>>> + return ret; >>>> Here we are returning ret directly if reset is NULL , so ret >>>> initialization is required. >>> You are right. I would just "return 0;" here, but this is correct as >>> is. >> Ok >>>>>> + >>>>>> + ret = reset_control_assert(reset); >>>>>> + if (ret) >>>>>> + return dev_err_probe(dev, ret, "core_reset assert failed\n"); >>>>> Missing reset_control_put(reset) in the error path. >>>> Sure will add >>>>>> + >>>>>> + /* >>>>>> + * The hardware requirement for delay between assert/deassert >>>>>> + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to >>>>>> + * ~125us (4/32768). To be on the safe side add 200us delay. >>>>>> + */ >>>>>> + usleep_range(200, 210); >>>>>> + >>>>>> + ret = reset_control_deassert(reset); >>>>>> + if (ret) >>>>>> + return dev_err_probe(dev, ret, "core_reset deassert failed\n"); >>>>> Same as above. Maybe make both ret = dev_err_probe() and goto ... >>>> In both cases error message is different so I think goto not good idea here. >>> You could goto after the error message. Either way is fine. >> Sorry didn't get this ..can you please help > I meant you could either use goto after the error messages: > > +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host) > +{ > [...] > + ret = reset_control_assert(reset); > + if (ret) { > + dev_err_probe(dev, ret, "core_reset assert failed\n"); > + goto out_reset_put; > + } > [...] > + ret = reset_control_deassert(reset); > + if (ret) { > + dev_err_probe(dev, ret, "core_reset deassert failed\n"); > + goto out_reset_put; > + } > + > + usleep_range(200, 210); > + > +out_reset_put: > + reset_control_put(reset); > + > + return ret; > +} > > Or not use goto and copy the reset_control_put() into each error path: > > +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host) > +{ > [...] > + ret = reset_control_assert(reset); > + if (ret) { > + reset_control_put(reset); > + return dev_err_probe(dev, ret, "core_reset assert failed\n"); > + } > [...] > + ret = reset_control_deassert(reset); > + if (ret) { > + reset_control_put(reset); > + return dev_err_probe(dev, ret, "core_reset deassert failed\n"); > + } > + > + usleep_range(200, 210); > + reset_control_put(reset); > + > + return 0; > +} > > regards > Philipp Sure I will call reset_control_put() in each error path
| |