| Message ID | 20231122074741.154228-1-gnstark@salutedevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] iio: adc: meson: add separate config for axg SoC family | expand |
On Wed, 22 Nov 2023 10:47:41 +0300 George Stark <gnstark@salutedevices.com> wrote: > According to Amlogic custom kernels ADC of axg SoC family has > vref_select and requires this setting to work nominally and thus > needs a separate config. > > Signed-off-by: George Stark <gnstark@salutedevices.com> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> Is this a fix? If so, please provide a fixes tag. Or is it aligning with the vendor kernels and we don't know if it's strictly necessary or not? > --- > drivers/iio/adc/meson_saradc.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > index db280da9edbf..34555a85f131 100644 > --- a/drivers/iio/adc/meson_saradc.c > +++ b/drivers/iio/adc/meson_saradc.c > @@ -1242,6 +1242,20 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = { > .cmv_select = 1, > }; > > +static const struct meson_sar_adc_param meson_sar_adc_axg_param = { > + .has_bl30_integration = true, > + .clock_rate = 1200000, > + .bandgap_reg = MESON_SAR_ADC_REG11, > + .regmap_config = &meson_sar_adc_regmap_config_gxbb, > + .resolution = 12, > + .disable_ring_counter = 1, > + .has_reg11 = true, > + .vref_volatge = 1, > + .has_vref_select = true, > + .vref_select = VREF_VDDA, > + .cmv_select = 1, > +}; > + > static const struct meson_sar_adc_param meson_sar_adc_g12a_param = { > .has_bl30_integration = false, > .clock_rate = 1200000, > @@ -1286,7 +1300,7 @@ static const struct meson_sar_adc_data meson_sar_adc_gxm_data = { > }; > > static const struct meson_sar_adc_data meson_sar_adc_axg_data = { > - .param = &meson_sar_adc_gxl_param, > + .param = &meson_sar_adc_axg_param, > .name = "meson-axg-saradc", > }; >
Hello Jonathan Thanks for the review. On 11/25/23 20:25, Jonathan Cameron wrote: > On Wed, 22 Nov 2023 10:47:41 +0300 > George Stark <gnstark@salutedevices.com> wrote: > >> According to Amlogic custom kernels ADC of axg SoC family has >> vref_select and requires this setting to work nominally and thus >> needs a separate config. >> >> Signed-off-by: George Stark <gnstark@salutedevices.com> >> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > > Is this a fix? If so, please provide a fixes tag. Actually yes, these changes could be done in the patch that introduced new configuration parameters. > Or is it aligning with the vendor kernels and we don't know if > it's strictly necessary or not? I do know it's necessary if we want kernel driver to be independent from ADC initialization in vendor u-boot code. I have Amlogic a113x device (axg family) and can provide test results w and w/o this patch. > >> --- >> drivers/iio/adc/meson_saradc.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c >> index db280da9edbf..34555a85f131 100644 >> --- a/drivers/iio/adc/meson_saradc.c >> +++ b/drivers/iio/adc/meson_saradc.c >> @@ -1242,6 +1242,20 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = { >> .cmv_select = 1, >> }; >> >> +static const struct meson_sar_adc_param meson_sar_adc_axg_param = { >> + .has_bl30_integration = true, >> + .clock_rate = 1200000, >> + .bandgap_reg = MESON_SAR_ADC_REG11, >> + .regmap_config = &meson_sar_adc_regmap_config_gxbb, >> + .resolution = 12, >> + .disable_ring_counter = 1, >> + .has_reg11 = true, >> + .vref_volatge = 1, >> + .has_vref_select = true, >> + .vref_select = VREF_VDDA, >> + .cmv_select = 1, >> +}; >> + >> static const struct meson_sar_adc_param meson_sar_adc_g12a_param = { >> .has_bl30_integration = false, >> .clock_rate = 1200000, >> @@ -1286,7 +1300,7 @@ static const struct meson_sar_adc_data meson_sar_adc_gxm_data = { >> }; >> >> static const struct meson_sar_adc_data meson_sar_adc_axg_data = { >> - .param = &meson_sar_adc_gxl_param, >> + .param = &meson_sar_adc_axg_param, >> .name = "meson-axg-saradc", >> }; >> >
On Mon, 27 Nov 2023 22:17:33 +0300 George Stark <gnstark@salutedevices.com> wrote: > Hello Jonathan > > Thanks for the review. > > On 11/25/23 20:25, Jonathan Cameron wrote: > > On Wed, 22 Nov 2023 10:47:41 +0300 > > George Stark <gnstark@salutedevices.com> wrote: > > > >> According to Amlogic custom kernels ADC of axg SoC family has > >> vref_select and requires this setting to work nominally and thus > >> needs a separate config. > >> > >> Signed-off-by: George Stark <gnstark@salutedevices.com> > >> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > Is this a fix? If so, please provide a fixes tag. > Actually yes, these changes could be done in the patch that introduced > new configuration parameters. > > > Or is it aligning with the vendor kernels and we don't know if > > it's strictly necessary or not? > I do know it's necessary if we want kernel driver to be independent from > ADC initialization in vendor u-boot code. I have Amlogic a113x device > (axg family) and can provide test results w and w/o this patch. Definitely best to not rely on firmware. So send a Fixes tag for this and I'll apply it as a fix (you get to figure out which patch it fixed :) Jonathan > > > > >> --- > >> drivers/iio/adc/meson_saradc.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > >> index db280da9edbf..34555a85f131 100644 > >> --- a/drivers/iio/adc/meson_saradc.c > >> +++ b/drivers/iio/adc/meson_saradc.c > >> @@ -1242,6 +1242,20 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = { > >> .cmv_select = 1, > >> }; > >> > >> +static const struct meson_sar_adc_param meson_sar_adc_axg_param = { > >> + .has_bl30_integration = true, > >> + .clock_rate = 1200000, > >> + .bandgap_reg = MESON_SAR_ADC_REG11, > >> + .regmap_config = &meson_sar_adc_regmap_config_gxbb, > >> + .resolution = 12, > >> + .disable_ring_counter = 1, > >> + .has_reg11 = true, > >> + .vref_volatge = 1, > >> + .has_vref_select = true, > >> + .vref_select = VREF_VDDA, > >> + .cmv_select = 1, > >> +}; > >> + > >> static const struct meson_sar_adc_param meson_sar_adc_g12a_param = { > >> .has_bl30_integration = false, > >> .clock_rate = 1200000, > >> @@ -1286,7 +1300,7 @@ static const struct meson_sar_adc_data meson_sar_adc_gxm_data = { > >> }; > >> > >> static const struct meson_sar_adc_data meson_sar_adc_axg_data = { > >> - .param = &meson_sar_adc_gxl_param, > >> + .param = &meson_sar_adc_axg_param, > >> .name = "meson-axg-saradc", > >> }; > >> > > >
Hello Jonathan On 12/1/23 21:08, Jonathan Cameron wrote: > On Mon, 27 Nov 2023 22:17:33 +0300 > George Stark <gnstark@salutedevices.com> wrote: > >> Hello Jonathan >> >> Thanks for the review. >> >> On 11/25/23 20:25, Jonathan Cameron wrote: >>> On Wed, 22 Nov 2023 10:47:41 +0300 >>> George Stark <gnstark@salutedevices.com> wrote: >>> >>>> According to Amlogic custom kernels ADC of axg SoC family has >>>> vref_select and requires this setting to work nominally and thus >>>> needs a separate config. >>>> >>>> Signed-off-by: George Stark <gnstark@salutedevices.com> >>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> >>> >>> Is this a fix? If so, please provide a fixes tag. >> Actually yes, these changes could be done in the patch that introduced >> new configuration parameters. >> >>> Or is it aligning with the vendor kernels and we don't know if >>> it's strictly necessary or not? >> I do know it's necessary if we want kernel driver to be independent from >> ADC initialization in vendor u-boot code. I have Amlogic a113x device >> (axg family) and can provide test results w and w/o this patch. > > Definitely best to not rely on firmware. > > So send a Fixes tag for this and I'll apply it as a fix > (you get to figure out which patch it fixed :) The patch is ready :) https://lore.kernel.org/lkml/20231127235558.71995-1-gnstark@salutedevices.com/ > > Jonathan > >> >>> >>>> --- >>>> drivers/iio/adc/meson_saradc.c | 16 +++++++++++++++- >>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c >>>> index db280da9edbf..34555a85f131 100644 >>>> --- a/drivers/iio/adc/meson_saradc.c >>>> +++ b/drivers/iio/adc/meson_saradc.c >>>> @@ -1242,6 +1242,20 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = { >>>> .cmv_select = 1, >>>> }; >>>> >>>> +static const struct meson_sar_adc_param meson_sar_adc_axg_param = { >>>> + .has_bl30_integration = true, >>>> + .clock_rate = 1200000, >>>> + .bandgap_reg = MESON_SAR_ADC_REG11, >>>> + .regmap_config = &meson_sar_adc_regmap_config_gxbb, >>>> + .resolution = 12, >>>> + .disable_ring_counter = 1, >>>> + .has_reg11 = true, >>>> + .vref_volatge = 1, >>>> + .has_vref_select = true, >>>> + .vref_select = VREF_VDDA, >>>> + .cmv_select = 1, >>>> +}; >>>> + >>>> static const struct meson_sar_adc_param meson_sar_adc_g12a_param = { >>>> .has_bl30_integration = false, >>>> .clock_rate = 1200000, >>>> @@ -1286,7 +1300,7 @@ static const struct meson_sar_adc_data meson_sar_adc_gxm_data = { >>>> }; >>>> >>>> static const struct meson_sar_adc_data meson_sar_adc_axg_data = { >>>> - .param = &meson_sar_adc_gxl_param, >>>> + .param = &meson_sar_adc_axg_param, >>>> .name = "meson-axg-saradc", >>>> }; >>>> >>> >> >
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c index db280da9edbf..34555a85f131 100644 --- a/drivers/iio/adc/meson_saradc.c +++ b/drivers/iio/adc/meson_saradc.c @@ -1242,6 +1242,20 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = { .cmv_select = 1, }; +static const struct meson_sar_adc_param meson_sar_adc_axg_param = { + .has_bl30_integration = true, + .clock_rate = 1200000, + .bandgap_reg = MESON_SAR_ADC_REG11, + .regmap_config = &meson_sar_adc_regmap_config_gxbb, + .resolution = 12, + .disable_ring_counter = 1, + .has_reg11 = true, + .vref_volatge = 1, + .has_vref_select = true, + .vref_select = VREF_VDDA, + .cmv_select = 1, +}; + static const struct meson_sar_adc_param meson_sar_adc_g12a_param = { .has_bl30_integration = false, .clock_rate = 1200000, @@ -1286,7 +1300,7 @@ static const struct meson_sar_adc_data meson_sar_adc_gxm_data = { }; static const struct meson_sar_adc_data meson_sar_adc_axg_data = { - .param = &meson_sar_adc_gxl_param, + .param = &meson_sar_adc_axg_param, .name = "meson-axg-saradc", };