Conversation
sdivarci
requested review from
CiprianRegus,
amiclaus,
btogorean,
buha,
chegbeli,
danmois,
dbogdan,
machschmitt,
mhennerich,
nunojsa and
rbolboac
as code owners
nunojsa
left a comment
nunojsa
left a comment
There was a problem hiding this comment.
Hi Sinan,
Just a first high level review. We first need to have this driver using modern hwmon API and have the userspace interface figured.
Also note that the expectation is for this to be first upstreamed (and accepted) before merging it in our tree
Documentation/hwmon/max31732.rst
Outdated
|
|
||
| * All temperatures reported via sysfs are in milli-degree Celsius. | ||
| * ``stop`` halts conversions but does not power-cycle the part; use ``soft_por`` | ||
| to restore all the registers to their default values. |
There was a problem hiding this comment.
this patch can typically be squashed with the one adding the driver
machschmitt
left a comment
machschmitt
left a comment
There was a problem hiding this comment.
Hi @sdivarci,
A few comments from me.
I'm not very familiar with hwmon so feel safe to disregard any potentially misleading suggestion.
| reg: | ||
| description: I2C address of the device. | ||
| maxItems: 1 | ||
|
|
There was a problem hiding this comment.
The guideline is to make dt docs as complete as possible. So, we can also describe the voltage supply here.
vcc-supply: true|
|
||
| required: | ||
| - compatible | ||
| - reg |
There was a problem hiding this comment.
Unless I'm missing something, the device requires a power supply to operate, so
required:
- compatible
- reg
- vcc-supply|
|
||
| sensor@1c { | ||
| compatible = "adi,max31732"; | ||
| reg = <0x1c>; |
There was a problem hiding this comment.
and it also needs to appear in the example. e.g.
vcc-supply = <&supply_3_3V>;
drivers/hwmon/max31732.c
Outdated
| dev_set_drvdata(dev, data); | ||
|
|
||
| if (fwnode_property_read_bool(dev_fwnode(dev), "adi,extended-range")) | ||
| ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_EXTRANGE); |
There was a problem hiding this comment.
The extended_range bit is only set here (at probe) and never changed after that. Then, there are a few places where it's read, (e.g. on max31732_read(), max31732_write(), max31732_highest_temperature_show(), ...). What about having a field for that in struct max31732_data so we can simplify code in those places where we currently do regmap_test_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_EXTRANGE);?
There was a problem hiding this comment.
Also, to me, adi,extended-range doesn't seem very related to how hardware is setup. It's mostly a tweak in data offset. I worry dt maintainers might object to accepting that. Do hwmon offer any interface for changing data offset at runtime?
Hi Sinan, this is Balaji from Celestica. Could you please let me know when this driver will be pushed upstream? It is important for our current project. |
Add support for Analog Devices MAX31732, a five-channel temperature monitor with per-channel alarms, calibration helpers, and optional IRQ handling. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>
Add documentation for the Analog Devices MAX31732 temperature sensor driver under Documentation/hwmon. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>
Add device tree bindings for the Analog Devices MAX31732 temperature sensor driver under Documentation/devicetree/bindings/hwmon/. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>
sdivarci
force-pushed
the
dev-max31732
branch
from
1d17693 to
2d1537e
Compare
|
Hi @nunojsa @machschmitt I've made changes to the driver code and documentation based on your review notes; could you please review them again? |
Will try to check it in the next few days. Ping me If I seem to forget |
6 participants
PR Description
Add MAX31732 hwmod driver and documentation.
PR Type
PR Checklist