Messages in this thread | | | Subject | Re: [PATCH v6 1/9] w1_therm: adding code comments and code reordering | From | Akira shimahara <> | Date | Mon, 11 May 2020 09:31:04 +0200 |
| |
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
| |