RE: [External] Re: [PATCH 2/2] platorm/x86: thinkpad_acpi: sysfs interface to interface to get wwan antenna type

[Nitin Joshi1 <njoshi1@lenovo.com>]

Hi

>-----Original Message-----
>From: Barnabás Pőcze <pobrn@protonmail.com>
>Sent: Sunday, February 14, 2021 5:43 AM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: hdegoede@redhat.com; ibm-acpi-devel@lists.sourceforge.net; platform-
>driver-x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH
>Pearson <markpearson@lenovo.com>
>Subject: [External] Re: [PATCH 2/2] platorm/x86: thinkpad_acpi: sysfs
>interface to interface to get wwan antenna type
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> [...]
>> +/* sysfs wwan antenna type entry */
>> +static ssize_t wwan_antenna_type_show(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	switch (wwan_antennatype) {
>> +	case 1:
>> +		return sysfs_emit(buf, "type a\n");
>> +	case 2:
>> +		return sysfs_emit(buf, "type b\n");
>> +	default:
>> +		return sysfs_emit(buf, "unknown type\n");
>
>I feel like returning -ENODATA would be more appropriate here. Or maybe
>you could choose not to create the attribute if the antenna type is unknown.
>And I'm not sure if the "type" prefix is necessary. I believe the name of the
>attribute 'wwan_antenna_type'
Ack . I will check it. 
Regarding prefix, it's not so necessary but let me keep "type" prefix.
   
>already implies that the content will describe a type. Furthermore, I think you
>could omit the `has_antennatype` variable altogether, storing only
>`wwan_antennatype` is enough.
Ack . I will check it
>
>
>> +	}
>> +}
>> +
>>  static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>>  				struct device_attribute *attr,
>>  				const char *buf, size_t count)
>> @@ -10076,24 +10114,29 @@ static ssize_t
>wlan_tx_strength_reduce_store(struct device *dev,
>>  	}
>>
>>  	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "wlan_tx_strength_reduce");
>> +
>
>If you want the empty line here, I think you should place it in the previous
>patch.
Ack . I will remove it.
>
>
>>  	return count;
>>  }
>>  static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> +static DEVICE_ATTR_RO(wwan_antenna_type);
>>
>>  static int tpacpi_dprc_init(struct ibm_init_struct *iibm)  {
>> -	int wlantx_err, err;
>> +	int wlantx_err, wwanantenna_err, err;
>>
>>  	wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	wwanantenna_err = get_wwan_antenna(&has_antennatype,
>&wwan_antennatype);
>>  	/*
>>  	 * If support isn't available (ENODEV) for both devices then quit, but
>>  	 * don't return an error.
>>  	 */
>> -	if (wlantx_err == -ENODEV)
>> +	if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
>>  		return 0;
>>  	/* Otherwise, if there was an error return it */
>>  	if (wlantx_err && (wlantx_err != -ENODEV))
>>  		return wlantx_err;
>> +	if (wwanantenna_err && (wwanantenna_err != -ENODEV))
>> +		return wwanantenna_err;
>>
>>  	if (has_wlantxreduce) {
>>  		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> @@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct
>ibm_init_struct *iibm)
>>  		if (err)
>>  			return err;
>>  	}
>> +
>> +	if (has_antennatype) {
>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>&dev_attr_wwan_antenna_type.attr);
>> +		if (err)
>> +			return err;
>> +	}
>>  	return 0;
>>  }
>> [...]
>
>
>Regards,
>Barnabás Pőcze

Thanks & regards,
Nitin Joshi