Ingenic: iio/adc: add JZ4760(B) support to the
Subsystems (4)
PATCH 1/1 Ingenic: iio/adc: add JZ4760(B) support to the
This is a set of patches to add support for the JZ4760(B) socs to ingenic-sadc, those socs have 3 aux channels, and the JZ4760B has an extra register to set the battery divider as internal or external.
PATCH 1/6 iio/adc: ingenic: rename has_aux2 to has_aux_md
The purpose of this property is to set the AUX_MD bits if true, no to describe the hardware. Rename it to a more appropriate name.
--- drivers/iio/adc/ingenic-adc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/iio/adc/ingenic-adc.c +5 -5
@@ -92,7 +92,7 @@ struct ingenic_adc_soc_data {const int *battery_scale_avail;size_t battery_scale_avail_size; unsigned int battery_vref_mode: 1; - unsigned int has_aux2: 1; + unsigned int has_aux_md: 1;const struct iio_chan_spec *channels;unsigned int num_channels;int (*init_clk_div)(struct device *dev,struct ingenic_adc *adc);
@@ -506,7 +506,7 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = { .battery_scale_avail = jz4725b_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels, .num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = jz4725b_adc_init_clk_div,
@@ -520,7 +520,7 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .battery_scale_avail = jz4740_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels, .num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = NULL, /* no ADCLK register on JZ4740 */
@@ -534,7 +534,7 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = { .battery_scale_avail = jz4770_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail), .battery_vref_mode = false, - .has_aux2 = true, + .has_aux_md = true, .channels = jz4770_channels, .num_channels = ARRAY_SIZE(jz4770_channels), .init_clk_div = jz4770_adc_init_clk_div,
@@ -581,7 +581,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,/* We cannot sample AUX/AUX2 in parallel. */ mutex_lock (&adc->aux_lock); -if (adc->soc_data->has_aux2 && engine ==0 ) { +if (adc->soc_data->has_aux_md && engine ==0 ) { bit =BIT (chan->channel ==INGENIC_ADC_AUX2 );ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit); }
Signature · 1 line
2.30.2
Re: [PATCH 1/6] iio/adc: ingenic: rename has_aux2 to has_aux_md
Hi Christophe,
Quoted 2 lines · Le mer., juil. 21 2021 at 12:53:12 +0200, citral23 <[email protected]> a…
The purpose of this property is to set the
AUX_MDbits if true, no to describe the hardware. Rename it to a more appropriate name.
You could add that this change is needed to support the JZ4760 which has three AUX channels.
Quoted 1 line
Signed-off-by Christophe Branchereau <[email protected]>
Cheers,
-Paul
Quoted 63 lines
--- drivers/iio/adc/ingenic-adc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/iio/adc/ingenic-adc.c +2
-2@@ -92,7 +92,7 @@ struct ingenic_adc_soc_data {const int *battery_scale_avail;size_t battery_scale_avail_size; unsigned int battery_vref_mode: 1; - unsigned int has_aux2: 1; + unsigned int has_aux_md: 1;const struct iio_chan_spec *channels;unsigned int num_channels;int (*init_clk_div)(struct device *dev,struct ingenic_adc *adc);@@ -506,7 +506,7 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = { .battery_scale_avail = jz4725b_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels,drivers/iio/adc/ingenic-adc.c +1
-1.num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = jz4725b_adc_init_clk_div,@@ -520,7 +520,7 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .battery_scale_avail = jz4740_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels,drivers/iio/adc/ingenic-adc.c +1
-1.num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = NULL, /* no ADCLK register on JZ4740 */@@ -534,7 +534,7 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = { .battery_scale_avail = jz4770_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail), .battery_vref_mode = false, - .has_aux2 = true, + .has_aux_md = true, .channels = jz4770_channels,drivers/iio/adc/ingenic-adc.c +1
-1.num_channels = ARRAY_SIZE(jz4770_channels), .init_clk_div = jz4770_adc_init_clk_div,@@ -581,7 +581,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,/* We cannot sample AUX/AUX2 in parallel. */ mutex_lock (&adc->aux_lock); -if (adc->soc_data->has_aux2 && engine ==0 ) { +if (adc->soc_data->has_aux_md && engine ==0 ) { bit =BIT (chan->channel ==INGENIC_ADC_AUX2 );ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit);}
--
2.30.2
PATCH 2/6 dt-bindings: iio/adc: add an INGENIC_ADC_AUX0 entry
In reply to citral23The JZ4760(B) socs have 3 AUX inputs, add an entry to prepare including the one named AUX in the sadc driver. Leaving the rest untouched as it's ABI.
--- include/dt-bindings/iio/adc/ingenic,adc.h | 1 + 1 file changed, 1 insertion(+)
include/dt-bindings/iio/adc/ingenic,adc.h +1 -0
@@ -13,5 +13,6 @@#define INGENIC_ADC_TOUCH_YN 6#define INGENIC_ADC_TOUCH_XD 7#define INGENIC_ADC_TOUCH_YD 8 +#define INGENIC_ADC_AUX0 9#endif
Signature · 1 line
2.30.2
Re: [PATCH 2/6] dt-bindings: iio/adc: add an INGENIC_ADC_AUX0 entry
Hi Christophe,
Quoted 4 lines · Le mer., juil. 21 2021 at 12:53:13 +0200, citral23 <[email protected]> a…
The JZ4760(B) socs have 3 AUX inputs, add an entry to prepare including the one named AUX in the sadc driver. Leaving the rest untouched as it's ABI.
Signed-off-by citral23 <[email protected]>
Please use your real name when you sign a commit.
Cheers,
-Paul
Quoted 18 lines
--- include/dt-bindings/iio/adc/ingenic,adc.h | 1 + 1 file changed, 1 insertion(+)include/dt-bindings/iio/adc/ingenic,adc.h +1
-1@@ -13,5 +13,6 @@#define INGENIC_ADC_TOUCH_YN 6#define INGENIC_ADC_TOUCH_XD 7#define INGENIC_ADC_TOUCH_YD 8 +#define INGENIC_ADC_AUX0 9#endif --2.30.2
PATCH 3/6 iio/adc: ingenic: add JZ4760 support to the sadc driver
In reply to citral23--- drivers/iio/adc/ingenic-adc.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)
drivers/iio/adc/ingenic-adc.c +64 -0
@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12
@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = { JZ_ADC_BATTERY_LOW_VREF, JZ_ADC_BATTERY_LOW_VREF_BITS, }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; +static const int jz4770_adc_battery_raw_avail[] = {0 ,1 , (1 <<JZ4770_ADC_BATTERY_VREF_BITS ) -1 , };
@@ -339,6 +344,8 @@ static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)return 0 ; } + +static int jz4770_adc_init_clk_div (struct device *dev,struct ingenic_adc *adc) {struct clk *parent_clk;
@@ -400,6 +407,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux0" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 , + }, + { + .extend_name ="aux" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX , + .scan_index =-1 , + }, + { + .extend_name ="battery" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_BATTERY , + .scan_index =-1 , + }, + { + .extend_name ="aux2" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX2 , + .scan_index =-1 , + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { { .type =IIO_VOLTAGE ,
@@ -526,6 +574,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = { .battery_high_vref =JZ4770_ADC_BATTERY_VREF , .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS ,
@@ -621,6 +683,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev,return ingenic_adc_read_chan_info_raw (iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 : *val =JZ_ADC_AUX_VREF ;
@@ -832,6 +895,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data, }, { .compatible ="ingenic,jz4770-adc" , .data = &jz4770_adc_soc_data, }, { }, };
Signature · 1 line
2.30.2
Paul Cercueil
maintainerRe: [PATCH 3/6] iio/adc: ingenic: add JZ4760 support to the sadc driver
Hi Christophe,
Quoted 31 lines · Le mer., juil. 21 2021 at 12:53:14 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)drivers/iio/adc/ingenic-adc.c +5
-0@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = {JZ_ADC_BATTERY_LOW_VREF ,JZ_ADC_BATTERY_LOW_VREF_BITS , }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; + static const int jz4770_adc_battery_raw_avail[] = { 0, 1, (1 << JZ4770_ADC_BATTERY_VREF_BITS) - 1,};
drivers/iio/adc/ingenic-adc.c static int jz4725b_adc_init_clk_div(struct device · line 349
Quoted 6 lines
drivers/iio/adc/ingenic-adc.c +2
-0@@ -339,6 +344,8 @@ static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)return 0 ; } + +
Unrelated cosmetic change - remove it.
Quoted 18 lines
drivers/iio/adc/ingenic-adc.c +0
-0static int jz4770_adc_init_clk_div (struct device *dev,struct ingenic_adc *adc){
drivers/iio/adc/ingenic-adc.c +9
-0struct clk *parent_clk;@@ -400,6 +407,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux0" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 ,
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec · line 429
Quoted 10 lines
drivers/iio/adc/ingenic-adc.c +10
-0+ }, + { + .extend_name = "aux", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX, + .scan_index = -1, + },
A couple of things. First, ".extend_name" is deprecated now... But since the driver used it before, I suppose it doesn't make sense to use labels just for this SoC (as we can't remove .extend_name for other SoCs because of ABI). So I suppose it works here, but maybe Jonathan disagrees.
Also, you should probably use "aux1" as the channel's name instead of "aux", independently of the macro name you used in the .channel field.
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec jz4740_channels[] = { · line 439
Quoted 11 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "battery", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_BATTERY, + .scan_index = -1, + },
Swap the battery channel at the end, no? First the three AUX then the battery channel?
The rest looks pretty good to me.
Cheers,
-Paul
Quoted 62 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "aux2", + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX2, + .scan_index = -1, + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { {drivers/iio/adc/ingenic-adc.c +14
-0.type = IIO_VOLTAGE,@@ -526,6 +574,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {drivers/iio/adc/ingenic-adc.c +1
-0.battery_high_vref = JZ4770_ADC_BATTERY_VREF, .battery_high_vref_bits = JZ4770_ADC_BATTERY_VREF_BITS,@@ -621,6 +683,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev, return ingenic_adc_read_chan_info_raw(iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 :drivers/iio/adc/ingenic-adc.c +1
-0*val =JZ_ADC_AUX_VREF ;@@ -832,6 +895,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data,},
{ .compatible = "ingenic,jz4770-adc", .data =&jz4770_adc_soc_data,
},
{ },
};
--
2.30.2
Re: [PATCH 3/6] iio/adc: ingenic: add JZ4760 support to the sadc driver
Hello Paul, thank you for all the feedback, duly noted I will V2, there is just one I disagree with:
Quoted 41 lines · Paul Cercueil
Hi Christophe,
Quoted 36 lines · Le mer., juil. 21 2021 at 12:53:14 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)drivers/iio/adc/ingenic-adc.c +5
-0@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = {JZ_ADC_BATTERY_LOW_VREF ,JZ_ADC_BATTERY_LOW_VREF_BITS , }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; + static const int jz4770_adc_battery_raw_avail[] = { 0, 1, (1 << JZ4770_ADC_BATTERY_VREF_BITS) - 1,};
drivers/iio/adc/ingenic-adc.c +0
-0@@ -339,6 +344,8 @@ static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)return 0 ; }
Quoted 4 lines
drivers/iio/adc/ingenic-adc.c static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc) · line 348
Quoted 2 lines
drivers/iio/adc/ingenic-adc.c +2
-0+ +Unrelated cosmetic change - remove it.
This is not cosmetic, but to add a VREF of 2.5V for the jz4760, as per specs
Quoted 113 lines
Quoted 19 lines
drivers/iio/adc/ingenic-adc.c +0
-0static int jz4770_adc_init_clk_div (struct device *dev,struct ingenic_adc *adc){ struct clk *parent_clk;drivers/iio/adc/ingenic-adc.c +9
-0@@ -400,6 +407,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux0" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 ,drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec · line 429
Quoted 10 lines
drivers/iio/adc/ingenic-adc.c +10
-0+ }, + { + .extend_name = "aux", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX, + .scan_index = -1, + },A couple of things. First, ".extend_name" is deprecated now... But since the driver used it before, I suppose it doesn't make sense to use labels just for this SoC (as we can't remove .extend_name for other SoCs because of ABI). So I suppose it works here, but maybe Jonathan disagrees.
Also, you should probably use "aux1" as the channel's name instead of "aux", independently of the macro name you used in the .channel field.
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec jz4740_channels[] = { · line 439
Quoted 11 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "battery", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_BATTERY, + .scan_index = -1, + },Swap the battery channel at the end, no? First the three AUX then the battery channel?
The rest looks pretty good to me.
Cheers,
-PaulQuoted 54 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "aux2", + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX2, + .scan_index = -1, + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { {.type =
IIO_VOLTAGE,drivers/iio/adc/ingenic-adc.c +14
-0@@ -526,6 +574,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {.battery_high_vref = JZ4770_ADC_BATTERY_VREF, .battery_high_vref_bits = JZ4770_ADC_BATTERY_VREF_BITS,drivers/iio/adc/ingenic-adc.c +1
-0@@ -621,6 +683,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev, return ingenic_adc_read_chan_info_raw(iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 :*val = JZ_ADC_AUX_VREF;drivers/iio/adc/ingenic-adc.c +1
-0@@ -832,6 +895,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data,
Quoted 10 lines
Quoted 8 lines
},
{ .compatible = "ingenic,jz4770-adc", .data = &jz4770_adc_soc_data, }, { },};
--
2.30.2
KR
CB
Re: [PATCH 3/6] iio/adc: ingenic: add JZ4760 support to the sadc driver
Quoted 45 lines · Christophe Branchereau
Hello Paul, thank you for all the feedback, duly noted I will V2, there is just one I disagree with:
Quoted 41 lines · Paul Cercueil
Hi Christophe,
Quoted 36 lines · Le mer., juil. 21 2021 at 12:53:14 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)drivers/iio/adc/ingenic-adc.c +5
-0@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = {JZ_ADC_BATTERY_LOW_VREF ,JZ_ADC_BATTERY_LOW_VREF_BITS , }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; + static const int jz4770_adc_battery_raw_avail[] = { 0, 1, (1 << JZ4770_ADC_BATTERY_VREF_BITS) - 1,};drivers/iio/adc/ingenic-adc.c +0
-0@@ -339,6 +344,8 @@ static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)return 0 ; }
Quoted 7 lines
Quoted 4 lines
drivers/iio/adc/ingenic-adc.c static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc) · line 348
Quoted 2 lines
drivers/iio/adc/ingenic-adc.c +2
-0+ +Unrelated cosmetic change - remove it.
This is not cosmetic, but to add a VREF of 2.5V for the jz4760, as per specs
Two added empty lines are not cosmetic ?
Guenter
Quoted 126 lines
Quoted 113 lines
Quoted 19 lines
drivers/iio/adc/ingenic-adc.c +0
-0static int jz4770_adc_init_clk_div (struct device *dev,struct ingenic_adc *adc){ struct clk *parent_clk;drivers/iio/adc/ingenic-adc.c +9
-0@@ -400,6 +407,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux0" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 ,drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec · line 429
Quoted 10 lines
drivers/iio/adc/ingenic-adc.c +10
-0+ }, + { + .extend_name = "aux", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX, + .scan_index = -1, + },A couple of things. First, ".extend_name" is deprecated now... But since the driver used it before, I suppose it doesn't make sense to use labels just for this SoC (as we can't remove .extend_name for other SoCs because of ABI). So I suppose it works here, but maybe Jonathan disagrees.
Also, you should probably use "aux1" as the channel's name instead of "aux", independently of the macro name you used in the .channel field.
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec jz4740_channels[] = { · line 439
Quoted 11 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "battery", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_BATTERY, + .scan_index = -1, + },Swap the battery channel at the end, no? First the three AUX then the battery channel?
The rest looks pretty good to me.
Cheers,
-PaulQuoted 54 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "aux2", + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX2, + .scan_index = -1, + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { {.type =
IIO_VOLTAGE,drivers/iio/adc/ingenic-adc.c +14
-0@@ -526,6 +574,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {.battery_high_vref = JZ4770_ADC_BATTERY_VREF, .battery_high_vref_bits = JZ4770_ADC_BATTERY_VREF_BITS,drivers/iio/adc/ingenic-adc.c +1
-0@@ -621,6 +683,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev, return ingenic_adc_read_chan_info_raw(iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 :*val = JZ_ADC_AUX_VREF;drivers/iio/adc/ingenic-adc.c +1
-0@@ -832,6 +895,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data,Quoted 10 lines
Quoted 8 lines
},
{ .compatible = "ingenic,jz4770-adc", .data = &jz4770_adc_soc_data, }, { },};
--
2.30.2KR
CB
v2 cover iio/adc: ingenic: add support for the JZ4760(B) Socs to the ingenic sadc driver
This is a set of patches to add support to the JZ4760(B) socs found in numerous gaming handhelds and some mp3 players.
Christophe Branchereau (5):
iio/adc: ingenic: rename has_aux2 to has_aux_md
dt-bindings: iio/adc: add an INGENIC_ADC_AUX0 entry iio/adc: ingenic: add JZ4760 support to the sadc driver iio/adc: ingenic: add JZ4760B support to the sadc driver
dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc
Documentation
PATCH v2 1/5 iio/adc: ingenic: rename has_aux2 to has_aux_md
The jz4760(b) socs have 3 aux channels.
The purpose of has_aux2 is to set the MD bits used to select the AUX channel to be sampled, not to describe the hardware.
Rename it to a more appropriate name.
--- drivers/iio/adc/ingenic-adc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/iio/adc/ingenic-adc.c +5 -5
@@ -92,7 +92,7 @@ struct ingenic_adc_soc_data {const int *battery_scale_avail;size_t battery_scale_avail_size; unsigned int battery_vref_mode: 1; - unsigned int has_aux2: 1; + unsigned int has_aux_md: 1;const struct iio_chan_spec *channels;unsigned int num_channels;int (*init_clk_div)(struct device *dev,struct ingenic_adc *adc);
@@ -506,7 +506,7 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = { .battery_scale_avail = jz4725b_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels, .num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = jz4725b_adc_init_clk_div,
@@ -520,7 +520,7 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .battery_scale_avail = jz4740_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail), .battery_vref_mode = true, - .has_aux2 = false, + .has_aux_md = false, .channels = jz4740_channels, .num_channels = ARRAY_SIZE(jz4740_channels), .init_clk_div = NULL, /* no ADCLK register on JZ4740 */
@@ -534,7 +534,7 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = { .battery_scale_avail = jz4770_adc_battery_scale_avail, .battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail), .battery_vref_mode = false, - .has_aux2 = true, + .has_aux_md = true, .channels = jz4770_channels, .num_channels = ARRAY_SIZE(jz4770_channels), .init_clk_div = jz4770_adc_init_clk_div,
@@ -581,7 +581,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,/* We cannot sample AUX/AUX2 in parallel. */ mutex_lock (&adc->aux_lock); -if (adc->soc_data->has_aux2 && engine ==0 ) { +if (adc->soc_data->has_aux_md && engine ==0 ) { bit =BIT (chan->channel ==INGENIC_ADC_AUX2 );ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit); }
Signature · 1 line
2.30.2
PATCH v2 2/5 dt-bindings: iio/adc: add an INGENIC_ADC_AUX0 entry
In reply to Christophe BranchereauThe JZ4760(B) socs have 3 AUX inputs, add an entry to prepare including the one named AUX in the sadc driver.
Leaving the rest untouched as it's ABI.
--- include/dt-bindings/iio/adc/ingenic,adc.h | 1 + 1 file changed, 1 insertion(+)
include/dt-bindings/iio/adc/ingenic,adc.h +1 -0
@@ -13,5 +13,6 @@#define INGENIC_ADC_TOUCH_YN 6#define INGENIC_ADC_TOUCH_XD 7#define INGENIC_ADC_TOUCH_YD 8 +#define INGENIC_ADC_AUX0 9#endif
Signature · 1 line
2.30.2
PATCH v2 3/5 iio/adc: ingenic: add JZ4760 support to the sadc driver
In reply to Christophe BranchereauThe jz4760 sadc is very similar to the jz4770 one, but has a VREF of 2.5V and 3 aux channels.
modify ingenic_adc_read_chan_info_raw() needs a change to account for it.
--- drivers/iio/adc/ingenic-adc.c | 82 +++++++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 4 deletions(-)
drivers/iio/adc/ingenic-adc.c +78 -4
@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12
@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = { JZ_ADC_BATTERY_LOW_VREF, JZ_ADC_BATTERY_LOW_VREF_BITS, }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; +static const int jz4770_adc_battery_raw_avail[] = {0 ,1 , (1 <<JZ4770_ADC_BATTERY_VREF_BITS ) -1 , };
@@ -400,6 +405,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 , + }, + { + .extend_name ="aux1" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX , + .scan_index =-1 , + }, + { + .extend_name ="aux2" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX2 , + .scan_index =-1 , + }, + { + .extend_name ="battery" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_BATTERY , + .scan_index =-1 , + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { { .type =IIO_VOLTAGE ,
@@ -526,6 +572,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = { .battery_high_vref =JZ4770_ADC_BATTERY_VREF , .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS ,
@@ -569,7 +629,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,struct iio_chan_spec const *chan, int *val) { - int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY); + int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);struct ingenic_adc *adc =iio_priv (iio_dev); ret =clk_enable (adc->clk);
@@ -579,11 +639,22 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,return ret; } -/* We cannot sample AUX/AUX2 in parallel. */ +/* We cannot sample the aux channels in parallel. */ mutex_lock (&adc->aux_lock);if (adc->soc_data->has_aux_md && engine ==0 ) { - bit =BIT (chan->channel ==INGENIC_ADC_AUX2 ); -ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit); +switch (chan->channel) { +case INGENIC_ADC_AUX0 : + cmd =0 ; +break ; +case INGENIC_ADC_AUX : + cmd =1 ; +break ; +case INGENIC_ADC_AUX2 : + cmd =2 ; +break ; + } + +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , cmd); } ret =ingenic_adc_capture (adc, engine);
@@ -591,6 +662,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev, goto out;switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 : *val =readw (adc->base +JZ_ADC_REG_ADSDAT );
@@ -621,6 +693,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev,return ingenic_adc_read_chan_info_raw (iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 : *val =JZ_ADC_AUX_VREF ;
@@ -832,6 +905,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data, }, { .compatible ="ingenic,jz4770-adc" , .data = &jz4770_adc_soc_data, }, { }, };
Signature · 1 line
2.30.2
PATCH v2 4/5 iio/adc: ingenic: add JZ4760B support to the sadc driver
In reply to Christophe BranchereauThe JZ4760B variant differs slightly from the JZ4760: it has a bit called VBAT_SEL in the CFG register.
In order to correctly sample the battery voltage on existing handhelds using this SOC, the bit must be cleared.
We leave the possibility to set the bit, by adding the "ingenic,use-internal-divider" property to a devicetree.
--- drivers/iio/adc/ingenic-adc.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/iio/adc/ingenic-adc.c +8 -0
@@ -37,6 +37,7 @@#define JZ_ADC_REG_CFG_SAMPLE_NUM (n) ((n) << 10)#define JZ_ADC_REG_CFG_PULL_UP (n) ((n) << 16)#define JZ_ADC_REG_CFG_CMD_SEL BIT(22) +#define JZ_ADC_REG_CFG_VBAT_SEL BIT(30)#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK (BIT(31) | GENMASK(23, 10))#define JZ_ADC_REG_ADCLK_CLKDIV_LSB 0#define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB 16
@@ -879,6 +880,12 @@ static int ingenic_adc_probe(struct platform_device *pdev)/* Put hardware in a known passive state. */ writeb (0x00 , adc->base +JZ_ADC_REG_ENABLE );writeb (0xff , adc->base +JZ_ADC_REG_CTRL ); + +if (device_property_present (dev,"ingenic,use-internal-divider" ))/* JZ4760B specific */ +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,JZ_ADC_REG_CFG_VBAT_SEL ); +else +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,0 ); +usleep_range (2000 ,3000 );/* Must wait at least 2ms. */ clk_disable (adc->clk);
@@ -906,6 +913,7 @@ static const struct of_device_id ingenic_adc_of_match[] = { { .compatible = "ingenic,jz4725b-adc", .data = &jz4725b_adc_soc_data, }, { .compatible = "ingenic,jz4740-adc", .data = &jz4740_adc_soc_data, }, { .compatible = "ingenic,jz4760-adc", .data = &jz4760_adc_soc_data, }, + { .compatible = "ingenic,jz4760b-adc", .data = &jz4760_adc_soc_data, }, { .compatible = "ingenic,jz4770-adc", .data = &jz4770_adc_soc_data, }, { }, };
Signature · 1 line
2.30.2
Re: [PATCH V2 4/5] iio/adc: ingenic: add JZ4760B support to the sadc driver
Quoted 10 lines · On Fri, 23 Jul 2021 10:58:12 +0200 Christophe Branchereau…
The JZ4760B variant differs slightly from the JZ4760: it has a bit called
VBAT_SELin the CFG register.In order to correctly sample the battery voltage on existing handhelds using this SOC, the bit must be cleared.
We leave the possibility to set the bit, by adding the "ingenic,use-internal-divider" property to a devicetree.
Signed-off-by Christophe Branchereau <[email protected]>
One minor formatting comment inline. If that is all that comes up in review I can just change it whilst applying.
Jonathan
Quoted 17 lines
--- drivers/iio/adc/ingenic-adc.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/iio/adc/ingenic-adc.c +1
-0@@ -37,6 +37,7 @@#define JZ_ADC_REG_CFG_SAMPLE_NUM (n) ((n) << 10)#define JZ_ADC_REG_CFG_PULL_UP (n) ((n) << 16)#define JZ_ADC_REG_CFG_CMD_SEL BIT(22) +#define JZ_ADC_REG_CFG_VBAT_SEL BIT(30)#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK (BIT(31) | GENMASK(23, 10))#define JZ_ADC_REG_ADCLK_CLKDIV_LSB 0#define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB 16
drivers/iio/adc/ingenic-adc.c static int ingenic_adc_probe(struct platform_device *pdev) · line 885
Quoted 6 lines
drivers/iio/adc/ingenic-adc.c +3
-0@@ -879,6 +880,12 @@ static int ingenic_adc_probe(struct platform_device *pdev)/* Put hardware in a known passive state. */ writeb (0x00 , adc->base +JZ_ADC_REG_ENABLE );writeb (0xff , adc->base +JZ_ADC_REG_CTRL ); + +if (device_property_present (dev,"ingenic,use-internal-divider" ))/* JZ4760B specific */ +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,JZ_ADC_REG_CFG_VBAT_SEL );
Please break this line and move the comment on the one above.
Whilst we have relaxed the kernel style to allow longer lines, it's nice to still keep them to the 80 char limit when it doesn't really hurt readability. Here I don't think it would make much difference.
Quoted 14 lines
drivers/iio/adc/ingenic-adc.c +4
-0+else +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,0 ); +usleep_range (2000 ,3000 );/* Must wait at least 2ms. */ clk_disable (adc->clk);@@ -906,6 +913,7 @@ static const struct of_device_id ingenic_adc_of_match[] = { { .compatible = "ingenic,jz4725b-adc", .data = &jz4725b_adc_soc_data, }, { .compatible = "ingenic,jz4740-adc", .data = &jz4740_adc_soc_data, }, { .compatible = "ingenic,jz4760-adc", .data = &jz4760_adc_soc_data, }, + { .compatible = "ingenic,jz4760b-adc", .data = &jz4760_adc_soc_data, }, { .compatible = "ingenic,jz4770-adc", .data = &jz4770_adc_soc_data, }, { }, };
PATCH v2 5/5 dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
In reply to Christophe BranchereauThe jz4760b variant differs slightly from the jz4760, add a property to let users sample the internal divider if needed and document it.
--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +9 -0
@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':
@@ -43,6 +45,13 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG register + to sample the battery voltage from the internal divider. If absent, it + will sample the external divider. + type: boolean + required: - compatible - '#io-channel-cells'
Signature · 1 line
2.30.2
Re: [PATCH V2 5/5] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
Quoted 22 lines · On Fri, 23 Jul 2021 10:58:13 +0200 Christophe Branchereau…
The jz4760b variant differs slightly from the jz4760, add a property to let users sample the internal divider if needed and document it.
Signed-off-by Christophe Branchereau <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +2
-0@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml properties: · line 54
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +7
-0@@ -43,6 +45,13 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG register + to sample the battery voltage from the internal divider. If absent, it + will sample the external divider. + type: boolean +
See reply to the v1 patch for hint on how to 'enforce' that this only exists for the jz4760b
Thanks,
Jonathan
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +0
-0required: - compatible - '#io-channel-cells'
Re: [PATCH V2 5/5] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
Hello Johnathan, am I allowed to declare the property within the if block like this?
# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2019-2020 Artur Rojek
%YAML 1.2
---
$id: "http://devicetree.org/schemas/iio/adc/ingenic,adc.yaml#" $schema: "http://devicetree.org/meta-schemas/core.yaml#"
title: Ingenic JZ47xx ADC controller IIO bindings
maintainers:
- Artur Rojek <[email protected]>
description: >
Industrial I/O subsystem bindings for ADC controller found in Ingenic JZ47xx SoCs.
ADC clients must use the format described in
https://github.com/devicetree-org/dt-schema/blob/master/schemas/iio/iio-consumer.yaml,
giving a phandle and IIO specifier pair ("io-channels") to the ADC controller.
properties:
compatible:
enum:
- ingenic,jz4725b-adc
- ingenic,jz4740-adc
- ingenic,jz4760-adc
- ingenic,jz4760b-adc
- ingenic,jz4770-adc
'#io-channel-cells':
const: 1
description:
Must be set to <1> to indicate channels are selected by index.
reg:
maxItems: 1
clocks:
maxItems: 1
clock-names:
items:
- const: adc
interrupts:
maxItems: 1
allOf:
- if:
properties:
compatible:
contains:
enum:
- ingenic,jz4760b-adc
then:
properties:
ingenic,use-internal-divider:
description:
If present, battery voltage is read from the VBAT_IR pin, which has an internal 1/4 divider. If absent, it is read through the VBAT_ER pin, which does not have such a divider.
type: boolean
required:
- compatible
- '#io-channel-cells'
- reg
- clocks
- clock-names
- interrupts
additionalProperties: false
examples:
- |
#include <dt-bindings/clock/jz4740-cgu.h>
#include <dt-bindings/iio/adc/ingenic,adc.h>
adc@10070000 {
compatible = "ingenic,jz4740-adc";
#io-channel-cells = <1>;
reg = <0x10070000 0x30>;
clocks = <&cgu JZ4740_CLK_ADC>;
clock-names = "adc";
interrupt-parent = <&intc>;
interrupts = <18>;
};Quoted 47 lines · Jonathan Cameron
Quoted 22 lines · On Fri, 23 Jul 2021 10:58:13 +0200 Christophe Branchereau…
The jz4760b variant differs slightly from the jz4760, add a property to let users sample the internal divider if needed and document it.
Signed-off-by Christophe Branchereau <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +2
-0@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml properties: · line 54
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +7
-0@@ -43,6 +45,13 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG register + to sample the battery voltage from the internal divider. If absent, it + will sample the external divider. + type: boolean +See reply to the v1 patch for hint on how to 'enforce' that this only exists for the jz4760b
Thanks,
Jonathan
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +0
-0required: - compatible - '#io-channel-cells'
Jonathan Cameron
maintainerRe: [PATCH V2 5/5] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
Quoted 2 lines · On Sat, 24 Jul 2021 09:33:46 +0200 Christophe Branchereau…
Hello Johnathan, am I allowed to declare the property within the if block like this?
Test it...
Short answer is no you aren't. As someone explained it to me the other day, each layer of the yaml is checked independently so if you declare a property in the if block and not the outer layer the additionalProperties check will fail should it be present.
So declare it outside, then set it false for the cases where it's not valid.
Jonathan
Quoted 141 lines
# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2019-2020 Artur Rojek
%YAML 1.2
---
$id: "http://devicetree.org/schemas/iio/adc/ingenic,adc.yaml#" $schema: "http://devicetree.org/meta-schemas/core.yaml#"title: Ingenic JZ47xx ADC controller IIO bindings
maintainers:
- Artur Rojek <[email protected]>description: >
Industrial I/O subsystem bindings for ADC controller found in Ingenic JZ47xx SoCs.ADC clients must use the format described in
https://github.com/devicetree-org/dt-schema/blob/master/schemas/iio/iio-consumer.yaml,
giving a phandle and IIO specifier pair ("io-channels") to the ADC controller.properties:
compatible:
enum:
- ingenic,jz4725b-adc
- ingenic,jz4740-adc
- ingenic,jz4760-adc
- ingenic,jz4760b-adc
- ingenic,jz4770-adc'#io-channel-cells':
const: 1
description:
Must be set to <1> to indicate channels are selected by index.reg:
maxItems: 1clocks:
maxItems: 1clock-names:
items:
- const: adcinterrupts:
maxItems: 1allOf:
- if:
properties:
compatible:
contains:
enum:
- ingenic,jz4760b-adc
then:
properties:
ingenic,use-internal-divider:
description:
If present, battery voltage is read from theVBAT_IRpin, which has an internal 1/4 divider. If absent, it is read through theVBAT_ERpin, which does not have such a divider.
type: booleanrequired:
- compatible
- '#io-channel-cells'
- reg
- clocks
- clock-names
- interruptsadditionalProperties: false
examples:
- |#include <dt-bindings/clock/jz4740-cgu.h> #include <dt-bindings/iio/adc/ingenic,adc.h> adc@10070000 { compatible = "ingenic,jz4740-adc"; #io-channel-cells = <1>; reg = <0x10070000 0x30>; clocks = <&cgu JZ4740_CLK_ADC>; clock-names = "adc"; interrupt-parent = <&intc>; interrupts = <18>; };Quoted 47 lines · Jonathan Cameron
Quoted 22 lines · On Fri, 23 Jul 2021 10:58:13 +0200 Christophe Branchereau…
The jz4760b variant differs slightly from the jz4760, add a property to let users sample the internal divider if needed and document it.
Signed-off-by Christophe Branchereau <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +2
-0@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml properties: · line 54
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +7
-0@@ -43,6 +45,13 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG register + to sample the battery voltage from the internal divider. If absent, it + will sample the external divider. + type: boolean +See reply to the v1 patch for hint on how to 'enforce' that this only exists for the jz4760b
Thanks,
Jonathan
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +0
-0required: - compatible - '#io-channel-cells'
Re: [PATCH V2 0/5] iio/adc: ingenic: add support for the JZ4760(B) Socs to the ingenic sadc driver
In reply to Christophe BranchereauQuoted 2 lines · On Fri, 23 Jul 2021 10:58:08 +0200 Christophe Branchereau…
This is a set of patches to add support to the JZ4760(B) socs found in numerous gaming handhelds and some mp3 players.
Process note. Please don't set the reply to for new revisions. They nest deep inside threads when people read email that way and it makes them hard to spot. I'd much rather see them as a new thread related only by the naming.
Thanks,
Jonathan
Quoted 9 lines
Christophe Branchereau (5):
iio/adc: ingenic: renamehas_aux2tohas_aux_md
dt-bindings: iio/adc: add anINGENIC_ADC_AUX0entry iio/adc: ingenic: add JZ4760 support to the sadc driver iio/adc: ingenic: add JZ4760B support to the sadc driver
dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc
Documentation
Re: [PATCH 3/6] iio/adc: ingenic: add JZ4760 support to the sadc driver
In reply to Paul CercueilQuoted 71 lines · On Wed, 21 Jul 2021 19:15:47 +0100 Paul Cercueil <[email protected]> wrote:
Hi Christophe,
Quoted 32 lines · Le mer., juil. 21 2021 at 12:53:14 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)drivers/iio/adc/ingenic-adc.c +5
-0@@ -71,6 +71,7 @@#define JZ4725B_ADC_BATTERY_HIGH_VREF_BITS 10#define JZ4740_ADC_BATTERY_HIGH_VREF (7500 * 0.986)#define JZ4740_ADC_BATTERY_HIGH_VREF_BITS 12 +#define JZ4760_ADC_BATTERY_VREF 2500#define JZ4770_ADC_BATTERY_VREF 1200#define JZ4770_ADC_BATTERY_VREF_BITS 12@@ -295,6 +296,10 @@ static const int jz4740_adc_battery_scale_avail[] = {JZ_ADC_BATTERY_LOW_VREF ,JZ_ADC_BATTERY_LOW_VREF_BITS , }; +static const int jz4760_adc_battery_scale_avail[] = { +JZ4760_ADC_BATTERY_VREF ,JZ4770_ADC_BATTERY_VREF_BITS , +}; + static const int jz4770_adc_battery_raw_avail[] = { 0, 1, (1 << JZ4770_ADC_BATTERY_VREF_BITS) - 1,};
drivers/iio/adc/ingenic-adc.c static int jz4725b_adc_init_clk_div(struct device · line 349
Quoted 6 lines
drivers/iio/adc/ingenic-adc.c +2
-0@@ -339,6 +344,8 @@ static int jz4725b_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)return 0 ; } + +Unrelated cosmetic change - remove it.
Quoted 26 lines
drivers/iio/adc/ingenic-adc.c +0
-0static int jz4770_adc_init_clk_div (struct device *dev,struct ingenic_adc *adc){
drivers/iio/adc/ingenic-adc.c +17
-0struct clk *parent_clk;@@ -400,6 +407,47 @@ static const struct iio_chan_spec jz4740_channels[] = { }, }; +static const struct iio_chan_spec jz4760_channels[] = { + { + .extend_name ="aux0" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX0 , + .scan_index =-1 , + }, + { + .extend_name ="aux" , + .type =IIO_VOLTAGE , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + .indexed =1 , + .channel =INGENIC_ADC_AUX ,
Quoted 8 lines
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec · line 429
Quoted 2 lines
drivers/iio/adc/ingenic-adc.c +2
-0+ .scan_index = -1, + },A couple of things. First, ".extend_name" is deprecated now... But since the driver used it before, I suppose it doesn't make sense to use labels just for this SoC (as we can't remove .extend_name for other SoCs because of ABI). So I suppose it works here, but maybe Jonathan disagrees.
Hmm. Interesting question. We could attempt to force new device support added to older drivers not to use extend_name but it seems likely in this case at least, that the same board specific code might run on this devices and the others, so I'll make an exception.
I'd be less keen if this was a stand alone ADC,
Jonathan
Quoted 88 lines
Also, you should probably use "aux1" as the channel's name instead of "aux", independently of the macro name you used in the .channel field.
drivers/iio/adc/ingenic-adc.c static const struct iio_chan_spec jz4740_channels[] = { · line 439
Quoted 11 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "battery", + .type = IIO_VOLTAGE, + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_BATTERY, + .scan_index = -1, + },Swap the battery channel at the end, no? First the three AUX then the battery channel?
The rest looks pretty good to me.
Cheers,
-PaulQuoted 62 lines
drivers/iio/adc/ingenic-adc.c +11
-0+ { + .extend_name = "aux2", + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .indexed = 1, + .channel = INGENIC_ADC_AUX2, + .scan_index = -1, + }, +}; +static const struct iio_chan_spec jz4770_channels[] = { {drivers/iio/adc/ingenic-adc.c +14
-0.type = IIO_VOLTAGE,@@ -526,6 +574,20 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = { .init_clk_div = NULL, /* no ADCLK register on JZ4740 */ }; +static const struct ingenic_adc_soc_data jz4760_adc_soc_data = { + .battery_high_vref =JZ4760_ADC_BATTERY_VREF , + .battery_high_vref_bits =JZ4770_ADC_BATTERY_VREF_BITS , + .battery_raw_avail = jz4770_adc_battery_raw_avail, + .battery_raw_avail_size =ARRAY_SIZE (jz4770_adc_battery_raw_avail), + .battery_scale_avail = jz4760_adc_battery_scale_avail, + .battery_scale_avail_size =ARRAY_SIZE (jz4760_adc_battery_scale_avail), + .battery_vref_mode = false, + .has_aux_md = true, + .channels = jz4760_channels, + .num_channels =ARRAY_SIZE (jz4760_channels), + .init_clk_div = jz4770_adc_init_clk_div, +}; +static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {drivers/iio/adc/ingenic-adc.c +1
-0.battery_high_vref = JZ4770_ADC_BATTERY_VREF, .battery_high_vref_bits = JZ4770_ADC_BATTERY_VREF_BITS,@@ -621,6 +683,7 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev, return ingenic_adc_read_chan_info_raw(iio_dev, chan, val);case IIO_CHAN_INFO_SCALE :switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 :drivers/iio/adc/ingenic-adc.c +1
-0*val =JZ_ADC_AUX_VREF ;@@ -832,6 +895,7 @@ static int ingenic_adc_probe(struct platform_device *pdev)static const struct of_device_id ingenic_adc_of_match[] = { { .compatible ="ingenic,jz4725b-adc" , .data = &jz4725b_adc_soc_data, }, { .compatible ="ingenic,jz4740-adc" , .data = &jz4740_adc_soc_data, }, + { .compatible ="ingenic,jz4760-adc" , .data = &jz4760_adc_soc_data,},
{ .compatible = "ingenic,jz4770-adc", .data =&jz4770_adc_soc_data,
},
{ },
};
--
2.30.2
PATCH 4/6 iio/adc: ingenic: add JZ4760B support to the sadc driver
In reply to citral23The JZ4760B variant differs slightly from the JZ4760, in that it has a bit called VBAT_SEL in the CFG register. In order to correctly sample the battery voltage on existing handhelds using this SOC, the bit must be cleared.
We leave the possibility to set the bit, by using the "ingenic,use-internal-divider" in the devicetree.
--- drivers/iio/adc/ingenic-adc.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/iio/adc/ingenic-adc.c +6 -0
@@ -37,6 +37,7 @@#define JZ_ADC_REG_CFG_SAMPLE_NUM (n) ((n) << 10)#define JZ_ADC_REG_CFG_PULL_UP (n) ((n) << 16)#define JZ_ADC_REG_CFG_CMD_SEL BIT(22) +#define JZ_ADC_REG_CFG_VBAT_SEL BIT(30)#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK (BIT(31) | GENMASK(23, 10))#define JZ_ADC_REG_ADCLK_CLKDIV_LSB 0#define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB 16
@@ -869,6 +870,10 @@ static int ingenic_adc_probe(struct platform_device *pdev)/* Put hardware in a known passive state. */ writeb (0x00 , adc->base +JZ_ADC_REG_ENABLE );writeb (0xff , adc->base +JZ_ADC_REG_CTRL ); + +if (!device_property_present (dev,"ingenic,use-internal-divider" ))/* JZ4760B specific */ +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,0 ); +usleep_range (2000 ,3000 );/* Must wait at least 2ms. */ clk_disable (adc->clk);
@@ -896,6 +901,7 @@ static const struct of_device_id ingenic_adc_of_match[] = { { .compatible = "ingenic,jz4725b-adc", .data = &jz4725b_adc_soc_data, }, { .compatible = "ingenic,jz4740-adc", .data = &jz4740_adc_soc_data, }, { .compatible = "ingenic,jz4760-adc", .data = &jz4760_adc_soc_data, }, + { .compatible = "ingenic,jz4760b-adc", .data = &jz4760_adc_soc_data, }, { .compatible = "ingenic,jz4770-adc", .data = &jz4770_adc_soc_data, }, { }, };
Signature · 1 line
2.30.2
Paul Cercueil
maintainerRe: [PATCH 4/6] iio/adc: ingenic: add JZ4760B support to the sadc driver
Hi Christophe,
Quoted 24 lines · Le mer., juil. 21 2021 at 12:53:15 +0200, citral23 <[email protected]> a…
The JZ4760B variant differs slightly from the JZ4760, in that it has a bit called
VBAT_SELin the CFG register. In order to correctly sample the battery voltage on existing handhelds using this SOC, the bit must be cleared.We leave the possibility to set the bit, by using the "ingenic,use-internal-divider" in the devicetree.
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/iio/adc/ingenic-adc.c +1
-0@@ -37,6 +37,7 @@#define JZ_ADC_REG_CFG_SAMPLE_NUM (n) ((n) << 10)#define JZ_ADC_REG_CFG_PULL_UP (n) ((n) << 16)#define JZ_ADC_REG_CFG_CMD_SEL BIT(22) +#define JZ_ADC_REG_CFG_VBAT_SEL BIT(30)#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK (BIT(31) | GENMASK(23, 10))#define JZ_ADC_REG_ADCLK_CLKDIV_LSB 0#define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB 16
drivers/iio/adc/ingenic-adc.c static int ingenic_adc_probe(struct · line 877
Quoted 8 lines
drivers/iio/adc/ingenic-adc.c +3
-0@@ -869,6 +870,10 @@ static int ingenic_adc_probe(struct platform_device *pdev)/* Put hardware in a known passive state. */ writeb(0x00, adc->base + JZ_ADC_REG_ENABLE);writeb (0xff , adc->base +JZ_ADC_REG_CTRL ); + +if (!device_property_present (dev,"ingenic,use-internal-divider" ))/* JZ4760B specific */ +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_VBAT_SEL ,0 );
You miss an "else" part, no? Set the bit if the property is present, clear it if it is missing? You can't really rely on the reset value, since (e.g.) the bootloader could have changed it.
Cheers,
-Paul
Quoted 21 lines
drivers/iio/adc/ingenic-adc.c +1
-0+usleep_range (2000 ,3000 );/* Must wait at least 2ms. */ drivers/iio/adc/ingenic-adc.c +0
-0clk_disable (adc->clk);@@ -896,6 +901,7 @@ static const struct of_device_id ingenic_adc_of_match[] = { { .compatible = "ingenic,jz4725b-adc", .data = &jz4725b_adc_soc_data, }, { .compatible = "ingenic,jz4740-adc", .data = &jz4740_adc_soc_data, }, { .compatible = "ingenic,jz4760-adc", .data = &jz4760_adc_soc_data,},
drivers/iio/adc/ingenic-adc.c +1
-0+ { .compatible = "ingenic,jz4760b-adc", .data =
&jz4760_adc_soc_data, },
{ .compatible = "ingenic,jz4770-adc", .data =&jz4770_adc_soc_data,
},
{ },
};
--
2.30.2
PATCH 5/6 iio/adc: ingenic: modify
In reply to citral23The current code does not allow to set MD to 0 to sample AUX0, fix it for the JZ4760(B).
--- drivers/iio/adc/ingenic-adc.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/iio/adc/ingenic-adc.c +16 -4
@@ -632,7 +632,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,struct iio_chan_spec const *chan, int *val) { - int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY); + int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);struct ingenic_adc *adc =iio_priv (iio_dev); ret =clk_enable (adc->clk);
@@ -642,11 +642,22 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev,return ret; } -/* We cannot sample AUX/AUX2 in parallel. */ +/* We cannot sample the aux channels in parallel. */ mutex_lock (&adc->aux_lock);if (adc->soc_data->has_aux_md && engine ==0 ) { - bit =BIT (chan->channel ==INGENIC_ADC_AUX2 ); -ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit); +switch (chan->channel) { +case INGENIC_ADC_AUX0 : + cmd =0 ; +break ; +case INGENIC_ADC_AUX : + cmd =1 ; +break ; +case INGENIC_ADC_AUX2 : + cmd =2 ; +break ; + } + +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , cmd); } ret =ingenic_adc_capture (adc, engine);
@@ -654,6 +665,7 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev, goto out;switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 : *val =readw (adc->base +JZ_ADC_REG_ADSDAT );
Signature · 1 line
2.30.2
Paul Cercueil
maintainerRe: [PATCH 5/6] iio/adc: ingenic: modify
Hi Christophe,
Quoted 1 line · Le mer., juil. 21 2021 at 12:53:16 +0200, citral23 <[email protected]> a…
The current code does not allow to set MD to 0 to sample AUX0, fix it for the JZ4760(B).
Well, then this should be merged with patch 3, because that means JZ4760 support does not work without it.
Also, concise commit messages are good, but "modify" is a bit too concise ;)
Cheers,
-Paul
Quoted 58 lines
Signed-off-by citral23 <[email protected]>--- drivers/iio/adc/ingenic-adc.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)drivers/iio/adc/ingenic-adc.c +1
-1@@ -632,7 +632,7 @@ static int ingenic_adc_read_chan_info_raw(structiio_dev *iio_dev, struct iio_chan_spec const *chan, int *val) { - int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY); + int cmd, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);struct ingenic_adc *adc =iio_priv (iio_dev);drivers/iio/adc/ingenic-adc.c +14
-3ret =clk_enable (adc->clk);@@ -642,11 +642,22 @@ static int ingenic_adc_read_chan_info_raw(struct iio_dev *iio_dev, return ret; } -/* We cannot sample AUX/AUX2 in parallel. */ +/* We cannot sample the aux channels in parallel. */ mutex_lock (&adc->aux_lock);if (adc->soc_data->has_aux_md && engine ==0 ) { - bit =BIT (chan->channel ==INGENIC_ADC_AUX2 ); -ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , bit); +switch (chan->channel) { +case INGENIC_ADC_AUX0 : + cmd =0 ; +break ; +case INGENIC_ADC_AUX : + cmd =1 ; +break ; +case INGENIC_ADC_AUX2 : + cmd =2 ; +break ; + } + +ingenic_adc_set_config (adc,JZ_ADC_REG_CFG_AUX_MD , cmd); }drivers/iio/adc/ingenic-adc.c +1
-0ret =ingenic_adc_capture (adc, engine);@@ -654,6 +665,7 @@ static int ingenic_adc_read_chan_info_raw(structiio_dev *iio_dev, goto out;switch (chan->channel) { +case INGENIC_ADC_AUX0 :case INGENIC_ADC_AUX :case INGENIC_ADC_AUX2 :*val =
readw(adc->base + JZ_ADC_REG_ADSDAT);
--
2.30.2
PATCH 6/6 dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
In reply to citral23--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +9 -0
@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':
@@ -43,6 +45,12 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG register + to sample the battery voltage from the internal divider. If absent, it + will sample the external divider. + required: - compatible - '#io-channel-cells'
@@ -53,6 +61,7 @@ required: additionalProperties: false + examples: - | #include <dt-bindings/clock/jz4740-cgu.h>
Signature · 1 line
2.30.2
Paul Cercueil
maintainerRe: [PATCH 6/6] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
Hi Christophe,
Please always add a short description in your patches, even if all you do is repeat the patch title.
Quoted 26 lines · Le mer., juil. 21 2021 at 12:53:17 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +5
-0@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':@@ -43,6 +45,12 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFG
Quoted 4 lines
register
+ to sample the battery voltage from the internal divider. If
absent, it
+ will sample the external divider.
Please remove trailing spaces. And you don't need to describe internal behaviour; you only need to explain the functionality in a user-facing perspective. Something like:
"If present, battery voltage is read from the VBAT_IR pin, which has an internal /4 divider. If absent, it is read through the VBAT_ER pin, which does not have such divider."
You also don't specify the type of the property, please add "type: boolean" before the description.
There should also be a way to make sure that this property can only be used with the JZ4760B SoC. So a dependency for this vendor property on the "ingenic,jz4760b-adc" compatible string. But I'm honestly not sure how to express that... Maybe Rob can help.
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml required: · line 64
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +2
-0+ required: - compatible - '#io-channel-cells'@@ -53,6 +61,7 @@ required: additionalProperties: false +
Remove the extra newline.
Cheers,
-Paul
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +0
-1examples: - | #include <dt-bindings/clock/jz4740-cgu.h> --2.30.2
Re: [PATCH 6/6] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
Quoted 49 lines · On Wed, 21 Jul 2021 20:17:45 +0100 Paul Cercueil <[email protected]> wrote:
Hi Christophe,
Please always add a short description in your patches, even if all you do is repeat the patch title.
Quoted 28 lines · Le mer., juil. 21 2021 at 12:53:17 +0200, citral23 <[email protected]> a…
Signed-off-by citral23 <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)(quoted diff) +0
-0
a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yamlb/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yamlindex433a3fb55a2e..1b423adba61d100644Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +5
-0@@ -23,6 +23,8 @@ properties: enum: - ingenic,jz4725b-adc - ingenic,jz4740-adc + - ingenic,jz4760-adc + - ingenic,jz4760b-adc - ingenic,jz4770-adc '#io-channel-cells':@@ -43,6 +45,12 @@ properties: interrupts: maxItems: 1 + ingenic,use-internal-divider: + description: + This property can be used to set VBAT_SEL in the JZ4760B CFGQuoted 4 lines
register
+ to sample the battery voltage from the internal divider. If
absent, it
+ will sample the external divider.Please remove trailing spaces. And you don't need to describe internal behaviour; you only need to explain the functionality in a user-facing perspective. Something like:
"If present, battery voltage is read from the
VBAT_IRpin, which has an internal /4 divider. If absent, it is read through theVBAT_ERpin, which does not have such divider."
Quoted 7 lines
You also don't specify the type of the property, please add "type:
boolean" before the description.There should also be a way to make sure that this property can only be used with the JZ4760B SoC. So a dependency for this vendor property on the "ingenic,jz4760b-adc" compatible string. But I'm honestly not sure how to express that... Maybe Rob can help.
Lots of examples in tree.
e.g.
https://elixir.bootlin.com/linux/v5.14-rc2/source/Documentation/devicetree/bindings/iio/st,st-sensors.yaml#L153
Basically you have an if block matching the compatible and for non matches set it to false. That combined with additionaProperties: false enforces the property can't exist for those other devices.
Quoted 24 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml required: · line 64
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +2
-0+ required: - compatible - '#io-channel-cells'@@ -53,6 +61,7 @@ required: additionalProperties: false +Remove the extra newline.
Cheers,
-PaulQuoted 6 lines
Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml +0
-1examples: - | #include <dt-bindings/clock/jz4740-cgu.h> --2.30.2
Rob Herring
maintainerRe: [PATCH 6/6] dt-bindings: iio/adc: ingenic: add the JZ4760(B) socs to the sadc Documentation
In reply to citral23Quoted 5 lines · On Wed, 21 Jul 2021 12:53:17 +0200, citral23 wrote:
Signed-off-by citral23 <[email protected]>--- .../devicetree/bindings/iio/adc/ingenic,adc.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml: properties:ingenic,use-internal-divider: 'oneOf' conditional failed, one must be fixed:
'type' is a required property
hint: A vendor boolean property can use "type: boolean"
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml: properties:ingenic,use-internal-divider: 'oneOf' conditional failed, one must be fixed:
'enum' is a required property
'const' is a required property
hint: A vendor string property with exact values has an implicit type
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml: properties:ingenic,use-internal-divider: 'oneOf' conditional failed, one must be fixed:
'$ref' is a required property
'allOf' is a required property
hint: A vendor property needs a $ref to types.yaml
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml: ignoring, error in schema: properties: ingenic,use-internal-divider
warning: no schema found in file: ./Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml Documentation/devicetree/bindings/iio/adc/ingenic,adc.example.dt.yaml:0:0: /example-0/adc@10070000: failed to match any schema with compatible: ['ingenic,jz4740-adc'] \ndoc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1508137
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.