Re: [PATCH v4] hwmon: (max6650.c) Add devicetree support

[Mike Looijmans]

(resent through different SMTP server, bounced as spam because of company 
server's unwanted modifications, sorry if you received this twice now)

On 19-08-16 04:43, Guenter Roeck wrote:
> On 08/16/2016 12:20 AM, Mike Looijmans wrote:
> > Parse devicetree parameters for voltage and prescaler setting. This allows
> > using multiple max6550 devices with varying settings, and also makes it
> > possible to instantiate and configure the device using devicetree.
> >
> > Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> > ---
> > v4: Vendor prefix "maxim," for devicetree properties and compatible string
> >     Split documentation into separate patch
> > v3: Resubmit because wrong mailing lists used
> >     Fix style errors as reported by checkpatch.pl
> >     Fix bug in DT parsing of fan-prescale
> > v2: Add devicetree binding documentation
> >     Code changes as suggested by Guenter
> >     Reduce log info, output only a single line
> >  drivers/hwmon/max6650.c | 47 ++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 36 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> > index 162a520..6beb62c 100644
> > --- a/drivers/hwmon/max6650.c
> > +++ b/drivers/hwmon/max6650.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/hwmon.h>
> >  #include <linux/hwmon-sysfs.h>
> >  #include <linux/err.h>
> > +#include <linux/of_device.h>
> >
> >  /*
> >   * Insmod parameters
> > @@ -48,7 +49,7 @@
> >  static int fan_voltage;
> >  /* prescaler: Possible values are 1, 2, 4, 8, 16 or 0 for don't change */
> >  static int prescaler;
> > -/* clock: The clock frequency of the chip the driver should assume */
> > +/* clock: The clock frequency of the chip (max6651 can be clocked
> > externally) */
> >  static int clock = 254000;
> >
> >  module_param(fan_voltage, int, S_IRUGO);
> > @@ -133,6 +134,19 @@ static const u8 tach_reg[] = {
> >      MAX6650_REG_TACH3,
> >  };
> >
> > +static const struct of_device_id max6650_dt_match[] = {
> > +    {
> > +        .compatible = "maxim,max6650",
> > +        .data = (void *)1
> > +    },
> > +    {
> > +        .compatible = "maxim,max6651",
> > +        .data = (void *)4
> > +    },
> > +    { },
> > +};
> > +MODULE_DEVICE_TABLE(of, max6650_dt_match);
> > +
> >  static struct max6650_data *max6650_update_device(struct device *dev)
> >  {
> >      struct max6650_data *data = dev_get_drvdata(dev);
> > @@ -566,6 +580,17 @@ static int max6650_init_client(struct max6650_data *data,
> >      struct device *dev = &client->dev;
> >      int config;
> >      int err = -EIO;
> > +    u32 voltage;
> > +    u32 prescale;
> > +
> > +    if (of_property_read_u32(dev->of_node, "maxim,fan-microvolt",
> > +                 &voltage))
> > +        voltage = fan_voltage;
> > +    else
> > +        voltage = voltage > 5500000 ? 12 : 5;
>
> I think this should only accept 5V or 12V.
>
> Guenter

A 4V, 10V or 13V fan will also work just fine, the voltage register just sets 
the range of the feedback tachometer.

I don't have strong feelings either way, so if you feel the drive should bail 
out on anything but 5 or 12, that's fine with me too.

Should I change this and send a v5 patch?



> > +    if (of_property_read_u32(dev->of_node, "maxim,fan-prescale",
> > +                 &prescale))
> > +        prescale = prescaler;
> >
> >      config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> >
> > @@ -574,7 +599,7 @@ static int max6650_init_client(struct max6650_data *data,
> >          return err;
> >      }
> >
> > -    switch (fan_voltage) {
> > +    switch (voltage) {
> >      case 0:
> >          break;
> >      case 5:
> > @@ -584,14 +609,10 @@ static int max6650_init_client(struct max6650_data *data,
> >          config |= MAX6650_CFG_V12;
> >          break;
> >      default:
> > -        dev_err(dev, "illegal value for fan_voltage (%d)\n",
> > -            fan_voltage);
> > +        dev_err(dev, "illegal value for fan_voltage (%d)\n", voltage);
> >      }
> >
> > -    dev_info(dev, "Fan voltage is set to %dV.\n",
> > -         (config & MAX6650_CFG_V12) ? 12 : 5);
> > -
> > -    switch (prescaler) {
> > +    switch (prescale) {
> >      case 0:
> >          break;
> >      case 1:
> > @@ -614,10 +635,11 @@ static int max6650_init_client(struct max6650_data *data,
> >               | MAX6650_CFG_PRESCALER_16;
> >          break;
> >      default:
> > -        dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
> > +        dev_err(dev, "illegal value for prescaler (%d)\n", prescale);
> >      }
> >
> > -    dev_info(dev, "Prescaler is set to %d.\n",
> > +    dev_info(dev, "Fan voltage: %dV, prescaler: %d.\n",
> > +         (config & MAX6650_CFG_V12) ? 12 : 5,
> >           1 << (config & MAX6650_CFG_PRESCALER_MASK));
> >
> >      /*
> > @@ -651,6 +673,8 @@ static int max6650_probe(struct i2c_client *client,
> >               const struct i2c_device_id *id)
> >  {
> >      struct device *dev = &client->dev;
> > +    const struct of_device_id *of_id =
> > +        of_match_device(of_match_ptr(max6650_dt_match), dev);
> >      struct max6650_data *data;
> >      struct device *hwmon_dev;
> >      int err;
> > @@ -661,7 +685,7 @@ static int max6650_probe(struct i2c_client *client,
> >
> >      data->client = client;
> >      mutex_init(&data->update_lock);
> > -    data->nr_fans = id->driver_data;
> > +    data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data;
> >
> >      /*
> >       * Initialize the max6650 chip
> > @@ -691,6 +715,7 @@ MODULE_DEVICE_TABLE(i2c, max6650_id);
> >  static struct i2c_driver max6650_driver = {
> >      .driver = {
> >          .name    = "max6650",
> > +        .of_match_table = of_match_ptr(max6650_dt_match),
> >      },
> >      .probe        = max6650_probe,
> >      .id_table    = max6650_id,