Re: [PATCH v6 1/9] w1_therm: adding code comments and code reordering

[Randy Dunlap <rdunlap@infradead.org>]

Hi,

A few more comments here (inline):

On 5/10/20 7:15 AM, Akira Shimahara wrote:

>  drivers/w1/slaves/w1_therm.c | 398 ++++++++++++++++++++---------------
>  1 file changed, 232 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 18f08d7..890cf09 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -41,42 +41,55 @@
>  static int w1_strong_pullup = 1;
>  module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>  
> +/* Helpers Macros */
> +
> +/* return the address of the refcnt in the family data */
> +#define THERM_REFCNT(family_data) \
> +	(&((struct w1_therm_family_data *)family_data)->refcnt)
> +
> +/* Structs definition */
> +
> +/**
> + * struct w1_therm_family_converter - bind device specific functions
> + * @broken: flag for non registred families

                        non-registered

> + * @reserved: not used here
> + * @f: pointer to the device binding structure
> + * @convert: pointer to the device conversion function
> + * @precision: pointer to the device precicion function

                                        precision

> + * @eeprom: pointer to eeprom function
> + */
> +struct w1_therm_family_converter {
> +	u8		broken;
> +	u16		reserved;
> +	struct w1_family	*f;
> +	int		(*convert)(u8 rom[9]);
> +	int		(*precision)(struct device *device, int val);
> +	int		(*eeprom)(struct device *device);
> +};
> +
> +/**
> + * struct w1_therm_family_data - device data
> + * @rom: ROM id of the device
> + * @refcnt: ref count
> + */
>  struct w1_therm_family_data {
>  	uint8_t rom[9];

Why is "rom" 9 bytes in length?  Does it come from some
spec or standard?  Can it be a macro instead of an arbitrary
magic number?

>  	atomic_t refcnt;
>  };
>  
> +/**
> + * struct therm_info - store temperature reading
> + * @rom: readen device data

            read

> + * @crc: computed crc from rom
> + * @verdict: 1 crc checked, 0 crc not matching
> + */
>  struct therm_info {
>  	u8 rom[9];
>  	u8 crc;
>  	u8 verdict;
>  };
>  
...

>  
> +/* Interface Functions declaration */
> +
> +/**
> + * w1_therm_add_slave() - Called when a new slave is discovered
> + * @sl: slave just discovered by the master.
> + *
> + * Called by the master when the slave is discovered on the bus.Used to

                                                               bus. Used to

> + * initialized slave state before the beginning of any communication.

      initialize

> + *
> + * Return: 0 - If success, negative kernel code otherwise
> + */
> +static int w1_therm_add_slave(struct w1_slave *sl);
> +
> +/**
> + * w1_therm_remove_slave() - Called when a slave is removed
> + * @sl: slave to be removed.
> + *
> + * Called by the master when the slave is considered not to be on the bus
> + * anymore. Used to free memory.
> + */
> +static void w1_therm_remove_slave(struct w1_slave *sl);
> +
> +/* Family attributes */
> +
>  static struct attribute *w1_therm_attrs[] = {
>  	&dev_attr_w1_slave.attr,
>  	NULL,
> @@ -101,6 +140,8 @@ static struct attribute *w1_ds28ea00_attrs[] = {
>  	NULL,
>  };
>  
> +/* Attribute groups */
> +
>  ATTRIBUTE_GROUPS(w1_therm);
>  ATTRIBUTE_GROUPS(w1_ds28ea00);
>  
> @@ -154,6 +195,8 @@ static const struct hwmon_chip_info w1_chip_info = {
>  #define W1_CHIPINFO	NULL
>  #endif
>  
> +/* Family operations */
> +
>  static struct w1_family_ops w1_therm_fops = {
>  	.add_slave	= w1_therm_add_slave,
>  	.remove_slave	= w1_therm_remove_slave,
> @@ -168,6 +211,8 @@ static struct w1_family_ops w1_ds28ea00_fops = {
>  	.chip_info	= W1_CHIPINFO,
>  };
>  
> +/* Family binding operations struct */
> +
>  static struct w1_family w1_therm_family_DS18S20 = {
>  	.fid = W1_THERM_DS18S20,
>  	.fops = &w1_therm_fops,
...

> @@ -407,6 +332,7 @@ error:
>  	return ret;
>  }
>  
> +/* The return value is millidegrees Centigrade. */

                                       Celsius. */

>  static inline int w1_DS18B20_convert_temp(u8 rom[9])
>  {
>  	s16 t = le16_to_cpup((__le16 *)rom);
> @@ -414,6 +340,7 @@ static inline int w1_DS18B20_convert_temp(u8 rom[9])
>  	return t*1000/16;
>  }
>  
> +/* The return value is millidegrees Centigrade. */

ditto.

>  static inline int w1_DS18S20_convert_temp(u8 rom[9])

9?

...

> @@ -564,6 +529,81 @@ error:
>  	return ret;
>  }
>  
> +static inline int w1_therm_eeprom(struct device *device)
> +{
> +	struct w1_slave *sl = dev_to_w1_slave(device);
> +	struct w1_master *dev = sl->master;
> +	u8 rom[9], external_power;

9?


thanks.
-- 
~Randy