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

[Akira shimahara <akira215corp@gmail.com>]

Hi,

Le dimanche 10 mai 2020 à 20:33 -0700, Randy Dunlap a écrit :
> 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

Noted, will be corrected

> 
> > + * @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

Noted, will be corrected

> 
> > + * @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?

9 bytes is required per device communication protocol (8 data bytes + 1 CRC). I
will detailled that in comments but I don't think that macro is required.

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

Noted, will be corrected

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

Same as above

> >  
> > +/* 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

Noted, will be corrected

> 
> > + *
> > + * 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. */

Noted, will be corrected

> 
> >  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.

Noted, will be corrected

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

Same as above

> 
> ...
> 
> > @@ -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?

Same as above

> 
> 
> thanks.

Thanks again for your time

Akira Shimahara