Re: [PATCH 2/2] ASoC: cs530x: Support for cs530x ADCs

[Paul Handrigan <paulha@opensource.cirrus.com>]

On Mon, Jun 10, 2024 at 10:46:26AM +0200, Krzysztof Kozlowski wrote:
> > +
> > +	cs530x = devm_kzalloc(&client->dev, sizeof(struct cs530x_priv),
> 
> sizeof(*)
> 
Ack

> > +
> > +	if (client->dev.of_node) {
> > +		const struct of_device_id *match;
> > +
> > +		match = of_match_node(cs530x_of_match, client->dev.of_node);
> > +		if (match == NULL)
> > +			return -EINVAL;
> > +		cs530x->devtype = (enum cs530x_type)match->data;
> 
> I think you open-coded device_get_match_data
> 
> 
> > +	} else {
> > +		const struct i2c_device_id *id =
> > +			i2c_match_id(cs530x_i2c_id, client);
> > +
> > +		cs530x->devtype = id->driver_data;
> > +	}
> 
> and for all of this there is a helper i2c_get_device_match_data.
> 
Ack

> > +	if (ret != 0) {
> > +		dev_err(dev, "Failed to request supplies: %d\n", ret);
> 
> return dev_err_probe()
Ack

> > +	if (ret != 0) {
> > +		dev_err(dev, "Failed to enable supplies: %d\n", ret);
> 
> return dev_err_probe()
Ack

> > +	if (IS_ERR(cs530x->reset_gpio)) {
> > +		dev_err(dev, "Reset gpio not available\n");
> 
> return dev_err_probe()
Ack

> > +
> > +	if (cs530x->reset_gpio) {
> > +		usleep_range(2000, 2100);
> > +		gpiod_set_value_cansleep(cs530x->reset_gpio, 1);
> 
> 1 is to keep device in reset? This looks like you are using logical
> values inverted.
Thanks.  The reset pin is active low. I will also change the dt binding
example to reflect that.

> > +
> > +	cs530x->dev_dai = devm_kmemdup(dev, &cs530x_dai,
> > +					sizeof(struct snd_soc_dai_driver),
> 
> sizeof(*)
> 
Ack

Regards,
Paul