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

From: Andy Shevchenko
Date: Sun Jun 25 2017 - 12:40:18 EST


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