Re: [PATCH 1/2] hwmon: (k8temp) update to use new hwmon registration API

[Guenter Roeck]

On 7/20/19 2:05 PM, Robert Karszniewicz wrote:
> Hello Guenter.
>
> Thank you for your feedback! I am working on a version 2.
>
> On Sat Jul 20, 2019 at 7:08 AM Guenter Roeck wrote:
> > > +static umode_t
> > > +k8temp_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> > > +		  u32 attr, int channel)
> > > +{
> > > +	if (type != hwmon_temp)
> > > +		return 0;
> >
> > Not really needed since only temperature sensors are registered in the first place.
> >
> > > +
> > > +	if (attr != hwmon_temp_input)
> > > +		return 0;
> > > +
> >
> > The idea here is to only create sensors if they actually exist, so I would expect
> > something like the following here.
>
> I realised that in the probe() function I deleted all the code which
> conditionally creates the sysfs files. Is that how it's supposed to be
> done and that's exactly what is_visible() is for, or is the proper way
> to still create the sysfs files conditionally, too?

Yes, that is what the function is for. sysfs files are created in the core,
based on the result from the call to the is_visible function. If the function
returns 0, the respective sysfs file won't be created.

Thanks,
Guenter

> > 	struct k8temp_data *data = drvdata;
> >
> > 	if (attr != hwmon_temp_input)
> > 		return 0;
> >
> > 	if ((channel & 1) && !(data->sensorsp & SEL_PLACE))
> > 		return 0;
> >
> > 	if ((channel & 2) && !(data->sensorsp & SEL_CORE))
> > 		return 0;
> >
> > > +	return 0444;
> > > +}
>
> > > +	if (data->swap_core_select)
> > > +		core = core ? 0 : 1;
> >
> > 		core = 1 - core;
> >
> > would accomplish the same without conditional.
>
> How do you like
> 	core ^= 1;
> ?
>
> > >    static int k8temp_probe(struct pci_dev *pdev,
> > >    				  const struct pci_device_id *id)
> > >    {
> > > -	int err;
> > >    	u8 scfg;
> > >    	u32 temp;
> > >    	u8 model, stepping;
> > >    	struct k8temp_data *data;
> > > +	struct device *hwmon_dev;
> > >    
> > >    	data = devm_kzalloc(&pdev->dev, sizeof(struct k8temp_data), GFP_KERNEL);
> > >    	if (!data)
> > > @@ -233,84 +274,23 @@ static int k8temp_probe(struct pci_dev *pdev,
> > >    
> > >    	data->name = "k8temp";
> > >    	mutex_init(&data->update_lock);
> > > -	pci_set_drvdata(pdev, data);
> > > -
> > > -	/* Register sysfs hooks */
> > > -	err = device_create_file(&pdev->dev,
> > > -			   &sensor_dev_attr_temp1_input.dev_attr);
> > > -	if (err)
> > > -		goto exit_remove;
> > > -
> > > -	/* sensor can be changed and reports something */
> > > -	if (data->sensorsp & SEL_PLACE) {
> > > -		err = device_create_file(&pdev->dev,
> > > -				   &sensor_dev_attr_temp2_input.dev_attr);
> > > -		if (err)
> > > -			goto exit_remove;
> > > -	}
> > > -
> > > -	/* core can be changed and reports something */
> > > -	if (data->sensorsp & SEL_CORE) {
> > > -		err = device_create_file(&pdev->dev,
> > > -				   &sensor_dev_attr_temp3_input.dev_attr);
> > > -		if (err)
> > > -			goto exit_remove;
> > > -		if (data->sensorsp & SEL_PLACE) {
> > > -			err = device_create_file(&pdev->dev,
> > > -					   &sensor_dev_attr_temp4_input.
> > > -					   dev_attr);
> > > -			if (err)
> > > -				goto exit_remove;
> > > -		}
> > > -	}