Re: [PATCH v2] rtc: ds3232: add temperature support

[Kirill Esipov]

2017-06-25 19:39 GMT+03:00 Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
> On Thu, Jun 22, 2017 at 7:58 PM, Kirill Esipov <yesipov@xxxxxxxxx> wrote:
>> DS3232/DS3234 has the temperature registers with a resolution of 0.25
>> degree celsius. This enables to get the value through hwmon.
>>
>>         # cat /sys/class/hwmon/hwmon0/temp1_input
>>         37250
>
>> +config RTC_DRV_DS3232_HWMON
>> +       bool "HWMON support for Dallas/Maxim DS3232/DS3234"
>
>> +       depends on RTC_DRV_DS3232 && HWMON
>> +       depends on !(RTC_DRV_DS3232=y && HWMON=m)
>
> Perhaps it might be squeezed into one line (something like that logic
> has been required by I2C related PMIC IIRC)
>
>> +       default y
>
> Is it really sane default?
>

At first sight i thought that yes it is sane default (and others RTC with
hwmon set it "default y" (ds1307, rv3029c2)).
But if it's not sane, then we should turn it off by default in others drivers?


>> +#ifdef CONFIG_RTC_DRV_DS3232_HWMON
>
> IS_BUILTIN() ?
>
>> +static int ds3232_hwmon_read_temp(struct device *dev, long int *mC)
>> +{
>> +       struct ds3232 *ds3232 = dev_get_drvdata(dev);
>> +       u8 temp_buf[2];
>> +       s16 temp;
>> +       int ret;
>> +
>> +       ret = regmap_bulk_read(ds3232->regmap, DS3232_REG_TEMPERATURE, temp_buf,
>> +                               sizeof(temp_buf));
>
>> +
>
> Remove.
>
>> +       if (ret < 0)
>> +               return ret;
>
>> +static umode_t ds3232_hwmon_is_visible(const void *data,
>> +                                        enum hwmon_sensor_types type,
>> +                                        u32 attr, int channel)
>> +{
>> +       if (type != hwmon_temp)
>> +               return 0;
>> +
>> +       switch (attr) {
>> +       case hwmon_temp_input:
>> +               return 0444;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static int ds3232_hwmon_read(struct device *dev,
>> +                              enum hwmon_sensor_types type,
>> +                              u32 attr, int channel, long *temp)
>> +{
>> +       int err;
>> +
>> +       switch (attr) {
>> +       case hwmon_temp_input:
>> +               ds3232_hwmon_read_temp(dev, temp);
>> +               err = 0;
>> +               break;
>> +       default:
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       return err;
>
> You may do as in previous function. Or what did you mean here by
> introducing an err variable?
>
>> +}
>
>> +
>> +
>
> Remove one of them.
>
>> +static void ds3232_hwmon_register(struct device *dev, const char *name)
>> +{
>> +       struct ds3232 *ds3232 = dev_get_drvdata(dev);
>> +       struct device *hwmon_dev;
>> +
>> +       hwmon_dev = devm_hwmon_device_register_with_info(dev, name, ds3232,
>> +                                                       &ds3232_hwmon_chip_info,
>> +                                                       NULL);
>
>> +
>
> Remove.
>
>> +       if (IS_ERR(hwmon_dev)) {
>> +               dev_warn(dev, "unable to register hwmon device %ld\n",
>> +                        PTR_ERR(hwmon_dev));
>> +       }
>> +}
>> +
>
>> +#else
>
> I dunno which style is preferred, though you may use
> if (IS_BUILTIN(...))
>  return;
>
> at the beginning of the function and allow gcc optimizer to take care
> of everything else.
>
>> +
>> +static void ds3232_hwmon_register(struct device *dev, const char *name)
>> +{
>> +}
>> +
>> +#endif
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
Kirill Esipov