Messages in this thread | | | Subject | Re: [PATCH v6 1/9] w1_therm: adding code comments and code reordering | From | Randy Dunlap <> | Date | Sun, 10 May 2020 20:33:40 -0700 |
| |
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
| |