On Thu, Oct 26, 2023 at 05:44:02PM +0300, Daniel Matyas wrote:
> When adi,flt-q and/or adi,alrm-pol are not mentioned,
> the default configuration is loaded.
>
That isn't really a complete patch description.
It should include (even if repeated) that support for
additional chips is added.
> Signed-off-by: Daniel Matyas <daniel.matyas@xxxxxxxxxx>
> ---
>
> v4 -> v5: Passed i2c_client to init_client(), because I needed it to
> retrieve device id.
> Used a simple if to load default configuration. No more switch.
>
> v3 -> v4: No change.
>
> v3: Added patch.
>
> drivers/hwmon/max31827.c | 51 +++++++++++++++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index 7976d668ffd4..446232fa1710 100644
> --- a/drivers/hwmon/max31827.c
> +++ b/drivers/hwmon/max31827.c
> @@ -39,10 +39,15 @@
>
> #define MAX31827_12_BIT_CNV_TIME 140
>
> +#define MAX31827_ALRM_POL_HIGH 0x1
> +#define MAX31827_FLT_Q_4 0x2
> +
> #define MAX31827_16_BIT_TO_M_DGR(x) (sign_extend32(x, 15) * 1000 / 16)
> #define MAX31827_M_DGR_TO_16_BIT(x) (((x) << 4) / 1000)
> #define MAX31827_DEVICE_ENABLE(x) ((x) ? 0xA : 0x0)
>
> +enum chips { max31827, max31828, max31829 };
> +
> enum max31827_cnv {
> MAX31827_CNV_1_DIV_64_HZ = 1,
> MAX31827_CNV_1_DIV_32_HZ,
> @@ -373,12 +378,22 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
> return -EOPNOTSUPP;
> }
>
> +static const struct i2c_device_id max31827_i2c_ids[] = {
> + { "max31827", max31827 },
> + { "max31828", max31828 },
> + { "max31829", max31829 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> +
> static int max31827_init_client(struct max31827_state *st,
> - struct device *dev)
> + struct i2c_client *client)
> {
> + struct device *dev = &client->dev;
> struct fwnode_handle *fwnode;
> unsigned int res = 0;
> u32 data, lsb_idx;
> + enum chips type;
> bool prop;
> int ret;
>
> @@ -395,13 +410,20 @@ static int max31827_init_client(struct max31827_state *st,
> prop = fwnode_property_read_bool(fwnode, "adi,timeout-enable");
> res |= FIELD_PREP(MAX31827_CONFIGURATION_TIMEOUT_MASK, !prop);
>
> + if (dev->of_node)
> + type = (enum chips)of_device_get_match_data(dev);
> + else
> + type = i2c_match_id(max31827_i2c_ids, client)->driver_data;
> +
This should be something like
type = (enum chips)(uintptr_t)device_get_match_data(dev);
though that requires that the enum values start with 1. This avoids
having to pass client to the function and is more generic.
> if (fwnode_property_present(fwnode, "adi,alarm-pol")) {
> ret = fwnode_property_read_u32(fwnode, "adi,alarm-pol", &data);
> if (ret)
> return ret;
>
> res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> - }
> + } else if (type == max31829)
Not sure why checkpatch ignores this (maybe because of 'else if'),
but the else path should also be in {}.
But why is this only for max31829 ? I mean, sure, the default for
that chip is different, but that doesn't mean the other chips have
that bit set. There is no guarantee that the chip is in its default
state when the driver is loaded.
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> + MAX31827_ALRM_POL_HIGH);
>
> if (fwnode_property_present(fwnode, "adi,fault-q")) {
> ret = fwnode_property_read_u32(fwnode, "adi,fault-q", &data);
> @@ -418,7 +440,9 @@ static int max31827_init_client(struct max31827_state *st,
> }
>
> res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> - }
> + } else if ((type == max31828) || (type == max31829))
I _really_ dislike the notion of excessive ( ). Also, {} for the else
branch.
I also don't understand why that would be chip specific. I don't see
anything along that line in the datasheet.
Ah, wait ... I guess that is supposed to reflect the chip default.
I don't see why the chip default makes a difference - a well defined
default must be set either way. Again, there is no guarantee that
the chip is in its default state when the driver is loaded.
Also, why are the default values added in this patch and not
in the previous patch ?
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> + MAX31827_FLT_Q_4);
>
> return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
> }
> @@ -464,7 +488,7 @@ static int max31827_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(st->regmap),
> "Failed to allocate regmap.\n");
>
> - err = max31827_init_client(st, dev);
> + err = max31827_init_client(st, client);
> if (err)
> return err;
>
> @@ -475,14 +499,19 @@ static int max31827_probe(struct i2c_client *client)
> return PTR_ERR_OR_ZERO(hwmon_dev);
> }
>
> -static const struct i2c_device_id max31827_i2c_ids[] = {
> - { "max31827", 0 },
> - { }
> -};
> -MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> -
> static const struct of_device_id max31827_of_match[] = {
> - { .compatible = "adi,max31827" },
> + {
> + .compatible = "adi,max31827",
> + .data = (void *)max31827
> + },
> + {
> + .compatible = "adi,max31828",
> + .data = (void *)max31828
> + },
> + {
> + .compatible = "adi,max31829",
> + .data = (void *)max31829
> + },
> { }
> };
> MODULE_DEVICE_TABLE(of, max31827_of_match);
