Hi Billy,
On Mon, 2 Oct 2023, at 19:20, Billy Tsai wrote:
> On Mon, 25 Sep 2023, at 17:48, Billy Tsai wrote:
>> > Create event sysfs for applying the deglitch condition. When
>> > in_voltageY_thresh_rising_en/in_voltageY_thresh_falling_en is set to true,
>> > the driver will use the in_voltageY_thresh_rising_value and
>> > in_voltageY_thresh_falling_value as threshold values. If the ADC value
>> > falls outside this threshold, the driver will wait for the ADC sampling
>> > period and perform an additional read once to achieve the deglitching
>> > purpose.
>> >
>> > Signed-off-by: Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
>> > ---
>> > drivers/iio/adc/aspeed_adc.c | 193 ++++++++++++++++++++++++++++++++++-
>> > 1 file changed, 189 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
>> > index 998e8bcc06e1..9e746c81d916 100644
>> > --- a/drivers/iio/adc/aspeed_adc.c
>> > +++ b/drivers/iio/adc/aspeed_adc.c
>> > @@ -95,6 +95,7 @@ struct aspeed_adc_model_data {
>> > bool wait_init_sequence;
>> > bool need_prescaler;
>> > bool bat_sense_sup;
>> > + bool require_extra_eoc;
>
>> What is "eoc"? Can we use a better name or add an explanatory comment?
>
> Hi Andrew,
> This is the signal name for our ADC controller, it means "End Of
> Conversion".
> The appearance of this signal period indicates that the ADC value is
> valid and being updated to the register.
Okay, searching for "conversion" in the datasheet didn't turn up anything like this. It seems I wasn't off-track with asking. If you go forward with the patch in some manner, can you add a comment to the code documenting the meaning of 'eoc'?
>
>> > /* Register ADC clock prescaler with source specified by device tree.
>> > */
>> > spin_lock_init(&data->clk_lock);
>> > snprintf(clk_parent_name, ARRAY_SIZE(clk_parent_name), "%s",
>> > @@ -632,7 +806,14 @@ static int aspeed_adc_probe(struct platform_device
>> > *pdev)
>> > adc_engine_control_reg_val |= ASPEED_ADC_CTRL_CHANNEL;
>> > writel(adc_engine_control_reg_val,
>> > data->base + ASPEED_REG_ENGINE_CONTROL);
>> > -
>> > + adc_engine_control_reg_val =
>> > + FIELD_GET(ASPEED_ADC_CTRL_CHANNEL,
>> > + readl(data->base + ASPEED_REG_ENGINE_CONTROL));
>> > + data->required_eoc_num = hweight_long(adc_engine_control_reg_val);
>> > + if (data->model_data->require_extra_eoc &&
>> > + (adc_engine_control_reg_val &
>> > + BIT(data->model_data->num_channels - 1)))
>> > + data->required_eoc_num += 12;
>
>> Why 12? Why add a value to the number of engines enabled? Have I misunderstood?
>
> This behavior is specified by the hardware. In our ADC, it requires 12
> dummy sampling
> periods to switch the sampling channel from CH7 to CH0. Hence, this
> condition checks
> the enable status of channel 7 to determine the necessary delay period
> for obtaining the
> updated ADC values for each channel.
Okay, I feel using a magic value '12' with what you wrote above as an explanation is asking a bit much of the reader. Again, if you go forward with this patch in some fashion, can you document the meaning of 12 in a comment (and possibly use a macro to name it)?
Cheers,
Andrew
