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?
> +#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
