On Wed, 30 Aug 2023 15:15:12 +0800 杨明金 <magicyangmingjin@xxxxxxxxx> wrote: > Jonathan Cameron <jic23@xxxxxxxxxx> 于2023年8月28日周一 23:56写道: > > Hi, Please crop replies to relevant part only. Hopefully I found it! > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel) > > > +{ > > > + int ret = 0; > > > + u32 reg_read = 0; > > > + > > > + if (data->pm_data.clk_regmap) { > > > + ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg, > > > + data->pm_data.clk_reg_mask, > > > + data->pm_data.clk_reg_mask); > > > + ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, ®_read); > > > + if (ret) { > > > + dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel); > > > + return ret; > > > + } > > > + dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read); > > > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces > > for this? > > This register is used to vote to enable/disable the pmic 26m clk which > is provided to modules like audio, typec and adc. > Therefore, this clk cannot be disabled or enabled directly. clk_enable() and friends support reference counted enable and disable so I don't understand why this needs something unusual. > > > > +static int sprd_adc_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *np = pdev->dev.of_node; > > > + struct sprd_adc_data *sprd_data; > > > + const struct sprd_adc_variant_data *pdata; > > > + struct iio_dev *indio_dev; > > > + int ret; > > > + > > > + pdata = of_device_get_match_data(&pdev->dev); > > > > device_get_match_data() > > > > > > > + if (!pdata) { > > > + dev_err(&pdev->dev, "No matching driver data found\n"); > > > + return -EINVAL; > > > + } > > > + > > > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data)); > > > + if (!indio_dev) > > > + return -ENOMEM; > > > + > > > + sprd_data = iio_priv(indio_dev); > > > + > > > + sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL); > > > + if (!sprd_data->regmap) { > > > + dev_err(&pdev->dev, "failed to get ADC regmap\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = of_property_read_u32(np, "reg", &sprd_data->base); > > > > Even though some elements of this (of_hwspin...) don't have generic firmware > > interfaces, I would prefer to see those from linux/property.h used > > wherever possible. It will take us a long time to make that a subsystem > > wide change, but good not to have more unnecessary instances of device tree > > specific property reading. > > Sorry, I don't understand what needs to be modified. Can you provide > more information or give an example? > Do you mean that the "reg" property reading is unnecessary? No. Where possibly use device_property_read_u32(dev, "reg".. etc and similar functions from include/linux/property.h rather than device tree specific ones. The generic property handling deals with various different types of firmware without needing drivers to be aware of it. Some elements that you need here do not have generic property handling so for those you will need to continue using the of_ variants. Note that this is to support long term move of everything to the generic firmware framework. Even if we drivers in IIO etc that are really device tree only there are benefits for maintenance in using one framework for all drivers. As some IIO drivers do support other firmware types (ACPI for example) the generic version is the preferred choice. Thanks, Jonathan