Hi Jonathan,
On 2021/8/29, 11:33 PM, "Jonathan Cameron" <jic23@xxxxxxxxxx> wrote:
On Tue, 24 Aug 2021 17:12:40 +0800
Billy Tsai <billy_tsai@xxxxxxxxxxxxxx> wrote:
>> Add the function to set the sampling rate and keep the sampling period
>> for a driver used to wait the lastest value.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
> Why move the code as well as factoring out the setter function?
> I doubt it does any harm, but I'd like to understand why you did it.
> Jonathan
>> + ret = clk_prepare_enable(data->clk_scaler->clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_add_action_or_reset(data->dev,
>> + aspeed_adc_clk_disable_unprepare,
>> + data->clk_scaler->clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
>> + if (ret)
>> + return ret;
>> +
>> ret = aspeed_adc_vref_config(indio_dev);
>> if (ret)
>> return ret;
>> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>> }
>>
>> /* Start all channels in normal mode. */
> Why move this code up?
Because the ADC clock is required when initializing the ADC device.
In our system, the clock is always on. Thus, the legacy driver won't encounter any issues.
I move the clk_prepare_enable ahead of initializing phase for making the driver probe logically closer to the hardware.
>> - ret = clk_prepare_enable(data->clk_scaler->clk);
>> - if (ret)
>> - return ret;
>> -
>> - ret = devm_add_action_or_reset(data->dev,
>> - aspeed_adc_clk_disable_unprepare,
>> - data->clk_scaler->clk);
>> - if (ret)
>> - return ret;
>> -
>> adc_engine_control_reg_val =
>> readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>> adc_engine_control_reg_val |=
Best Regards,
Billy Tsai
