| Message ID | 20220106125947.139523-4-gengcixi@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: sc27xx: adjust structure and add PMIC's support | expand |
On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > From: Cixi Geng <cixi.geng1@unisoc.com> > > Introduce one variant device data structure to be compatible > with SC2731 PMIC since it has different scale and ratio calculation > and so on. > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > --- > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > 1 file changed, 79 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > index aee076c8e2b1..d2712e54ee79 100644 > --- a/drivers/iio/adc/sc27xx_adc.c > +++ b/drivers/iio/adc/sc27xx_adc.c > @@ -12,9 +12,9 @@ > #include <linux/slab.h> > > /* PMIC global registers definition */ > -#define SC27XX_MODULE_EN 0xc08 > +#define SC2731_MODULE_EN 0xc08 > #define SC27XX_MODULE_ADC_EN BIT(5) > -#define SC27XX_ARM_CLK_EN 0xc10 > +#define SC2731_ARM_CLK_EN 0xc10 > #define SC27XX_CLK_ADC_EN BIT(5) > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > u32 base; > int irq; > + const struct sc27xx_adc_variant_data *var_data; > +}; > + > +/* > + * Since different PMICs of SC27xx series can have different > + * address and ratio, we should save ratio config and base > + * in the device data structure. > + */ > +struct sc27xx_adc_variant_data { > + u32 module_en; > + u32 clk_en; > + u32 scale_shift; > + u32 scale_mask; > + const struct sc27xx_adc_linear_graph *bscale_cal; > + const struct sc27xx_adc_linear_graph *sscale_cal; > + void (*init_scale)(struct sc27xx_adc_data *data); > + int (*get_ratio)(int channel, int scale); > }; > > struct sc27xx_adc_linear_graph { > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > 100, 341, > }; > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > + 4200, 850, > + 3600, 728, > +}; > + > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > + 1000, 838, > + 100, 84, > +}; The original big_scale_graph_calib and small_scale_graph_calib are for SC2731 PMIC, why add new structure definition for SC2731? > + > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > 4200, 856, > 3600, 733, > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > size_t len; > > if (big_scale) { > - calib_graph = &big_scale_graph_calib; > + calib_graph = data->var_data->bscale_cal; > graph = &big_scale_graph; > cell_name = "big_scale_calib"; > } else { > - calib_graph = &small_scale_graph_calib; > + calib_graph = data->var_data->sscale_cal; > graph = &small_scale_graph; > cell_name = "small_scale_calib"; > } > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > return 0; > } > > -static int sc27xx_adc_get_ratio(int channel, int scale) > +static int sc2731_adc_get_ratio(int channel, int scale) > { > switch (channel) { > case 1: > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > return SC27XX_VOLT_RATIO(1, 1); > } > > +/* > + * According to the datasheet set specific value on some channel. > + */ > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > +{ > + int i; > + > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > + if (i == 5) > + data->channel_scale[i] = 1; > + else > + data->channel_scale[i] = 0; > + } > +} This is unnecessary I think, please see sc27xx_adc_write_raw() that can set the channel scale. > + > static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > int scale, int *val) > { > @@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > goto disable_adc; > > /* Configure the channel id and scale */ > - tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK; > + tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask; > tmp |= channel & SC27XX_ADC_CHN_ID_MASK; > ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG, > - SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK, > + SC27XX_ADC_CHN_ID_MASK | > + data->var_data->scale_mask, > tmp); > if (ret) > goto disable_adc; > @@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data, > int channel, int scale, > u32 *div_numerator, u32 *div_denominator) > { > - u32 ratio = sc27xx_adc_get_ratio(channel, scale); > + u32 ratio; > > + ratio = data->var_data->get_ratio(channel, scale); > *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET; > *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK; > } > @@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > { > int ret; > > - ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > + ret = regmap_update_bits(data->regmap, data->var_data->module_en, > SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN); > if (ret) > return ret; > > /* Enable ADC work clock and controller clock */ > - ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > + ret = regmap_update_bits(data->regmap, data->var_data->clk_en, > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN); > if (ret) > @@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > return 0; > > disable_clk: > - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > + regmap_update_bits(data->regmap, data->var_data->clk_en, > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); > disable_adc: > - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > + regmap_update_bits(data->regmap, data->var_data->module_en, > SC27XX_MODULE_ADC_EN, 0); > > return ret; > @@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data) > struct sc27xx_adc_data *data = _data; > > /* Disable ADC work clock and controller clock */ > - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > + regmap_update_bits(data->regmap, data->var_data->clk_en, > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); > > - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > + regmap_update_bits(data->regmap, data->var_data->module_en, > SC27XX_MODULE_ADC_EN, 0); > } > > +static const struct sc27xx_adc_variant_data sc2731_data = { > + .module_en = SC2731_MODULE_EN, > + .clk_en = SC2731_ARM_CLK_EN, > + .scale_shift = SC27XX_ADC_SCALE_SHIFT, > + .scale_mask = SC27XX_ADC_SCALE_MASK, > + .bscale_cal = &sc2731_big_scale_graph_calib, > + .sscale_cal = &sc2731_small_scale_graph_calib, > + .init_scale = sc2731_adc_scale_init, > + .get_ratio = sc2731_adc_get_ratio, > +}; > + > static int sc27xx_adc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct sc27xx_adc_data *sc27xx_data; > + const struct sc27xx_adc_variant_data *pdata; > struct iio_dev *indio_dev; > int ret; > > + pdata = of_device_get_match_data(dev); > + if (!pdata) { > + dev_err(dev, "No matching driver data found\n"); > + return -EINVAL; > + } > + > indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data)); > if (!indio_dev) > return -ENOMEM; > @@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > } > > sc27xx_data->dev = dev; > + sc27xx_data->var_data = pdata; > + sc27xx_data->var_data->init_scale(sc27xx_data); > > ret = sc27xx_adc_enable(sc27xx_data); > if (ret) { > @@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > } > > static const struct of_device_id sc27xx_adc_of_match[] = { > - { .compatible = "sprd,sc2731-adc", }, > + { .compatible = "sprd,sc2731-adc", .data = &sc2731_data}, > { } > }; > MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match); > -- > 2.25.1 >
Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > Introduce one variant device data structure to be compatible > > with SC2731 PMIC since it has different scale and ratio calculation > > and so on. > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > --- > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > index aee076c8e2b1..d2712e54ee79 100644 > > --- a/drivers/iio/adc/sc27xx_adc.c > > +++ b/drivers/iio/adc/sc27xx_adc.c > > @@ -12,9 +12,9 @@ > > #include <linux/slab.h> > > > > /* PMIC global registers definition */ > > -#define SC27XX_MODULE_EN 0xc08 > > +#define SC2731_MODULE_EN 0xc08 > > #define SC27XX_MODULE_ADC_EN BIT(5) > > -#define SC27XX_ARM_CLK_EN 0xc10 > > +#define SC2731_ARM_CLK_EN 0xc10 > > #define SC27XX_CLK_ADC_EN BIT(5) > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > u32 base; > > int irq; > > + const struct sc27xx_adc_variant_data *var_data; > > +}; > > + > > +/* > > + * Since different PMICs of SC27xx series can have different > > + * address and ratio, we should save ratio config and base > > + * in the device data structure. > > + */ > > +struct sc27xx_adc_variant_data { > > + u32 module_en; > > + u32 clk_en; > > + u32 scale_shift; > > + u32 scale_mask; > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > + void (*init_scale)(struct sc27xx_adc_data *data); > > + int (*get_ratio)(int channel, int scale); > > }; > > > > struct sc27xx_adc_linear_graph { > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > 100, 341, > > }; > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > + 4200, 850, > > + 3600, 728, > > +}; > > + > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > + 1000, 838, > > + 100, 84, > > +}; > > The original big_scale_graph_calib and small_scale_graph_calib are for > SC2731 PMIC, why add new structure definition for SC2731? > > > + > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > 4200, 856, > > 3600, 733, > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > size_t len; > > > > if (big_scale) { > > - calib_graph = &big_scale_graph_calib; > > + calib_graph = data->var_data->bscale_cal; > > graph = &big_scale_graph; > > cell_name = "big_scale_calib"; > > } else { > > - calib_graph = &small_scale_graph_calib; > > + calib_graph = data->var_data->sscale_cal; > > graph = &small_scale_graph; > > cell_name = "small_scale_calib"; > > } > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > return 0; > > } > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > +static int sc2731_adc_get_ratio(int channel, int scale) > > { > > switch (channel) { > > case 1: > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > return SC27XX_VOLT_RATIO(1, 1); > > } > > > > +/* > > + * According to the datasheet set specific value on some channel. > > + */ > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > +{ > > + int i; > > + > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > + if (i == 5) > > + data->channel_scale[i] = 1; > > + else > > + data->channel_scale[i] = 0; > > + } > > +} > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > can set the channel scale. Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw? but the scale_init is all different by each PMIC, if implemented in the write_raw, will add a lot of if or switch_case branch > > > + > > static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > > int scale, int *val) > > { > > @@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, > > goto disable_adc; > > > > /* Configure the channel id and scale */ > > - tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK; > > + tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask; > > tmp |= channel & SC27XX_ADC_CHN_ID_MASK; > > ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG, > > - SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK, > > + SC27XX_ADC_CHN_ID_MASK | > > + data->var_data->scale_mask, > > tmp); > > if (ret) > > goto disable_adc; > > @@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data, > > int channel, int scale, > > u32 *div_numerator, u32 *div_denominator) > > { > > - u32 ratio = sc27xx_adc_get_ratio(channel, scale); > > + u32 ratio; > > > > + ratio = data->var_data->get_ratio(channel, scale); > > *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET; > > *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK; > > } > > @@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > > { > > int ret; > > > > - ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > > + ret = regmap_update_bits(data->regmap, data->var_data->module_en, > > SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN); > > if (ret) > > return ret; > > > > /* Enable ADC work clock and controller clock */ > > - ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > > + ret = regmap_update_bits(data->regmap, data->var_data->clk_en, > > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, > > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN); > > if (ret) > > @@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) > > return 0; > > > > disable_clk: > > - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > > + regmap_update_bits(data->regmap, data->var_data->clk_en, > > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); > > disable_adc: > > - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > > + regmap_update_bits(data->regmap, data->var_data->module_en, > > SC27XX_MODULE_ADC_EN, 0); > > > > return ret; > > @@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data) > > struct sc27xx_adc_data *data = _data; > > > > /* Disable ADC work clock and controller clock */ > > - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, > > + regmap_update_bits(data->regmap, data->var_data->clk_en, > > SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); > > > > - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, > > + regmap_update_bits(data->regmap, data->var_data->module_en, > > SC27XX_MODULE_ADC_EN, 0); > > } > > > > +static const struct sc27xx_adc_variant_data sc2731_data = { > > + .module_en = SC2731_MODULE_EN, > > + .clk_en = SC2731_ARM_CLK_EN, > > + .scale_shift = SC27XX_ADC_SCALE_SHIFT, > > + .scale_mask = SC27XX_ADC_SCALE_MASK, > > + .bscale_cal = &sc2731_big_scale_graph_calib, > > + .sscale_cal = &sc2731_small_scale_graph_calib, > > + .init_scale = sc2731_adc_scale_init, > > + .get_ratio = sc2731_adc_get_ratio, > > +}; > > + > > static int sc27xx_adc_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > struct sc27xx_adc_data *sc27xx_data; > > + const struct sc27xx_adc_variant_data *pdata; > > struct iio_dev *indio_dev; > > int ret; > > > > + pdata = of_device_get_match_data(dev); > > + if (!pdata) { > > + dev_err(dev, "No matching driver data found\n"); > > + return -EINVAL; > > + } > > + > > indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data)); > > if (!indio_dev) > > return -ENOMEM; > > @@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > > } > > > > sc27xx_data->dev = dev; > > + sc27xx_data->var_data = pdata; > > + sc27xx_data->var_data->init_scale(sc27xx_data); > > > > ret = sc27xx_adc_enable(sc27xx_data); > > if (ret) { > > @@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev) > > } > > > > static const struct of_device_id sc27xx_adc_of_match[] = { > > - { .compatible = "sprd,sc2731-adc", }, > > + { .compatible = "sprd,sc2731-adc", .data = &sc2731_data}, > > { } > > }; > > MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match); > > -- > > 2.25.1 > > > > > -- > Baolin Wang
On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > Introduce one variant device data structure to be compatible > > > with SC2731 PMIC since it has different scale and ratio calculation > > > and so on. > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > --- > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > index aee076c8e2b1..d2712e54ee79 100644 > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > @@ -12,9 +12,9 @@ > > > #include <linux/slab.h> > > > > > > /* PMIC global registers definition */ > > > -#define SC27XX_MODULE_EN 0xc08 > > > +#define SC2731_MODULE_EN 0xc08 > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > u32 base; > > > int irq; > > > + const struct sc27xx_adc_variant_data *var_data; > > > +}; > > > + > > > +/* > > > + * Since different PMICs of SC27xx series can have different > > > + * address and ratio, we should save ratio config and base > > > + * in the device data structure. > > > + */ > > > +struct sc27xx_adc_variant_data { > > > + u32 module_en; > > > + u32 clk_en; > > > + u32 scale_shift; > > > + u32 scale_mask; > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > + int (*get_ratio)(int channel, int scale); > > > }; > > > > > > struct sc27xx_adc_linear_graph { > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > 100, 341, > > > }; > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > + 4200, 850, > > > + 3600, 728, > > > +}; > > > + > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > + 1000, 838, > > > + 100, 84, > > > +}; > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > + > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > 4200, 856, > > > 3600, 733, > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > size_t len; > > > > > > if (big_scale) { > > > - calib_graph = &big_scale_graph_calib; > > > + calib_graph = data->var_data->bscale_cal; > > > graph = &big_scale_graph; > > > cell_name = "big_scale_calib"; > > > } else { > > > - calib_graph = &small_scale_graph_calib; > > > + calib_graph = data->var_data->sscale_cal; > > > graph = &small_scale_graph; > > > cell_name = "small_scale_calib"; > > > } > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > return 0; > > > } > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > { > > > switch (channel) { > > > case 1: > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > return SC27XX_VOLT_RATIO(1, 1); > > > } > > > > > > +/* > > > + * According to the datasheet set specific value on some channel. > > > + */ > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > + if (i == 5) > > > + data->channel_scale[i] = 1; > > > + else > > > + data->channel_scale[i] = 0; > > > + } > > > +} > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > can set the channel scale. > Did you mean that all the PMIC's scale_init function should put into > the sc27xx_adc_write_raw? No. > but the scale_init is all different by each PMIC, if implemented in > the write_raw, will add a lot of > if or switch_case branch What I mean is we should follow the original method to set the channel scale by iio_info. Please also refer to other drivers how ot handle the channel scale.
Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道： > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > Introduce one variant device data structure to be compatible > > > > with SC2731 PMIC since it has different scale and ratio calculation > > > > and so on. > > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > > --- > > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > > index aee076c8e2b1..d2712e54ee79 100644 > > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > > @@ -12,9 +12,9 @@ > > > > #include <linux/slab.h> > > > > > > > > /* PMIC global registers definition */ > > > > -#define SC27XX_MODULE_EN 0xc08 > > > > +#define SC2731_MODULE_EN 0xc08 > > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > > u32 base; > > > > int irq; > > > > + const struct sc27xx_adc_variant_data *var_data; > > > > +}; > > > > + > > > > +/* > > > > + * Since different PMICs of SC27xx series can have different > > > > + * address and ratio, we should save ratio config and base > > > > + * in the device data structure. > > > > + */ > > > > +struct sc27xx_adc_variant_data { > > > > + u32 module_en; > > > > + u32 clk_en; > > > > + u32 scale_shift; > > > > + u32 scale_mask; > > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > > + int (*get_ratio)(int channel, int scale); > > > > }; > > > > > > > > struct sc27xx_adc_linear_graph { > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > > 100, 341, > > > > }; > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > > + 4200, 850, > > > > + 3600, 728, > > > > +}; > > > > + > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > > + 1000, 838, > > > > + 100, 84, > > > > +}; > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > > > + > > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > > 4200, 856, > > > > 3600, 733, > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > size_t len; > > > > > > > > if (big_scale) { > > > > - calib_graph = &big_scale_graph_calib; > > > > + calib_graph = data->var_data->bscale_cal; > > > > graph = &big_scale_graph; > > > > cell_name = "big_scale_calib"; > > > > } else { > > > > - calib_graph = &small_scale_graph_calib; > > > > + calib_graph = data->var_data->sscale_cal; > > > > graph = &small_scale_graph; > > > > cell_name = "small_scale_calib"; > > > > } > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > return 0; > > > > } > > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > > { > > > > switch (channel) { > > > > case 1: > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > > return SC27XX_VOLT_RATIO(1, 1); > > > > } > > > > > > > > +/* > > > > + * According to the datasheet set specific value on some channel. > > > > + */ > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > > + if (i == 5) > > > > + data->channel_scale[i] = 1; > > > > + else > > > > + data->channel_scale[i] = 0; > > > > + } > > > > +} > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > > can set the channel scale. > > Did you mean that all the PMIC's scale_init function should put into > > the sc27xx_adc_write_raw? > > No. > > > but the scale_init is all different by each PMIC, if implemented in > > the write_raw, will add a lot of > > if or switch_case branch > > What I mean is we should follow the original method to set the channel > scale by iio_info. Please also refer to other drivers how ot handle > the channel scale. Hi Baolin, I understand the adc_write_raw() function is the method to set channal scale for the userspace, we can change the channel scale by write a value on a user code. did i understand right? out scale_init is to set scale value when the driver probe stage, and I also did not found other adc driver use the adc_write_raw() during the driver initialization phase. > > -- > Baolin Wang
On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote: > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道： > > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > > > Introduce one variant device data structure to be compatible > > > > > with SC2731 PMIC since it has different scale and ratio calculation > > > > > and so on. > > > > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > > > --- > > > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > > > index aee076c8e2b1..d2712e54ee79 100644 > > > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > > > @@ -12,9 +12,9 @@ > > > > > #include <linux/slab.h> > > > > > > > > > > /* PMIC global registers definition */ > > > > > -#define SC27XX_MODULE_EN 0xc08 > > > > > +#define SC2731_MODULE_EN 0xc08 > > > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > > > u32 base; > > > > > int irq; > > > > > + const struct sc27xx_adc_variant_data *var_data; > > > > > +}; > > > > > + > > > > > +/* > > > > > + * Since different PMICs of SC27xx series can have different > > > > > + * address and ratio, we should save ratio config and base > > > > > + * in the device data structure. > > > > > + */ > > > > > +struct sc27xx_adc_variant_data { > > > > > + u32 module_en; > > > > > + u32 clk_en; > > > > > + u32 scale_shift; > > > > > + u32 scale_mask; > > > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > > > + int (*get_ratio)(int channel, int scale); > > > > > }; > > > > > > > > > > struct sc27xx_adc_linear_graph { > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > > > 100, 341, > > > > > }; > > > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > > > + 4200, 850, > > > > > + 3600, 728, > > > > > +}; > > > > > + > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > > > + 1000, 838, > > > > > + 100, 84, > > > > > +}; > > > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > > > > > + > > > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > > > 4200, 856, > > > > > 3600, 733, > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > size_t len; > > > > > > > > > > if (big_scale) { > > > > > - calib_graph = &big_scale_graph_calib; > > > > > + calib_graph = data->var_data->bscale_cal; > > > > > graph = &big_scale_graph; > > > > > cell_name = "big_scale_calib"; > > > > > } else { > > > > > - calib_graph = &small_scale_graph_calib; > > > > > + calib_graph = data->var_data->sscale_cal; > > > > > graph = &small_scale_graph; > > > > > cell_name = "small_scale_calib"; > > > > > } > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > return 0; > > > > > } > > > > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > > > { > > > > > switch (channel) { > > > > > case 1: > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > return SC27XX_VOLT_RATIO(1, 1); > > > > > } > > > > > > > > > > +/* > > > > > + * According to the datasheet set specific value on some channel. > > > > > + */ > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > > > + if (i == 5) > > > > > + data->channel_scale[i] = 1; > > > > > + else > > > > > + data->channel_scale[i] = 0; > > > > > + } > > > > > +} > > > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > > > can set the channel scale. > > > Did you mean that all the PMIC's scale_init function should put into > > > the sc27xx_adc_write_raw? > > > > No. > > > > > but the scale_init is all different by each PMIC, if implemented in > > > the write_raw, will add a lot of > > > if or switch_case branch > > > > What I mean is we should follow the original method to set the channel > > scale by iio_info. Please also refer to other drivers how ot handle > > the channel scale. > Hi Baolin, I understand the adc_write_raw() function is the method to set > channal scale for the userspace, we can change the channel scale by write > a value on a user code. did i understand right? > out scale_init is to set scale value when the driver probe stage, and I also > did not found other adc driver use the adc_write_raw() during the driver > initialization phase. Hi Jonathan, How do you think about the method in this patch to set the channel scale? Thanks.
Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道： > > On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道： > > > > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > > > > > Introduce one variant device data structure to be compatible > > > > > > with SC2731 PMIC since it has different scale and ratio calculation > > > > > > and so on. > > > > > > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > --- > > > > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > > > > index aee076c8e2b1..d2712e54ee79 100644 > > > > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > > > > @@ -12,9 +12,9 @@ > > > > > > #include <linux/slab.h> > > > > > > > > > > > > /* PMIC global registers definition */ > > > > > > -#define SC27XX_MODULE_EN 0xc08 > > > > > > +#define SC2731_MODULE_EN 0xc08 > > > > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > > > > u32 base; > > > > > > int irq; > > > > > > + const struct sc27xx_adc_variant_data *var_data; > > > > > > +}; > > > > > > + > > > > > > +/* > > > > > > + * Since different PMICs of SC27xx series can have different > > > > > > + * address and ratio, we should save ratio config and base > > > > > > + * in the device data structure. > > > > > > + */ > > > > > > +struct sc27xx_adc_variant_data { > > > > > > + u32 module_en; > > > > > > + u32 clk_en; > > > > > > + u32 scale_shift; > > > > > > + u32 scale_mask; > > > > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > > > > + int (*get_ratio)(int channel, int scale); > > > > > > }; > > > > > > > > > > > > struct sc27xx_adc_linear_graph { > > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > > > > 100, 341, > > > > > > }; > > > > > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > > > > + 4200, 850, > > > > > > + 3600, 728, > > > > > > +}; > > > > > > + > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > > > > + 1000, 838, > > > > > > + 100, 84, > > > > > > +}; > > > > > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > > > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > > > > > > > + > > > > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > > > > 4200, 856, > > > > > > 3600, 733, > > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > size_t len; > > > > > > > > > > > > if (big_scale) { > > > > > > - calib_graph = &big_scale_graph_calib; > > > > > > + calib_graph = data->var_data->bscale_cal; > > > > > > graph = &big_scale_graph; > > > > > > cell_name = "big_scale_calib"; > > > > > > } else { > > > > > > - calib_graph = &small_scale_graph_calib; > > > > > > + calib_graph = data->var_data->sscale_cal; > > > > > > graph = &small_scale_graph; > > > > > > cell_name = "small_scale_calib"; > > > > > > } > > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > > > > { > > > > > > switch (channel) { > > > > > > case 1: > > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > return SC27XX_VOLT_RATIO(1, 1); > > > > > > } > > > > > > > > > > > > +/* > > > > > > + * According to the datasheet set specific value on some channel. > > > > > > + */ > > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > > > > +{ > > > > > > + int i; > > > > > > + > > > > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > > > > + if (i == 5) > > > > > > + data->channel_scale[i] = 1; > > > > > > + else > > > > > > + data->channel_scale[i] = 0; > > > > > > + } > > > > > > +} > > > > > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > > > > can set the channel scale. > > > > Did you mean that all the PMIC's scale_init function should put into > > > > the sc27xx_adc_write_raw? > > > > > > No. > > > > > > > but the scale_init is all different by each PMIC, if implemented in > > > > the write_raw, will add a lot of > > > > if or switch_case branch > > > > > > What I mean is we should follow the original method to set the channel > > > scale by iio_info. Please also refer to other drivers how ot handle > > > the channel scale. > > Hi Baolin, I understand the adc_write_raw() function is the method to set > > channal scale for the userspace, we can change the channel scale by write > > a value on a user code. did i understand right? > > out scale_init is to set scale value when the driver probe stage, and I also > > did not found other adc driver use the adc_write_raw() during the driver > > initialization phase. > > Hi Jonathan, > > How do you think about the method in this patch to set the channel > scale? Thanks. > Hi Jonathan, Could you have a loot at this patch ,and give some advice about the method to set the channel scale? Thanks very much. > -- > Baolin Wang
On Wed, 23 Feb 2022 20:46:08 +0800 Cixi Geng <gengcixi@gmail.com> wrote: > Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道： > > > > On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道： > > > > > > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > > > > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > > > > > > > Introduce one variant device data structure to be compatible > > > > > > > with SC2731 PMIC since it has different scale and ratio calculation > > > > > > > and so on. > > > > > > > > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > --- > > > > > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > > > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > > > > > index aee076c8e2b1..d2712e54ee79 100644 > > > > > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > > > > > @@ -12,9 +12,9 @@ > > > > > > > #include <linux/slab.h> > > > > > > > > > > > > > > /* PMIC global registers definition */ > > > > > > > -#define SC27XX_MODULE_EN 0xc08 > > > > > > > +#define SC2731_MODULE_EN 0xc08 > > > > > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > > > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > > > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > > > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > > > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > > > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > > > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > > > > > u32 base; > > > > > > > int irq; > > > > > > > + const struct sc27xx_adc_variant_data *var_data; > > > > > > > +}; > > > > > > > + > > > > > > > +/* > > > > > > > + * Since different PMICs of SC27xx series can have different > > > > > > > + * address and ratio, we should save ratio config and base > > > > > > > + * in the device data structure. > > > > > > > + */ > > > > > > > +struct sc27xx_adc_variant_data { > > > > > > > + u32 module_en; > > > > > > > + u32 clk_en; > > > > > > > + u32 scale_shift; > > > > > > > + u32 scale_mask; > > > > > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > > > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > > > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > > > > > + int (*get_ratio)(int channel, int scale); > > > > > > > }; > > > > > > > > > > > > > > struct sc27xx_adc_linear_graph { > > > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > > > > > 100, 341, > > > > > > > }; > > > > > > > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > > > > > + 4200, 850, > > > > > > > + 3600, 728, > > > > > > > +}; > > > > > > > + > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > > > > > + 1000, 838, > > > > > > > + 100, 84, > > > > > > > +}; > > > > > > > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > > > > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > > > > > > > > > + > > > > > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > > > > > 4200, 856, > > > > > > > 3600, 733, > > > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > > size_t len; > > > > > > > > > > > > > > if (big_scale) { > > > > > > > - calib_graph = &big_scale_graph_calib; > > > > > > > + calib_graph = data->var_data->bscale_cal; > > > > > > > graph = &big_scale_graph; > > > > > > > cell_name = "big_scale_calib"; > > > > > > > } else { > > > > > > > - calib_graph = &small_scale_graph_calib; > > > > > > > + calib_graph = data->var_data->sscale_cal; > > > > > > > graph = &small_scale_graph; > > > > > > > cell_name = "small_scale_calib"; > > > > > > > } > > > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > > > > > { > > > > > > > switch (channel) { > > > > > > > case 1: > > > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > > return SC27XX_VOLT_RATIO(1, 1); > > > > > > > } > > > > > > > > > > > > > > +/* > > > > > > > + * According to the datasheet set specific value on some channel. > > > > > > > + */ > > > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > > > > > +{ > > > > > > > + int i; > > > > > > > + > > > > > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > > > > > + if (i == 5) > > > > > > > + data->channel_scale[i] = 1; > > > > > > > + else > > > > > > > + data->channel_scale[i] = 0; > > > > > > > + } > > > > > > > +} > > > > > > > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > > > > > can set the channel scale. > > > > > Did you mean that all the PMIC's scale_init function should put into > > > > > the sc27xx_adc_write_raw? > > > > > > > > No. > > > > > > > > > but the scale_init is all different by each PMIC, if implemented in > > > > > the write_raw, will add a lot of > > > > > if or switch_case branch > > > > > > > > What I mean is we should follow the original method to set the channel > > > > scale by iio_info. Please also refer to other drivers how ot handle > > > > the channel scale. > > > Hi Baolin, I understand the adc_write_raw() function is the method to set > > > channal scale for the userspace, we can change the channel scale by write > > > a value on a user code. did i understand right? > > > out scale_init is to set scale value when the driver probe stage, and I also > > > did not found other adc driver use the adc_write_raw() during the driver > > > initialization phase. > > > > Hi Jonathan, > > > > How do you think about the method in this patch to set the channel > > scale? Thanks. > > > Hi Jonathan, > Could you have a loot at this patch ,and give some advice about the > method to set the channel scale? Thanks very much. Hi, thanks for poking me on this - I'd missed the question buried deep in the thread! Anyhow, I don't quite follow the discussion but think it could be focused on one of 2 questions... 1) Does setting an initial default make sense? Yes, this is an acceptable thing to do if there is a particular set of defaults and there is no risk of regressions (i.e. the device wasn't previously supported with different defaults). 2) Should you use the write_raw callback to actually do the setting? Probably not as it has a set of parameters that don't make as much sense from within the driver. It 'might' make sense to have a common _set() function for this feature which is called both in this initialization case and from the write_raw() function however as that could do bounds checking etc in one common place. However, it is very simple here, so perhaps not necessary. Jonathan > > -- > > Baolin Wang
Jonathan Cameron <Jonathan.Cameron@huawei.com> 于2022年2月25日周五 18:19写道： > > On Wed, 23 Feb 2022 20:46:08 +0800 > Cixi Geng <gengcixi@gmail.com> wrote: > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道： > > > > > > On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道： > > > > > > > > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道： > > > > > > > > > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote: > > > > > > > > > > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > > > > > > > > > Introduce one variant device data structure to be compatible > > > > > > > > with SC2731 PMIC since it has different scale and ratio calculation > > > > > > > > and so on. > > > > > > > > > > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com> > > > > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com> > > > > > > > > --- > > > > > > > > drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------ > > > > > > > > 1 file changed, 79 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c > > > > > > > > index aee076c8e2b1..d2712e54ee79 100644 > > > > > > > > --- a/drivers/iio/adc/sc27xx_adc.c > > > > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c > > > > > > > > @@ -12,9 +12,9 @@ > > > > > > > > #include <linux/slab.h> > > > > > > > > > > > > > > > > /* PMIC global registers definition */ > > > > > > > > -#define SC27XX_MODULE_EN 0xc08 > > > > > > > > +#define SC2731_MODULE_EN 0xc08 > > > > > > > > #define SC27XX_MODULE_ADC_EN BIT(5) > > > > > > > > -#define SC27XX_ARM_CLK_EN 0xc10 > > > > > > > > +#define SC2731_ARM_CLK_EN 0xc10 > > > > > > > > #define SC27XX_CLK_ADC_EN BIT(5) > > > > > > > > #define SC27XX_CLK_ADC_CLK_EN BIT(6) > > > > > > > > > > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data { > > > > > > > > int channel_scale[SC27XX_ADC_CHANNEL_MAX]; > > > > > > > > u32 base; > > > > > > > > int irq; > > > > > > > > + const struct sc27xx_adc_variant_data *var_data; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +/* > > > > > > > > + * Since different PMICs of SC27xx series can have different > > > > > > > > + * address and ratio, we should save ratio config and base > > > > > > > > + * in the device data structure. > > > > > > > > + */ > > > > > > > > +struct sc27xx_adc_variant_data { > > > > > > > > + u32 module_en; > > > > > > > > + u32 clk_en; > > > > > > > > + u32 scale_shift; > > > > > > > > + u32 scale_mask; > > > > > > > > + const struct sc27xx_adc_linear_graph *bscale_cal; > > > > > > > > + const struct sc27xx_adc_linear_graph *sscale_cal; > > > > > > > > + void (*init_scale)(struct sc27xx_adc_data *data); > > > > > > > > + int (*get_ratio)(int channel, int scale); > > > > > > > > }; > > > > > > > > > > > > > > > > struct sc27xx_adc_linear_graph { > > > > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { > > > > > > > > 100, 341, > > > > > > > > }; > > > > > > > > > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { > > > > > > > > + 4200, 850, > > > > > > > > + 3600, 728, > > > > > > > > +}; > > > > > > > > + > > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { > > > > > > > > + 1000, 838, > > > > > > > > + 100, 84, > > > > > > > > +}; > > > > > > > > > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for > > > > > > > SC2731 PMIC, why add new structure definition for SC2731? > > > > > > > > > > > > > > > + > > > > > > > > static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { > > > > > > > > 4200, 856, > > > > > > > > 3600, 733, > > > > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > > > size_t len; > > > > > > > > > > > > > > > > if (big_scale) { > > > > > > > > - calib_graph = &big_scale_graph_calib; > > > > > > > > + calib_graph = data->var_data->bscale_cal; > > > > > > > > graph = &big_scale_graph; > > > > > > > > cell_name = "big_scale_calib"; > > > > > > > > } else { > > > > > > > > - calib_graph = &small_scale_graph_calib; > > > > > > > > + calib_graph = data->var_data->sscale_cal; > > > > > > > > graph = &small_scale_graph; > > > > > > > > cell_name = "small_scale_calib"; > > > > > > > > } > > > > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > > > +static int sc2731_adc_get_ratio(int channel, int scale) > > > > > > > > { > > > > > > > > switch (channel) { > > > > > > > > case 1: > > > > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) > > > > > > > > return SC27XX_VOLT_RATIO(1, 1); > > > > > > > > } > > > > > > > > > > > > > > > > +/* > > > > > > > > + * According to the datasheet set specific value on some channel. > > > > > > > > + */ > > > > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) > > > > > > > > +{ > > > > > > > > + int i; > > > > > > > > + > > > > > > > > + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { > > > > > > > > + if (i == 5) > > > > > > > > + data->channel_scale[i] = 1; > > > > > > > > + else > > > > > > > > + data->channel_scale[i] = 0; > > > > > > > > + } > > > > > > > > +} > > > > > > > > > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that > > > > > > > can set the channel scale. > > > > > > Did you mean that all the PMIC's scale_init function should put into > > > > > > the sc27xx_adc_write_raw? > > > > > > > > > > No. > > > > > > > > > > > but the scale_init is all different by each PMIC, if implemented in > > > > > > the write_raw, will add a lot of > > > > > > if or switch_case branch > > > > > > > > > > What I mean is we should follow the original method to set the channel > > > > > scale by iio_info. Please also refer to other drivers how ot handle > > > > > the channel scale. > > > > Hi Baolin, I understand the adc_write_raw() function is the method to set > > > > channal scale for the userspace, we can change the channel scale by write > > > > a value on a user code. did i understand right? > > > > out scale_init is to set scale value when the driver probe stage, and I also > > > > did not found other adc driver use the adc_write_raw() during the driver > > > > initialization phase. > > > > > > Hi Jonathan, > > > > > > How do you think about the method in this patch to set the channel > > > scale? Thanks. > > > > > Hi Jonathan, > > Could you have a loot at this patch ,and give some advice about the > > method to set the channel scale? Thanks very much. > > Hi, thanks for poking me on this - I'd missed the question buried deep in the thread! > > Anyhow, I don't quite follow the discussion but think it could be focused > on one of 2 questions... > > 1) Does setting an initial default make sense? > Yes, this is an acceptable thing to do if there is a particular set of defaults > and there is no risk of regressions (i.e. the device wasn't previously supported > with different defaults). > 2) Should you use the write_raw callback to actually do the setting? > Probably not as it has a set of parameters that don't make as much sense from within > the driver. It 'might' make sense to have a common _set() function for this > feature which is called both in this initialization case and from the write_raw() > function however as that could do bounds checking etc in one common place. > However, it is very simple here, so perhaps not necessary. > > Jonathan > Hi Jonathan, thanks for your comment ! And Baolin, I will send a new verision for the patches tto keep the scale_init and fix other issues . thanks! > > > -- > > > Baolin Wang >
diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c index aee076c8e2b1..d2712e54ee79 100644 --- a/drivers/iio/adc/sc27xx_adc.c +++ b/drivers/iio/adc/sc27xx_adc.c @@ -12,9 +12,9 @@ #include <linux/slab.h> /* PMIC global registers definition */ -#define SC27XX_MODULE_EN 0xc08 +#define SC2731_MODULE_EN 0xc08 #define SC27XX_MODULE_ADC_EN BIT(5) -#define SC27XX_ARM_CLK_EN 0xc10 +#define SC2731_ARM_CLK_EN 0xc10 #define SC27XX_CLK_ADC_EN BIT(5) #define SC27XX_CLK_ADC_CLK_EN BIT(6) @@ -78,6 +78,23 @@ struct sc27xx_adc_data { int channel_scale[SC27XX_ADC_CHANNEL_MAX]; u32 base; int irq; + const struct sc27xx_adc_variant_data *var_data; +}; + +/* + * Since different PMICs of SC27xx series can have different + * address and ratio, we should save ratio config and base + * in the device data structure. + */ +struct sc27xx_adc_variant_data { + u32 module_en; + u32 clk_en; + u32 scale_shift; + u32 scale_mask; + const struct sc27xx_adc_linear_graph *bscale_cal; + const struct sc27xx_adc_linear_graph *sscale_cal; + void (*init_scale)(struct sc27xx_adc_data *data); + int (*get_ratio)(int channel, int scale); }; struct sc27xx_adc_linear_graph { @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = { 100, 341, }; +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = { + 4200, 850, + 3600, 728, +}; + +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = { + 1000, 838, + 100, 84, +}; + static const struct sc27xx_adc_linear_graph big_scale_graph_calib = { 4200, 856, 3600, 733, @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, size_t len; if (big_scale) { - calib_graph = &big_scale_graph_calib; + calib_graph = data->var_data->bscale_cal; graph = &big_scale_graph; cell_name = "big_scale_calib"; } else { - calib_graph = &small_scale_graph_calib; + calib_graph = data->var_data->sscale_cal; graph = &small_scale_graph; cell_name = "small_scale_calib"; } @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data, return 0; } -static int sc27xx_adc_get_ratio(int channel, int scale) +static int sc2731_adc_get_ratio(int channel, int scale) { switch (channel) { case 1: @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale) return SC27XX_VOLT_RATIO(1, 1); } +/* + * According to the datasheet set specific value on some channel. + */ +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data) +{ + int i; + + for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) { + if (i == 5) + data->channel_scale[i] = 1; + else + data->channel_scale[i] = 0; + } +} + static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, int scale, int *val) { @@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, goto disable_adc; /* Configure the channel id and scale */ - tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK; + tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask; tmp |= channel & SC27XX_ADC_CHN_ID_MASK; ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG, - SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK, + SC27XX_ADC_CHN_ID_MASK | + data->var_data->scale_mask, tmp); if (ret) goto disable_adc; @@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data, int channel, int scale, u32 *div_numerator, u32 *div_denominator) { - u32 ratio = sc27xx_adc_get_ratio(channel, scale); + u32 ratio; + ratio = data->var_data->get_ratio(channel, scale); *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET; *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK; } @@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) { int ret; - ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN, + ret = regmap_update_bits(data->regmap, data->var_data->module_en, SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN); if (ret) return ret; /* Enable ADC work clock and controller clock */ - ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, + ret = regmap_update_bits(data->regmap, data->var_data->clk_en, SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN); if (ret) @@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data) return 0; disable_clk: - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, + regmap_update_bits(data->regmap, data->var_data->clk_en, SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); disable_adc: - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, + regmap_update_bits(data->regmap, data->var_data->module_en, SC27XX_MODULE_ADC_EN, 0); return ret; @@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data) struct sc27xx_adc_data *data = _data; /* Disable ADC work clock and controller clock */ - regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN, + regmap_update_bits(data->regmap, data->var_data->clk_en, SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0); - regmap_update_bits(data->regmap, SC27XX_MODULE_EN, + regmap_update_bits(data->regmap, data->var_data->module_en, SC27XX_MODULE_ADC_EN, 0); } +static const struct sc27xx_adc_variant_data sc2731_data = { + .module_en = SC2731_MODULE_EN, + .clk_en = SC2731_ARM_CLK_EN, + .scale_shift = SC27XX_ADC_SCALE_SHIFT, + .scale_mask = SC27XX_ADC_SCALE_MASK, + .bscale_cal = &sc2731_big_scale_graph_calib, + .sscale_cal = &sc2731_small_scale_graph_calib, + .init_scale = sc2731_adc_scale_init, + .get_ratio = sc2731_adc_get_ratio, +}; + static int sc27xx_adc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct sc27xx_adc_data *sc27xx_data; + const struct sc27xx_adc_variant_data *pdata; struct iio_dev *indio_dev; int ret; + pdata = of_device_get_match_data(dev); + if (!pdata) { + dev_err(dev, "No matching driver data found\n"); + return -EINVAL; + } + indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data)); if (!indio_dev) return -ENOMEM; @@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev) } sc27xx_data->dev = dev; + sc27xx_data->var_data = pdata; + sc27xx_data->var_data->init_scale(sc27xx_data); ret = sc27xx_adc_enable(sc27xx_data); if (ret) { @@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev) } static const struct of_device_id sc27xx_adc_of_match[] = { - { .compatible = "sprd,sc2731-adc", }, + { .compatible = "sprd,sc2731-adc", .data = &sc2731_data}, { } }; MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);