Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com>
To:     Jonathan Cameron <jic23@kernel.org>
CC:     "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: RE: [PATCH v6 1/2] iio: frequency: adrf6780: add support for ADRF6780
Thread-Topic: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
 ADRF6780
Thread-Index: AQHXejemy+Zld3yeGkqXu4f9qC/DS6tHKjyAgAS3GWA=
Date:   Tue, 20 Jul 2021 13:34:08 +0000
Message-ID: <CY4PR03MB3399CAA3C32707CE3564A44B9BE29@CY4PR03MB3399.namprd03.prod.outlook.com>
References: <20210716114210.141560-1-antoniu.miclaus@analog.com>
 <20210717142623.45f96a22@jic23-huawei>
In-Reply-To: <20210717142623.45f96a22@jic23-huawei>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-2?Q?QygXgiThupRQaEyJ/edbkP7D3PgZYvk5xsgvWQsZLaLsKoumY3eObo1iR3?=
 =?iso-8859-2?Q?nS0Gtv+ttEmKg2BXqgXAxQ8YD6t1sy2/WXiXx1U2raqs6UXhojj1VBEUED?=
 =?iso-8859-2?Q?R/RQCjtgwmu2FIFxc0EATlnz/hgOwA00n+TjxMHEa2y8VZyv27pAK+zduF?=
 =?iso-8859-2?Q?B7EpvHgGL970rdpuU4tt1TkYdhaEFE5XLizuPYcCnvHgMY2XE6IaNBD/RM?=
 =?iso-8859-2?Q?wsXl9pl2ZBehnqG5vjPo+rSo+5bk39urMNBdFWw/HlPO1NG/V5Rf2AaIpt?=
 =?iso-8859-2?Q?BA3RcGMw7V4CBUYskDgDOhg6FPG4GAzLoN0XPDQl+dE/5Va33L5VRCJwuQ?=
 =?iso-8859-2?Q?Wo5RkOyLYSS41zIWs70EJo3yRQl2Uq9Y2rQ/3qkLt5uqRJWhTGj026/6w4?=
 =?iso-8859-2?Q?bATmeFp1CpCdYJd3UVsgLkuysVK5AEmuyzOsiX6fJkJ3oKlVdB7F2Mz4Mr?=
 =?iso-8859-2?Q?8kgqt11huRQSM5gUcUIsg6m7bN2llKbX0kTrUCaP7f/AOgW0YyHsnzym/a?=
 =?iso-8859-2?Q?FrU/pADwt9f8/YMD2WDNgPWvCcikPnNvdvutcsKQrcZ2BIjyYnzgQyOxpy?=
 =?iso-8859-2?Q?iIw3Cavy1Ueb0l4vjtkH2Th0SdCcKMjF8NfT1ywe5IO9MT0NXHGn3fwlcG?=
 =?iso-8859-2?Q?Be5BHG2RGEQlpA8kR/XnASZiDJZvly0NgJY+DZfdBG8GgDCLELPUlV+t0+?=
 =?iso-8859-2?Q?AczyuSMN7OPvDRYITGo0NZsiNqkMwGI5dUNxRVGaCf0Q1NSKBpflxGV9Uh?=
 =?iso-8859-2?Q?ckmoHyyR8Fp+JyXCv7vmramHeLbT9ATdP/YcQyUhWUeDoYzjDr2xyNA7jo?=
 =?iso-8859-2?Q?opdkGPCwH56RFYWznjGB6xDTqEwu+gpF61PkZn9YUd1zV6OyqCGFXgaFQ9?=
 =?iso-8859-2?Q?pOnxl2fYBaQ7Y6Yv0kRtEiDSu616+n1nqWbQ5Po1NSv4xhK1Xs/T/gnmkV?=
 =?iso-8859-2?Q?nbgqV9opVTdEX+7Vuq39v073yVsw2XGhQD2ghDdHKD9j30nbke/9eee1Jm?=
 =?iso-8859-2?Q?vB9byxZxfwW7KK9y0AsYZekja9zT4tooemEUHFQc2+hj68mEbgbLi0CSPG?=
 =?iso-8859-2?Q?JG0UKDKQGbeFpXGP7xxRe33draIpBU7qwEqTm5V3Ea8a10fbwF2vrRoTcx?=
 =?iso-8859-2?Q?k/193MRpgjDRiQgsH5FcvfdqpedmqyYQoBRnTZQv9mUYegJ61frBuST0EC?=
 =?iso-8859-2?Q?dA4eP67GFs91WcFuTxYQl2CK4sX0woip5oKn05EfgAyhf+X5s5Uy0X8k63?=
 =?iso-8859-2?Q?6RsPaaqxioU9O9/KtgI5yb3dXRij69Unf9rB3p8u5t8Sg37JrOu6wQ/S1L?=
 =?iso-8859-2?Q?qADkoWZGoBXmIVP1fbw2ikzTKBFKlk9Ig+Rnj435zUH5DREjnK8K3LiVdi?=
 =?iso-8859-2?Q?o5CI1fAL6H?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: analog.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY4PR03MB3399.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f6078f0-3482-4b8b-4672-08d94b830d90
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2021 13:34:08.9444
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: WckTDJfVyLo9kZnM31Um0tr1MRcoLzjoAP/kFfRtz9aS+2fTTi2C8sP3EM3awAVKTQlzIpbDuufRThBHp8PEsknqzrk9ro2QncV/lKRpedM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR03MB3317
X-Proofpoint-ORIG-GUID: PTaJ92YB2WrKs08ZXvt8IxdHryAh2xFk
X-Proofpoint-GUID: PTaJ92YB2WrKs08ZXvt8IxdHryAh2xFk
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-07-20_07:2021-07-19,2021-07-20 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 lowpriorityscore=0
 adultscore=0 bulkscore=0 malwarescore=0 spamscore=0 suspectscore=0
 mlxlogscore=999 mlxscore=0 phishscore=0 impostorscore=0 priorityscore=1501
 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2107200087
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Jonathan,


> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 17, 2021 4:26 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>=20
> [External]
>=20
> On Fri, 16 Jul 2021 14:42:09 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
>=20
> > The ADRF6780 is a silicon germanium (SiGe) design, wideband,
> > microwave upconverter optimized for point to point microwave
> > radio designs operating in the 5.9 GHz to 23.6 GHz frequency
> > range.
> >
> > Datasheet:
> > https://www.analog.com/media/en/technical-documentation/data-
> sheets/ADRF6780.pdf
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Hi Antoniu
>=20
> As it looks like we'll have a v7, a few trivial comments from me.
>=20
> ...
>=20
> > +
> > +static int adrf6780_init(struct adrf6780_dev *dev)
> > +{
> > +	int ret;
> > +	unsigned int chip_id, enable_reg, enable_reg_msk;
> > +	struct spi_device *spi =3D dev->spi;
> > +
> > +	/* Perform a software reset */
> > +	ret =3D adrf6780_reset(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =3D adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	chip_id =3D FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id);
> > +	if (chip_id !=3D ADRF6780_CHIP_ID) {
> > +		dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	enable_reg_msk =3D ADRF6780_VGA_BUFFER_EN_MSK |
> > +			ADRF6780_DETECTOR_EN_MSK |
> > +			ADRF6780_LO_BUFFER_EN_MSK |
> > +			ADRF6780_IF_MODE_EN_MSK |
> > +			ADRF6780_IQ_MODE_EN_MSK |
> > +			ADRF6780_LO_X2_EN_MSK |
> > +			ADRF6780_LO_PPF_EN_MSK |
> > +			ADRF6780_LO_EN_MSK |
> > +			ADRF6780_UC_BIAS_EN_MSK;
> > +
> > +	enable_reg =3D FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev-
> >vga_buff_en) |
> > +			FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) |
> > +			FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev-
> >lo_buff_en) |
> > +			FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev-
> >if_mode_en) |
> > +			FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev-
> >iq_mode_en) |
> > +			FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev-
> >lo_x2_en) |
> > +			FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev-
> >lo_ppf_en) |
> > +			FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) |
> > +			FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev-
> >uc_bias_en);
>=20
> Here we are probably turning on a bunch of stuff which will result in pow=
er
> usage.
> Would it be sensible to turn it off again in remove path?  (devm managed
> should be fine).

Most of these attributes are enabled by default after device reset.
Taking into account this statement, is it still worth adding a 'custom' rem=
ove path?

>=20
>=20
> > +
> > +	ret =3D adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE,
> enable_reg_msk, enable_reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =3D adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH,
> > +
> 	ADRF6780_LO_SIDEBAND_MSK,
> > +
> 	FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return adrf6780_spi_update_bits(dev,
> ADRF6780_REG_ADC_CONTROL,
> > +
> 	ADRF6780_VDET_OUTPUT_SELECT_MSK,
> > +
> 	FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev-
> >vdet_out_en));
> > +}
> > +
>=20
> > +static void adrf6780_dt_parse(struct adrf6780_dev *dev)
>=20
> Trivial nitpick, but given this is all device property based (great)
> the dt naming is now misleading.  Perhaps _properties_parse() ?
>=20
> > +{
> > +	struct spi_device *spi =3D dev->spi;
> > +
> > +	dev->vga_buff_en =3D device_property_read_bool(&spi->dev,
> "adi,vga-buff-en");
> > +	dev->lo_buff_en =3D device_property_read_bool(&spi->dev, "adi,lo-
> buff-en");
> > +	dev->if_mode_en =3D device_property_read_bool(&spi->dev, "adi,if-
> mode-en");
> > +	dev->iq_mode_en =3D device_property_read_bool(&spi->dev, "adi,iq-
> mode-en");
> > +	dev->lo_x2_en =3D device_property_read_bool(&spi->dev, "adi,lo-x2-
> en");
> > +	dev->lo_ppf_en =3D device_property_read_bool(&spi->dev, "adi,lo-
> ppf-en");
> > +	dev->lo_en =3D device_property_read_bool(&spi->dev, "adi,lo-en");
> > +	dev->uc_bias_en =3D device_property_read_bool(&spi->dev, "adi,uc-
> bias-en");
> > +	dev->lo_sideband =3D device_property_read_bool(&spi->dev, "adi,lo-
> sideband");
> > +	dev->vdet_out_en =3D device_property_read_bool(&spi->dev,
> "adi,vdet-out-en");
> > +}

--
Antoniu Micl=E3u=BA