| Message ID | 20180411141555.15044-3-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/11/2018 09:15 AM, Peter Rosin wrote: > This is a current sense amplifier from Analog Devices. > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/iio/afe/Kconfig | 3 +- > drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig > index 642ce4eb12a6..0e10fe8f459a 100644 > --- a/drivers/iio/afe/Kconfig > +++ b/drivers/iio/afe/Kconfig > @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER > depends on OF || COMPILE_TEST > help > Say yes here to build support for the IIO unit converter > - that handles voltage dividers and current sense shunts. > + that handles voltage dividers, current sense shunts and > + the LT6106 Current Sense Amplifier from Analog Devices. Could work better to split these out into separate drivers. Maybe a iio-shunt-resistor.c that does just voltage->current with the appropriate scaling. Then make a a separate lt6106.c. "iio-unit-converter.c" isn't really doing what it says it is, it is not a generic "unit converter" like one would assume. Having the driver name describe what kind of device it physically represents will be better when more complex AFEs show up that, for instance, have programmable gains and need a larger driver. Andrew > > To compile this driver as a module, choose M here: the > module will be called iio-unit-converter. > diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c > index fc50290d7e5e..4b0ae5ab9c2d 100644 > --- a/drivers/iio/afe/iio-unit-converter.c > +++ b/drivers/iio/afe/iio-unit-converter.c > @@ -144,6 +144,53 @@ static int unit_converter_configure_channel(struct device *dev, > return 0; > } > > +static int unit_converter_adi_lt6106_props(struct device *dev, > + struct unit_converter *uc) > +{ > + u32 sense; > + u32 rin; > + u32 rout; > + u32 factor; > + int ret; > + > + ret = device_property_read_u32(dev, "sense-resistor-micro-ohms", > + &sense); > + if (ret) { > + dev_err(dev, "failed to read the sense resistance: %d\n", ret); > + return ret; > + } > + > + ret = device_property_read_u32(dev, "input-resistor-ohms", &rin); > + if (ret) { > + dev_err(dev, "failed to read the input resistance: %d\n", ret); > + return ret; > + } > + > + ret = device_property_read_u32(dev, "output-resistor-ohms", &rout); > + if (ret) { > + dev_err(dev, "failed to read the output resistance: %d\n", ret); > + return ret; > + } > + > + /* > + * Calculate the scaling factor, rin / (rout * sense), while trying > + * to keep the fraction from overflowing. > + */ > + factor = gcd(sense, 1000000); > + uc->numerator = 1000000 / factor; > + uc->denominator = sense / factor; > + > + factor = gcd(uc->numerator, rout); > + uc->numerator /= factor; > + uc->denominator *= rout / factor; > + > + factor = gcd(uc->denominator, rin); > + uc->numerator *= rin / factor; > + uc->denominator /= factor; > + > + return 0; > +} > + > static int unit_converter_current_sense_shunt_props(struct device *dev, > struct unit_converter *uc) > { > @@ -175,11 +222,16 @@ static int unit_converter_voltage_divider_props(struct device *dev, > } > > enum unit_converter_variant { > + ADI_LT6106, > CURRENT_SENSE_SHUNT, > VOLTAGE_DIVIDER, > }; > > static const struct unit_converter_cfg unit_converter_cfg[] = { > + [ADI_LT6106] = { > + .type = IIO_CURRENT, > + .props = unit_converter_adi_lt6106_props, > + }, > [CURRENT_SENSE_SHUNT] = { > .type = IIO_CURRENT, > .props = unit_converter_current_sense_shunt_props, > @@ -191,6 +243,8 @@ static const struct unit_converter_cfg unit_converter_cfg[] = { > }; > > static const struct of_device_id unit_converter_match[] = { > + { .compatible = "adi,lt6106", > + .data = &unit_converter_cfg[ADI_LT6106], }, > { .compatible = "current-sense-shunt", > .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], }, > { .compatible = "voltage-divider", > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/11/2018 05:43 PM, Andrew F. Davis wrote: > On 04/11/2018 09:15 AM, Peter Rosin wrote: >> This is a current sense amplifier from Analog Devices. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/iio/afe/Kconfig | 3 +- >> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >> index 642ce4eb12a6..0e10fe8f459a 100644 >> --- a/drivers/iio/afe/Kconfig >> +++ b/drivers/iio/afe/Kconfig >> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >> depends on OF || COMPILE_TEST >> help >> Say yes here to build support for the IIO unit converter >> - that handles voltage dividers and current sense shunts. >> + that handles voltage dividers, current sense shunts and >> + the LT6106 Current Sense Amplifier from Analog Devices. > > Could work better to split these out into separate drivers. Maybe a > iio-shunt-resistor.c that does just voltage->current with the > appropriate scaling. Then make a a separate lt6106.c. I don't think we need a separate driver here. There are tons of circuits that all work the same way and all require the same properties. If we'd add a driver for each of them we'd get buried in boilerplate code. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: > On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>> This is a current sense amplifier from Analog Devices. >>> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/iio/afe/Kconfig | 3 +- >>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 56 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>> index 642ce4eb12a6..0e10fe8f459a 100644 >>> --- a/drivers/iio/afe/Kconfig >>> +++ b/drivers/iio/afe/Kconfig >>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>> depends on OF || COMPILE_TEST >>> help >>> Say yes here to build support for the IIO unit converter >>> - that handles voltage dividers and current sense shunts. >>> + that handles voltage dividers, current sense shunts and >>> + the LT6106 Current Sense Amplifier from Analog Devices. >> >> Could work better to split these out into separate drivers. Maybe a >> iio-shunt-resistor.c that does just voltage->current with the >> appropriate scaling. Then make a a separate lt6106.c. > > I don't think we need a separate driver here. There are tons of circuits > that all work the same way and all require the same properties. If we'd add > a driver for each of them we'd get buried in boilerplate code. > Fair enough, then it should at least be renamed to something generic like current-sense-amplifier, as you said lots of circuits do this, not just lt6106s. We will have then have support for: current-sense-amplifier current-sense-shunt voltage-divider compatibles in this driver called "unit-converter" which is still a misnomer IMHO. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-11 17:43, Andrew F. Davis wrote: > On 04/11/2018 09:15 AM, Peter Rosin wrote: >> This is a current sense amplifier from Analog Devices. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/iio/afe/Kconfig | 3 +- >> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >> index 642ce4eb12a6..0e10fe8f459a 100644 >> --- a/drivers/iio/afe/Kconfig >> +++ b/drivers/iio/afe/Kconfig >> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >> depends on OF || COMPILE_TEST >> help >> Say yes here to build support for the IIO unit converter >> - that handles voltage dividers and current sense shunts. >> + that handles voltage dividers, current sense shunts and >> + the LT6106 Current Sense Amplifier from Analog Devices. > > Could work better to split these out into separate drivers. Maybe a > iio-shunt-resistor.c that does just voltage->current with the > appropriate scaling. Then make a a separate lt6106.c. > > "iio-unit-converter.c" isn't really doing what it says it is, it is not > a generic "unit converter" like one would assume. Having the driver name > describe what kind of device it physically represents will be better > when more complex AFEs show up that, for instance, have programmable > gains and need a larger driver. If an AFE needs programming etc, then it is definitely an option to write a specific driver for it. It was never my intention to cover all AFEs in this one driver, only those that make sense. Presumably that ends up being those doing linear scaling and perhaps requiring a change of the iio channel type. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-11 18:13, Andrew F. Davis wrote: > On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: >> On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >>> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>>> This is a current sense amplifier from Analog Devices. >>>> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/iio/afe/Kconfig | 3 +- >>>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>>> index 642ce4eb12a6..0e10fe8f459a 100644 >>>> --- a/drivers/iio/afe/Kconfig >>>> +++ b/drivers/iio/afe/Kconfig >>>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>>> depends on OF || COMPILE_TEST >>>> help >>>> Say yes here to build support for the IIO unit converter >>>> - that handles voltage dividers and current sense shunts. >>>> + that handles voltage dividers, current sense shunts and >>>> + the LT6106 Current Sense Amplifier from Analog Devices. >>> >>> Could work better to split these out into separate drivers. Maybe a >>> iio-shunt-resistor.c that does just voltage->current with the >>> appropriate scaling. Then make a a separate lt6106.c. >> >> I don't think we need a separate driver here. There are tons of circuits >> that all work the same way and all require the same properties. If we'd add >> a driver for each of them we'd get buried in boilerplate code. >> > > Fair enough, then it should at least be renamed to something generic > like current-sense-amplifier, as you said lots of circuits do this, not > just lt6106s. We will have then have support for: > > current-sense-amplifier > current-sense-shunt > voltage-divider For the compatible "current-sense-amplifier", I would advocate the properties... sense-resistor-micro-ohms sense-gain (or something close to that) ...and not input-resistor-ohms and output-resistor-ohms which are way more particular to the LT6106. But as I said in the cover letter, I didn't go with sense-gain since I thought I would end up with requests for non-integer gains. There is yet to be a comment on the non-integer gain problem, and before there is a path forward for that case, I'm reluctant. > compatibles in this driver called "unit-converter" which is still a > misnomer IMHO. I don't remember you having presented your preference, and I think that goes against the established bike-shedding protocol? Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/2018 09:29 AM, Peter Rosin wrote: > On 2018-04-11 18:13, Andrew F. Davis wrote: >> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: >>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >>>> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>>>> This is a current sense amplifier from Analog Devices. >>>>> >>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>> --- >>>>> drivers/iio/afe/Kconfig | 3 +- >>>>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>>>> index 642ce4eb12a6..0e10fe8f459a 100644 >>>>> --- a/drivers/iio/afe/Kconfig >>>>> +++ b/drivers/iio/afe/Kconfig >>>>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>>>> depends on OF || COMPILE_TEST >>>>> help >>>>> Say yes here to build support for the IIO unit converter >>>>> - that handles voltage dividers and current sense shunts. >>>>> + that handles voltage dividers, current sense shunts and >>>>> + the LT6106 Current Sense Amplifier from Analog Devices. >>>> >>>> Could work better to split these out into separate drivers. Maybe a >>>> iio-shunt-resistor.c that does just voltage->current with the >>>> appropriate scaling. Then make a a separate lt6106.c. >>> >>> I don't think we need a separate driver here. There are tons of circuits >>> that all work the same way and all require the same properties. If we'd add >>> a driver for each of them we'd get buried in boilerplate code. >>> >> >> Fair enough, then it should at least be renamed to something generic >> like current-sense-amplifier, as you said lots of circuits do this, not >> just lt6106s. We will have then have support for: >> >> current-sense-amplifier >> current-sense-shunt >> voltage-divider > > For the compatible "current-sense-amplifier", I would advocate the > properties... > > sense-resistor-micro-ohms > sense-gain > > (or something close to that) > > ...and not input-resistor-ohms and output-resistor-ohms which are way > more particular to the LT6106. > > But as I said in the cover letter, I didn't go with sense-gain since I > thought I would end up with requests for non-integer gains. There is > yet to be a comment on the non-integer gain problem, and before there > is a path forward for that case, I'm reluctant. > Why not similar to what you had before with the resistor: sense-gain-multiplier sense-gain-divider if either are missing assume they are 1. >> compatibles in this driver called "unit-converter" which is still a >> misnomer IMHO. > > I don't remember you having presented your preference, and I think > that goes against the established bike-shedding protocol? > True, how about "current-sense-from-voltage" ? Andrew > Cheers, > Peter > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-12 17:35, Andrew F. Davis wrote: > On 04/12/2018 09:29 AM, Peter Rosin wrote: >> On 2018-04-11 18:13, Andrew F. Davis wrote: >>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: >>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>>>>> This is a current sense amplifier from Analog Devices. >>>>>> >>>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>>> --- >>>>>> drivers/iio/afe/Kconfig | 3 +- >>>>>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>>>>> index 642ce4eb12a6..0e10fe8f459a 100644 >>>>>> --- a/drivers/iio/afe/Kconfig >>>>>> +++ b/drivers/iio/afe/Kconfig >>>>>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>>>>> depends on OF || COMPILE_TEST >>>>>> help >>>>>> Say yes here to build support for the IIO unit converter >>>>>> - that handles voltage dividers and current sense shunts. >>>>>> + that handles voltage dividers, current sense shunts and >>>>>> + the LT6106 Current Sense Amplifier from Analog Devices. >>>>> >>>>> Could work better to split these out into separate drivers. Maybe a >>>>> iio-shunt-resistor.c that does just voltage->current with the >>>>> appropriate scaling. Then make a a separate lt6106.c. >>>> >>>> I don't think we need a separate driver here. There are tons of circuits >>>> that all work the same way and all require the same properties. If we'd add >>>> a driver for each of them we'd get buried in boilerplate code. >>>> >>> >>> Fair enough, then it should at least be renamed to something generic >>> like current-sense-amplifier, as you said lots of circuits do this, not >>> just lt6106s. We will have then have support for: >>> >>> current-sense-amplifier >>> current-sense-shunt >>> voltage-divider >> >> For the compatible "current-sense-amplifier", I would advocate the >> properties... >> >> sense-resistor-micro-ohms >> sense-gain >> >> (or something close to that) >> >> ...and not input-resistor-ohms and output-resistor-ohms which are way >> more particular to the LT6106. >> >> But as I said in the cover letter, I didn't go with sense-gain since I >> thought I would end up with requests for non-integer gains. There is >> yet to be a comment on the non-integer gain problem, and before there >> is a path forward for that case, I'm reluctant. >> > > Why not similar to what you had before with the resistor: > > sense-gain-multiplier > sense-gain-divider > > if either are missing assume they are 1. Hmm, how about sense-gain for the normal integer case, and then divide by sense-attenuation if needed? I.e. exactly the same functionality as you describe, just different names. >>> compatibles in this driver called "unit-converter" which is still a >>> misnomer IMHO. >> >> I don't remember you having presented your preference, and I think >> that goes against the established bike-shedding protocol? >> > > True, how about "current-sense-from-voltage" ? Doesn't cover "voltage-divider" (and we don't need separate drivers doing the exact same calculations, that's a maintenance nightmare). Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/13/2018 12:31 AM, Peter Rosin wrote: > On 2018-04-12 17:35, Andrew F. Davis wrote: >> On 04/12/2018 09:29 AM, Peter Rosin wrote: >>> On 2018-04-11 18:13, Andrew F. Davis wrote: >>>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: >>>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >>>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>>>>>> This is a current sense amplifier from Analog Devices. >>>>>>> >>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>>>> --- >>>>>>> drivers/iio/afe/Kconfig | 3 +- >>>>>>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>>>>>> index 642ce4eb12a6..0e10fe8f459a 100644 >>>>>>> --- a/drivers/iio/afe/Kconfig >>>>>>> +++ b/drivers/iio/afe/Kconfig >>>>>>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>>>>>> depends on OF || COMPILE_TEST >>>>>>> help >>>>>>> Say yes here to build support for the IIO unit converter >>>>>>> - that handles voltage dividers and current sense shunts. >>>>>>> + that handles voltage dividers, current sense shunts and >>>>>>> + the LT6106 Current Sense Amplifier from Analog Devices. >>>>>> >>>>>> Could work better to split these out into separate drivers. Maybe a >>>>>> iio-shunt-resistor.c that does just voltage->current with the >>>>>> appropriate scaling. Then make a a separate lt6106.c. >>>>> >>>>> I don't think we need a separate driver here. There are tons of circuits >>>>> that all work the same way and all require the same properties. If we'd add >>>>> a driver for each of them we'd get buried in boilerplate code. >>>>> >>>> >>>> Fair enough, then it should at least be renamed to something generic >>>> like current-sense-amplifier, as you said lots of circuits do this, not >>>> just lt6106s. We will have then have support for: >>>> >>>> current-sense-amplifier >>>> current-sense-shunt >>>> voltage-divider >>> >>> For the compatible "current-sense-amplifier", I would advocate the >>> properties... >>> >>> sense-resistor-micro-ohms >>> sense-gain >>> >>> (or something close to that) >>> >>> ...and not input-resistor-ohms and output-resistor-ohms which are way >>> more particular to the LT6106. >>> >>> But as I said in the cover letter, I didn't go with sense-gain since I >>> thought I would end up with requests for non-integer gains. There is >>> yet to be a comment on the non-integer gain problem, and before there >>> is a path forward for that case, I'm reluctant. >>> >> >> Why not similar to what you had before with the resistor: >> >> sense-gain-multiplier >> sense-gain-divider >> >> if either are missing assume they are 1. > > Hmm, how about sense-gain for the normal integer case, and then divide > by sense-attenuation if needed? I.e. exactly the same functionality as > you describe, just different names. There is some precedence in the clock bindings for using -mult and -div as the suffix for fractional scales. See fixed-factor-clock.txt. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/2018 05:31 PM, Peter Rosin wrote: > On 2018-04-12 17:35, Andrew F. Davis wrote: >> On 04/12/2018 09:29 AM, Peter Rosin wrote: >>> On 2018-04-11 18:13, Andrew F. Davis wrote: >>>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote: >>>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote: >>>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote: >>>>>>> This is a current sense amplifier from Analog Devices. >>>>>>> >>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>>>>> --- >>>>>>> drivers/iio/afe/Kconfig | 3 +- >>>>>>> drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig >>>>>>> index 642ce4eb12a6..0e10fe8f459a 100644 >>>>>>> --- a/drivers/iio/afe/Kconfig >>>>>>> +++ b/drivers/iio/afe/Kconfig >>>>>>> @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER >>>>>>> depends on OF || COMPILE_TEST >>>>>>> help >>>>>>> Say yes here to build support for the IIO unit converter >>>>>>> - that handles voltage dividers and current sense shunts. >>>>>>> + that handles voltage dividers, current sense shunts and >>>>>>> + the LT6106 Current Sense Amplifier from Analog Devices. >>>>>> >>>>>> Could work better to split these out into separate drivers. Maybe a >>>>>> iio-shunt-resistor.c that does just voltage->current with the >>>>>> appropriate scaling. Then make a a separate lt6106.c. >>>>> >>>>> I don't think we need a separate driver here. There are tons of circuits >>>>> that all work the same way and all require the same properties. If we'd add >>>>> a driver for each of them we'd get buried in boilerplate code. >>>>> >>>> >>>> Fair enough, then it should at least be renamed to something generic >>>> like current-sense-amplifier, as you said lots of circuits do this, not >>>> just lt6106s. We will have then have support for: >>>> >>>> current-sense-amplifier >>>> current-sense-shunt >>>> voltage-divider >>> >>> For the compatible "current-sense-amplifier", I would advocate the >>> properties... >>> >>> sense-resistor-micro-ohms >>> sense-gain >>> >>> (or something close to that) >>> >>> ...and not input-resistor-ohms and output-resistor-ohms which are way >>> more particular to the LT6106. >>> >>> But as I said in the cover letter, I didn't go with sense-gain since I >>> thought I would end up with requests for non-integer gains. There is >>> yet to be a comment on the non-integer gain problem, and before there >>> is a path forward for that case, I'm reluctant. >>> >> >> Why not similar to what you had before with the resistor: >> >> sense-gain-multiplier >> sense-gain-divider >> >> if either are missing assume they are 1. > > Hmm, how about sense-gain for the normal integer case, and then divide > by sense-attenuation if needed? I.e. exactly the same functionality as > you describe, just different names. > I like these names, but I think gain/attenuation sound very analog and I would be tempted to assume they are floating point numbers or the units are logarithmic (dB). To prevent any more needless bike-shedding on my part I'd like to say either yours, mine, or Lars-Peter's suggestion all work for me. >>>> compatibles in this driver called "unit-converter" which is still a >>>> misnomer IMHO. >>> >>> I don't remember you having presented your preference, and I think >>> that goes against the established bike-shedding protocol? >>> >> >> True, how about "current-sense-from-voltage" ? > > Doesn't cover "voltage-divider" (and we don't need separate drivers > doing the exact same calculations, that's a maintenance nightmare). > The driver name doesn't have to cover every use, just more than the other name. > Cheers, > Peter > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-13 16:47, Andrew F. Davis wrote: > On 04/12/2018 05:31 PM, Peter Rosin wrote: >> On 2018-04-12 17:35, Andrew F. Davis wrote: >>> True, how about "current-sense-from-voltage" ? >> >> Doesn't cover "voltage-divider" (and we don't need separate drivers >> doing the exact same calculations, that's a maintenance nightmare). > > The driver name doesn't have to cover every use, just more than the > other name. I also find current-sense-from-voltage unwieldy. It's simply too long. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-04-13 10:11, Lars-Peter Clausen wrote: > On 04/13/2018 12:31 AM, Peter Rosin wrote: >> On 2018-04-12 17:35, Andrew F. Davis wrote: >>> On 04/12/2018 09:29 AM, Peter Rosin wrote: >>>> But as I said in the cover letter, I didn't go with sense-gain since I >>>> thought I would end up with requests for non-integer gains. There is >>>> yet to be a comment on the non-integer gain problem, and before there >>>> is a path forward for that case, I'm reluctant. >>> >>> Why not similar to what you had before with the resistor: >>> >>> sense-gain-multiplier >>> sense-gain-divider >>> >>> if either are missing assume they are 1. >> >> Hmm, how about sense-gain for the normal integer case, and then divide >> by sense-attenuation if needed? I.e. exactly the same functionality as >> you describe, just different names. > > There is some precedence in the clock bindings for using -mult and -div as > the suffix for fractional scales. See fixed-factor-clock.txt. Ok, I'm going with sense-gain-mult and sense-gain-div, but I'm going to diverge a little bit from the clock binding and make them optional with a default of 1 if missing. I'm also going to use the compatible "current-sense-amplifier", and not mention the LT6106, because I suspect that part can conceivable be used in other ways... But I'll wait for a bit with this and give everybody a last chance to pitch suggestions and opinions. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig index 642ce4eb12a6..0e10fe8f459a 100644 --- a/drivers/iio/afe/Kconfig +++ b/drivers/iio/afe/Kconfig @@ -10,7 +10,8 @@ config IIO_UNIT_CONVERTER depends on OF || COMPILE_TEST help Say yes here to build support for the IIO unit converter - that handles voltage dividers and current sense shunts. + that handles voltage dividers, current sense shunts and + the LT6106 Current Sense Amplifier from Analog Devices. To compile this driver as a module, choose M here: the module will be called iio-unit-converter. diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c index fc50290d7e5e..4b0ae5ab9c2d 100644 --- a/drivers/iio/afe/iio-unit-converter.c +++ b/drivers/iio/afe/iio-unit-converter.c @@ -144,6 +144,53 @@ static int unit_converter_configure_channel(struct device *dev, return 0; } +static int unit_converter_adi_lt6106_props(struct device *dev, + struct unit_converter *uc) +{ + u32 sense; + u32 rin; + u32 rout; + u32 factor; + int ret; + + ret = device_property_read_u32(dev, "sense-resistor-micro-ohms", + &sense); + if (ret) { + dev_err(dev, "failed to read the sense resistance: %d\n", ret); + return ret; + } + + ret = device_property_read_u32(dev, "input-resistor-ohms", &rin); + if (ret) { + dev_err(dev, "failed to read the input resistance: %d\n", ret); + return ret; + } + + ret = device_property_read_u32(dev, "output-resistor-ohms", &rout); + if (ret) { + dev_err(dev, "failed to read the output resistance: %d\n", ret); + return ret; + } + + /* + * Calculate the scaling factor, rin / (rout * sense), while trying + * to keep the fraction from overflowing. + */ + factor = gcd(sense, 1000000); + uc->numerator = 1000000 / factor; + uc->denominator = sense / factor; + + factor = gcd(uc->numerator, rout); + uc->numerator /= factor; + uc->denominator *= rout / factor; + + factor = gcd(uc->denominator, rin); + uc->numerator *= rin / factor; + uc->denominator /= factor; + + return 0; +} + static int unit_converter_current_sense_shunt_props(struct device *dev, struct unit_converter *uc) { @@ -175,11 +222,16 @@ static int unit_converter_voltage_divider_props(struct device *dev, } enum unit_converter_variant { + ADI_LT6106, CURRENT_SENSE_SHUNT, VOLTAGE_DIVIDER, }; static const struct unit_converter_cfg unit_converter_cfg[] = { + [ADI_LT6106] = { + .type = IIO_CURRENT, + .props = unit_converter_adi_lt6106_props, + }, [CURRENT_SENSE_SHUNT] = { .type = IIO_CURRENT, .props = unit_converter_current_sense_shunt_props, @@ -191,6 +243,8 @@ static const struct unit_converter_cfg unit_converter_cfg[] = { }; static const struct of_device_id unit_converter_match[] = { + { .compatible = "adi,lt6106", + .data = &unit_converter_cfg[ADI_LT6106], }, { .compatible = "current-sense-shunt", .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], }, { .compatible = "voltage-divider",
This is a current sense amplifier from Analog Devices. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/iio/afe/Kconfig | 3 +- drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-)